Increase Test Coverage

.github/workflows/l1-contracts-ci.yaml

@@ -217,7 +217,7 @@ jobs:
           minimum-coverage: 85 # Set coverage threshold.
 
   gas-report:
-    needs: [build, lint, test-foundry]
+    needs: [build, lint]
     runs-on: ubuntu-latest
 
     steps:
@@ -258,7 +258,7 @@ jobs:
           summaryQuantile: 0.0 # only display the 10% most significant gas diffs in the summary (defaults to 20%)
           sortCriteria: avg,max # sort diff rows by criteria
           sortOrders: desc,asc # and directions
-          ignore: test-foundry/**/* # filter out gas reports from specific paths (test/ is included by default)
+          ignore: test-foundry/**/*,l1-contracts/contracts/dev-contracts/**/*,l1-contracts/lib/**/*,l1-contracts/contracts/common/Dependencies.sol
         id: gas_diff
 
       - name: Add gas diff to sticky comment

l1-contracts/contracts/bridgehub/IMessageRoot.sol

@@ -11,7 +11,5 @@ interface IMessageRoot {
 
     function addChainBatchRoot(uint256 _chainId, uint256 _batchNumber, bytes32 _chainBatchRoot) external;
 
-    function clearTreeAndProvidePubdata() external returns (bytes memory pubdata);
-
     function addNewChainIfNeeded(uint256 _chainId) external;
 }

l1-contracts/contracts/bridgehub/MessageRoot.sol

@@ -154,46 +154,4 @@ contract MessageRoot is IMessageRoot, ReentrancyGuard {
         // slither-disable-next-line unused-return
         sharedTree.updateAllLeaves(newLeaves);
     }
-
-    // It is expected that the root is present
-    // `_updateTree` should be false only if the caller ensures that it is followed by updating the entire tree.
-    function _unsafeResetChainRoot(uint256 _index, bool _updateTree) internal {
-        uint256 chainId = chainIndexToId[_index];
-        bytes32 initialRoot = chainTree[chainId].setup(CHAIN_TREE_EMPTY_ENTRY_HASH);
-
-        if (_updateTree) {
-            // slither-disable-next-line unused-return
-            sharedTree.updateLeaf(_index, Messaging.chainIdLeafHash(initialRoot, chainId));
-        }
-    }
-
-    /// IMPORTANT FIXME!!!: split into two: provide pubdata and clear state. The "provide pubdata" part should be used by SL.
-    /// NO DA is provided here ATM !!!
-    /// @notice To be called by the bootloader by the L1Messenger at the end of the batch to produce the final root and send it to the underlying layer.
-    /// @return pubdata The pubdata to be relayed to the DA layer.
-    function clearTreeAndProvidePubdata() external returns (bytes memory) {
-        // FIXME: access control: only to be called by the l1 messenger.
-        // uint256 cachedChainCount = chainCount;
-        // // We will send the updated roots for all chains.
-        // // While it will mean that we'll pay even for unchanged roots:
-        // // - It is the simplest approach
-        // // - The alternative is to send pairs of (chainId, root), which is less efficient if at least half of the chains are active.
-        // //
-        // // There are of course ways to optimize it further, but it will be done in the future.
-        // bytes memory pubdata = new bytes(cachedChainCount * 32);
-        // for (uint256 i = 0; i < cachedChainCount; i++) {
-        //     // It is the responsibility of each chain to provide the roots of its L2->L1 messages if it wants to see those.
-        //     // However, for the security of the system as a whole, the chain roots need to be provided for all chains.
-        //     bytes32 chainRoot = chainTree[chainIndexToId[i]].root();
-        //     assembly {
-        //         mstore(add(pubdata, add(32, mul(i, 32))), chainRoot)
-        //     }
-        //     // Clearing up the state.
-        //     // Note that it *does not* delete any storage slots, so in terms of pubdata savings, it is useless.
-        //     // However, the chains paid for these changes anyway, so it is considered acceptable.
-        //     // In the future, further optimizations will be available.
-        //     _unsafeResetChainRoot(i, false);
-        // }
-        // updateFullTree();
-    }
 }

