Test/integration arbitrum to gnosis #319
Conversation
WalkthroughThe changes encompass modifications to the deployment logic and configuration of the Arbitrum to Gnosis bridge contracts. Key updates include increased deposit amounts, renamed parameters, and significant adjustments to timeout durations. New mock contracts have been introduced for testing purposes, enhancing the deployment process. The deployment functions have been restructured to support local and live deployments, improving modularity and clarity. Additionally, a comprehensive suite of integration tests has been added to validate the bridge's functionality across various scenarios. Changes
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (4)
Files skipped from review as they are similar to previous changes (3)
Additional comments not posted (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
❌ Deploy Preview for veascan failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
contracts/src/test/bridge-mocks/arbitrum/BridgeMock.sol (1)
31-42: LGTM! Consider a more meaningful return value.The
executeL1Messagefunction is a well-implemented addition to theBridgeMockcontract, serving a crucial purpose in simulating message execution for testing and development. The use of low-levelcalland inline assembly is appropriate for this specific use case, providing fine-grained control over execution and error handling.The function signature and documentation are clear and concise, accurately describing its purpose and behavior.
One minor suggestion would be to consider if a more meaningful return value could be provided, such as a boolean indicating the success status of the call, instead of always returning
0. However, this is not a critical issue and may depend on the specific requirements and intended usage of the function.contracts/src/test/tokens/gnosis/MockWETH.sol (1)
1-81: LGTM! The contract correctly implements the WETH token functionality.The contract is a good mock implementation of the WETH token with the following features:
- Allows users to deposit and withdraw Ether and get equivalent WETH tokens.
- Implements the standard ERC20 functions like
transfer,transferFrom,approve, etc.- Maintains the total supply of tokens equal to the Ether balance.
- Uses the latest Solidity version 0.8.0 for security and performance.
- Is well-documented with NatSpec comments.
Consider adding more NatSpec comments for the public and external functions to improve the documentation. For example:
+ /// @notice Deposits Ether and mints equivalent WETH tokens to the sender + /// @dev Emits a {Deposit} event function deposit() public payable { balanceOf[msg.sender] += msg.value; emit Deposit(msg.sender, msg.value); } + /// @notice Withdraws Ether by burning equivalent WETH tokens from the sender + /// @dev Emits a {Withdrawal} event + /// @param wad The amount of Ether to withdraw function withdraw(uint wad) public { require(balanceOf[msg.sender] >= wad, "WETH: insufficient balance"); balanceOf[msg.sender] -= wad; payable(msg.sender).transfer(wad); emit Withdrawal(msg.sender, wad); }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts (2 hunks)
- contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (1 hunks)
- contracts/deploy/03-routers/03-arb-to-gnosis-router.ts (1 hunks)
- contracts/src/test/ArbToGnosis/VeaInboxArbToGnosisMock.sol (1 hunks)
- contracts/src/test/bridge-mocks/arbitrum/ArbSysMockWithBridge.sol (1 hunks)
- contracts/src/test/bridge-mocks/arbitrum/BridgeMock.sol (2 hunks)
- contracts/src/test/bridge-mocks/gnosis/MockAMB.sol (4 hunks)
- contracts/src/test/tokens/gnosis/MockWETH.sol (1 hunks)
- contracts/test/integration/ArbToGnosis.ts (1 hunks)
Additional comments not posted (32)
contracts/src/test/bridge-mocks/arbitrum/ArbSysMockWithBridge.sol (1)
1-27: LGTM!The
ArbSysMockWithBridgecontract is well-structured and implements the required functionality correctly. The use of immutable variable for storing theBridgeMockinstance is gas-efficient. The contract does not have any security vulnerabilities or code smells.contracts/src/test/ArbToGnosis/VeaInboxArbToGnosisMock.sol (1)
1-39: LGTM!The
VeaInboxArbToGnosisMockcontract is well-structured and follows the Solidity coding conventions. The use of a mock contract for testing is a good practice as it allows for isolated testing of theVeaInboxArbToGnosiscontract. The overriddensendSnapshotfunction is correctly using themockArbSysinstead of the constantARB_SYS, and the contract is correctly emitting theSnapshotSentevent after sending the transaction to L1.contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (5)
4-4: LGTM!The import statement is necessary for using ethers functionality in the deployment script.
7-8: LGTM!The addition of new enum values is necessary to support Gnosis chain deployments.
13-14: Verify if the empty objects need to be populated with actual parameters.The addition of new properties is necessary to store deployment parameters for Gnosis chains. However, the properties are initialized with empty objects.
Please verify if the empty objects need to be populated with actual parameters for the Gnosis chain deployments.
16-17: LGTM!The modifications to the
HARDHATproperty are likely for testing purposes on a local hardhat network. TheepochPeriodandrouterAddressvalues seem reasonable for a local test environment.
25-75: LGTM!The restructuring of the
deployInboxfunction improves the modularity and clarity of the deployment process. The addition of thehardhatDeployerfunction specifically for local deployments is a good practice for testing. The calculation of the future router address and deployment of theArbSysMockcontract in thehardhatDeployerfunction is necessary for local testing. The use of mock contracts for theVeaInboxArbToGnosisandArbToGnosisSenderGatewaydeployments in thehardhatDeployerfunction is appropriate for local testing. The placeholderliveDeployerfunction indicates future expansion for live chain deployments, which is a good practice for incremental development. The updated control flow to differentiate between local and live deployments improves the flexibility of the deployment process.contracts/src/test/bridge-mocks/gnosis/MockAMB.sol (6)
8-8: LGTM!The addition of the
_dataparameter to theMockedEventevent enhances event logging by capturing more data related to the event emission. This change can aid in debugging and tracking message flows.
37-37: LGTM!The change in the source chain ID from
1337to31337reflects a shift in the context or environment in which the contract operates. This change is consistently applied across the contract.
38-44: Verify the safety of the inline assembly code.The addition of capturing the return data from the
_contract.callinvocation and using inline assembly to revert the transaction with the return data if the call fails is a good enhancement. It provides better feedback on failures, allowing for more robust error handling in the contract's operations.However, the use of inline assembly is a low-level operation that requires careful review. Please ensure that the inline assembly code is safe and does not introduce any vulnerabilities.
73-74: LGTM!The update in the calculation of the
bridgeIdto use the new source chain ID31337is consistent with the previous change in the source chain ID.
86-87: LGTM!The update of the source and destination chain IDs in the
eventDatato31337is consistent with the previous changes in the source chain ID.
91-91: LGTM!The inclusion of the
_dataparameter in theMockedEventemission is consistent with the modification of theMockedEventevent to include this parameter.contracts/deploy/03-routers/03-arb-to-gnosis-router.ts (4)
42-43: LGTM!The deployment of the mock AMB contract is necessary for testing the router deployment in a local environment. The commented-out
ArbSysMockcontract suggests that it may have been considered for inclusion but was ultimately excluded, which is fine.
45-54: Clarify the purpose of the argument passed to theSequencerInboxMock.The deployment of the
SequencerInboxMockandOutboxMockcontracts is necessary for simulating the behavior of the Arbitrum to Gnosis bridge components in a testing environment. TheOutboxMockis correctly initialized with the address of theveaInboxcontract, which is crucial for its operation.However, the purpose of the argument passed to the
SequencerInboxMockis not clear from the provided context. Could you please clarify what the argument "10" represents and how it affects the behavior of the mock contract?
56-60: LGTM!The deployment of the
BridgeMockcontract is necessary for simulating the behavior of the Arbitrum bridge in a testing environment. TheBridgeMockis correctly initialized with the addresses of theOutboxMockandSequencerInboxMockcontracts, which is crucial for its operation.
65-65: LGTM!The updated arguments passed to the
RouterArbToGnosisdeployment ensure that the router is correctly initialized with the addresses of the mock contracts, which is crucial for its operation in the testing environment. Using the addresses of the mock contracts instead of the contract instances directly is a good practice, as it decouples the router from the specific instances of the mock contracts.contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts (10)
39-39: LGTM!The increased deposit amount aligns with the updated budget for timeouts.
42-42: LGTM!The parameter renaming improves clarity without changing the functionality.
43-43: LGTM!The increased
numEpochTimeoutvalue aligns with the "6 hours" requirement mentioned in the comment.
49-49: Please clarify the reason for the reduced sequencer limit.The
sequencerLimitvalue forHARDHAThas been significantly reduced from 86400 (24 hours) to 864. Could you please provide more context on the reason for this change and its implications?
88-89: Please clarify the reason for the adjusted nonce value.The
routerAddresscalculation now usesnonce + 10instead ofnonce + 4, suggesting a change in the expected transaction order for deploying the router contract. Could you please provide more context on the reason for this adjustment and its implications on the overall deployment sequence?
91-92: LGTM!The adjusted nonce value for calculating the
senderGatewayAddressaligns with the provided comment about the expected transaction order.
94-98: LGTM!The deployment of the
MockAMBcontract is a reasonable addition for testing or local development purposes.
100-104: LGTM!The deployment of the
MockWETHcontract is a reasonable addition for testing or local development purposes.
106-119: LGTM!The deployment of the
VeaOutboxArbToGnosiscontract with the updated name and the inclusion of theMockAMBandMockWETHaddresses as arguments is a reasonable change for testing or local development purposes.
123-130: LGTM!The deployment of the
ArbToGnosisReceiverGatewaycontract using theReceiverGatewayMockimplementation and the inclusion of theVeaOutboxArbToGnosisaddress and thesenderGatewayAddressas arguments is a reasonable addition for testing or local development purposes.contracts/test/integration/ArbToGnosis.ts (5)
116-351: LGTM!The "Honest Claim - No Challenge - Bridger Paid" test case is well-structured and covers the important aspects of an honest claim scenario. The sub-tests are logically organized and test the expected behavior at each step, including sending a message, allowing the bridger to claim, starting verification, verifying the snapshot, relaying the message, and allowing the bridger to withdraw the deposit.
353-538: LGTM!The "Honest Claim - Dishonest Challenge - Bridger paid, challenger deposit forfeited" test case thoroughly covers the scenario of an honest claim with a dishonest challenge. It tests the important aspects such as submitting a challenge, resolving the dispute, and the correct handling of deposits for the bridger and challenger. The sub-tests are well-organized and test the expected behavior at each step, including allowing the challenger to submit a challenge, handling the entire cross-chain dispute resolution process, allowing the bridger to withdraw the deposit plus reward, not allowing the challenger to withdraw the deposit, and allowing message relay after dispute resolution.
540-758: LGTM!The "Dishonest Claim - Honest Challenge - Bridger deposit forfeited, Challenger paid" test case comprehensively covers the scenario of a dishonest claim with an honest challenge. It tests the crucial aspects such as submitting a challenge to a dishonest claim, resolving the dispute in favor of the challenger, handling the deposits correctly for the bridger and challenger, and ensuring the correct state is maintained after the dispute resolution. The sub-tests are well-structured and cover the expected behavior at each step, including allowing the challenger to submit a challenge to a dishonest claim, initiating cross-chain dispute resolution for the dishonest claim, resolving the dispute in favor of the challenger, not allowing the dishonest bridger to withdraw the deposit, allowing the challenger to withdraw the deposit plus reward, allowing message relay with the correct state root after dispute resolution, updating the latest verified epoch and state root correctly after dispute resolution, and not allowing multiple withdrawals for the same challenge.
42-88: LGTM!The helper functions
createClaim,simulateDisputeResolution, andsetupClaimAndChallengeare well-defined and serve their purpose effectively. They encapsulate common setup and simulation logic, making the test cases more readable and maintainable. The functions have clear names and parameters, making it easy to understand their purpose.
90-114: LGTM!The setup and initialization code is properly structured using the
beforeandbeforeEachhooks. Thebeforehook initializes the test participants (wallets), which are used throughout the tests. ThebeforeEachhook ensures that the contracts are freshly deployed and the token balances are set up correctly before each test case. This setup ensures a clean state for each test case.
| @@ -36,17 +36,17 @@ const paramsByChainId = { | |||
| sequencerLimit: 86400, // 24 hours | |||
| }, | |||
| HARDHAT: { | |||
| deposit: parseEther("5"), // 120 xDAI budget for timeout | |||
| deposit: parseEther("10"), // 120 xDAI budget for timeout | |||
There was a problem hiding this comment.
It looks like the comment doesnt match the code. Am I reading it correctly?
| const deployer = (await getNamedAccounts()).deployer; | ||
| console.log("deployer: %s", deployer); | ||
| // fallback to hardhat node signers on local network | ||
| const deployer = (await getNamedAccounts()).deployer ?? (await hre.ethers.getSigners())[0].address; |
There was a problem hiding this comment.
It will be more verbose (maybe more readable, on the other side), but the getNamedAccounts and getChanId operations can be run concurrently with Promise.all.
There was a problem hiding this comment.
we're using this script to deploy test contracts in HRE, so we don't really need to sweat the performance too much. Do you think it's still worth making the code a bit wordier for the sake of readability? Or should we keep it simple?
There was a problem hiding this comment.
though being a test script ,performance might make sense here so as the project and test suite grows it wouldn't block the CI.
There was a problem hiding this comment.
Usually, the rule of thumb for me is that behaviors can be concurrent should be running concurrently by default.
| @@ -39,11 +39,30 @@ const deployRouter: DeployFunction = async (hre: HardhatRuntimeEnvironment) => { | |||
| const hardhatDeployer = async () => { | |||
| const veaOutbox = await deployments.get("VeaOutboxArbToGnosis"); | |||
| const veaInbox = await deployments.get("VeaInboxArbToGnosis"); | |||
| const amb = await deployments.get("MockAMB"); | |||
| //const ArbSysMock = await deployments.get('arbSysMock'); | |||
There was a problem hiding this comment.
Just checking: is this commented line intentional or a leftover that slipped?
There was a problem hiding this comment.
reminisce of what was before
There was a problem hiding this comment.
IMHO, we would be doing fine by removing code that does not get executed (commented, unreachable, etc). With git we can always go back in history if we decide it was crucial, and it helps to keep the codebase tidy
madhurMongia
force-pushed
the
test/integration-arbitrum-to-gnosis
branch
from
4f26534
to
aba723d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (1)
13-14: Add a TODO comment to fill in the configurations.The addition of the new chains to the configuration object is consistent with the PR objective. However, the configurations are empty.
Add a TODO comment to fill in the configurations in the future:
const paramsByChainId = { - GNOSIS_MAINNET: {}, + GNOSIS_MAINNET: {}, // TODO: Fill in the configuration - GNOSIS_CHIADO: {}, + GNOSIS_CHIADO: {}, // TODO: Fill in the configuration HARDHAT: { epochPeriod: 600, // 10 min routerAddress: ethers.constants.AddressZero, }, };
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts (3 hunks)
- contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (1 hunks)
- contracts/deploy/03-routers/03-arb-to-gnosis-router.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts
- contracts/deploy/03-routers/03-arb-to-gnosis-router.ts
Additional comments not posted (4)
contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (4)
4-4: LGTM!The import statement is necessary for using ethers functionality in the script.
7-8: LGTM!The addition of the new chains to the enum is consistent with the PR objective of adding support for Gnosis Mainnet and Chiado chains.
16-17: LGTM!The updates to the
HARDHATchain configuration seem reasonable for local testing purposes.
22-75: LGTM!The restructuring of the deployment logic improves the clarity and organization of the script. The introduction of the
hardhatDeployerfunction enhances the modularity of the deployment process for local testing. The placeholderliveDeployerfunction indicates potential future expansion for live deployments.
madhurMongia
force-pushed
the
test/integration-arbitrum-to-gnosis
branch
from
aba723d
to
81a8e28
Compare
…allenger deposit forfeited
…forfeited, challenger paid
madhurMongia
force-pushed
the
test/integration-arbitrum-to-gnosis
branch
from
81a8e28
to
32018b4
Compare
| const deployer = (await getNamedAccounts()).deployer; | ||
| console.log("deployer: %s", deployer); | ||
| // fallback to hardhat node signers on local network | ||
| const [namedAccounts, defaultSigner] = await Promise.all([ |
There was a problem hiding this comment.
This code does not seem equivalent to the old one. You only wanted the defaultSigner when there was no deployer on namedAccounts. Meanwhile, the getChainId, which is always part of the happy path with the namedAccounts, is not resolved concurrently.
We have two options:
- Resolve the happy path concurrently, and the fallback asynchronous. If the fallback is slow or the happy path is the most frequent outcome, I would pick this one.
- Resolve the three behaviors concurrently otherwise.
The first one would look like:
const [namedAccounts, rawChainId] = await Promise.all([
getNamedAccounts(),
getChainId(),
]);
let deployer = namedAccounts.deployer;
if (!deployer) (await hre.ethers.getSingers())[0].address;
const chainId = Number(rawChainId)That version resolves the happy path concurrently, falling back in a second call for the unhappy one. I would personally break if (!deployer) (await hre.ethers.getSingers())[0].address; line into multiple statements, as I do not like lines that perform too many operations (especially with await), but that is personal preference.
The second one would look like:
const [namedAccounts, signers, rawChainId] = await Promise.all([
getNamedAccounts(),
hre.ethers.getSingers()
getChainId(),
]);
const deployer = namedAccounts.deployer ?? signers[0].address
const chainId = Number(rawChainId)That results in clearer code in this particular case. If the fallback operation (getting the signers) is slower, it would not be advisable.
In general, I do not think the performance difference will be critical. All operations (but getNamedAccounts(), which I do not remember the implementation details right now) looks like they will be resolving in a matter of nanoseconds. I am sharing this feedback more to have a discussion and agree on a set of shared practices.
There was a problem hiding this comment.
Thank you for your detailed analysis. option two seems better ,will fixed it .
| @@ -36,17 +36,17 @@ const paramsByChainId = { | |||
| sequencerLimit: 86400, // 24 hours | |||
| }, | |||
| HARDHAT: { | |||
| deposit: parseEther("5"), // 120 xDAI budget for timeout | |||
| deposit: parseEther("10"), // | |||
There was a problem hiding this comment.
Nice, no comment leaves no chance to discrepancies! That is even better than the update I was expecting. It looks like the comment slashes where left though.
There was a problem hiding this comment.
I am also not sure what did the 120 xDai comment meant, its not a direct conversion for sure as 5 eth was 120 in mid 2017 ,maybe JB can help with it
madhurMongia
force-pushed
the
test/integration-arbitrum-to-gnosis
branch
from
3c9c329
to
b4dbc42
Compare
PR-Codex overview
This PR adds new functionalities and mocks for the Arbitrum and Gnosis bridge systems.
Detailed summary
executeL1Messagefunction toBridgeMock.solArbSysMockWithBridgecontractsendSnapshotinVeaInboxArbToGnosisMockMockAMB.solMockWETHcontractSummary by CodeRabbit
New Features
Bug Fixes
Documentation