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
Possibly related PRs
Poem
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 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
2 times, most recently
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
…allenger deposit forfeited
…forfeited, challenger paid
madhurMongia
force-pushed
the
test/integration-arbitrum-to-gnosis
branch
from
b4dbc42
to
46247de
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (12)
contracts/src/test/bridge-mocks/arbitrum/ArbSysMockWithBridge.sol (2)
15-19: Constructor looks good, consider adding input validation.The constructor is simple and uses an immutable variable for gas efficiency, which is good. However, consider adding input validation for the
_bridgeparameter to ensure it's not a zero address.You could add a check like this:
constructor(BridgeMock _bridge) { + require(address(_bridge) != address(0), "Bridge address cannot be zero"); bridge = _bridge; }
21-26: Function implementation is correct, consider handling sent Ether and emitting an event.The
sendTxToL1function correctly implements the IArbSys interface and delegates execution to the bridge mock. However, there are a couple of potential improvements:
The function is marked as
payable, but it doesn't handle the sent Ether. Consider either removing thepayablekeyword if Ether handling is not required, or implement proper Ether handling.For better testability and transparency, consider emitting an event when a transaction is sent to L1.
Here's a suggested implementation with these improvements:
event TxSentToL1(address indexed destination, uint256 value, uint256 withdrawalId); function sendTxToL1( address destination, bytes calldata calldataForL1 ) external payable returns (uint256 _withdrawal_ID) { _withdrawal_ID = bridge.executeL1Message(destination, calldataForL1); emit TxSentToL1(destination, msg.value, _withdrawal_ID); return _withdrawal_ID; }If Ether handling is not required, you can remove the
payablekeyword:function sendTxToL1( address destination, bytes calldata calldataForL1 ) external returns (uint256 _withdrawal_ID) { _withdrawal_ID = bridge.executeL1Message(destination, calldataForL1); emit TxSentToL1(destination, 0, _withdrawal_ID); return _withdrawal_ID; }contracts/src/test/bridge-mocks/arbitrum/BridgeMock.sol (1)
31-42: Function implementation looks good, with some suggestions for improvement.The
executeL1Messagefunction effectively simulates L1 to L2 message execution, which is useful for testing purposes. The use of inline assembly to preserve error information is a good practice.However, there are a few points to consider:
Consider adding a comment explaining the purpose and behavior of this function, especially its role in simulating L1 to L2 message execution.
The hardcoded return value of 0 seems like a placeholder. If this is intentional, please add a comment explaining why. Otherwise, consider returning a meaningful value or removing the return statement if it's not needed.
There's no access control on this function. While this might be intentional for a mock contract, it could lead to unexpected behavior if misused. Consider adding a modifier to restrict access if appropriate for your testing scenarios.
Here's a suggested improvement:
+ /// @notice Simulates the execution of an L1 to L2 message + /// @dev This function is for testing purposes and lacks access control + /// @param destination The address of the contract to call + /// @param data The calldata to send to the destination contract + /// @return A placeholder value (currently always 0) function executeL1Message(address destination, bytes calldata data) external returns (uint256) { // Simulate the bridge calling the destination contract (bool success, bytes memory returnData) = destination.call(data); if (!success) { assembly { revert(add(returnData, 32), mload(returnData)) } } - return 0; + return 0; // TODO: Consider returning a meaningful value or removing if not needed }contracts/src/test/tokens/gnosis/MockWETH.sol (4)
24-31: LGTM with a minor suggestion: Consider adding a check for zero value deposits.The receive and deposit functions are correctly implemented. However, you might want to consider adding a check for zero value deposits to save gas and prevent unnecessary event emissions.
Consider adding a check like this at the beginning of the deposit function:
require(msg.value > 0, "Cannot deposit 0 ETH");
33-38: LGTM with a suggestion: Consider using call() instead of transfer() for Ether transfers.The withdraw function is well-implemented, following the checks-effects-interactions pattern. However, using transfer() for Ether transfers is outdated and can cause issues with some contracts.
Consider replacing the transfer() call with a low-level call():
(bool success, ) = payable(msg.sender).call{value: wad}(""); require(success, "ETH transfer failed");This change will make the contract more robust and compatible with a wider range of recipient contracts.
54-68: LGTM with a suggestion: Consider adding a check for transfers to the zero address.The transferFrom function is well-implemented, correctly handling balances, allowances, and events. However, it's common in ERC20 implementations to prevent transfers to the zero address.
Consider adding a check at the beginning of the function:
require(dst != address(0), "WETH: transfer to the zero address");This will prevent accidental burns and align the implementation more closely with common ERC20 practices.
76-81: LGTM with a suggestion: Consider adding a check for burning zero tokens.The burn function is correctly implemented, properly checking balance and emitting the appropriate event. However, you might want to consider adding a check for burning zero tokens.
Consider adding a check at the beginning of the function:
require(wad > 0, "WETH: cannot burn 0 tokens");This will prevent unnecessary function calls and event emissions, potentially saving gas.
contracts/src/test/bridge-mocks/gnosis/MockAMB.sol (2)
73-73: DefineCHAIN_IDas a constant to improve maintainabilityThe chain ID
31337is hardcoded in multiple places. Defining it as a constant will enhance readability and make future updates easier.Add a constant declaration at the beginning of the contract:
contract MockAMB is IAMB { + uint16 public constant CHAIN_ID = 31337; event MockedEvent(bytes32 indexed messageId, bytes encodedData, bytes _data); // Rest of the contract... }Then update the occurrences of
31337:- messageSourceChainId = bytes32(uint256(31337)); + messageSourceChainId = bytes32(uint256(CHAIN_ID)); - bytes32 bridgeId = keccak256(abi.encodePacked(uint16(31337), address(this))) & + bytes32 bridgeId = keccak256(abi.encodePacked(CHAIN_ID, address(this))) & - uint16(31337), + CHAIN_ID,Also applies to: 86-87
8-8: Rename event parameters for clarity and consistencyThe event
MockedEventhas parametersencodedDataand_data, which might be confusing due to the underscore prefix and similar naming. Consider renaming_datato enhance clarity.For example:
- event MockedEvent(bytes32 indexed messageId, bytes encodedData, bytes _data); + event MockedEvent(bytes32 indexed messageId, bytes encodedData, bytes rawData);This change makes the parameter names more descriptive and consistent.
contracts/deploy/03-routers/03-arb-to-gnosis-router.ts (1)
53-68: Clarify the sequential deployment order due to dependencies.The sequential deployment of
SequencerInboxMock,OutboxMock, andBridgeMockis necessary due to their interdependencies. Consider adding comments to explain the dependency chain for better maintainability and to aid future developers' understanding.contracts/test/integration/ArbToGnosis.ts (2)
832-888: Refactor variable naming for clarityVariables like
newEpoch1,newEpoch2,newMerkleRoot1,newMerkleRoot2,newClaimTxOne, andnewClaimTxTwocan be renamed for better clarity and readability. Consider using more descriptive names that reflect their purpose in the test.
661-665: Improve test coverage for challenge scenariosThe test for submitting a challenge to a dishonest claim could include additional assertions to verify that the challenge process correctly updates the contract's state and that events are emitted as expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts (3 hunks)
- contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (2 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 (13)
contracts/src/test/bridge-mocks/arbitrum/ArbSysMockWithBridge.sol (1)
1-14: LGTM: Contract structure and imports are well-organized.The contract structure, including the license, custom tags, imports, and contract declaration, is clear and appropriate for a mock contract. The name
ArbSysMockWithBridgeaccurately reflects its purpose.contracts/src/test/bridge-mocks/arbitrum/BridgeMock.sol (1)
3-3: LGTM: Authorship update is appropriate.The addition of a new author to the custom authorship comment is a good practice for maintaining accurate attribution.
contracts/src/test/ArbToGnosis/VeaInboxArbToGnosisMock.sol (2)
1-14: LGTM: Contract structure and imports are well-organized.The contract structure, including the license, custom tags, imports, and Solidity version, is appropriate for a test contract. The use of a recent Solidity version (0.8.24) is commendable for security and feature utilization.
15-24: LGTM: Contract declaration and constructor are well-implemented.The contract
VeaInboxArbToGnosisMockis properly structured:
- It extends
VeaInboxArbToGnosisfor testing purposes.- The immutable
mockArbSysvariable allows for controlled testing of ArbSys interactions.- The constructor correctly initializes both the parent contract and the mock-specific functionality.
contracts/src/test/tokens/gnosis/MockWETH.sol (4)
1-22: LGTM: Contract structure and state variables are well-defined.The contract declaration, state variables, and events are correctly implemented. They follow the standard ERC20 and WETH token conventions.
40-52: LGTM: totalSupply, approve, and transfer functions are correctly implemented.These functions follow standard ERC20 implementations and are correctly adapted for a WETH contract. The use of transferFrom in the transfer function is an efficient approach.
70-74: LGTM: mintMock function is correctly implemented for testing purposes.The mintMock function is well-implemented for its intended use in testing. It correctly updates balances and emits the appropriate event.
Note that this function has no access control, which is acceptable for a mock contract but should not be included in a production WETH contract.
1-81: Overall, the MockWETH contract is well-implemented and suitable for testing purposes.The contract correctly implements all necessary functions for a WETH mock, including deposit, withdraw, and standard ERC20 functionalities. It follows good practices such as the checks-effects-interactions pattern and emits appropriate events for all state-changing operations.
While there are a few minor suggestions for improvement (such as adding checks for zero value operations and using call() instead of transfer()), these are not critical issues for a mock contract used in testing.
Great job on implementing this mock WETH contract!
contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (1)
28-33: Concurrent fetching of deployment parameters looks goodThe use of
Promise.allto fetchnamedAccounts,signers, andchainIdconcurrently enhances performance and aligns with best practices.contracts/deploy/03-routers/03-arb-to-gnosis-router.ts (3)
32-37: Efficient use ofPromise.allfor concurrent operations.Using
Promise.allto fetchnamedAccounts,signers, andchainIdconcurrently improves performance and code efficiency. Good job!
47-52: Enhanced deployment retrieval withPromise.all.Fetching deployments concurrently optimizes the code and reduces deployment time. This is a great improvement.
73-73: Ensure correct passing of contract addresses in deployment arguments.When deploying
RouterArbToGnosis, you're using the addresses of the deployed contracts as arguments. Please verify that these addresses correspond to the expected contract instances to prevent any misconfiguration.contracts/test/integration/ArbToGnosis.ts (1)
766-779: Ensure accurate calculation of withdrawal amountsIn the challenger’s withdrawal test, confirm that the reward calculation (deposit plus half of the bridger's deposit) is accurate. Any miscalculations could lead to incorrect fund distribution.
Run the following script to verify the calculation logic:
| beforeEach("Setup contracts and tokens", async () => { | ||
| // Deploy contracts | ||
| await deployments.fixture(["ArbToGnosisOutbox", "ArbToGnosisInbox", "ArbToGnosisRouter"], { | ||
| fallbackToGlobal: true, | ||
| keepExistingDeployments: false, | ||
| }); | ||
|
|
||
| // Get contract instances | ||
| veaOutbox = (await ethers.getContract("VeaOutboxArbToGnosis")) as VeaOutboxArbToGnosis; | ||
| receiverGateway = (await ethers.getContract("ArbToGnosisReceiverGateway")) as ReceiverGatewayMock; | ||
| veaInbox = (await ethers.getContract("VeaInboxArbToGnosis")) as VeaInboxArbToGnosis; | ||
| senderGateway = (await ethers.getContract("ArbToGnosisSenderGateway")) as SenderGatewayMock; | ||
| router = (await ethers.getContract("RouterArbToGnosis")) as RouterArbToGnosis; | ||
| amb = (await ethers.getContract("MockAMB")) as MockAMB; | ||
| weth = (await ethers.getContract("MockWETH")) as MockWETH; | ||
| arbsysMock = (await ethers.getContract("ArbSysMock")) as ArbSysMock; | ||
|
|
||
| // Setup initial token balances | ||
| await weth.deposit({ value: TEN_ETH.mul(100) }); | ||
| await weth.transfer(bridger.address, TEN_ETH.mul(10)); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reuse contract and token setup across tests
The contract deployments and initial token setups are repeated. To improve maintainability, you can utilize a shared before or beforeEach hook. This will make your tests cleaner and reduce redundancy.
| it("should allow bridger to withdraw deposit plus reward", async () => { | ||
| const { claimBlock } = await setupClaimAndChallenge(epoch, batchMerkleRoot, 0); | ||
|
|
||
| await simulateDisputeResolution(epoch, { | ||
| stateRoot: batchMerkleRoot, | ||
| claimer: bridger.address, | ||
| timestampClaimed: claimBlock.timestamp, | ||
| timestampVerification: 0, | ||
| blocknumberVerification: 0, | ||
| honest: 0, | ||
| challenger: challenger.address, | ||
| }); | ||
|
|
||
| const bridgerInitialBalance = await weth.balanceOf(bridger.address); | ||
| await veaOutbox.connect(bridger).withdrawClaimDeposit(epoch, { | ||
| stateRoot: batchMerkleRoot, | ||
| claimer: bridger.address, | ||
| timestampClaimed: claimBlock.timestamp, | ||
| timestampVerification: 0, | ||
| blocknumberVerification: 0, | ||
| honest: 1, | ||
| challenger: challenger.address, | ||
| }); | ||
| const bridgerFinalBalance = await weth.balanceOf(bridger.address); | ||
| expect(bridgerFinalBalance.sub(bridgerInitialBalance)).to.equal( | ||
| TEN_ETH.add(TEN_ETH.div(2)), | ||
| "Incorrect withdrawal amount" | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Missing assertions for deposit forfeiture
In the test case checking the bridger's withdrawal after a dishonest challenge, there are no assertions verifying that the challenger's deposit is correctly forfeited. Adding these assertions will ensure that the contract behaves as expected in this scenario.
| const { claimBlock } = await setupClaimAndChallenge(epoch, dishonestMerkleRoot, 0); | ||
|
|
||
| await simulateDisputeResolution(epoch, { | ||
| stateRoot: dishonestMerkleRoot, | ||
| claimer: bridger.address, | ||
| timestampClaimed: claimBlock.timestamp, | ||
| timestampVerification: 0, | ||
| blocknumberVerification: 0, | ||
| honest: 0, | ||
| challenger: challenger.address, | ||
| }); | ||
|
|
||
| // First withdrawal should succeed | ||
| await veaOutbox.connect(challenger).withdrawChallengeDeposit(epoch, { | ||
| stateRoot: dishonestMerkleRoot, | ||
| claimer: bridger.address, | ||
| timestampClaimed: claimBlock.timestamp, | ||
| timestampVerification: 0, | ||
| blocknumberVerification: 0, | ||
| honest: 2, | ||
| challenger: challenger.address, | ||
| }); | ||
|
|
||
| // Second withdrawal should fail | ||
| await expect( | ||
| veaOutbox.connect(challenger).withdrawChallengeDeposit(epoch, { | ||
| stateRoot: dishonestMerkleRoot, | ||
| claimer: bridger.address, | ||
| timestampClaimed: claimBlock.timestamp, | ||
| timestampVerification: 0, | ||
| blocknumberVerification: 0, | ||
| honest: 2, | ||
| challenger: challenger.address, | ||
| }) | ||
| ).to.be.revertedWith("Invalid claim."); | ||
| }); |
There was a problem hiding this comment.
Prevent multiple withdrawals with proper state updates
In the test for preventing multiple withdrawals, ensure that the contract correctly updates the state after the first withdrawal to prevent the second one. Add assertions to verify that the internal state changes as expected.
| const events = await amb.queryFilter(amb.filters.MockedEvent()); | ||
| const lastEvent = events[events.length - 1]; |
There was a problem hiding this comment.
Ensure 'events' array is not empty before accessing its elements
In the simulateDisputeResolution function, you access the last event without checking if the events array is not empty. If no events are emitted, this will cause an error.
Apply this diff to add a check before accessing the last event:
async function simulateDisputeResolution(epoch: number, claim: any) {
await veaInbox.connect(bridger).sendSnapshot(epoch, 100000, claim, { gasLimit: 100000 });
await network.provider.send("evm_increaseTime", [CHALLENGE_PERIOD + SEQUENCER_DELAY]);
await network.provider.send("evm_mine");
const events = await amb.queryFilter(amb.filters.MockedEvent());
+ if (events.length === 0) {
+ throw new Error("No MockedEvent emitted");
+ }
const lastEvent = events[events.length - 1];
await amb.executeMessageCall(
veaOutbox.address,
router.address,
lastEvent.args._data,
lastEvent.args.messageId,
1000000
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const events = await amb.queryFilter(amb.filters.MockedEvent()); | |
| const lastEvent = events[events.length - 1]; | |
| const events = await amb.queryFilter(amb.filters.MockedEvent()); | |
| if (events.length === 0) { | |
| throw new Error("No MockedEvent emitted"); | |
| } | |
| const lastEvent = events[events.length - 1]; |
| const to = "0x" + msg.slice(18, 58); | ||
| const msgData = "0x" + msg.slice(58); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hard-coded indices when parsing message data
Using fixed indices to slice the msg string can lead to errors if the message format changes. Consider parsing the message based on its structure or using a serialization library for robustness.
Apply this diff to refactor the parsing logic:
const msg = MessageSentEvent[0].args._nodeData;
-const nonce = "0x" + msg.slice(2, 18);
-const to = "0x" + msg.slice(18, 58);
-const msgData = "0x" + msg.slice(58);
+const nonce = msg.slice(0, 10);
+const to = ethers.utils.getAddress(msg.slice(10, 42));
+const msgData = msg.slice(42);Committable suggestion was skipped due to low confidence.
PR-Codex overview
This PR focuses on enhancing the functionality of the bridge contracts in the Arbitrum to Gnosis ecosystem. It includes new features for message execution and dispute resolution, as well as the introduction of mock contracts for testing.
Detailed summary
BridgeMockto includeexecuteL1Messagefunction.ArbSysMockWithBridgecontract for interacting withBridgeMock.VeaInboxArbToGnosisMockto simulate inbox functionality.MockAMBto include additional event data.MockWETHcontract with minting and burning functions.Summary by CodeRabbit
New Features
Bug Fixes
Documentation