l1-contracts/contracts/state-transition/StateTransitionManager.sol

@@ -102,7 +102,7 @@ contract StateTransitionManager is IStateTransitionManager, ReentrancyGuard, Own
         chainAddresses = new address[](keys.length);
         uint256 keysLength = keys.length;
         for (uint256 i = 0; i < keysLength; ++i) {
-            chainAddresses[i] = hyperchainMap.get(i);
+            chainAddresses[i] = hyperchainMap.get(keys[i]);
         }
     }
 

l1-contracts/test/foundry/unit/concrete/Bridgehub/MessageRoot.t.sol

@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity 0.8.24;
+
+import {Test} from "forge-std/Test.sol";
+import {MessageRoot} from "contracts/bridgehub/MessageRoot.sol";
+import {IBridgehub} from "contracts/bridgehub/IBridgehub.sol";
+
+// Chain tree consists of batch commitments as their leaves. We use hash of "new bytes(96)" as the hash of an empty leaf.
+bytes32 constant CHAIN_TREE_EMPTY_ENTRY_HASH = bytes32(
+    0x46700b4d40ac5c35af2c22dda2787a91eb567b06c924a8fb8ae9a05b20c08c21
+);
+
+// Chain tree consists of batch commitments as their leaves. We use hash of "new bytes(96)" as the hash of an empty leaf.
+bytes32 constant SHARED_ROOT_TREE_EMPTY_HASH = bytes32(
+    0x46700b4d40ac5c35af2c22dda2787a91eb567b06c924a8fb8ae9a05b20c08c21
+);
+
+contract MessageRootTest is Test {
+    address bridgeHub;
+    MessageRoot messageRoot;
+
+    function setUp() public {
+        bridgeHub = makeAddr("bridgeHub");
+        messageRoot = new MessageRoot(IBridgehub(bridgeHub));
+    }
+
+    function test_init() public {
+        assertEq(messageRoot.getAggregatedRoot(), CHAIN_TREE_EMPTY_ENTRY_HASH);
+    }
+
+    function test_RevertWhen_addChainNotBridgeHub() public {
+        uint256 alphaChainId = uint256(uint160(makeAddr("alphaChainId")));
+        uint256 betaChainId = uint256(uint160(makeAddr("betaChainId")));
+
+        assertFalse(messageRoot.chainRegistered(alphaChainId), "alpha chain 1");
+
+        vm.expectRevert("MR: only bridgehub");
+        messageRoot.addNewChain(alphaChainId);
+
+        assertFalse(messageRoot.chainRegistered(alphaChainId), "alpha chain 2");
+    }
+
+    function test_addNewChain() public {
+        uint256 alphaChainId = uint256(uint160(makeAddr("alphaChainId")));
+        uint256 betaChainId = uint256(uint160(makeAddr("betaChainId")));
+
+        assertFalse(messageRoot.chainRegistered(alphaChainId), "alpha chain 1");
+        assertFalse(messageRoot.chainRegistered(betaChainId), "beta chain 1");
+
+        vm.prank(bridgeHub);
+        vm.expectEmit(true, false, false, false);
+        emit MessageRoot.AddedChain(alphaChainId, 0);
+        messageRoot.addNewChain(alphaChainId);
+
+        assertTrue(messageRoot.chainRegistered(alphaChainId), "alpha chain 2");
+        assertFalse(messageRoot.chainRegistered(betaChainId), "beta chain 2");
+
+        assertEq(messageRoot.getChainRoot(alphaChainId), bytes32(0));
+    }
+
+    function test_RevertWhen_ChainNotRegistered() public {
+        address alphaChainSender = makeAddr("alphaChainSender");
+        uint256 alphaChainId = uint256(uint160(makeAddr("alphaChainId")));
+        vm.mockCall(
+            bridgeHub,
+            abi.encodeWithSelector(IBridgehub.getHyperchain.selector, alphaChainId),
+            abi.encode(alphaChainSender)
+        );
+
+        vm.prank(alphaChainSender);
+        vm.expectRevert("MR: not registered");
+        messageRoot.addChainBatchRoot(alphaChainId, 1, bytes32(alphaChainId));
+    }
+
+    function test_addChainBatchRoot() public {
+        address alphaChainSender = makeAddr("alphaChainSender");
+        uint256 alphaChainId = uint256(uint160(makeAddr("alphaChainId")));
+        vm.mockCall(
+            bridgeHub,
+            abi.encodeWithSelector(IBridgehub.getHyperchain.selector, alphaChainId),
+            abi.encode(alphaChainSender)
+        );
+
+        vm.prank(bridgeHub);
+        messageRoot.addNewChain(alphaChainId);
+
+        vm.prank(alphaChainSender);
+        vm.expectEmit(true, false, false, false);
+        emit MessageRoot.Preimage(bytes32(0), bytes32(0));
+        vm.expectEmit(true, false, false, false);
+        emit MessageRoot.AppendedChainBatchRoot(alphaChainId, 1, bytes32(alphaChainId));
+        messageRoot.addChainBatchRoot(alphaChainId, 1, bytes32(alphaChainId));
+    }
+
+    function test_updateFullTree() public {
+        address alphaChainSender = makeAddr("alphaChainSender");
+        uint256 alphaChainId = uint256(uint160(makeAddr("alphaChainId")));
+        vm.mockCall(
+            bridgeHub,
+            abi.encodeWithSelector(IBridgehub.getHyperchain.selector, alphaChainId),
+            abi.encode(alphaChainSender)
+        );
+
+        vm.prank(bridgeHub);
+        messageRoot.addNewChain(alphaChainId);
+
+        vm.prank(alphaChainSender);
+        messageRoot.addChainBatchRoot(alphaChainId, 1, bytes32(alphaChainId));
+
+        messageRoot.updateFullTree();
+
+        assertEq(messageRoot.getAggregatedRoot(), 0xbad7e1cf889e30252b8ce93820f79d50651b78587844bc1c588dea123effa4ea);
+    }
+}

