Hyperchains Integration #439

neotheprogramist · 2024-05-02T09:55:00Z

No description provided.

tejks · 2024-05-03T17:36:14Z

What ❔

This PR introduces invariant tests with fuzzed inputs to test the integration of hyperchains. These tests involve depositing and withdrawing tokens through the bridgehub interface. The primary functionality is based on selecting random calls from the withdraw/deposit functions, executing the calls, and verifying how balances change after each call and if the invariants are kept.

Type of deposits:

depositERC20
depositETH

Type of withdrawals

withdrawETH
withdrawERC20

Calls such as deposit and withdraw are categorized based on the type of token used (ERC20/ETH). Additionally, deposit functions consider whether the token is base or non-base and select the appropriate call.

Changed l1 contract deployment base scripts to l1-contract-foundry scripts.

Minor changes have been made to these scripts so that they can be used for integration testing (mainly added getters).
Moved scripts with appropriate configs to test directory to avoid circular dependency.
RegisterHyperchain.s.sol script has been changed to read multiple hyperchains from config.

Important!!! forge-std was bump to v1.8.2

Used scripts:

DeployErc20.s.sol
DeployL1.s.sol
RegisterHyperchain.s.sol

vladbochok · 2024-05-12T14:47:59Z

l1-contracts/test/foundry/integration/deploy-scripts/script/Utils.sol

+    address internal constant VM_ADDRESS = address(uint160(uint256(keccak256("hevm cheat code"))));
+    // Create2Factory deterministic bytecode.
+    // https://github.com/Arachnid/deterministic-deployment-proxy
+    bytes internal constant CREATE2_FACTORY_BYTECODE =


Can you use Solidity version of create2 factory instead? This way it would be easier for us to migrate zkSync L1 tests when we would use the L1 contract codebase on L2.

vladbochok · 2024-05-12T15:05:31Z

l1-contracts/test/foundry/integration/deploy-scripts/script/Utils.sol

+        bool hasGetName = false;
+        for (uint256 i = 0; i < selectors.length; i++) {
+            if (selectors[i] == bytes4(keccak256("getName()"))) {
+                selectors[i] = selectors[selectors.length - 1];
+                hasGetName = true;
+                break;
+            }
+        }
+        if (hasGetName) {
+            bytes4[] memory newSelectors = new bytes4[](selectors.length - 1);
+            for (uint256 i = 0; i < selectors.length - 1; i++) {
+                newSelectors[i] = selectors[i];
+            }
+            return newSelectors;
+        }


Suggested change

bool hasGetName = false;

for (uint256 i = 0; i < selectors.length; i++) {

if (selectors[i] == bytes4(keccak256("getName()"))) {

selectors[i] = selectors[selectors.length - 1];

hasGetName = true;

break;

}

}

if (hasGetName) {

bytes4[] memory newSelectors = new bytes4[](selectors.length - 1);

for (uint256 i = 0; i < selectors.length - 1; i++) {

newSelectors[i] = selectors[i];

}

return newSelectors;

}

for (uint256 i = 0; i < selectors.length; i++) {

if (selectors[i] == bytes4(keccak256("getName()"))) {

selectors[i] = selectors[selectors.length - 1];

// Pop the last element from the array

assembly {

mstore(selectors, sub(mload(selectors), 1))

}

break;

}

}

should we mirror this change to main scripts (in l1-contracts-foundry) in another pr?

You can change it in this PR

vladbochok · 2024-05-12T15:07:53Z

l1-contracts/test/foundry/integration/deploy-scripts/script/Utils.sol

+        return selectors;
+    }
+
+    function getAdminSelectors() public pure returns (bytes4[] memory) {


I think you can always use getAllSelectors over getAdminSelectors /getExecutorSelectors/getGettersSelectors/getMailboxSelectors

We used it, but this function requires ffi, which has to be set explicitly in test environment

FFI in test is not ideal, maybe we can find middle ground solution.

Here is how consensus-shipyard did it, can we make it similar?

https://github.com/consensus-shipyard/ipc/blob/main/contracts/scripts/python/build_selector_library.py

This approach uses command from foundry to inspect the contract, I don't see a way to call foundry command in testing environment without ffi.

.github/workflows/buld-release.yaml

l1-contracts/test/foundry/integration/BaseIntegrationTests.t.sol

.github/workflows/build-release-manual.yaml

.github/workflows/build-release.yaml

vladbochok · 2024-05-17T11:30:33Z

l1-contracts-foundry/script/DeployErc20.s.sol

@@ -77,7 +77,7 @@ contract DeployErc20Script is Script {
                mint: token.mint
            });
            console.log("Token deployed at:", tokenAddress);
-            token.addr = tokenAddress;
+            config.tokens[i].addr = tokenAddress;


Is this an intentional change? Why?

Yes, because the token variable is just a copy and does not overwrite the config value. As a result, all token addresses were the default value (address(0)), which was visible in the deployment output configuration.

vladbochok · 2024-05-17T11:31:24Z

l1-contracts-foundry/script/DeployErc20.s.sol

-            (bool success, ) = tokenAddress.call(
-                abi.encodeWithSignature("mint(address,uint256)", config.deployerAddress, mint)
-            );
+            (bool success,) =
+                tokenAddress.call(abi.encodeWithSignature("mint(address,uint256)", config.deployerAddress, mint));


And even better to cast tokenAddress to Token type and call mint there.

l1-contracts/foundry.toml

vladbochok · 2024-05-17T11:42:40Z

l1-contracts/test/foundry/integration/deploy-scripts/script/Utils.sol

