chore(contract): applied new solhint rules across l1, l2, and system contracts #473

koloz193 · 2024-05-15T19:09:20Z

What ❔

In an effort to make sure our contracts are held up to the highest standards, we applied new solhint rules and fixed the contracts accordingly. Note that while l2 and system contracts have been converted from string based reverts to custom errors, the changes for L1 contracts will come in a separate PR.

Why ❔

Checklist

PR title corresponds to the body of PR (we generate changelog entries from PRs).
Tests for the changes have been added / updated.
Documentation comments have been added / updated.

…lhint-rules

dnkolegov

Great work @koloz193 !

vladbochok · 2024-05-21T15:31:51Z

l1-contracts/contracts/state-transition/chain-deps/facets/Executor.sol

@@ -212,7 +215,7 @@ contract ExecutorFacet is ZkSyncHyperchainBase, IExecutor {

    /// @inheritdoc IExecutor
    function commitBatches(
-        StoredBatchInfo memory _lastCommittedBatchData,
+        StoredBatchInfo calldata _lastCommittedBatchData,


@koloz193 Can you please check what is the gas differences for your changes?

i updated the comment on #477 with the report

vladbochok · 2024-05-21T15:33:39Z

l1-contracts/contracts/state-transition/chain-deps/facets/Mailbox.sol

@@ -34,7 +36,7 @@ contract MailboxFacet is ZkSyncHyperchainBase, IMailbox {
    string public constant override getName = "MailboxFacet";

    /// @dev Era's chainID
-    uint256 immutable ERA_CHAIN_ID;
+    uint256 public immutable ERA_CHAIN_ID;


It shouldn't be public. This is diamond proxy facet, if you make the function public then it will be a imported as a getter for proxy that is unexpected to be a part of Mailbox and not Getters

ill add it to the changes for #477

vladbochok · 2024-05-21T15:37:38Z

l2-contracts/contracts/bridge/L2StandardERC20.sol

@@ -158,29 +167,29 @@ contract L2StandardERC20 is ERC20PermitUpgradeable, IL2StandardToken, ERC1967Upg

    function name() public view override returns (string memory) {
        // If method is not available, behave like a token that does not implement this method - revert on call.
-        if (availableGetters.ignoreName) revert();
+        if (availableGetters.ignoreName) revert Unimplemented();


No, there are should be an empty revert, similar if method wasn't implemented at all

fixed it in #505

vladbochok · 2024-05-21T15:37:43Z

l2-contracts/contracts/bridge/L2StandardERC20.sol

        return super.name();
    }

    function symbol() public view override returns (string memory) {
        // If method is not available, behave like a token that does not implement this method - revert on call.
-        if (availableGetters.ignoreSymbol) revert();
+        if (availableGetters.ignoreSymbol) revert Unimplemented();


fixed it in #505

vladbochok · 2024-05-21T15:37:48Z

l2-contracts/contracts/bridge/L2StandardERC20.sol

        return super.symbol();
    }

    function decimals() public view override returns (uint8) {
        // If method is not available, behave like a token that does not implement this method - revert on call.
-        if (availableGetters.ignoreDecimals) revert();
+        if (availableGetters.ignoreDecimals) revert Unimplemented();


fixed it in #505

vladbochok · 2024-06-08T11:20:31Z

system-contracts/contracts/Compressor.sol

-                "Encoded data length should be 4 times shorter than the original bytecode"
-            );
+            if (encodedData.length * 4 != _bytecode.length) {
+                revert MalformedBytecode(BytecodeError.Length);


Should be different error for different error reasons.

vladbochok · 2024-06-08T11:22:23Z

system-contracts/contracts/Compressor.sol


                uint64 encodedChunk = dictionary.readUint64(indexOfEncodedChunk);
                uint64 realChunk = _bytecode.readUint64(encodedDataPointer * 4);

-                require(encodedChunk == realChunk, "Encoded chunk does not match the original bytecode");
+                if (encodedChunk != realChunk) {
+                    revert ValuesNotEqual(realChunk, encodedChunk);


Which values? need to make a specific error for compressor encoded chunks rather than reuse ValuesNotEqual imho

vladbochok · 2024-06-08T11:22:46Z

system-contracts/contracts/Compressor.sol


            bytes32 derivedKey = stateDiff.readBytes32(52);
            uint256 initValue = stateDiff.readUint256(92);
            uint256 finalValue = stateDiff.readUint256(124);
-            require(derivedKey == _compressedStateDiffs.readBytes32(stateDiffPtr), "iw: initial key mismatch");
+            if (derivedKey != _compressedStateDiffs.readBytes32(stateDiffPtr)) {
+                revert ValuesNotEqual(uint256(derivedKey), _compressedStateDiffs.readUint256(stateDiffPtr));


vladbochok · 2024-06-08T11:26:11Z

system-contracts/contracts/ContractDeployer.sol

-            "It is only possible to change from sequential to arbitrary ordering"
-        );
+        if (
+            _nonceOrdering != AccountNonceOrdering.Arbitrary &&


Should be | | instead of && here.

Counterexample for mismatch between new and old code:

_nonceOrdering != AccountNonceOrdering.Arbitrary

currentInfo.nonceOrdering == AccountNonceOrdering.Sequential

Will fail in the old code and not in the current code.

vladbochok · 2024-06-08T11:28:22Z

system-contracts/contracts/DefaultAccount.sol

@@ -165,7 +168,9 @@ contract DefaultAccount is IAccount {
    /// @param _signature The signature of the transaction.
    /// @return EIP1271_SUCCESS_RETURN_VALUE if the signature is correct. It reverts otherwise.
    function _isValidSignature(bytes32 _hash, bytes memory _signature) internal view returns (bool) {
-        require(_signature.length == 65, "Signature length is incorrect");
+        if (_signature.length != 65) {
+            revert InvalidSig(SigField.Length, _signature.length);


InvalidSigLength?

vladbochok · 2024-06-08T11:29:00Z

system-contracts/contracts/GasBoundCaller.sol

@@ -79,7 +82,9 @@ contract GasBoundCaller {
        if (pubdataGas != 0) {
            // Here we double check that the additional cost is not higher than the maximum allowed.
            // Note, that the `gasleft()` can be spent on pubdata too.
-            require(pubdataAllowance + gasleft() >= pubdataGas + CALL_RETURN_OVERHEAD, "Not enough gas for pubdata");
+            if (pubdataAllowance + gasleft() < pubdataGas + CALL_RETURN_OVERHEAD) {
+                revert InsufficientGas();


Should be two differnt errors

vladbochok · 2024-06-08T11:33:37Z

system-contracts/contracts/libraries/EfficientCall.sol

@@ -36,7 +37,9 @@ library EfficientCall {
    /// @return The `keccak256` hash.
    function keccak(bytes calldata _data) internal view returns (bytes32) {
        bytes memory returnData = staticCall(gasleft(), KECCAK256_SYSTEM_CONTRACT, _data);
-        require(returnData.length == 32, "keccak256 returned invalid data");
+        if (returnData.length != 32) {
+            revert InvalidData();


Confusing error, it should Keccak256InvalidReturnData or at least InvalidReturnData

vladbochok · 2024-06-08T11:33:51Z

system-contracts/contracts/libraries/EfficientCall.sol

@@ -45,7 +48,9 @@ library EfficientCall {
    /// @return The `sha256` hash.
    function sha(bytes calldata _data) internal view returns (bytes32) {
        bytes memory returnData = staticCall(gasleft(), SHA256_SYSTEM_CONTRACT, _data);
-        require(returnData.length == 32, "sha returned invalid data");
+        if (returnData.length != 32) {
+            revert InvalidData();


vladbochok

This is huge progress, well done! We need to polish it a bit

…contracts (matter-labs#473)

koloz193 added 3 commits May 8, 2024 15:08

update solhint rules and code for l1 contracts

88f8ff7

fixed solhint issues in l1 scripts

1515917

added l2 and sc custom errors

2dfb996

koloz193 requested review from vladbochok and StanislavBreadless as code owners May 15, 2024 19:09

koloz193 added 12 commits May 15, 2024 15:10

spell fix

3d18057

rename file

9695d9f

spelling

ad3d2f1

fix revert

ab0894a

Merge branch 'dev' into zk-evm-508-testing-solidity-code-style-and-so…

be9073e

…lhint-rules

lint fixes

1f94d39

update slither ignore

b0e4f9b

remove unused imports

06950a2

Merge branch 'dev' into zk-evm-508-testing-solidity-code-style-and-so…

3832cce

…lhint-rules

Merge branch 'dev' into zk-evm-508-testing-solidity-code-style-and-so…

5575ab4

…lhint-rules

calculate hashes

212403e

tests pass with custom errors

7631871

koloz193 requested a review from dnkolegov May 16, 2024 16:17

dnkolegov approved these changes May 16, 2024

View reviewed changes

koloz193 merged commit 994897b into dev May 16, 2024
22 checks passed

koloz193 deleted the zk-evm-508-testing-solidity-code-style-and-solhint-rules branch May 16, 2024 23:27

This was referenced May 16, 2024

chore(l1-contracts): Migrate L1 Contracts to Custom Errors #476

Closed

chore(l1-contracts): Migrate L1 Contracts to Custom Errors #477

Merged

vladbochok reviewed May 21, 2024

View reviewed changes

vladbochok reviewed Jun 8, 2024

View reviewed changes

Yberjon pushed a commit to neotheprogramist/era-contracts that referenced this pull request Aug 12, 2024

chore(contract): applied new solhint rules across l1, l2, and system …

130b62a

…contracts (matter-labs#473)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(contract): applied new solhint rules across l1, l2, and system contracts #473

chore(contract): applied new solhint rules across l1, l2, and system contracts #473

koloz193 commented May 15, 2024 •

edited

Loading

dnkolegov left a comment

vladbochok May 21, 2024

koloz193 Jun 4, 2024 •

edited

Loading

vladbochok May 21, 2024

koloz193 Jun 4, 2024

vladbochok May 21, 2024

koloz193 Jun 4, 2024

vladbochok May 21, 2024

koloz193 Jun 4, 2024

vladbochok May 21, 2024

koloz193 Jun 4, 2024

vladbochok Jun 8, 2024

vladbochok Jun 8, 2024

vladbochok Jun 8, 2024

vladbochok Jun 8, 2024

vladbochok Jun 8, 2024

vladbochok Jun 8, 2024

vladbochok Jun 8, 2024

vladbochok Jun 8, 2024

vladbochok left a comment

chore(contract): applied new solhint rules across l1, l2, and system contracts #473

chore(contract): applied new solhint rules across l1, l2, and system contracts #473

Conversation

koloz193 commented May 15, 2024 • edited Loading

What ❔

Why ❔

Checklist

dnkolegov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koloz193 Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladbochok left a comment

Choose a reason for hiding this comment

koloz193 commented May 15, 2024 •

edited

Loading

koloz193 Jun 4, 2024 •

edited

Loading