chore: introduce solhint linter + fixes (#245)

* fix: bug id 6 * fix: bug id 7 + tests * fix: bug id 8 * fix: bug id 9 * fix: bug id 11 * fix: suppress linter * fix: revert prev commit, will be included in separate PR This reverts commit 34d6035. * fix: bug id 12 * fix: bug id 13 * fix: bug id 14 * chore: update abis + bindings * fix: missed two required strings * feat: introduce solhint json and integrate with ci * fix: linting issues for scripts dir * feat: updates and fixes * fix: nit * fix: nit * fix: nit * chore: update abis + bindings * chore: run solhint directly, don't use make target
primev · Jul 18, 2024 · 4d60ace · 4d60ace
1 parent 536d57e
commit 4d60ace
Show file tree

Hide file tree

Showing 19 changed files with 165 additions and 102 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -188,6 +188,9 @@ jobs:
 
       - name: Install Hardhat
         run: npm install -g hardhat
+
+      - name: Install solhint
+        run: npm install -g solhint
 
       - name: Install Dependencies
         run: npm install
@@ -198,6 +201,7 @@ jobs:
           node --version
           npm --version
           forge --version
+          solhint --version
 
       - name: Build
         run: npm run build --if-present
@@ -216,6 +220,10 @@ jobs:
           git diff --name-only --exit-code . || (echo "Generated files not in parity with the source files." && exit 1)
           git reset --hard HEAD
         working-directory: contracts-abi
+
+      - name: Run solhint solidity linter
+        run: solhint '**/*.sol'
+        working-directory: contracts
 
   infrastructure:
     uses: ./.github/workflows/infrastructure.yml

diff --git a/contracts-abi/abi/ValidatorRegistryV1.abi b/contracts-abi/abi/ValidatorRegistryV1.abi
@@ -122,16 +122,16 @@
             "type": "bool",
             "internalType": "bool"
           },
-          {
-            "name": "balance",
-            "type": "uint256",
-            "internalType": "uint256"
-          },
           {
             "name": "withdrawalAddress",
             "type": "address",
             "internalType": "address"
           },
+          {
+            "name": "balance",
+            "type": "uint256",
+            "internalType": "uint256"
+          },
           {
             "name": "unstakeHeight",
             "type": "tuple",
@@ -455,16 +455,16 @@
         "type": "bool",
         "internalType": "bool"
       },
-      {
-        "name": "balance",
-        "type": "uint256",
-        "internalType": "uint256"
-      },
       {
         "name": "withdrawalAddress",
         "type": "address",
         "internalType": "address"
       },
