fix: resolve review comments

bnb-chain · Dec 26, 2023 · 8650392 · 8650392
1 parent e06f8d4
commit 8650392
Show file tree

Hide file tree

Showing 23 changed files with 87 additions and 206 deletions.
diff --git a/abi/bscvalidatorset.abi b/abi/bscvalidatorset.abi
@@ -1062,19 +1062,6 @@
     ],
     "stateMutability": "view"
   },
-  {
-    "type": "function",
-    "name": "jailValidator",
-    "inputs": [
-      {
-        "name": "consensusAddress",
-        "type": "address",
-        "internalType": "address"
-      }
-    ],
-    "outputs": [],
-    "stateMutability": "nonpayable"
-  },
   {
     "type": "function",
     "name": "maintainSlashScale",
@@ -1224,6 +1211,19 @@
     ],
     "stateMutability": "view"
   },
+  {
+    "type": "function",
+    "name": "removeTmpMigratedValidator",
+    "inputs": [
+      {
+        "name": "validator",
+        "type": "address",
+        "internalType": "address"
+      }
+    ],
+    "outputs": [],
+    "stateMutability": "nonpayable"
+  },
   {
     "type": "function",
     "name": "systemRewardRatio",

diff --git a/abi/slashindicator.abi b/abi/slashindicator.abi
@@ -865,25 +865,6 @@
     ],
     "anonymous": false
   },
-  {
-    "type": "event",
-    "name": "failedMaliciousVoteSlash",
-    "inputs": [
-      {
-        "name": "voteAddrSlice",
-        "type": "bytes32",
-        "indexed": true,
-        "internalType": "bytes32"
-      },
-      {
-        "name": "failReason",
-        "type": "bytes",
-        "indexed": false,
-        "internalType": "bytes"
-      }
-    ],
-    "anonymous": false
-  },
   {
     "type": "event",
     "name": "indicatorCleaned",

diff --git a/abi/stakehub.abi b/abi/stakehub.abi
@@ -1009,7 +1009,7 @@
         "internalType": "address"
       },
       {
-        "name": "commissionRate",
+        "name": "newCommissionRate",
         "type": "uint64",
         "indexed": false,
         "internalType": "uint64"

diff --git a/abi/systemreward.abi b/abi/systemreward.abi
@@ -159,19 +159,6 @@
     ],
     "stateMutability": "view"
   },
-  {
-    "type": "function",
-    "name": "MAX_REWARDS_FOR_FINALITY",
-    "inputs": [],
-    "outputs": [
-      {
-        "name": "",
-        "type": "uint256",
-        "internalType": "uint256"
-      }
-    ],
-    "stateMutability": "view"
-  },
   {
     "type": "function",
     "name": "RELAYERHUB_CONTRACT_ADDR",
@@ -417,30 +404,6 @@
     ],
     "stateMutability": "nonpayable"
   },
-  {
-    "type": "function",
-    "name": "claimRewardsforFinality",
-    "inputs": [
-      {
-        "name": "to",
-        "type": "address",
-        "internalType": "address payable"
-      },
-      {
-        "name": "amount",
-        "type": "uint256",
-        "internalType": "uint256"
-      }
-    ],
-    "outputs": [
-      {
-        "name": "",
-        "type": "uint256",
-        "internalType": "uint256"
-      }
-    ],
-    "stateMutability": "nonpayable"
-  },
   {
     "type": "function",
     "name": "isOperator",

diff --git a/abi/tokenhub.abi b/abi/tokenhub.abi
@@ -750,30 +750,6 @@
     ],
     "stateMutability": "nonpayable"
   },
