Enable Solhint (#10869)

* enable solhint for /shared/ folder

add others to ignore file
add solhint to CICD

* add chainlink-solidity ruleset

* reduce ignore scope & fix interface folder issues

* rm unused imports

* clean /libraries

* set no-unused-import to error

* allow 968 warnings, the number we currently have

* fix second layer imports

* contracts/v0.8: fix solhint warnings for vrf contracts (#10875)

* rm no-inline-assembly rule

---------

Co-authored-by: Makram <makramkd@users.noreply.github.com>

.github/workflows/solidity.yml

@@ -106,6 +106,8 @@ jobs:
         uses: ./.github/actions/setup-nodejs
       - name: Run pnpm lint
         run: pnpm lint
+      - name: Run solhint
+        run: pnpm solhint
       - name: Collect Metrics
         id: collect-gha-metrics
         uses: smartcontractkit/push-gha-metrics-action@d2c2b7bdc9012651230b2608a1bcb0c48538b6ec

CODEOWNERS

@@ -67,12 +67,12 @@ core/scripts/gateway @bolekk @pinebit
 /operator-ui/ @DeividasK @jkongie
 
 # Contracts
-/contracts/ @se3000 @connorwstein
+/contracts/ @se3000 @connorwstein @RensR
 /contracts/**/*Keeper* @smartcontractkit/keepers
 /contracts/**/*Upkeep* @smartcontractkit/keepers
 /contracts/**/*Functions* @smartcontractkit/functions
 /contracts/src/v0.8/functions @smartcontractkit/functions
-contracts/test/v0.8/functions @smartcontractkit/functions
+/contracts/test/v0.8/functions @smartcontractkit/functions
 /contracts/src/v0.8/llo-feeds @austinborn @Fletch153
 
 # Tests

contracts/.solhint.json

@@ -1,7 +1,42 @@
 {
 	"extends": "solhint:recommended",
-	"plugins": ["prettier"],
+	"plugins": ["prettier", "chainlink-solidity"],
 	"rules": {
-	  "prettier/prettier": "error"
+		"compiler-version": ["off", "^0.8.0"],
+		"const-name-snakecase": "off",
+		"constructor-syntax": "error",
+		"var-name-mixedcase": "off",
+		"func-named-parameters": "off",
+		"immutable-vars-naming": "off",
+		"no-inline-assembly": "off",
+		"no-unused-import": "error",
+		"func-visibility": [
+			"error",
+			{
+				"ignoreConstructors": true
+			}
+		],
+		"not-rely-on-time": "off",
+		"prettier/prettier": [
+			"off",
+			{
+				"endOfLine": "auto"
+			}
+		],
+		"no-empty-blocks": "off",
+		"quotes": ["error", "double"],
+		"reason-string": [
+			"warn",
+			{
+				"maxLength": 64
+			}
+		],
+		"chainlink-solidity/prefix-internal-functions-with-underscore": "warn",
+		"chainlink-solidity/prefix-private-functions-with-underscore": "warn",
+		"chainlink-solidity/prefix-storage-variables-with-s-underscore": "warn",
+		"chainlink-solidity/prefix-immutable-variables-with-i": "warn",
+		"chainlink-solidity/all-caps-constant-storage-variables": "warn",
+		"chainlink-solidity/no-hardhat-imports": "warn",
+		"chainlink-solidity/inherited-constructor-args-not-in-contract-definition": "warn"
 	}
 }

contracts/.solhintignore

@@ -1 +1,32 @@
-node_modules/
+# 368 warnings
+#./src/v0.8/automation
+# 302 warnings
+#./src/v0.8/dev
+# 91 warnings
+#./src/v0.8/functions
+# 91 warnings
+#./src/v0.8/l2ep/dev
+# 116 warnings
+#./src/v0.8/vrf
+
+# Temp ignore the following files as they contain issues.
+./src/v0.8/ChainlinkClient.sol
+./src/v0.8/Flags.sol
+./src/v0.8/KeepersVRFConsumer.sol
+./src/v0.8/PermissionedForwardProxy.sol
+./src/v0.8/ValidatorProxy.sol
+./src/v0.8/Chainlink.sol
+./src/v0.8/ChainSpecificUtil.sol
+
+
+# Ignore tests, this should not be the long term plan but is OK in the short term
+./src/v0.8/**/*.t.sol
+./src/v0.8/mocks
+./src/v0.8/tests
+./src/v0.8/llo-feeds/test
+./src/v0.8/vrf/testhelpers
+./src/v0.8/functions/tests
+
+# Always ignore vendor
+./src/v0.8/vendor
+./node_modules/

contracts/package.json

@@ -17,7 +17,8 @@
     "coverage": "hardhat coverage",
     "prepublishOnly": "pnpm compile && ./scripts/prepublish_generate_abi_folder",
     "publish-beta": "pnpm publish --tag beta",
-    "publish-prod": "npm dist-tag add @chainlink/contracts@0.8.0 latest"
+    "publish-prod": "npm dist-tag add @chainlink/contracts@0.8.0 latest",
+    "solhint": "solhint --max-warnings 593 \"./src/v0.8/**/*.sol\""
   },
   "files": [
     "src/",
@@ -73,6 +74,7 @@
     "prettier-plugin-solidity": "1.1.3",
     "rlp": "^2.2.7",
     "solhint": "^3.6.2",
+    "solhint-plugin-chainlink-solidity": "git+https://github.com/smartcontractkit/chainlink-solhint-rules.git",
     "solhint-plugin-prettier": "^0.0.5",
     "solidity-coverage": "^0.8.4",
     "ts-node": "^10.9.1",

contracts/scripts/native_solc_compile_all_vrf

@@ -54,6 +54,7 @@ compileContract vrf/testhelpers/VRFCoordinatorV2TestHelper.sol
 compileContractAltOpts vrf/VRFCoordinatorV2.sol 10000
 compileContract mocks/VRFCoordinatorV2Mock.sol
 compileContract vrf/VRFOwner.sol
+compileContract dev/VRFSubscriptionBalanceMonitor.sol
 
 # VRF V2Plus
 compileContract dev/interfaces/IVRFCoordinatorV2PlusInternal.sol

contracts/src/v0.8/automation/KeeperBase.sol

@@ -3,4 +3,5 @@
  * @notice This is a deprecated interface. Please use AutomationBase directly.
  */
 pragma solidity ^0.8.0;
+// solhint-disable-next-line no-unused-import
 import {AutomationBase as KeeperBase} from "./AutomationBase.sol";

contracts/src/v0.8/automation/KeeperCompatible.sol

@@ -3,6 +3,9 @@
  * @notice This is a deprecated interface. Please use AutomationCompatible directly.
  */
 pragma solidity ^0.8.0;
+// solhint-disable-next-line no-unused-import
 import {AutomationCompatible as KeeperCompatible} from "./AutomationCompatible.sol";
+// solhint-disable-next-line no-unused-import
 import {AutomationBase as KeeperBase} from "./AutomationBase.sol";
+// solhint-disable-next-line no-unused-import
 import {AutomationCompatibleInterface as KeeperCompatibleInterface} from "./interfaces/AutomationCompatibleInterface.sol";

contracts/src/v0.8/automation/interfaces/KeeperCompatibleInterface.sol

@@ -3,4 +3,5 @@
  * @notice This is a deprecated interface. Please use AutomationCompatibleInterface directly.
  */
 pragma solidity ^0.8.0;
+// solhint-disable-next-line no-unused-import
 import {AutomationCompatibleInterface as KeeperCompatibleInterface} from "./AutomationCompatibleInterface.sol";

contracts/src/v0.8/automation/interfaces/v1_2/KeeperRegistryInterface1_2.sol

@@ -3,7 +3,11 @@
  * @notice This is a deprecated interface. Please use AutomationRegistryInterface1_2 directly.
  */
 pragma solidity ^0.8.0;
+// solhint-disable-next-line no-unused-import
 import {Config, State} from "./AutomationRegistryInterface1_2.sol";
+// solhint-disable-next-line no-unused-import
 import {AutomationRegistryBaseInterface as KeeperRegistryBaseInterface} from "./AutomationRegistryInterface1_2.sol";
+// solhint-disable-next-line no-unused-import
 import {AutomationRegistryInterface as KeeperRegistryInterface} from "./AutomationRegistryInterface1_2.sol";
+// solhint-disable-next-line no-unused-import
 import {AutomationRegistryExecutableInterface as KeeperRegistryExecutableInterface} from "./AutomationRegistryInterface1_2.sol";

contracts/src/v0.8/automation/v1_3/KeeperRegistry1_3.sol

@@ -6,7 +6,7 @@ import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
 import "@openzeppelin/contracts/utils/Address.sol";
 import "./KeeperRegistryBase1_3.sol";
 import "./KeeperRegistryLogic1_3.sol";
-import {AutomationRegistryExecutableInterface} from "../interfaces/v1_3/AutomationRegistryInterface1_3.sol";
+import {AutomationRegistryExecutableInterface, State} from "../interfaces/v1_3/AutomationRegistryInterface1_3.sol";
 import "../interfaces/MigratableKeeperRegistryInterface.sol";
 import "../../interfaces/TypeAndVersionInterface.sol";
 import "../../shared/interfaces/IERC677Receiver.sol";

contracts/src/v0.8/automation/v1_3/KeeperRegistryBase1_3.sol

@@ -6,7 +6,7 @@ import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
 import "../../vendor/@arbitrum/nitro-contracts/src/precompiles/ArbGasInfo.sol";
 import "../../vendor/@eth-optimism/contracts/v0.8.6/contracts/L2/predeploys/OVM_GasPriceOracle.sol";
 import "../ExecutionPrevention.sol";
-import {Config, State, Upkeep} from "../interfaces/v1_3/AutomationRegistryInterface1_3.sol";
+import {Config, Upkeep} from "../interfaces/v1_3/AutomationRegistryInterface1_3.sol";
 import "../../shared/access/ConfirmedOwner.sol";
 import "../../interfaces/AggregatorV3Interface.sol";
 import "../../shared/interfaces/LinkTokenInterface.sol";

contracts/src/v0.8/automation/v2_0/KeeperRegistry2_0.sol

@@ -5,7 +5,7 @@ import "../../vendor/openzeppelin-solidity/v4.7.3/contracts/proxy/Proxy.sol";
 import "../../vendor/openzeppelin-solidity/v4.7.3/contracts/utils/structs/EnumerableSet.sol";
 import "../../vendor/openzeppelin-solidity/v4.7.3/contracts/utils/Address.sol";
 import "./KeeperRegistryBase2_0.sol";
-import {AutomationRegistryExecutableInterface, UpkeepInfo} from "../interfaces/v2_0/AutomationRegistryInterface2_0.sol";
+import {AutomationRegistryExecutableInterface, UpkeepInfo, State, OnchainConfig, UpkeepFailureReason} from "../interfaces/v2_0/AutomationRegistryInterface2_0.sol";
 import "../interfaces/MigratableKeeperRegistryInterface.sol";
 import "../interfaces/MigratableKeeperRegistryInterfaceV2.sol";
 import "../../shared/interfaces/IERC677Receiver.sol";

contracts/src/v0.8/automation/v2_0/KeeperRegistryBase2_0.sol

@@ -6,7 +6,6 @@ import "../../vendor/@arbitrum/nitro-contracts/src/precompiles/ArbGasInfo.sol";
 import "../../vendor/@eth-optimism/contracts/v0.8.6/contracts/L2/predeploys/OVM_GasPriceOracle.sol";
 import {ArbSys} from "../../vendor/@arbitrum/nitro-contracts/src/precompiles/ArbSys.sol";
 import "../ExecutionPrevention.sol";
-import {OnchainConfig, State, UpkeepFailureReason} from "../interfaces/v2_0/AutomationRegistryInterface2_0.sol";
 import "../../shared/access/ConfirmedOwner.sol";
 import "../../interfaces/AggregatorV3Interface.sol";
 import "../../shared/interfaces/LinkTokenInterface.sol";

contracts/src/v0.8/automation/v2_0/KeeperRegistryLogic2_0.sol

@@ -4,6 +4,7 @@ pragma solidity 0.8.6;
 import "../../vendor/openzeppelin-solidity/v4.7.3/contracts/utils/structs/EnumerableSet.sol";
 import "../../vendor/openzeppelin-solidity/v4.7.3/contracts/utils/Address.sol";
 import "./KeeperRegistryBase2_0.sol";
+import {UpkeepFailureReason} from "../interfaces/v2_0/AutomationRegistryInterface2_0.sol";
 import "../interfaces/MigratableKeeperRegistryInterfaceV2.sol";
 import "../interfaces/UpkeepTranscoderInterfaceV2.sol";
 

contracts/src/v0.8/automation/v2_1/AutomationUtils2_1.sol

@@ -2,7 +2,7 @@
 pragma solidity 0.8.16;
 
 import {KeeperRegistryBase2_1} from "./KeeperRegistryBase2_1.sol";
-import {ILogAutomation, Log} from "../interfaces/ILogAutomation.sol";
+import {Log} from "../interfaces/ILogAutomation.sol";
 
 /**
  * @notice this file exposes structs that are otherwise internal to the automation registry

contracts/src/v0.8/automation/v2_1/KeeperRegistry2_1.sol

@@ -3,7 +3,6 @@ pragma solidity 0.8.16;
 
 import {EnumerableSet} from "../../vendor/openzeppelin-solidity/v4.7.3/contracts/utils/structs/EnumerableSet.sol";
 import {Address} from "../../vendor/openzeppelin-solidity/v4.7.3/contracts/utils/Address.sol";
-import {Proxy} from "../../vendor/openzeppelin-solidity/v4.7.3/contracts/proxy/Proxy.sol";
 import {KeeperRegistryBase2_1} from "./KeeperRegistryBase2_1.sol";
 import {KeeperRegistryLogicB2_1} from "./KeeperRegistryLogicB2_1.sol";
 import {Chainable} from "../Chainable.sol";

contracts/src/v0.8/automation/v2_1/KeeperRegistryBase2_1.sol

@@ -14,7 +14,7 @@ import {ConfirmedOwner} from "../../shared/access/ConfirmedOwner.sol";
 import {AggregatorV3Interface} from "../../interfaces/AggregatorV3Interface.sol";
 import {LinkTokenInterface} from "../../shared/interfaces/LinkTokenInterface.sol";
 import {KeeperCompatibleInterface} from "../interfaces/KeeperCompatibleInterface.sol";
-import {UpkeepTranscoderInterface, UpkeepFormat} from "../interfaces/UpkeepTranscoderInterface.sol";
+import {UpkeepFormat} from "../interfaces/UpkeepTranscoderInterface.sol";
 
 /**
  * @notice Base Keeper Registry contract, contains shared logic between

contracts/src/v0.8/automation/v2_1/UpkeepTranscoder4_0.sol

@@ -6,7 +6,6 @@ import {UpkeepTranscoderInterfaceV2} from "../interfaces/UpkeepTranscoderInterfa
 import {TypeAndVersionInterface} from "../../interfaces/TypeAndVersionInterface.sol";
 import {KeeperRegistryBase2_1 as R21} from "./KeeperRegistryBase2_1.sol";
 import {IAutomationForwarder} from "../interfaces/IAutomationForwarder.sol";
-import {AutomationRegistryBaseInterface, UpkeepInfo} from "../interfaces/v2_0/AutomationRegistryInterface2_0.sol";
 
 enum RegistryVersion {
   V12,

contracts/src/v0.8/dev/BlockhashStore.sol

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: MIT
 pragma solidity 0.8.6;
 
-import "../ChainSpecificUtil.sol";
+import {ChainSpecificUtil} from "../ChainSpecificUtil.sol";
 
 /**
  * @title BlockhashStore
@@ -14,14 +14,15 @@ import "../ChainSpecificUtil.sol";
  *   would have to be deployed.
  */
 contract BlockhashStore {
-  mapping(uint => bytes32) internal s_blockhashes;
+  mapping(uint256 => bytes32) internal s_blockhashes;
 
   /**
    * @notice stores blockhash of a given block, assuming it is available through BLOCKHASH
    * @param n the number of the block whose blockhash should be stored
    */
   function store(uint256 n) public {
     bytes32 h = ChainSpecificUtil.getBlockhash(uint64(n));
+    // solhint-disable-next-line custom-errors
     require(h != 0x0, "blockhash(n) failed");
     s_blockhashes[n] = h;
   }
@@ -40,6 +41,7 @@ contract BlockhashStore {
    *   that it hashes to a stored blockhash, and then extract parentHash to get the n-th blockhash.
    */
   function storeVerifyHeader(uint256 n, bytes memory header) public {
+    // solhint-disable-next-line custom-errors
     require(keccak256(header) == s_blockhashes[n + 1], "header has unknown blockhash");
 
     // At this point, we know that header is the correct blockheader for block n+1.
@@ -72,6 +74,7 @@ contract BlockhashStore {
    */
   function getBlockhash(uint256 n) external view returns (bytes32) {
     bytes32 h = s_blockhashes[n];
+    // solhint-disable-next-line custom-errors
     require(h != 0x0, "blockhash not found in store");
     return h;
   }

contracts/src/v0.8/dev/VRFConsumerBaseV2Upgradeable.sol

@@ -96,7 +96,7 @@ pragma solidity ^0.8.4;
  * @dev and so remains effective only in the case of unmodified oracle software).
  */
 
-import "@openzeppelin/contracts-upgradeable-4.7.3/proxy/utils/Initializable.sol";
+import {Initializable} from "@openzeppelin/contracts-upgradeable-4.7.3/proxy/utils/Initializable.sol";
 
 /**
  * @dev The VRFConsumerBaseV2Upgradable is an upgradable variant of VRFConsumerBaseV2
@@ -106,19 +106,23 @@ import "@openzeppelin/contracts-upgradeable-4.7.3/proxy/utils/Initializable.sol"
  */
 abstract contract VRFConsumerBaseV2Upgradeable is Initializable {
   error OnlyCoordinatorCanFulfill(address have, address want);
+  // solhint-disable-next-line chainlink-solidity/prefix-storage-variables-with-s-underscore
   address private vrfCoordinator;
 
   // See https://github.com/OpenZeppelin/openzeppelin-sdk/issues/37.
   // Each uint256 covers a single storage slot, see https://docs.soliditylang.org/en/latest/internals/layout_in_storage.html.
+  // solhint-disable-next-line chainlink-solidity/prefix-storage-variables-with-s-underscore
   uint256[49] private __gap;
 
   /**
    * @param _vrfCoordinator the VRFCoordinatorV2 address.
    * @dev See https://docs.chain.link/docs/vrf/v2/supported-networks/ for coordinator
    * @dev addresses on your preferred network.
    */
+  // solhint-disable-next-line func-name-mixedcase
   function __VRFConsumerBaseV2_init(address _vrfCoordinator) internal onlyInitializing {
     if (_vrfCoordinator == address(0)) {
+      // solhint-disable-next-line custom-errors
       revert("must give valid coordinator address");
     }
 
@@ -139,6 +143,7 @@ abstract contract VRFConsumerBaseV2Upgradeable is Initializable {
    * @param requestId The Id initially returned by requestRandomness
    * @param randomWords the VRF output expanded to the requested number of words
    */
+  // solhint-disable-next-line chainlink-solidity/prefix-internal-functions-with-underscore
   function fulfillRandomWords(uint256 requestId, uint256[] memory randomWords) internal virtual;
 
   // rawFulfillRandomness is called by VRFCoordinator when it receives a valid VRF

contracts/src/v0.8/dev/VRFSubscriptionBalanceMonitor.sol

@@ -2,11 +2,11 @@
 
 pragma solidity 0.8.6;
 
-import "../shared/access/ConfirmedOwner.sol";
-import "../automation/interfaces/KeeperCompatibleInterface.sol";
-import "../interfaces/VRFCoordinatorV2Interface.sol";
-import "../shared/interfaces/LinkTokenInterface.sol";
-import "@openzeppelin/contracts/security/Pausable.sol";
+import {ConfirmedOwner} from "../shared/access/ConfirmedOwner.sol";
+import {AutomationCompatibleInterface as KeeperCompatibleInterface} from "../automation/interfaces/AutomationCompatibleInterface.sol";
+import {VRFCoordinatorV2Interface} from "../interfaces/VRFCoordinatorV2Interface.sol";
+import {LinkTokenInterface} from "../shared/interfaces/LinkTokenInterface.sol";
+import {Pausable} from "@openzeppelin/contracts/security/Pausable.sol";
 
 /**
  * @title The VRFSubscriptionBalanceMonitor contract.
@@ -197,6 +197,7 @@ contract VRFSubscriptionBalanceMonitor is ConfirmedOwner, Pausable, KeeperCompat
    * @param payee the address to pay
    */
   function withdraw(uint256 amount, address payable payee) external onlyOwner {
+    // solhint-disable-next-line custom-errors, reason-string
     require(payee != address(0));
     emit FundsWithdrawn(amount, payee);
     LINKTOKEN.transfer(payee, amount);
@@ -206,6 +207,7 @@ contract VRFSubscriptionBalanceMonitor is ConfirmedOwner, Pausable, KeeperCompat
    * @notice Sets the LINK token address.
    */
   function setLinkTokenAddress(address linkTokenAddress) public onlyOwner {
+    // solhint-disable-next-line custom-errors, reason-string
     require(linkTokenAddress != address(0));
     emit LinkTokenAddressUpdated(address(LINKTOKEN), linkTokenAddress);
     LINKTOKEN = LinkTokenInterface(linkTokenAddress);
@@ -215,6 +217,7 @@ contract VRFSubscriptionBalanceMonitor is ConfirmedOwner, Pausable, KeeperCompat
    * @notice Sets the VRF coordinator address.
    */
   function setVRFCoordinatorV2Address(address coordinatorAddress) public onlyOwner {
+    // solhint-disable-next-line custom-errors, reason-string
     require(coordinatorAddress != address(0));
     emit VRFCoordinatorV2AddressUpdated(address(COORDINATOR), coordinatorAddress);
     COORDINATOR = VRFCoordinatorV2Interface(coordinatorAddress);
@@ -224,6 +227,7 @@ contract VRFSubscriptionBalanceMonitor is ConfirmedOwner, Pausable, KeeperCompat
    * @notice Sets the keeper registry address.
    */
   function setKeeperRegistryAddress(address keeperRegistryAddress) public onlyOwner {
+    // solhint-disable-next-line custom-errors, reason-string
     require(keeperRegistryAddress != address(0));
     emit KeeperRegistryAddressUpdated(s_keeperRegistryAddress, keeperRegistryAddress);
     s_keeperRegistryAddress = keeperRegistryAddress;