From a0f5571c6c106286a2a2472009f7339756f0a487 Mon Sep 17 00:00:00 2001
From: seolaoh <osa8361@gmail.com>
Date: Thu, 18 Apr 2024 13:14:01 +0900
Subject: [PATCH] feat(contracts): apply PR reviews

---
 packages/contracts/contracts/L1/AssetManager.sol   |  9 ++++++++-
 .../contracts/contracts/L1/ValidatorManager.sol    |  8 ++++----
 packages/contracts/contracts/L1/ValidatorPool.sol  |  4 ++--
 .../contracts/contracts/test/AssetManager.t.sol    | 14 ++++++++++++++
 packages/contracts/deploy/L1/007-ValidatorPool.ts  |  2 +-
 ...010-L2OutputOracle.ts => 008-L2OutputOracle.ts} |  2 +-
 .../L1/{011-ZKVerifier.ts => 009-ZKVerifier.ts}    |  0
 .../L1/{012-Colosseum.ts => 010-Colosseum.ts}      |  7 +++++--
 .../L1/{013-KromaPortal.ts => 011-KromaPortal.ts}  |  0
 .../{014-SystemConfig.ts => 012-SystemConfig.ts}   |  0
 ...CouncilToken.ts => 013-SecurityCouncilToken.ts} |  0
 .../deploy/L1/{016-TimeLock.ts => 014-TimeLock.ts} |  0
 ...7-UpgradeGovernor.ts => 015-UpgradeGovernor.ts} |  0
 ...8-SecurityCouncil.ts => 016-SecurityCouncil.ts} |  0
 .../{008-AssetManager.ts => 018-AssetManager.ts}   |  2 +-
 ...ValidatorManager.ts => 019-ValidatorManager.ts} |  2 +-
 16 files changed, 37 insertions(+), 13 deletions(-)
 rename packages/contracts/deploy/L1/{010-L2OutputOracle.ts => 008-L2OutputOracle.ts} (96%)
 rename packages/contracts/deploy/L1/{011-ZKVerifier.ts => 009-ZKVerifier.ts} (100%)
 rename packages/contracts/deploy/L1/{012-Colosseum.ts => 010-Colosseum.ts} (93%)
 rename packages/contracts/deploy/L1/{013-KromaPortal.ts => 011-KromaPortal.ts} (100%)
 rename packages/contracts/deploy/L1/{014-SystemConfig.ts => 012-SystemConfig.ts} (100%)
 rename packages/contracts/deploy/L1/{015-SecurityCouncilToken.ts => 013-SecurityCouncilToken.ts} (100%)
 rename packages/contracts/deploy/L1/{016-TimeLock.ts => 014-TimeLock.ts} (100%)
 rename packages/contracts/deploy/L1/{017-UpgradeGovernor.ts => 015-UpgradeGovernor.ts} (100%)
 rename packages/contracts/deploy/L1/{018-SecurityCouncil.ts => 016-SecurityCouncil.ts} (100%)
 rename packages/contracts/deploy/L1/{008-AssetManager.ts => 018-AssetManager.ts} (96%)
 rename packages/contracts/deploy/L1/{009-ValidatorManager.ts => 019-ValidatorManager.ts} (97%)