-  {
-    "type": "function",
-    "name": "claimRewardsforFinality",
-    "inputs": [
-      {
-        "name": "",
-        "type": "address",
-        "internalType": "address payable"
-      },
-      {
-        "name": "",
-        "type": "uint256",
-        "internalType": "uint256"
-      }
-    ],
-    "outputs": [
-      {
-        "name": "",
-        "type": "uint256",
-        "internalType": "uint256"
-      }
-    ],
-    "stateMutability": "nonpayable"
-  },
   {
     "type": "function",
     "name": "getBep2SymbolByContractAddr",

diff --git a/contracts/BC_fusion/StakeHub.sol b/contracts/BC_fusion/StakeHub.sol
@@ -176,7 +176,7 @@ contract StakeHub is System, Initializable {
         bytes voteAddress
     );
     event ConsensusAddressEdited(address indexed operatorAddress, address indexed newConsensusAddress);
-    event CommissionRateEdited(address indexed operatorAddress, uint64 commissionRate);
+    event CommissionRateEdited(address indexed operatorAddress, uint64 newCommissionRate);
     event DescriptionEdited(address indexed operatorAddress);
     event VoteAddressEdited(address indexed operatorAddress, bytes newVoteAddress);
     event Delegated(address indexed operatorAddress, address indexed delegator, uint256 shares, uint256 bnbAmount);
@@ -994,8 +994,8 @@ contract StakeHub is System, Initializable {
         }
         if (IStakeCredit(valInfo.creditContract).getPooledBNB(operatorAddress) < minSelfDelegationBNB) {
             _jailValidator(valInfo, downtimeJailTime);
-            // need to inform BSCValidatorSet contract to remove the validator
-            IBSCValidatorSet(VALIDATOR_CONTRACT_ADDR).jailValidator(valInfo.consensusAddress);
+            IBSCValidatorSet(VALIDATOR_CONTRACT_ADDR).removeTmpMigratedValidator(valInfo.consensusAddress);
+            IBSCValidatorSet(VALIDATOR_CONTRACT_ADDR).felony(valInfo.consensusAddress);
         }
     }
 

diff --git a/contracts/BC_fusion/interface/IBSCValidatorSet.sol b/contracts/BC_fusion/interface/IBSCValidatorSet.sol
@@ -2,5 +2,6 @@
 pragma solidity 0.8.17;
 
 interface IBSCValidatorSet {
-    function jailValidator(address consensusAddress) external;
+    function felony(address consensusAddress) external;
+    function removeTmpMigratedValidator(address consensusAddress) external;
 }
diff --git a/contracts/BSCValidatorSet.sol b/contracts/BSCValidatorSet.sol
@@ -188,8 +188,7 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
   receive() external payable {}
 
   /*********************** Cross Chain App Implement **************************/
-  // TODO: add `onlyCrossChainContract`
-  function handleSynPackage(uint8, bytes calldata msgBytes) onlyInit initValidatorExtraSet external override returns(bytes memory responsePayload) {
+  function handleSynPackage(uint8, bytes calldata msgBytes) onlyInit onlyCrossChainContract initValidatorExtraSet external override returns(bytes memory responsePayload) {
     (IbcValidatorSetPackage memory validatorSetPackage, bool ok) = decodeValidatorSetSynPackage(msgBytes);
     if (!ok) {
       return CmnPkg.encodeCommonAckPackage(ERROR_FAIL_DECODE);
@@ -202,13 +201,18 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
         emit failReasonWithStr("length of jail validators must be one");
         resCode = ERROR_LEN_OF_VAL_MISMATCH;
       } else {
-        uint256 index = currentValidatorSetMap[validatorSetPackage.validatorSet[0].consensusAddress];
-        if (index==0 || currentValidatorSet[index-1].jailed) {
-          emit validatorEmptyJailed(validatorSetPackage.validatorSet[0].consensusAddress);
-          resCode = CODE_OK;
+        address validator = validatorSetPackage.validatorSet[0].consensusAddress;
+        uint256 index = currentValidatorSetMap[validator];
+        if (index == 0 || currentValidatorSet[index-1].jailed) {
+          emit validatorEmptyJailed(validator);
         } else {
-          resCode = _jailValidator(index);
+          // felony will failed if the validator is the only one in the validator set
+          bool success = _felony(validator, index-1);
+          if (!success) {
+            emit validatorEmptyJailed(validator);
+          }
         }
+        resCode = CODE_OK;
       }
     } else {
       resCode = ERROR_UNKNOWN_PACKAGE_TYPE;
@@ -357,36 +361,6 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
     }
   }
 
-  function jailValidator(address consensusAddress) external onlyStakeHub {
-    for (uint256 i; i < _tmpMigratedValidatorSet.length; ++i) {
-      if (_tmpMigratedValidatorSet[i].consensusAddress == consensusAddress) {
-        _tmpMigratedValidatorSet[i].jailed = true;
-        break;
-      }
-    }
-
-    uint256 index = currentValidatorSetMap[consensusAddress];
-    if (index==0 || currentValidatorSet[index-1].jailed) {
-      emit validatorEmptyJailed(consensusAddress);
-    } else {
-      _jailValidator(index);
-    }
-  }
-
-  function _jailValidator(uint256 index) internal returns (uint32) {
-    uint n = currentValidatorSet.length;
-    bool shouldKeep = (numOfJailed >= n-1);
-    // will not jail if it is the last valid validator
-    if (shouldKeep) {
-      emit validatorEmptyJailed(currentValidatorSet[index-1].consensusAddress);
-      return CODE_OK;
-    }
-    ++numOfJailed;
-    currentValidatorSet[index-1].jailed = true;
-    emit validatorJailed(currentValidatorSet[index-1].consensusAddress);
-    return CODE_OK;
-  }
-
   function updateValidatorSet(Validator[] memory validatorSet, bytes[] memory voteAddrs) internal returns (uint32) {
     {
       // do verify.
@@ -701,7 +675,7 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
       return;
     }
 
-    totalValue = ISystemReward(SYSTEM_REWARD_ADDR).claimRewardsforFinality(payable(address(this)), totalValue);
+    totalValue = ISystemReward(SYSTEM_REWARD_ADDR).claimRewards(payable(address(this)), totalValue);
     if (totalValue == 0) {
       return;
     }
@@ -758,7 +732,9 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
     }
   }
 
