[StakingVesting] Request bonus should not be reverted (#45)

* Merge two bonus requests bonus into one

* Rename in tests

* Rename deploy scripts

* Use unsafeSendRON for staking vesting

* Add tests

* Fix rebase

* Add contract balance for transfer failed event

* Handle no request bonus for deprecated block producer

contracts/extensions/RONTransferHelper.sol

@@ -3,6 +3,14 @@
 pragma solidity ^0.8.9;
 
 abstract contract RONTransferHelper {
+  /**
+   * @dev See `_sendRON`.
+   * Reverts if the recipient does not receive RON.
+   */
+  function _transferRON(address payable _recipient, uint256 _amount) internal {
+    require(_sendRON(_recipient, _amount), "RONTransfer: unable to transfer value, recipient may have reverted");
+  }
+
   /**
    * @dev Send `_amount` RON to the address `_recipient`.
    * Returns whether the recipient receives RON or not.
@@ -13,14 +21,20 @@ abstract contract RONTransferHelper {
    */
   function _sendRON(address payable _recipient, uint256 _amount) internal returns (bool _success) {
     require(address(this).balance >= _amount, "RONTransfer: insufficient balance");
-    (_success, ) = _recipient.call{ value: _amount }("");
+    return _unsafeSendRON(_recipient, _amount);
   }
 
   /**
-   * @dev See `_sendRON`.
-   * Reverts if the recipient does not receive RON.
+   * @dev Unsafe send `_amount` RON to the address `_recipient`. If the sender's balance is insufficient,
+   * the call does not revert.
+   *
+   * Note:
+   * - Does not assert whether the balance of sender is sufficient.
+   * - Does not assert whether the recipient accepts RON.
+   * - Consider using `ReentrancyGuard` before calling this function.
+   *
    */
-  function _transferRON(address payable _recipient, uint256 _amount) internal {
-    require(_sendRON(_recipient, _amount), "RONTransfer: unable to transfer value, recipient may have reverted");
+  function _unsafeSendRON(address payable _recipient, uint256 _amount) internal returns (bool _success) {
+    (_success, ) = _recipient.call{ value: _amount }("");
   }
 }

contracts/interfaces/IStakingVesting.sol

@@ -3,19 +3,30 @@
 pragma solidity ^0.8.9;
 
 interface IStakingVesting {
-  /// @dev Emitted when the block bonus for validator is transferred.
-  event ValidatorBonusTransferred(uint256 indexed blockNumber, address indexed recipient, uint256 amount);
-  /// @dev Emitted when the block bonus for bridge operator is transferred.
-  event BridgeOperatorBonusTransferred(uint256 indexed blockNumber, address indexed recipient, uint256 amount);
-  /// @dev Emitted when the block bonus for validator is updated
-  event ValidatorBonusPerBlockUpdated(uint256);
+  /// @dev Emitted when the block bonus for block producer is transferred.
+  event BonusTransferred(
+    uint256 indexed blockNumber,
+    address indexed recipient,
+    uint256 blockProducerAmount,
+    uint256 bridgeOperatorAmount
+  );
+  /// @dev Emitted when the transfer of block bonus for block producer is failed.
+  event BonusTransferFailed(
+    uint256 indexed blockNumber,
+    address indexed recipient,
+    uint256 blockProducerAmount,
+    uint256 bridgeOperatorAmount,
+    uint256 contractBalance
+  );
+  /// @dev Emitted when the block bonus for block producer is updated
+  event BlockProducerBonusPerBlockUpdated(uint256);
   /// @dev Emitted when the block bonus for bridge operator is updated
   event BridgeOperatorBonusPerBlockUpdated(uint256);
 
   /**
-   * @dev Returns the bonus amount for the validator at `_block`.
+   * @dev Returns the bonus amount for the block producer at `_block`.
    */
-  function validatorBlockBonus(uint256 _block) external view returns (uint256);
+  function blockProducerBlockBonus(uint256 _block) external view returns (uint256);
 
   /**
    * @dev Returns the bonus amount for the bridge validator at `_block`.
@@ -28,51 +39,36 @@ interface IStakingVesting {
   function receiveRON() external payable;
 
   /**
-   * @dev Returns the last block number that the staking vesting is sent for the validator.
+   * @dev Returns the last block number that the staking vesting is sent.
    */
-  function lastBlockSendingValidatorBonus() external view returns (uint256);
+  function lastBlockSendingBonus() external view returns (uint256);
 
   /**
-   * @dev Returns the last block number that the staking vesting is sent for the bridge operator.
-   */
-  function lastBlockSendingBridgeOperatorBonus() external view returns (uint256);
-
-  /**
-   * @dev Does two actions as in `requestValidatorBonus` and `requestBridgeOperatorBonus`. Returns
-   * two amounts of bonus correspondingly.
-   *
-   * Requirements:
-   * - The method caller is validator contract.
-   * - The method must be called only once per block.
-   *
-   * Emits the event `ValidatorBonusTransferred` and/or `BridgeOperatorBonusTransferred`
-   *
-   */
-  function requestBonus() external returns (uint256 _validatorBonus, uint256 _bridgeOperatorBonus);
-
-  /**
-   * @dev Transfers the staking vesting for the validator whenever a new block is mined.
-   * Returns the amount of RON sent to validator contract.
+   * @dev Transfers the staking vesting for the block producer and the bridge operator whenever a new block is mined.
    *
    * Requirements:
-   * - The method caller is validator contract.
+   * - The method caller must be validator contract.
    * - The method must be called only once per block.
    *
-   * Emits the event `ValidatorBonusTransferred`.
+   * Emits the event `BonusTransferred` or `BonusTransferFailed`.
    *
-   */
-  function requestValidatorBonus() external returns (uint256);
-
-  /**
-   * @dev Transfers the staking vesting for the bridge operator whenever a new block is mined.
-   * Returns the amount of RON sent to validator contract.
+   * Notes:
+   * - The method does not revert when the contract balance is insufficient to send bonus. This assure the submit reward method
+   * will not be reverted, and the underlying nodes does not hang.
    *
-   * Requirements:
-   * - The method caller is validator contract.
-   * - The method must be called only once per block.
+   * @param _forBlockProducer Indicates whether requesting the bonus for the block procucer, in case of being in jail or relevance.
+   * @param _forBridgeOperator Indicates whether requesting the bonus for the bridge operator.
    *
-   * Emits the event `BridgeOperatorBonusTransferred`.
+   * @return _success Whether the transfer is successfully. This returns false mostly because this contract is out of balance.
+   * @return _blockProducerBonus The amount of bonus actually sent for the block producer, returns 0 when the transfer is failed.
+   * @return _bridgeOperatorBonus The amount of bonus actually sent for the bridge operator, returns 0 when the transfer is failed.
    *
    */
-  function requestBridgeOperatorBonus() external returns (uint256);
+  function requestBonus(bool _forBlockProducer, bool _forBridgeOperator)
+    external
+    returns (
+      bool _success,
+      uint256 _blockProducerBonus,
+      uint256 _bridgeOperatorBonus
+    );
 }

contracts/ronin/StakingVesting.sol

@@ -8,14 +8,12 @@ import "../extensions/collections/HasValidatorContract.sol";
 import "../extensions/RONTransferHelper.sol";
 
 contract StakingVesting is IStakingVesting, HasValidatorContract, RONTransferHelper, Initializable {
-  /// @dev The block bonus for the validator whenever a new block is mined.
-  uint256 internal _validatorBonusPerBlock;
+  /// @dev The block bonus for the block producer whenever a new block is mined.
+  uint256 internal _blockProducerBonusPerBlock;
   /// @dev The block bonus for the bridge operator whenever a new block is mined.
   uint256 internal _bridgeOperatorBonusPerBlock;
-  /// @dev The last block number that the staking vesting for validator sent.
-  uint256 public lastBlockSendingValidatorBonus;
-  /// @dev The last block number that the staking vesting for bridge operator sent.
-  uint256 public lastBlockSendingBridgeOperatorBonus;
+  /// @dev The last block number that the staking vesting sent.
+  uint256 public lastBlockSendingBonus;
 
   constructor() {
     _disableInitializers();
@@ -26,11 +24,11 @@ contract StakingVesting is IStakingVesting, HasValidatorContract, RONTransferHel
    */
   function initialize(
     address __validatorContract,
-    uint256 __validatorBonusPerBlock,
+    uint256 __blockProducerBonusPerBlock,
     uint256 __bridgeOperatorBonusPerBlock
   ) external payable initializer {
     _setValidatorContract(__validatorContract);
-    _setValidatorBonusPerBlock(__validatorBonusPerBlock);
+    _setBlockProducerBonusPerBlock(__blockProducerBonusPerBlock);
     _setBridgeOperatorBonusPerBlock(__bridgeOperatorBonusPerBlock);
   }
 
@@ -42,81 +40,71 @@ contract StakingVesting is IStakingVesting, HasValidatorContract, RONTransferHel
   /**
    * @inheritdoc IStakingVesting
    */
-  function validatorBlockBonus(
+  function blockProducerBlockBonus(
     uint256 /* _block */
-  ) public view returns (uint256) {
-    return _validatorBonusPerBlock;
+  ) public view override returns (uint256) {
+    return _blockProducerBonusPerBlock;
   }
 
   /**
    * @inheritdoc IStakingVesting
    */
   function bridgeOperatorBlockBonus(
     uint256 /* _block */
-  ) public view returns (uint256) {
+  ) public view override returns (uint256) {
     return _bridgeOperatorBonusPerBlock;
   }
 
   /**
    * @inheritdoc IStakingVesting
    */
-  function requestBonus()
+  function requestBonus(bool _forBlockProducer, bool _forBridgeOperator)
     external
+    override
     onlyValidatorContract
-    returns (uint256 _validatorBonus, uint256 _bridgeOperatorBonus)
+    returns (
+      bool _success,
+      uint256 _blockProducerBonus,
+      uint256 _bridgeOperatorBonus
+    )
   {
-    _validatorBonus = requestValidatorBonus();
-    _bridgeOperatorBonus = requestBridgeOperatorBonus();
-  }
+    require(block.number > lastBlockSendingBonus, "StakingVesting: bonus for already sent");
+    lastBlockSendingBonus = block.number;
 
-  /**
-   * @inheritdoc IStakingVesting
-   */
-  function requestValidatorBonus() public onlyValidatorContract returns (uint256 _amount) {
-    require(block.number > lastBlockSendingValidatorBonus, "StakingVesting: bonus for validator already sent");
-    lastBlockSendingValidatorBonus = block.number;
-    _amount = validatorBlockBonus(block.number);
+    _blockProducerBonus = _forBlockProducer ? blockProducerBlockBonus(block.number) : 0;
+    _bridgeOperatorBonus = _forBridgeOperator ? bridgeOperatorBlockBonus(block.number) : 0;
 
-    if (_amount > 0) {
-      address payable _validatorContractAddr = payable(validatorContract());
-      require(
-        _sendRON(_validatorContractAddr, _amount),
-        "StakingVesting: could not transfer RON to validator contract"
-      );
-      emit ValidatorBonusTransferred(block.number, _validatorContractAddr, _amount);
-    }
-  }
+    uint256 _totalAmount = _blockProducerBonus + _bridgeOperatorBonus;
 
-  /**
-   * @inheritdoc IStakingVesting
-   */
-  function requestBridgeOperatorBonus() public onlyValidatorContract returns (uint256 _amount) {
-    require(
-      block.number > lastBlockSendingBridgeOperatorBonus,
-      "StakingVesting: bonus for bridge operator already sent"
-    );
-    lastBlockSendingBridgeOperatorBonus = block.number;
-    _amount = bridgeOperatorBlockBonus(block.number);
-
-    if (_amount > 0) {
+    if (_totalAmount > 0) {
       address payable _validatorContractAddr = payable(validatorContract());
-      require(
-        _sendRON(_validatorContractAddr, _amount),
-        "StakingVesting: could not transfer RON to validator contract"
-      );
-      emit BridgeOperatorBonusTransferred(block.number, _validatorContractAddr, _amount);
+
+      _success = _unsafeSendRON(_validatorContractAddr, _totalAmount);
+
+      if (!_success) {
+        emit BonusTransferFailed(
+          block.number,
+          _validatorContractAddr,
+          _blockProducerBonus,
+          _bridgeOperatorBonus,
+          address(this).balance
+        );
+        return (_success, 0, 0);
+      }
+
+      emit BonusTransferred(block.number, _validatorContractAddr, _blockProducerBonus, _bridgeOperatorBonus);
     }
   }
 
   /**
-   * @dev Sets the bonus amount per block for validator.
+   * @dev Sets the bonus amount per block for block producer.
    *
-   * Emits the event `ValidatorBonusPerBlockUpdated`.
+   * Emits the event `BlockProducerBonusPerBlockUpdated`.
    *
    */
-  function _setValidatorBonusPerBlock(uint256 _amount) internal {
-    _validatorBonusPerBlock = _amount;
-    emit ValidatorBonusPerBlockUpdated(_amount);
+  function _setBlockProducerBonusPerBlock(uint256 _amount) internal {
+    _blockProducerBonusPerBlock = _amount;
+    emit BlockProducerBonusPerBlockUpdated(_amount);
   }
 
   /**

contracts/ronin/validator/RoninValidatorSet.sol

@@ -140,31 +140,33 @@ contract RoninValidatorSet is
   function submitBlockReward() external payable override onlyCoinbase {
     uint256 _submittedReward = msg.value;
     address _coinbaseAddr = msg.sender;
+    bool _requestForBlockProducer = isBlockProducer(_coinbaseAddr) &&
+      !_jailed(_coinbaseAddr) &&
+      !_miningRewardDeprecated(_coinbaseAddr, currentPeriod());
+    bool _requestForBridgeOperator = true;
+
+    (, uint256 _blockProducerBonus, uint256 _bridgeOperatorBonus) = _stakingVestingContract.requestBonus(
+      _requestForBlockProducer,
+      _requestForBridgeOperator
+    );
 
-    uint256 _bridgeOperatorBonus = _stakingVestingContract.requestBridgeOperatorBonus();
     _totalBridgeReward += _bridgeOperatorBonus;
 
     // Deprecates reward for non-validator or slashed validator
-    if (
-      !isBlockProducer(_coinbaseAddr) ||
-      _jailed(_coinbaseAddr) ||
-      _miningRewardDeprecated(_coinbaseAddr, currentPeriod())
-    ) {
+    if (_requestForBlockProducer) {
+      uint256 _reward = _submittedReward + _blockProducerBonus;
+      uint256 _rate = _candidateInfo[_coinbaseAddr].commissionRate;
+
+      uint256 _miningAmount = (_rate * _reward) / 100_00;
+      _miningReward[_coinbaseAddr] += _miningAmount;
+
+      uint256 _delegatingAmount = _reward - _miningAmount;
+      _delegatingReward[_coinbaseAddr] += _delegatingAmount;
+      _stakingContract.recordReward(_coinbaseAddr, _delegatingAmount);
+      emit BlockRewardSubmitted(_coinbaseAddr, _submittedReward, _blockProducerBonus);
+    } else {
       emit BlockRewardRewardDeprecated(_coinbaseAddr, _submittedReward);
-      return;
     }
-
-    uint256 _blockProducerBonus = _stakingVestingContract.requestValidatorBonus();
-    uint256 _reward = _submittedReward + _blockProducerBonus;
-    uint256 _rate = _candidateInfo[_coinbaseAddr].commissionRate;
-
-    uint256 _miningAmount = (_rate * _reward) / 100_00;
-    _miningReward[_coinbaseAddr] += _miningAmount;
-
-    uint256 _delegatingAmount = _reward - _miningAmount;
-    _delegatingReward[_coinbaseAddr] += _delegatingAmount;
-    _stakingContract.recordReward(_coinbaseAddr, _delegatingAmount);
-    emit BlockRewardSubmitted(_coinbaseAddr, _submittedReward, _blockProducerBonus);
   }
 
   /**

src/config.ts

@@ -73,7 +73,7 @@ export const stakingConfig: StakingConfig = {
 export const stakingVestingConfig: StakingVestingConfig = {
   [Network.Hardhat]: undefined,
   [Network.Devnet]: {
-    validatorBonusPerBlock: BigNumber.from(10).pow(18), // 1 RON per block
+    blockProducerBonusPerBlock: BigNumber.from(10).pow(18), // 1 RON per block
     bridgeOperatorBonusPerBlock: BigNumber.from(10).pow(18), // 1 RON per block
     topupAmount: BigNumber.from(10).pow(18).mul(BigNumber.from(10).pow(4)), // 10.000 RON
   },

src/deploy/proxy/bonus-reward-proxy.ts → src/deploy/proxy/staking-vesting-proxy.ts

@@ -18,7 +18,7 @@ const deploy = async ({ getNamedAccounts, deployments }: HardhatRuntimeEnvironme
 
   const data = new StakingVesting__factory().interface.encodeFunctionData('initialize', [
     generalRoninConf[network.name]!.validatorContract?.address,
-    stakingVestingConfig[network.name]!.validatorBonusPerBlock,
+    stakingVestingConfig[network.name]!.blockProducerBonusPerBlock,
     stakingVestingConfig[network.name]!.bridgeOperatorBonusPerBlock,
   ]);
 

src/utils.ts

@@ -79,7 +79,7 @@ export interface StakingConfig {
 }
 
 export interface StakingVestingArguments {
-  validatorBonusPerBlock?: BigNumberish;
+  blockProducerBonusPerBlock?: BigNumberish;
   bridgeOperatorBonusPerBlock?: BigNumberish;
   topupAmount?: BigNumberish;
 }

test/helpers/fixture.ts

@@ -66,7 +66,7 @@ export const defaultTestConfig: InitTestInput = {
   },
 
   stakingVestingArguments: {
-    validatorBonusPerBlock: 1,
+    blockProducerBonusPerBlock: 1,
     bridgeOperatorBonusPerBlock: 1,
     topupAmount: BigNumber.from(100000000000),
   },