+      {
+        "name": "balance",
+        "type": "uint256",
+        "internalType": "uint256"
+      },
       {
         "name": "unstakeHeight",
         "type": "tuple",

diff --git a/contracts-abi/clients/ValidatorRegistryV1/ValidatorRegistryV1.go b/contracts-abi/clients/ValidatorRegistryV1/ValidatorRegistryV1.go
diff --git a/contracts/.solhint.json b/contracts/.solhint.json
@@ -0,0 +1,21 @@
+{
+  "extends": "solhint:recommended",
+  "rules": {
+    "max-line-length": ["error", 1000],
+    "func-visibility": ["error", { "ignoreConstructors": true }],
+    "modifier-name-mixedcase": "error",
+    "func-param-name-mixedcase": "error",
+    "gas-calldata-parameters": "error",
+    "gas-increment-by-one": "error",
+    "gas-struct-packing": "error",
+    "no-unused-vars": "error",
+    "interface-starts-with-i": "error",
+    "reason-string": "warn",
+    "private-vars-leading-underscore": "warn",
+    "gas-length-in-loops": "warn",
+    "gas-strict-inequalities": "warn",
+    "ordering": "warn",
+    "gas-indexed-events": "warn",
+    "gas-custom-errors": "warn"
+  }
+}
diff --git a/contracts/.solhintignore b/contracts/.solhintignore
@@ -0,0 +1,3 @@
+lib/
+test/
+node_modules/
diff --git a/contracts/Makefile b/contracts/Makefile
@@ -1,3 +1,10 @@
+solhint:
+	if !command -v solhint &>/dev/null; then \
+		echo "Install solhint with 'npm install -g solhint'"; \
+		exit 1; \
+	fi
+	solhint '**/*.sol'
+
 deploy-val-reg:
 	forge clean
 	forge script scripts/validator-registry/DeployValidatorRegistryV1.s.sol:DeployAnvil \

diff --git a/contracts/contracts/BlockTracker.sol b/contracts/contracts/BlockTracker.sol
@@ -76,7 +76,7 @@ contract BlockTracker is Ownable2StepUpgradeable, UUPSUpgradeable {
      * @param builderAddress The Ethereum address of the builder.
      */
     function addBuilderAddress(
-        string memory builderName,
+        string calldata builderName,
         address builderAddress
     ) external onlyOracle {
         blockBuilderNameToAddress[builderName] = builderAddress;

diff --git a/contracts/contracts/Oracle.sol b/contracts/contracts/Oracle.sol
@@ -6,8 +6,8 @@ import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/U
 
 import {PreConfCommitmentStore} from "./PreConfCommitmentStore.sol";
 import {IProviderRegistry} from "./interfaces/IProviderRegistry.sol";
-import {IPreConfCommitmentStore} from './interfaces/IPreConfCommitmentStore.sol';
-import {IBidderRegistry} from './interfaces/IBidderRegistry.sol';
+import {IPreConfCommitmentStore} from "./interfaces/IPreConfCommitmentStore.sol";
+import {IBidderRegistry} from "./interfaces/IBidderRegistry.sol";
 import {IBlockTracker} from "./interfaces/IBlockTracker.sol";
 
 /// @title Oracle Contract
@@ -50,7 +50,7 @@ contract Oracle is Ownable2StepUpgradeable, UUPSUpgradeable {
     /// @dev Reference to the BlockTracker contract interface.
     IBlockTracker private blockTrackerContract;
 
-    function _authorizeUpgrade(address) internal override onlyOwner {}
+    function _authorizeUpgrade(address) internal override onlyOwner {} // solhint-disable no-empty-blocks
 
     /**
      * @dev Initializes the contract with a PreConfirmations contract.

diff --git a/contracts/contracts/PreConfCommitmentStore.sol b/contracts/contracts/PreConfCommitmentStore.sol
@@ -503,7 +503,7 @@ contract PreConfCommitmentStore is Ownable2StepUpgradeable, UUPSUpgradeable {
             blockNumber
         );
 
-        commitmentsCount[commiterAddress] += 1;
+        ++commitmentsCount[commiterAddress];
 
         emit CommitmentStored(
             commitmentIndex,
@@ -619,7 +619,7 @@ contract PreConfCommitmentStore is Ownable2StepUpgradeable, UUPSUpgradeable {
         require(!commitment.isUsed, "Commitment already used");
 
         commitment.isUsed = true;
-        commitmentsCount[commitment.commiter] -= 1;
+        --commitmentsCount[commitment.commiter];
 
         uint256 windowToSettle = WindowFromBlockNumber.getWindowFromBlockNumber(
             commitment.blockNumber,
@@ -653,7 +653,7 @@ contract PreConfCommitmentStore is Ownable2StepUpgradeable, UUPSUpgradeable {
         );
 
         commitment.isUsed = true;
-        commitmentsCount[commitment.commiter] -= 1;
+        --commitmentsCount[commitment.commiter];
 
         bidderRegistry.retrieveFunds(
             windowToSettle,

diff --git a/contracts/contracts/interfaces/IValidatorRegistryV1.sol b/contracts/contracts/interfaces/IValidatorRegistryV1.sol
@@ -40,8 +40,8 @@ interface IValidatorRegistryV1 {
     /// @dev Struct representing a validator staked with the registry.
     struct StakedValidator {
         bool exists;
-        uint256 balance;
         address withdrawalAddress;
+        uint256 balance;
         EventHeightLib.EventHeight unstakeHeight;
     }
 

diff --git a/contracts/contracts/standard-bridge/Gateway.sol b/contracts/contracts/standard-bridge/Gateway.sol
@@ -27,7 +27,7 @@ abstract contract Gateway is Ownable2StepUpgradeable, UUPSUpgradeable {
     // The counterparty's finalization fee (wei), included for UX purposes
     uint256 public counterpartyFee;
 
-    function _authorizeUpgrade(address) internal override onlyOwner {}
+    function _authorizeUpgrade(address) internal override onlyOwner {} // solhint-disable no-empty-blocks
 
     function initiateTransfer(address _recipient, uint256 _amount
     ) external payable returns (uint256 returnIdx) {

diff --git a/contracts/contracts/validator-registry/avs/MevCommitAVS.sol b/contracts/contracts/validator-registry/avs/MevCommitAVS.sol
@@ -157,11 +157,11 @@ contract MevCommitAVS is IMevCommitAVS, MevCommitAVSStorage,
     }
 
     /// @dev Authorizes contract upgrades, restricted to contract owner.
-    function _authorizeUpgrade(address newImplementation) internal override onlyOwner { }
+    function _authorizeUpgrade(address newImplementation) internal override onlyOwner {} // solhint-disable no-empty-blocks
 
     /// @dev Registers an operator with the MevCommitAVS.
     function registerOperator (
-        ISignatureUtils.SignatureWithSaltAndExpiry memory operatorSignature
+        ISignatureUtils.SignatureWithSaltAndExpiry calldata operatorSignature
     ) external whenNotPaused() onlyNonRegisteredOperator() onlyEigenCoreOperator() {
         _registerOperator(operatorSignature);
     }
@@ -235,7 +235,7 @@ contract MevCommitAVS is IMevCommitAVS, MevCommitAVSStorage,
     }
 
     /// @dev Allows any account to unfreeze validators which have been frozen, for a fee.
-    function unfreeze(bytes[] calldata valPubKey) payable external 
+    function unfreeze(bytes[] calldata valPubKey) external payable
         whenNotPaused() onlyRegisteredValidators(valPubKey) onlyFrozenValidators(valPubKey) {
         uint256 requiredFee = unfreezeFee * valPubKey.length;
         require(msg.value >= requiredFee,
@@ -323,7 +323,7 @@ contract MevCommitAVS is IMevCommitAVS, MevCommitAVSStorage,
     }
 
     /// @dev Updates the eigenlayer metadata URI, restricted to contract owner.
-    function updateMetadataURI(string memory metadataURI_) external onlyOwner {
+    function updateMetadataURI(string calldata metadataURI_) external onlyOwner {
         _updateMetadataURI(metadataURI_);
     }
 
@@ -501,50 +501,50 @@ contract MevCommitAVS is IMevCommitAVS, MevCommitAVSStorage,
     }
 
     /// @dev Internal function to set the freeze oracle account.
-    function _setFreezeOracle(address _freezeOracle) internal {
-        freezeOracle = _freezeOracle;
-        emit FreezeOracleSet(_freezeOracle);
+    function _setFreezeOracle(address freezeOracle_) internal {
+        freezeOracle = freezeOracle_;
+        emit FreezeOracleSet(freezeOracle_);
     }
 
     /// @dev Internal function to set the unfreeze fee.
-    function _setUnfreezeFee(uint256 _unfreezeFee) internal {
-        unfreezeFee = _unfreezeFee;
-        emit UnfreezeFeeSet(_unfreezeFee);
+    function _setUnfreezeFee(uint256 unfreezeFee_) internal {
+        unfreezeFee = unfreezeFee_;
+        emit UnfreezeFeeSet(unfreezeFee_);
     }
 
     /// @dev Internal function to set the unfreeze receiver.
-    function _setUnfreezeReceiver(address _unfreezeReceiver) internal {
-        unfreezeReceiver = _unfreezeReceiver;
-        emit UnfreezeReceiverSet(_unfreezeReceiver);
+    function _setUnfreezeReceiver(address unfreezeReceiver_) internal {
+        unfreezeReceiver = unfreezeReceiver_;
+        emit UnfreezeReceiverSet(unfreezeReceiver_);
     }
 
     /// @dev Internal function to set the unfreeze period in blocks.
-    function _setUnfreezePeriodBlocks(uint256 _unfreezePeriodBlocks) internal {
-        unfreezePeriodBlocks = _unfreezePeriodBlocks;
-        emit UnfreezePeriodBlocksSet(_unfreezePeriodBlocks);
+    function _setUnfreezePeriodBlocks(uint256 unfreezePeriodBlocks_) internal {
+        unfreezePeriodBlocks = unfreezePeriodBlocks_;
+        emit UnfreezePeriodBlocksSet(unfreezePeriodBlocks_);
     }
 
     /// @dev Internal function to set the operator deregistration period in blocks.
-    function _setOperatorDeregPeriodBlocks(uint256 _operatorDeregPeriodBlocks) internal {
-        operatorDeregPeriodBlocks = _operatorDeregPeriodBlocks;
-        emit OperatorDeregPeriodBlocksSet(_operatorDeregPeriodBlocks);
+    function _setOperatorDeregPeriodBlocks(uint256 operatorDeregPeriodBlocks_) internal {
+        operatorDeregPeriodBlocks = operatorDeregPeriodBlocks_;
+        emit OperatorDeregPeriodBlocksSet(operatorDeregPeriodBlocks_);
     }
 
     /// @dev Internal function to set the validator deregistration period in blocks.
-    function _setValidatorDeregPeriodBlocks(uint256 _validatorDeregPeriodBlocks) internal {
-        validatorDeregPeriodBlocks = _validatorDeregPeriodBlocks;
-        emit ValidatorDeregPeriodBlocksSet(_validatorDeregPeriodBlocks);
+    function _setValidatorDeregPeriodBlocks(uint256 validatorDeregPeriodBlocks_) internal {
+        validatorDeregPeriodBlocks = validatorDeregPeriodBlocks_;
+        emit ValidatorDeregPeriodBlocksSet(validatorDeregPeriodBlocks_);
     }
 
     /// @dev Internal function to set the LST restaker deregistration period in blocks.
-    function _setLstRestakerDeregPeriodBlocks(uint256 _lstRestakerDeregPeriodBlocks) internal {
-        lstRestakerDeregPeriodBlocks = _lstRestakerDeregPeriodBlocks;
-        emit LSTRestakerDeregPeriodBlocksSet(_lstRestakerDeregPeriodBlocks);
+    function _setLstRestakerDeregPeriodBlocks(uint256 lstRestakerDeregPeriodBlocks_) internal {
+        lstRestakerDeregPeriodBlocks = lstRestakerDeregPeriodBlocks_;
+        emit LSTRestakerDeregPeriodBlocksSet(lstRestakerDeregPeriodBlocks_);
     }
 
     /// @dev Internal function to update the eigenlayer metadata URI.
-    function _updateMetadataURI(string memory _metadataURI) internal {
-        _eigenAVSDirectory.updateAVSMetadataURI(_metadataURI);
+    function _updateMetadataURI(string memory metadataURI_) internal {
+        _eigenAVSDirectory.updateAVSMetadataURI(metadataURI_);
     }
 
     /// @dev Returns the list of restakeable strategies.

diff --git a/contracts/contracts/validator-registry/avs/MevCommitAVSStorage.sol b/contracts/contracts/validator-registry/avs/MevCommitAVSStorage.sol
@@ -3,7 +3,6 @@ pragma solidity 0.8.20;
 
 
 import {IMevCommitAVS} from "../../interfaces/IMevCommitAVS.sol";
-import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
 import {IDelegationManager} from "eigenlayer-contracts/src/contracts/interfaces/IDelegationManager.sol";
 import {IEigenPodManager} from "eigenlayer-contracts/src/contracts/interfaces/IEigenPodManager.sol";
 import {IStrategyManager} from "eigenlayer-contracts/src/contracts/interfaces/IStrategyManager.sol";

diff --git a/contracts/scripts/DeployScripts.s.sol b/contracts/scripts/DeployScripts.s.sol
@@ -1,13 +1,19 @@
 // SPDX-License-Identifier: BSL 1.1
+
+// solhint-disable no-console
+// solhint-disable one-contract-per-file
+
 pragma solidity 0.8.20;
-import "forge-std/Script.sol";
-import "../contracts/BidderRegistry.sol";
-import "../contracts/ProviderRegistry.sol";
-import "../contracts/PreConfCommitmentStore.sol";
-import "../contracts/Oracle.sol";
-import "../contracts/Whitelist.sol";
+
+import {Script} from "forge-std/Script.sol";
+import {BidderRegistry} from "../contracts/BidderRegistry.sol";
+import {ProviderRegistry} from "../contracts/ProviderRegistry.sol";
+import {PreConfCommitmentStore} from "../contracts/PreConfCommitmentStore.sol";
+import {Oracle} from "../contracts/Oracle.sol";
+import {Whitelist} from "../contracts/Whitelist.sol";
 import {Upgrades} from "openzeppelin-foundry-upgrades/Upgrades.sol";
-import "../contracts/BlockTracker.sol";
+import {BlockTracker} from "../contracts/BlockTracker.sol";
+import {console} from "forge-std/console.sol";
 
 // Deploys core contracts
 contract DeployScript is Script {
@@ -90,7 +96,7 @@ contract DeployWhitelist is Script {
         address hypERC20Addr = vm.envAddress("HYP_ERC20_ADDR");
         require(
             hypERC20Addr != address(0),
-            "Address to whitelist not provided"
+            "hypERC20 addr not provided"
         );
 
         address whitelistProxy = Upgrades.deployUUPSProxy(

diff --git a/contracts/scripts/DeployStandardBridge.s.sol b/contracts/scripts/DeployStandardBridge.s.sol
@@ -1,11 +1,17 @@
 // SPDX-License-Identifier: BSL 1.1
+
+// solhint-disable no-console
+// solhint-disable one-contract-per-file
+
 pragma solidity 0.8.20;
-import "forge-std/Script.sol";
+
+import {Script} from "forge-std/Script.sol";
 import {SettlementGateway} from "../contracts/standard-bridge/SettlementGateway.sol";
 import {L1Gateway} from "../contracts/standard-bridge/L1Gateway.sol";
 import {Whitelist} from "../contracts/Whitelist.sol";
 import {Strings} from "@openzeppelin/contracts/utils/Strings.sol";
 import {Upgrades} from "openzeppelin-foundry-upgrades/Upgrades.sol";
+import {console} from "forge-std/console.sol";
 
 contract DeploySettlementGateway is Script {
     function run() external {
@@ -39,11 +45,11 @@ contract DeploySettlementGateway is Script {
         console.log("Settlement gateway has been whitelisted. Gateway contract address:", address(gateway));
 
         string memory jsonOutput = string.concat(
-            '{"settlement_gateway_addr": "',
+            "{'settlement_gateway_addr': '",
             Strings.toHexString(address(gateway)),
-            '", "whitelist_addr": "',
+            "', 'whitelist_addr': '",
             Strings.toHexString(address(whitelist)),
-            '"}'
+            "'}"
         );
         console.log("JSON_DEPLOY_ARTIFACT:", jsonOutput); 
 
@@ -70,9 +76,9 @@ contract DeployL1Gateway is Script {
             address(gateway));
 
         string memory jsonOutput = string.concat(
-            '{"l1_gateway_addr": "',
+            "{'l1_gateway_addr': '",
             Strings.toHexString(address(gateway)),
-            '"}'
+            "'}"
         );
         console.log("JSON_DEPLOY_ARTIFACT:", jsonOutput);