-  function felony(address validator)external onlySlash initValidatorExtraSet override{
+  function felony(address validator) external initValidatorExtraSet override {
+    require(msg.sender == SLASH_CONTRACT_ADDR || msg.sender == STAKE_HUB_ADDR, "only slash or stakeHub contract");
+
     uint256 index = currentValidatorSetMap[validator];
     if (index <= 0) {
       return;
@@ -772,6 +748,15 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
     }
   }
 
+  function removeTmpMigratedValidator(address validator) external onlyStakeHub {
+    for (uint256 i; i < _tmpMigratedValidatorSet.length; ++i) {
+      if (_tmpMigratedValidatorSet[i].consensusAddress == validator) {
+        _tmpMigratedValidatorSet[i].jailed = true;
+        break;
+      }
+    }
+  }
+
   /*********************** For Temporary Maintenance **************************/
   function getCurrentValidatorIndex(address _validator) public view returns (uint256) {
     uint256 index = currentValidatorSetMap[_validator];
@@ -1064,6 +1049,8 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
       // should not happen, but still protect
       return index;
     }
+
+    // averageDistribute*rest may less than income, but it is ok, the dust income will go to system reward eventually.
     uint256 averageDistribute = income / rest;
     if (averageDistribute != 0) {
       for (uint i; i<index; ++i) {
@@ -1074,12 +1061,11 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
         currentValidatorSet[i].incoming = currentValidatorSet[i].incoming + averageDistribute;
       }
     }
-    // averageDistribute*rest may less than income, but it is ok, the dust income will go to system reward eventually.
 
     return index;
   }
 
-  function _felony(address validator, uint256 index) private returns (bool){
+  function _felony(address validator, uint256 index) private returns (bool) {
     uint256 income = currentValidatorSet[index].incoming;
     uint256 rest = currentValidatorSet.length - 1;
     if (getValidators().length <= 1) {

diff --git a/contracts/GovHub.sol b/contracts/GovHub.sol
@@ -27,8 +27,7 @@ contract GovHub is System, IApplication{
     address  target;
   }
 
-  //TODO: add onlyCrossChainContract
-  function handleSynPackage(uint8, bytes calldata msgBytes) external override returns(bytes memory responsePayload) {
+  function handleSynPackage(uint8, bytes calldata msgBytes) external onlyCrossChainContract override returns(bytes memory responsePayload) {
     (ParamChangePackage memory proposal, bool success) = decodeSynPackage(msgBytes);
     if (!success) {
       return CmnPkg.encodeCommonAckPackage(ERROR_FAIL_DECODE);