l1-contracts/test/foundry/unit/concrete/Utils/Utils.sol

@@ -208,7 +208,7 @@ library Utils {
     }
 
     function getGettersSelectors() public pure returns (bytes4[] memory) {
-        bytes4[] memory selectors = new bytes4[](29);
+        bytes4[] memory selectors = new bytes4[](30);
         selectors[0] = GettersFacet.getVerifier.selector;
         selectors[1] = GettersFacet.getAdmin.selector;
         selectors[2] = GettersFacet.getPendingAdmin.selector;
@@ -238,6 +238,7 @@ library Utils {
         selectors[26] = GettersFacet.getTotalBatchesVerified.selector;
         selectors[27] = GettersFacet.getTotalBatchesExecuted.selector;
         selectors[28] = GettersFacet.getL2SystemContractsUpgradeTxHash.selector;
+        selectors[29] = GettersFacet.getProtocolVersion.selector;
         return selectors;
     }
 

l1-contracts/test/foundry/unit/concrete/state-transition/StateTransitionManager/Admin.t.sol

@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.24;
+
+import {IStateTransitionManager} from "contracts/state-transition/IStateTransitionManager.sol";
+import {StateTransitionManagerTest} from "./_StateTransitionManager_Shared.t.sol";
+
+contract AdminTest is StateTransitionManagerTest {
+    function test_setPendingAdmin() public {
+        address newAdmin = makeAddr("newAdmin");
+
+        vm.expectEmit(true, true, true, false);
+        emit IStateTransitionManager.NewPendingAdmin(address(0), newAdmin);
+        chainContractAddress.setPendingAdmin(newAdmin);
+    }
+
+    function test_acceptPendingAdmin() public {
+        address newAdmin = makeAddr("newAdmin");
+
+        chainContractAddress.setPendingAdmin(newAdmin);
+
+        // Need this because in shared setup we start a prank as the governor
+        vm.stopPrank();
+        vm.prank(newAdmin);
+        vm.expectEmit(true, true, true, false);
+        emit IStateTransitionManager.NewPendingAdmin(newAdmin, address(0));
+        vm.expectEmit(true, true, true, false);
+        emit IStateTransitionManager.NewAdmin(address(0), newAdmin);
+        chainContractAddress.acceptAdmin();
+
+        address currentAdmin = chainContractAddress.admin();
+
+        assertEq(currentAdmin, newAdmin);
+    }
+}

l1-contracts/test/foundry/unit/concrete/state-transition/StateTransitionManager/CreateNewChain.t.sol

@@ -28,13 +28,24 @@ contract createNewChainTest is StateTransitionManagerTest {
         });
     }
 