+        return selectors;
+    }
+
+    function getAdminSelectors() public pure returns (bytes4[] memory) {


FFI in test is not ideal, maybe we can find middle ground solution.

vladbochok · 2024-05-17T11:45:46Z

l1-contracts/test/foundry/integration/deploy-scripts/script/Utils.sol

+        return selectors;
+    }
+
+    function getAdminSelectors() public pure returns (bytes4[] memory) {


Here is how consensus-shipyard did it, can we make it similar?

https://github.com/consensus-shipyard/ipc/blob/main/contracts/scripts/python/build_selector_library.py

vladbochok · 2024-05-17T13:33:24Z

l1-contracts/test/foundry/integration/deploy-scripts/script/DeployL1.s.sol

+import {L1ERC20Bridge} from "contracts/bridge/L1ERC20Bridge.sol";
+import {DiamondProxy} from "contracts/state-transition/chain-deps/DiamondProxy.sol";
+
+contract DeployL1Script is Script {


Can we maybe import this contract directly from the repo to not copy-pasting it?

l1-contracts/test/foundry/integration/_SharedL1ContractDeployer.t.sol

vladbochok · 2024-05-17T13:41:26Z

l1-contracts/test/foundry/integration/_SharedHyperchainDeployer.t.sol

+        description = RegisterHyperchainsScript.HyperchainDescription({
+            hyperchainChainId: __chainId,
+            baseToken: __baseToken,
+            bridgehubCreateNewChainSalt: 0,


Worth to test it too?

…abs#271)

revert changes revert changes revert changes revert changes

github-actions bot added the external-contribution label May 2, 2024

neotheprogramist marked this pull request as ready for review May 3, 2024 09:21

neotheprogramist requested review from vladbochok and StanislavBreadless as code owners May 3, 2024 09:21

vladbochok reviewed May 12, 2024

View reviewed changes

vladbochok reviewed May 17, 2024

View reviewed changes

tommysr and others added 23 commits May 20, 2024 14:40

create new chain

4ba46a0

spawn multiple hyperchains

b163342

test calls to bridgehub changes shared bridge balances

a14d5f3

feat: Code refactor

519992d

feat: Hyperchain direct deposit test with ETH token

608fb72

two bridges tx with non base token

47b2374

chore: Compile yul files in parallel within a single folder (matter-l…

9e68caa

…abs#271)

ci: Make build release manual triggered only (matter-labs#287)

5fdbbfb

feat: Use scripts deploy

5bcb5f3

hyperchains register script

4cc0216

fix: Broadcast with bridgehub owner

c441521

generate config for hyperchains deployment

d3e9089

feat: Implement DeployErc20 and DeployL1 scripts to tests

c48fe6d

deploy hyperchain success

b5e0d20

test hyperchainconfig save

3daa43f

feat: New files structure

fb5ce41

feat: Adding new hyperchains

629eddb

feat: Integration of the new deployer with tests

f1c798c

cleanup

8d24a6c

remove unused imports

bfc9040

feat: Bump forge-std to v1.8.2

60be529

fix: Move the deployment scripts to the test folder

81626a6

fix: Unused imports

98bad3f

tejks and others added 21 commits May 20, 2024 14:43

fix: Overwriting the token address

588dc4d

feat: Add SharedBridge balance setter

b8780ed

feat: Withdraw test

fd9892a

structure for invariant testing

f5fc00b

add base token invariant test

d1b4d4e

use valid requestl2transaction

87d6410

get priority tx data from logs and read into struct

9e7b466

mocked l2 contract, send eth on l2 log

cb28827

add token address to mock

d1f8303

base token deposit with appropriate transfer

c0aab8e

feat: Move invariant tests to the separate file

121dfb1

two bridges deposit with eliminated mocking

722e0af

add verifier implementation to config

8d16a8b

deposit two bridges to erc20 chain

caa1608

feat: Add IsWithdrawalFinalized setter for SharedBridge

ba819c8

feat: Use ERC20 withdraw to invariant tests

3d1aa58

feat: Withdrawal invariant test of the ETH token

589b612

fixed lint errors

c7f6d2c

unused vars removed

0834d38

revert changes

272376e

revert changes revert changes revert changes revert changes

Add comments and fixes

aaa587f

tejks force-pushed the hyperchains-integration branch from 57d20fc to aaa587f Compare May 20, 2024 13:07

fix: Suggestions from comments

d37a354

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hyperchains Integration #439

Hyperchains Integration #439

neotheprogramist commented May 2, 2024

tejks commented May 3, 2024 •

edited

vladbochok May 12, 2024

vladbochok May 12, 2024

tommysr May 15, 2024

vladbochok May 17, 2024

vladbochok May 12, 2024

tommysr May 15, 2024 •

edited

vladbochok May 17, 2024

vladbochok May 17, 2024

tommysr May 20, 2024

vladbochok May 17, 2024

tejks May 18, 2024 •

edited

vladbochok May 17, 2024

vladbochok May 17, 2024

vladbochok May 17, 2024

vladbochok May 17, 2024

vladbochok May 17, 2024

Hyperchains Integration #439

Are you sure you want to change the base?

Hyperchains Integration #439

Conversation

neotheprogramist commented May 2, 2024

tejks commented May 3, 2024 • edited

What ❔

Type of deposits:

Type of withdrawals

Used scripts:

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

tommysr May 15, 2024 • edited

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

tejks May 18, 2024 • edited

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

tejks commented May 3, 2024 •

edited

tommysr May 15, 2024 •

edited

tejks May 18, 2024 •

edited