diff --git a/packages/contracts/contracts/L1/AssetManager.sol b/packages/contracts/contracts/L1/AssetManager.sol
index 04b935a2d..4431af3a3 100644
--- a/packages/contracts/contracts/L1/AssetManager.sol
+++ b/packages/contracts/contracts/L1/AssetManager.sol
@@ -339,6 +339,11 @@ contract AssetManager is ISemver, IERC721Receiver {
      */
     error NotAllowedCaller();
 
+    /**
+     * @notice Reverts when constructor parameters are invalid.
+     */
+    error InvalidConstructorParams();
+
     /**
      * @notice Reverts when the status of validator is improper.
      */
@@ -426,6 +431,8 @@ contract AssetManager is ISemver, IERC721Receiver {
         uint128 _slashingRate,
         uint128 _minSlashingAmount
     ) {
+        if (_slashingRate > SLASHING_RATE_DENOM) revert InvalidConstructorParams();
+
         ASSET_TOKEN = _assetToken;
         KGH = _kgh;
         KGH_MANAGER = _kghManager;
@@ -774,7 +781,7 @@ contract AssetManager is ISemver, IERC721Receiver {
     }
 
     /**
-     * @notice Initiate KRO undelegation of given shares for given validator.
+     * @notice Initiate the KRO undelegation of given shares for the given validator.
      *
      * @param validator Address of the validator.
      * @param shares    The amount of shares to undelegate.
diff --git a/packages/contracts/contracts/L1/ValidatorManager.sol b/packages/contracts/contracts/L1/ValidatorManager.sol
index 5a78bcdea..69fb32b8b 100644
--- a/packages/contracts/contracts/L1/ValidatorManager.sol
+++ b/packages/contracts/contracts/L1/ValidatorManager.sol
@@ -239,7 +239,7 @@ contract ValidatorManager is ISemver, IValidatorManager {
      * @inheritdoc IValidatorManager
      */
     function changeCommissionRate(uint8 newCommissionRate) external {
-        if (getStatus(msg.sender) <= ValidatorStatus.INACTIVE) revert ImproperValidatorStatus();
+        if (getStatus(msg.sender) < ValidatorStatus.ACTIVE) revert ImproperValidatorStatus();
 
         Validator storage validatorInfo = _validatorInfo[msg.sender];
 
@@ -453,12 +453,12 @@ contract ValidatorManager is ISemver, IValidatorManager {
     }
 
     /**
-     * @notice Internal function to add output submission rewards to the vaults of finalized output
+     * @notice Private function to add output submission rewards to the vaults of finalized output
      *         submitters.
      *
      * @return Whether the reward distribution is done at least once or not.
      */
-    function _distributeReward() internal returns (bool) {
+    function _distributeReward() private returns (bool) {
         uint256 outputIndex = L2_ORACLE.latestFinalizedOutputIndex() + 1;
         uint256 latestOutputIndex = L2_ORACLE.latestOutputIndex();
 
@@ -578,9 +578,9 @@ contract ValidatorManager is ISemver, IValidatorManager {
      */
     function _updatePriorityValidator() private {
         uint120 weightSum = startedValidatorTotalWeight();
-        uint256 latestFinalizedOutputIndex = L2_ORACLE.latestFinalizedOutputIndex();
 
         if (weightSum > 0) {
+            uint256 latestFinalizedOutputIndex = L2_ORACLE.latestFinalizedOutputIndex();
             Types.CheckpointOutput memory output = L2_ORACLE.getL2Output(
                 latestFinalizedOutputIndex
             );
diff --git a/packages/contracts/contracts/L1/ValidatorPool.sol b/packages/contracts/contracts/L1/ValidatorPool.sol
index d1e2d1f0d..725188ee0 100644
--- a/packages/contracts/contracts/L1/ValidatorPool.sol
+++ b/packages/contracts/contracts/L1/ValidatorPool.sol
@@ -179,9 +179,9 @@ contract ValidatorPool is ReentrancyGuardUpgradeable, ISemver {
     }
     /**
      * @notice Semantic version.
-     * @custom:semver 1.1.1
+     * @custom:semver 1.1.0
      */
-    string public constant version = "1.1.1";
+    string public constant version = "1.1.0";
 
     /**
      * @notice Constructs the ValidatorPool contract.
diff --git a/packages/contracts/contracts/test/AssetManager.t.sol b/packages/contracts/contracts/test/AssetManager.t.sol
index 4a15ab0dc..900cc08a3 100644
--- a/packages/contracts/contracts/test/AssetManager.t.sol
+++ b/packages/contracts/contracts/test/AssetManager.t.sol
@@ -231,6 +231,20 @@ contract AssetManagerTest is L2OutputOracle_ValidatorSystemUpgrade_Initializer {
         assertEq(assetManager.MIN_SLASHING_AMOUNT(), minSlashingAmount);
     }
 
+    function test_constructor_largeSlashingRate_reverts() external {
+        vm.expectRevert(AssetManager.InvalidConstructorParams.selector);
+        new MockAssetManager(
+            kro,
+            kgh,
+            kghManager,
+            address(guardian),
+            valMan,
+            uint128(undelegationPeriod),
+            1001,
+            minSlashingAmount
+        );
+    }
+
     function test_delegate_succeeds() external {
         _setUpKroDelegation(100e18);
 
diff --git a/packages/contracts/deploy/L1/007-ValidatorPool.ts b/packages/contracts/deploy/L1/007-ValidatorPool.ts
index e12b66836..c0f452c4d 100644
--- a/packages/contracts/deploy/L1/007-ValidatorPool.ts
+++ b/packages/contracts/deploy/L1/007-ValidatorPool.ts
@@ -65,6 +65,6 @@ const deployFn: DeployFunction = async (hre) => {
   })
 }
 
-deployFn.tags = ['ValidatorPool', 'setup', 'l1']
+deployFn.tags = ['ValidatorPool', 'setup', 'l1', 'validatorSystemUpgrade']
 
 export default deployFn
diff --git a/packages/contracts/deploy/L1/010-L2OutputOracle.ts b/packages/contracts/deploy/L1/008-L2OutputOracle.ts
similarity index 96%
rename from packages/contracts/deploy/L1/010-L2OutputOracle.ts
rename to packages/contracts/deploy/L1/008-L2OutputOracle.ts
index b084e2cd5..be5644a5c 100644
--- a/packages/contracts/deploy/L1/010-L2OutputOracle.ts
+++ b/packages/contracts/deploy/L1/008-L2OutputOracle.ts
@@ -84,6 +84,6 @@ const deployFn: DeployFunction = async (hre) => {
   })
 }
 
-deployFn.tags = ['L2OutputOracle', 'setup', 'l1']
+deployFn.tags = ['L2OutputOracle', 'setup', 'l1', 'validatorSystemUpgrade']
 
 export default deployFn
diff --git a/packages/contracts/deploy/L1/011-ZKVerifier.ts b/packages/contracts/deploy/L1/009-ZKVerifier.ts
similarity index 100%
rename from packages/contracts/deploy/L1/011-ZKVerifier.ts
rename to packages/contracts/deploy/L1/009-ZKVerifier.ts
diff --git a/packages/contracts/deploy/L1/012-Colosseum.ts b/packages/contracts/deploy/L1/010-Colosseum.ts
similarity index 93%
rename from packages/contracts/deploy/L1/012-Colosseum.ts
rename to packages/contracts/deploy/L1/010-Colosseum.ts
index 6496dd123..f0c3684a3 100644
--- a/packages/contracts/deploy/L1/012-Colosseum.ts
+++ b/packages/contracts/deploy/L1/010-Colosseum.ts
@@ -8,7 +8,10 @@ import {
 } from '../../src/deploy-utils'
 
 const deployFn: DeployFunction = async (hre) => {
-  const zkVerifierProxyAddress = await getDeploymentAddress(hre, 'ZKVerifierProxy')
+  const zkVerifierProxyAddress = await getDeploymentAddress(
+    hre,
+    'ZKVerifierProxy'
+  )
   const l2OutputOracleProxyAddress = await getDeploymentAddress(
     hre,
     'L2OutputOracleProxy'
@@ -90,6 +93,6 @@ const deployFn: DeployFunction = async (hre) => {
   })
 }
 
-deployFn.tags = ['Colosseum', 'setup', 'l1']
+deployFn.tags = ['Colosseum', 'setup', 'l1', 'validatorSystemUpgrade']
 
 export default deployFn
diff --git a/packages/contracts/deploy/L1/013-KromaPortal.ts b/packages/contracts/deploy/L1/011-KromaPortal.ts
similarity index 100%
rename from packages/contracts/deploy/L1/013-KromaPortal.ts
rename to packages/contracts/deploy/L1/011-KromaPortal.ts
diff --git a/packages/contracts/deploy/L1/014-SystemConfig.ts b/packages/contracts/deploy/L1/012-SystemConfig.ts
similarity index 100%
rename from packages/contracts/deploy/L1/014-SystemConfig.ts
rename to packages/contracts/deploy/L1/012-SystemConfig.ts
diff --git a/packages/contracts/deploy/L1/015-SecurityCouncilToken.ts b/packages/contracts/deploy/L1/013-SecurityCouncilToken.ts
similarity index 100%
rename from packages/contracts/deploy/L1/015-SecurityCouncilToken.ts
rename to packages/contracts/deploy/L1/013-SecurityCouncilToken.ts
diff --git a/packages/contracts/deploy/L1/016-TimeLock.ts b/packages/contracts/deploy/L1/014-TimeLock.ts
similarity index 100%
rename from packages/contracts/deploy/L1/016-TimeLock.ts
rename to packages/contracts/deploy/L1/014-TimeLock.ts
diff --git a/packages/contracts/deploy/L1/017-UpgradeGovernor.ts b/packages/contracts/deploy/L1/015-UpgradeGovernor.ts
similarity index 100%
rename from packages/contracts/deploy/L1/017-UpgradeGovernor.ts
rename to packages/contracts/deploy/L1/015-UpgradeGovernor.ts
diff --git a/packages/contracts/deploy/L1/018-SecurityCouncil.ts b/packages/contracts/deploy/L1/016-SecurityCouncil.ts
similarity index 100%
rename from packages/contracts/deploy/L1/018-SecurityCouncil.ts
rename to packages/contracts/deploy/L1/016-SecurityCouncil.ts
diff --git a/packages/contracts/deploy/L1/008-AssetManager.ts b/packages/contracts/deploy/L1/018-AssetManager.ts
similarity index 96%
rename from packages/contracts/deploy/L1/008-AssetManager.ts
rename to packages/contracts/deploy/L1/018-AssetManager.ts
index 49a0c7c64..a57ba66f2 100644
--- a/packages/contracts/deploy/L1/008-AssetManager.ts
+++ b/packages/contracts/deploy/L1/018-AssetManager.ts
@@ -72,6 +72,6 @@ const deployFn: DeployFunction = async (hre) => {
   })
 }
 
-deployFn.tags = ['AssetManager', 'setup', 'l1']
+deployFn.tags = ['AssetManager', 'setup', 'l1', 'validatorSystemUpgrade']
 
 export default deployFn
diff --git a/packages/contracts/deploy/L1/009-ValidatorManager.ts b/packages/contracts/deploy/L1/019-ValidatorManager.ts
similarity index 97%
rename from packages/contracts/deploy/L1/009-ValidatorManager.ts
rename to packages/contracts/deploy/L1/019-ValidatorManager.ts
index 5069b02c3..c07d0ed4b 100644
--- a/packages/contracts/deploy/L1/009-ValidatorManager.ts
+++ b/packages/contracts/deploy/L1/019-ValidatorManager.ts
@@ -107,6 +107,6 @@ const deployFn: DeployFunction = async (hre) => {
   })
 }
 
-deployFn.tags = ['ValidatorManager', 'setup', 'l1']
+deployFn.tags = ['ValidatorManager', 'setup', 'l1', 'validatorSystemUpgrade']
 
 export default deployFn