-    // function test_SuccessfulCreationOfNewChain() public {
-    //     createNewChain(getDiamondCutData(diamondInit));
+    function test_SuccessfulCreationOfNewChain() public {
+        createNewChain(getDiamondCutData(diamondInit));
 
-    //     address admin = chainContractAddress.getChainAdmin(chainId);
-    //     address newChainAddress = chainContractAddress.getHyperchain(chainId);
+        address admin = chainContractAddress.getChainAdmin(chainId);
+        address newChainAddress = chainContractAddress.getHyperchain(chainId);
 
-    //     assertEq(newChainAdmin, admin);
-    //     assertNotEq(newChainAddress, address(0));
-    // }
+        assertEq(newChainAdmin, admin);
+        assertNotEq(newChainAddress, address(0));
+
+        address[] memory chainAddresses = chainContractAddress.getAllHyperchains();
+        assertEq(chainAddresses.length, 1);
+        assertEq(chainAddresses[0], newChainAddress);
+
+        uint256[] memory chainIds = chainContractAddress.getAllHyperchainChainIDs();
+        assertEq(chainIds.length, 1);
+        assertEq(chainIds[0], chainId);
+
+        uint256 protocolVersion = chainContractAddress.getProtocolVersion(chainId);
+        assertEq(protocolVersion, 0);
+    }
 }

l1-contracts/test/foundry/unit/concrete/state-transition/StateTransitionManager/SetNewVersionUpgrade.t.sol

@@ -16,5 +16,11 @@ contract setNewVersionUpgradeTest is StateTransitionManagerTest {
 
         assertEq(chainContractAddress.upgradeCutHash(0), newCutHash, "Diamond cut upgrade was not successful");
         assertEq(chainContractAddress.protocolVersion(), 1, "New protocol version is not correct");
+
+        (uint32 major, uint32 minor, uint32 patch) = chainContractAddress.getSemverProtocolVersion();
+
+        assertEq(major, 0);
+        assertEq(minor, 0);
+        assertEq(patch, 1);
     }
 }

l1-contracts/test/foundry/unit/concrete/state-transition/StateTransitionManager/SetValidatorTimelock.t.sol

@@ -20,4 +20,23 @@ contract setValidatorTimelockTest is StateTransitionManagerTest {
             "Validator timelock update was not successful"
         );
     }
+
+    function test_RevertWhen_NotOwner() public {
+        // Need this because in shared setup we start a prank as the governor
+        vm.stopPrank();
+
+        address notOwner = makeAddr("notOwner");
+        assertEq(
+            chainContractAddress.validatorTimelock(),
+            validator,
+            "Initial validator timelock address is not correct"
+        );
+
+        vm.prank(notOwner);
+        vm.expectRevert("STM: not owner or admin");
+        address newValidatorTimelock = address(0x0000000000000000000000000000000000004235);
+        chainContractAddress.setValidatorTimelock(newValidatorTimelock);
+
+        assertEq(chainContractAddress.validatorTimelock(), validator, "Validator should not have been updated");
+    }
 }