Feat/arb sep chaido deploy #359
Conversation
madhurMongia
requested review from
jaybuidl,
mani99brar and
alcercu
as code owners
WalkthroughThe pull request introduces significant modifications across several files related to the deployment logic and configuration for blockchain networks. Key changes include adjustments to parameters for Changes
Possibly related PRs
Suggested reviewers
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: 25
🧹 Outside diff range and nitpick comments (31)
validator-cli/src/utils/arbMsgExecutor.ts (2)
9-9: Consider renamingtrnxHashtotxHashfor clarityThe parameter
trnxHashcan be renamed totxHashortxnHashto follow common naming conventions and improve readability.Apply this diff to rename the parameter and its usages:
export default async function messageExecutor( - trnxHash: string, + txHash: string, childRpc: string, parentRpc: string ): Promise<ContractTransaction> { const PRIVATE_KEY = process.env.PRIVATE_KEY; const childJsonRpc = new JsonRpcProvider(childRpc); const childProvider = new ArbitrumProvider(childJsonRpc); const parentProvider = new JsonRpcProvider(parentRpc); let childReceipt: TransactionReceipt | null; try { - childReceipt = await childProvider.getTransactionReceipt(trnxHash); + childReceipt = await childProvider.getTransactionReceipt(txHash); } catch (error) { - throw new Error(`Failed to get child transaction receipt: ${error.message}`); + throw new Error(`Failed to get child transaction receipt: ${error.message}`); } if (!childReceipt) { - throw new Error(`Transaction receipt not found for hash: ${trnxHash}`); + throw new Error(`Transaction receipt not found for hash: ${txHash}`); } // ... rest of the code }
22-22: Sanitize error messages to prevent sensitive data exposureIncluding the original error messages when throwing new errors might expose sensitive internal details. Consider sanitizing the error messages or logging the detailed errors internally while providing user-friendly messages to the caller.
Example of adjusted error handling:
- throw new Error(`Failed to get child transaction receipt: ${error.message}`); + throw new Error('Failed to get child transaction receipt.'); + // Optionally log the detailed error internally + console.error(error); ... - throw new Error(`Failed to retrieve child-to-parent messages: ${error.message}`); + throw new Error('Failed to retrieve child-to-parent messages.'); + // Optionally log the detailed error internally + console.error(error); ... - throw new Error(`Message execution failed: ${error.message}`); + throw new Error('Message execution failed.'); + // Optionally log the detailed error internally + console.error(error);Also applies to: 39-39, 47-47
contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (2)
4-5: Ensure consistent imports from 'hardhat' packageSince
ethersis imported from"hardhat", consider importingprovidersfrom"hardhat"as well for consistency and to leverage any Hardhat-specific enhancements.Apply this diff to adjust the import statements:
-import { providers } from "ethers"; +import { providers } from "hardhat";
91-91: Use enum constants for chain ID comparison to improve readabilityIn the chain ID comparison
(chainId === 42161 ? "" : "Testnet"), consider using theSenderChains.ARBITRUMenum constant instead of the numeric literal to enhance code clarity and maintainability.Apply this diff to update the code:
-await deploy("VeaInboxArbToGnosis" + (chainId === 42161 ? "" : "Testnet"), { +await deploy("VeaInboxArbToGnosis" + (chainId === SenderChains.ARBITRUM ? "" : "Testnet"), {contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts (1)
129-135: Review gas limits and configurations forReceiverGatewayMockdeploymentThe
gasLimitis set to4000000for deployingReceiverGatewayMock. Confirm that this value is sufficient and appropriate for the network you're deploying to.Consider adjusting the
gasLimitbased on the network requirements or making it configurable.contracts/test/integration/ArbToGnosis.ts (5)
127-127: Redundant assertion in testThe assertion compares
veaInbox.snapshots(epoch)to itself, which is redundant sincebatchMerkleRootis assigned the same value just before. Consider verifying thatbatchMerkleRootis notnullor matches an expected value instead.
43-51: Define a TypeScript interface for theclaimobjectCurrently, the
claimobject is typed asany. Defining a specific TypeScript interface forclaimwould improve type safety and code readability.
54-55: RedundantgasLimitparameter insendSnapshotcallThe
gasLimitis passed both as a parameter and in the transaction options:await veaInbox.connect(bridger).sendSnapshot(epoch, 100000, claim, { gasLimit: 100000 });Consider removing the redundant parameter if it's not required by the function signature.
132-153: Refactor repeated setup code in test casesMultiple test cases have repeated code for setting up data, sending messages, saving snapshots, and advancing time. Consider creating helper functions to encapsulate this logic, improving maintainability and reducing code duplication.
Also applies to: 158-188, 192-235, 238-350
554-554: Use a realistic value fordishonestMerkleRootUsing
ethers.utils.keccak256("0x123456")as thedishonestMerkleRootmay not effectively simulate a dishonest state root. Consider generating a value that is structurally similar to an actual Merkle root but intentionally incorrect to enhance test validity.validator-cli/src/ArbToEth/watcherArbToGnosis.ts (3)
214-214: Address the TODO comments regarding epoch synchronization and profitability checks.
Line 214: The TODO mentions that
veaEpochOutboxClaimableNowis sometimes one epoch behindveaEpochOutboxClaimableNowOld. Investigate and resolve this discrepancy to ensure accurate epoch tracking and prevent potential off-by-one errors.Line 340: There's a TODO to check the profitability of the whole dispute resolution process. Implement this logic to ensure the watcher engages in disputes only when it is financially viable.
Would you like assistance in addressing these issues or should I open new GitHub issues to track them?
Also applies to: 340-340
337-337: Fix typographical errors in log messages and comments.
Line 337: The word "progess" should be corrected to "progress" in the log message.
Line 1089: In the comment, "challenged progess" should be "challenge progress", and "recontructed" should be "reconstructed".
Also applies to: 1089-1089
739-747: Complete the implementation of L2 to L1 gas cost estimation.In the
calculateDisputeResolutionProfitabilityfunction, there's a TODO at line 785 indicating that the L2 to L1 message execution gas cost calculation is pending. This is essential for accurately assessing the profitability of dispute resolutions.Would you like assistance in implementing the gas cost estimation for L2 to L1 message execution?
Also applies to: 785-786
relayer-cli/package.json (1)
Line range hint
11-13: Consider removingnpxfrom the script command.Since ts-node is now explicitly installed as a dependency, using
npxis unnecessary. The script can directly use the locally installed package."scripts": { - "start-devnet-relayer": "npx ts-node ./src/devnetRelayExample.ts" + "start-devnet-relayer": "ts-node ./src/devnetRelayExample.ts" },contracts/src/test/bridge-mocks/arbitrum/ArbSysMockWithBridge.sol (1)
4-7: Consider assigning reviewers and completing documentation tagsThe custom documentation tags for reviewers, auditors, bounties, and deployments are empty. These tags are important for tracking the review process and deployment history.
contracts/src/canonical/gnosis-chain/IAMB.sol (1)
16-21: Consider adding NatSpec documentation for the event.Adding NatSpec documentation would improve interface clarity by explaining the purpose and parameters of the event.
+ /// @notice Emitted when a cross-chain message affirmation is completed + /// @param sender The address that initiated the message + /// @param executor The contract address that executed the message + /// @param messageId The unique identifier of the message + /// @param status The execution status of the message event AffirmationCompleted( address indexed sender, address indexed executor, bytes32 indexed messageId, bool status );contracts/src/test/bridge-mocks/arbitrum/BridgeMock.sol (2)
Line range hint
3-8: Add reviewers to the documentation.The
@custom:reviewersfield should be populated before merging to maintain a clear record of code review ownership.
31-42: Implementation looks good with minor suggestions.The function correctly simulates L1 message execution with proper error propagation. Consider these improvements:
- Add an event emission for better debugging visibility
- Add input validation for the destination address (e.g., check for address(0))
function executeL1Message(address destination, bytes calldata data) external returns (uint256) { + require(destination != address(0), "Invalid destination"); + // Simulate the bridge calling the destination contract (bool success, bytes memory returnData) = destination.call(data); if (!success) { assembly { revert(add(returnData, 32), mload(returnData)) } } + emit L1MessageExecuted(destination, data); return 0; } + event L1MessageExecuted(address destination, bytes data);contracts/src/test/ArbToGnosis/VeaInboxArbToGnosisMock.sol (2)
3-7: Consider completing the documentation tagsWhile this is a test contract, completing the documentation tags would improve maintainability by clearly indicating:
- Who has reviewed the contract
- Whether it has been audited
- Any relevant bounties
- Where it's deployed in test environments
18-24: Consider adding parameter validation in constructorWhile this is a test contract, it's good practice to validate constructor parameters to prevent deployment with invalid values:
_epochPeriodshould be greater than 0_routerArbToGnosisshould not be address(0)_mockArbSysshould not be address(0)constructor( uint256 _epochPeriod, address _routerArbToGnosis, IArbSys _mockArbSys ) VeaInboxArbToGnosis(_epochPeriod, _routerArbToGnosis) { + require(_epochPeriod > 0, "Invalid epoch period"); + require(address(_mockArbSys) != address(0), "Invalid mock address"); mockArbSys = _mockArbSys; }contracts/src/test/tokens/gnosis/MockWETH.sol (1)
9-9: Consider using a more specific Solidity version.Using a floating pragma (
^0.8.0) could lead to inconsistent behavior across different compiler versions. Consider pinning to a specific version for better reproducibility and security.-pragma solidity ^0.8.0; +pragma solidity 0.8.19;relayer-cli/src/devnetRelayExample.ts (3)
5-6: Fix incorrect comment for epochPeriodThe comment states "30 min" but the value 3600 seconds equals 1 hour.
-const epochPeriod = 3600; // 30 min +const epochPeriod = 3600; // 1 hour
37-37: Improve error message for chain ID validationThe current error message "Invalid chainid" could be more helpful by including:
- The invalid chain ID that was provided
- The list of supported chain IDs
- if (!chain_ids.includes(chain_id)) throw new Error("Invalid chainid"); + if (!chain_ids.includes(chain_id)) { + throw new Error(`Invalid chain ID ${chain_id}. Supported chain IDs: ${chain_ids.join(", ")}`); + }
Line range hint
46-52: Add environment variable validation and improve error handlingSeveral potential issues in the state management:
- Missing validation for required STATE_DIR environment variable
- No error handling for file system operations
- Potential race condition in lock file management
Consider adding these safety checks:
+ if (!process.env.STATE_DIR) { + throw new Error("STATE_DIR environment variable is required"); + } + const state_file = process.env.STATE_DIR + chain_id + ".json"; if (!fs.existsSync(state_file)) { // No state file so initialize starting now const tsnow = Math.floor(Date.now() / 1000); - await updateStateFile(chain_id, tsnow, 0); + try { + await updateStateFile(chain_id, tsnow, 0); + } catch (error) { + console.error(`Failed to initialize state file for chain ${chain_id}:`, error); + throw error; + } }contracts/src/test/bridge-mocks/gnosis/MockAMB.sol (1)
37-37: Consider making chain IDs configurableThe contract currently uses hardcoded chain IDs (31337). For better testing flexibility and reusability, consider:
- Making chain IDs configurable through constructor parameters
- Adding setter functions for dynamic updates during tests
- Using constants for commonly used test chain IDs
This would make the mock more versatile for testing different chain configurations.
Also applies to: 86-87
contracts/deploy/03-routers/03-arb-to-gnosis-router.ts (3)
33-38: Add error handling for concurrent operationsWhile the concurrent fetching of accounts is more efficient, consider adding proper error handling for the Promise.all operation to gracefully handle potential failures in account retrieval.
- const [namedAccounts, defaultSigner] = await Promise.all([ - getNamedAccounts(), - hre.ethers.getSigners().then((signers) => signers[0]), - ]); + try { + const [namedAccounts, defaultSigner] = await Promise.all([ + getNamedAccounts(), + hre.ethers.getSigners().then((signers) => signers[0]), + ]); + const deployer = namedAccounts.deployer ?? defaultSigner.address; + console.log("deployer: %s", deployer); + } catch (error) { + throw new Error(`Failed to retrieve deployer address: ${error.message}`); + }
51-66: Optimize mock contracts deployment and document magic numbersThe sequential deployment of mock contracts could be optimized, and the hardcoded value needs documentation.
- Document the significance of "10" in SequencerInboxMock
- Consider parallel deployment for independent contracts:
- const sequencerInbox = await deploy("SequencerInboxMock", { - from: deployer, - contract: "SequencerInboxMock", - args: ["10"], - }); - const outbox = await deploy("OutboxMock", { - from: deployer, - args: [veaInbox.address], - log: true, - }); + // Deploy independent contracts concurrently + const [sequencerInbox, outbox] = await Promise.all([ + deploy("SequencerInboxMock", { + from: deployer, + contract: "SequencerInboxMock", + args: ["10"], // Represents the default delay in blocks for sequencer + }), + deploy("OutboxMock", { + from: deployer, + args: [veaInbox.address], + log: true, + }) + ]);
Line range hint
1-1: Consider implementing deterministic deploymentsThe TODO comment at the beginning suggests implementing deterministic deployments. This would provide several benefits:
- Predictable contract addresses across different networks
- Easier verification and auditing
- More reliable deployment process
Consider implementing deterministic deployments using:
- The
@openzeppelin/hardhat-upgradesplugin for proxy-based upgradeable contracts- Custom salt for create2 deployments
- Hardhat's built-in deterministic deployment features
Would you like assistance in implementing deterministic deployments?
contracts/deployments/arbitrumSepolia/VeaInboxArbToGnosisTestnet.json (3)
20-76: Consider adding events for critical state changesWhile the contract has well-defined events for message sending and snapshot operations, consider adding events for critical state changes such as:
- Contract initialization (in constructor)
- Error cases in message sending
- Failed snapshot operations
This would improve the contract's observability and make it easier to monitor its health.
392-446: Verify storage limitations are acceptable for production useThe contract has the following storage limitations:
- Fixed inbox array size of 64 entries for the merkle tree
- Message count limited to uint64 (2^64-1)
While the comment mentions "it would take at least 8 million years of full blocks to overflow", consider:
- Documenting these limitations in the contract's interface
- Adding runtime checks for the merkle tree depth
168-196: Consider additional gas optimizationsWhile the contract already implements several gas optimizations, consider:
- Using
calldatainstead ofmemoryfor_dataparameter insendMessagewhen the data is not modified- Caching array length in loops
- Using
uncheckedfor more arithmetic operations where overflow is impossibleExample optimization for the
sendMessagefunction:-function sendMessage(address _to, bytes4 _fnSelector, bytes memory _data) external override returns (uint64) +function sendMessage(address _to, bytes4 _fnSelector, bytes calldata _data) external override returns (uint64)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (20)
contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts(4 hunks)contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts(3 hunks)contracts/deploy/03-routers/03-arb-to-gnosis-router.ts(1 hunks)contracts/deployments/arbitrumSepolia/VeaInboxArbToGnosisTestnet.json(1 hunks)contracts/deployments/chiado/VeaInboxGnosisToArbTestnet.json(0 hunks)contracts/deployments/sepolia/RouterArbToGnosisTestnet.json(1 hunks)contracts/src/canonical/gnosis-chain/IAMB.sol(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)relayer-cli/package.json(1 hunks)relayer-cli/src/devnetRelayExample.ts(3 hunks)validator-cli/.env.dist(1 hunks)validator-cli/package.json(2 hunks)validator-cli/src/ArbToEth/watcherArbToGnosis.ts(14 hunks)validator-cli/src/utils/arbMsgExecutor.ts(1 hunks)validator-cli/src/utils/ethers.ts(3 hunks)
💤 Files with no reviewable changes (1)
- contracts/deployments/chiado/VeaInboxGnosisToArbTestnet.json
🔇 Additional comments (34)
contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (3)
22-22: Initialization of 'routerAddress' for HARDHAT chain is appropriate
Adding routerAddress: ethers.constants.AddressZero correctly initializes the router address for the HARDHAT chain in paramsByChainId.
30-36: Fallback mechanism for deployer account is well-implemented
The code effectively retrieves the deployer account, falling back to the default signer when namedAccounts.deployer is undefined. This approach enhances flexibility across different network environments.
100-104: Conditional deployment logic correctly selects deployment strategy
The script appropriately chooses between the hardhatDeployer and liveDeployer functions based on the chain ID, ensuring the correct deployment process for the specific network environment.
contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts (4)
29-29: Confirm epochPeriod reduction aligns with network requirements
The epochPeriod for GNOSIS_CHIADO has been reduced from 7200 seconds (2 hours) to 3600 seconds (1 hour). Ensure this change is intentional and that the network can handle the increased frequency of epochs without adverse effects.
Would you like to run a verification script to assess the impact of this change?
39-49: Review updated parameters for HARDHAT network
The adjustments to the HARDHAT network parameters include:
- Deposit increased: From an unspecified previous value to
parseEther("10"). minChallengePeriodset: Defined explicitly as 600 seconds (10 minutes).numEpochTimeoutextended: Set to 21600 seconds (6 hours).sequencerLimitreduced: From 86400 seconds (24 hours) to 864 seconds (14.4 minutes).
Ensure these changes align with your testing requirements and that they reflect the intended simulation conditions.
59-64: Efficient asynchronous retrieval of accounts
Great use of Promise.all to fetch namedAccounts and defaultSigner concurrently. This optimization improves the efficiency of asynchronous operations.
111-124: Verify deployment arguments for accuracy
In the deployment of VeaOutboxArbToGnosis, ensure that the arguments provided match the intended deployment environment:
ambAddress: Confirm thatambMock.addressis correct for local testing and that the actualambaddress is used in live deployments.routerAddress: Verify that the calculatedrouterAddressis accurate and aligns with the router deployment.WETHAddress: EnsurewethMock.addressis appropriate for testing and the correct WETH address is used in production.
Would you like to run a script to check the correctness of these addresses across different environments?
relayer-cli/package.json (1)
24-25: LGTM! Adding ts-node as explicit dependency.
The addition of ts-node as a devDependency is correct as it formalizes a previously implicit dependency used in the start-devnet-relayer script.
contracts/src/test/bridge-mocks/arbitrum/ArbSysMockWithBridge.sol (3)
11-12: LGTM! Clean and appropriate imports
The imports clearly separate the canonical interface from the mock implementation.
17-19: LGTM! Clean constructor implementation
The constructor correctly initializes the immutable bridge reference.
14-15: Verify complete implementation of IArbSys interface
The contract implements IArbSys but only provides sendTxToL1. Let's verify if there are other required interface methods.
✅ Verification successful
Interface implementation is complete
The IArbSys interface in the canonical implementation only requires one method sendTxToL1, which is already implemented in the contract. The interface is intentionally pruned to include only the relevant function as noted in the comment "interface is pruned for relevant function stubs".
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the complete IArbSys interface definition
ast-grep --pattern 'interface IArbSys {
$$$
}'
Length of output: 74
Script:
#!/bin/bash
# Let's try to find the interface file first
fd -e sol -e abi IArbSys
# Then let's search for interface or contract definitions containing ArbSys
rg -l "interface.*IArbSys|contract.*IArbSys"
Length of output: 758
Script:
#!/bin/bash
# Let's examine the interface definition
cat contracts/src/canonical/arbitrum/IArbSys.sol
Length of output: 1191
contracts/src/canonical/gnosis-chain/IAMB.sol (1)
16-21: LGTM! Well-structured event declaration for cross-chain message tracking.
The event parameters are appropriately indexed for efficient filtering and the structure provides comprehensive tracking of message affirmations between chains.
validator-cli/.env.dist (1)
10-14: Add documentation for bridge contract addresses
The new arbToGnosis bridge configuration lacks documentation about:
- Which network each address belongs to (Arbitrum or Gnosis)
- The purpose and role of each contract (especially GNO_AMB_ADDRESS)
- Any specific requirements or dependencies between these contracts
Add comments to clarify the contract addresses:
#For arbToGnosis bridge
+# Addresses on Arbitrum network
VEAINBOX_ARB_TO_GNOSIS_ADDRESS=0x854374483572FFcD4d0225290346279d0718240b
VEAOUTBOX_ARB_TO_GNOSIS_ADDRESS=0x2f1788F7B74e01c4C85578748290467A5f063B0b
VEAROUTER_ARB_TO_GNOSIS_ADDRESS=0x5BE03fDE7794Bc188416ba16932510Ed1277b193
+# Gnosis network AMB (Arbitrary Message Bridge) contract
GNO_AMB_ADDRESS=0x8448E15d0e706C0298dECA99F0b4744030e59d7dLet's verify these contract addresses:
validator-cli/package.json (3)
15-16: LGTM! Script changes are well-structured
The new script for Chaido testnet follows the existing naming pattern and properly utilizes ts-node for TypeScript execution.
29-31: LGTM! Proper separation of development dependency
The addition of ts-node as a devDependency is appropriate as it's only needed during development for running TypeScript files directly. The version is compatible with the specified Node.js engine requirement.
19-19: Verify compatibility with @arbitrum/sdk v4.0.1
The major version bump from ^3.1.2 to 4.0.1 indicates breaking changes. Additionally, removing the caret (^) enforces a strict version requirement.
Let's verify the breaking changes and their impact:
✅ Verification successful
Let me check the actual usage of the SDK to understand if we're affected by the breaking change.
Breaking changes in @arbitrum/sdk v4.0.1 do not affect this codebase
The codebase uses getArbitrumNetwork() helper function instead of creating custom network instances. The breaking change in v4.0.1 only affects cases where custom networks are defined using new ArbitrumNetwork(), which is not present in this codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in @arbitrum/sdk v4.0.1
# Search for @arbitrum/sdk usage in the codebase
echo "Searching for @arbitrum/sdk imports and usage:"
rg -l "@arbitrum/sdk"
# Check the changelog or release notes
echo -e "\nFetching release information:"
gh api repos/OffchainLabs/arbitrum-sdk/releases/tags/v4.0.1 --jq '.body'
Length of output: 771
Script:
#!/bin/bash
# Check how ArbitrumNetwork is used in the codebase
echo "Searching for ArbitrumNetwork usage:"
rg -A 5 "ArbitrumNetwork"
# Check for custom network definitions
echo -e "\nSearching for network definitions:"
ast-grep --pattern 'new ArbitrumNetwork($_)'
Length of output: 2163
contracts/src/test/bridge-mocks/arbitrum/BridgeMock.sol (1)
31-42: Verify consistency with production bridge behavior.
Let's ensure this mock implementation matches the actual Arbitrum bridge behavior.
contracts/src/test/ArbToGnosis/VeaInboxArbToGnosisMock.sol (2)
9-13: LGTM! Good choice of Solidity version and clean imports.
Using the latest Solidity version (0.8.24) provides access to the newest security features and optimizations.
15-16: LGTM! Good use of immutable storage.
The contract correctly extends the base contract and uses immutable storage for the mock system interface, which is both gas efficient and ensures the mock can't be changed after deployment.
contracts/src/test/tokens/gnosis/MockWETH.sol (1)
11-81: Verify interface compatibility with Gnosis WETH.
Please ensure this mock implementation matches the interface of the actual WETH contract deployed on Gnosis chain.
✅ Verification successful
The mock WETH implementation matches the required interface and functionality
The MockWETH contract correctly implements all the required functions from the IWETH interface:
- burn(uint256)
- balanceOf(address)
- transferFrom(address,address,uint256)
- transfer(address,uint256)
- allowance(address,address)
- approve(address,uint256)
The implementation includes proper balance checks, allowance handling, and event emissions, making it suitable for testing the bridge functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify interface compatibility with Gnosis WETH
# Search for WETH interface or implementation references
rg -l "WETH" --type solidity
# Search for WETH address configurations
rg -l "WETH_ADDRESS" --type typescript --type javascript
Length of output: 153
Script:
#!/bin/bash
# Search for WETH related files and content
rg -l "WETH"
# Search for actual WETH implementation or interface details
rg -A 5 "contract.*WETH|interface.*WETH"
# Look for WETH address configurations
rg "WETH.*address|weth.*address" -i
Length of output: 332890
Script:
#!/bin/bash
# Search for WETH interface implementation in the mock
ast-grep --pattern $'interface IWETH {
$$$
function burn(uint256 _value) external;
$$$
function balanceOf(address _owner) external view returns (uint256);
$$$
function transferFrom(address _from, address _to, uint256 _value) external returns (bool);
$$$
function transfer(address _to, uint256 _value) external returns (bool);
$$$
function allowance(address _owner, address _spender) external view returns (uint256);
$$$
function approve(address _spender, uint256 _value) external returns (bool);
$$$
}'
# Search for WETH implementation in the mock
ast-grep --pattern $'contract MockWETH {
$$$
function burn(uint256 wad) public {
$$$
}
$$$
function balanceOf(address) public view returns (uint) {
$$$
}
$$$
function transferFrom(address src, address dst, uint wad) public returns (bool) {
$$$
}
$$$
function transfer(address dst, uint wad) public returns (bool) {
$$$
}
$$$
function allowance(address, address) public view returns (uint) {
$$$
}
$$$
function approve(address guy, uint wad) public returns (bool) {
$$$
}
$$$
}'
Length of output: 1070
Script:
#!/bin/bash
# Search for WETH interface and implementation
rg -A 5 "interface IWETH|contract MockWETH" --type-add 'sol:*.sol' --type sol
# Search for WETH implementation details
rg -A 10 "function burn|function balanceOf|function transferFrom|function transfer|function allowance|function approve" --type-add 'sol:*.sol' --type sol contracts/src/test/tokens/gnosis/MockWETH.sol
Length of output: 2208
relayer-cli/src/devnetRelayExample.ts (1)
25-25: Verify sender address configuration and permissions
The hardcoded sender address should be verified for:
- Existence on Arbitrum Sepolia
- Proper permissions for the lightbulb switch contract
- Consider making this configurable via environment variables for different environments
contracts/src/test/bridge-mocks/gnosis/MockAMB.sol (3)
8-8: LGTM: Enhanced event logging
The addition of the _data parameter to MockedEvent improves observability by including the raw message data in the event logs.
73-73: LGTM: Consistent chain ID updates with secure implementation
The changes maintain security while updating chain IDs:
- The bridgeId calculation preserves the secure bit manipulation pattern
- Chain ID updates are consistent across the contract
- Event emission properly includes the new data parameter
Also applies to: 86-87, 91-91
37-44: Verify the chain ID change and approve error handling improvements
The changes include:
- Chain ID update from 1337 to 31337
- Enhanced error handling that properly propagates the revert reason from the called contract
The error handling improvement is excellent, but let's verify the chain ID change.
✅ Verification successful
Chain ID change to 31337 is consistent across the codebase
The chain ID change from 1337 to 31337 is verified to be correct. The search results show that:
- 31337 is consistently used as the Hardhat chain ID across the codebase
- It's properly configured in
hardhat.config.ts - All deployment scripts and test files use 31337
- The MockAMB contract's usage of 31337 aligns with the rest of the codebase
The error handling improvements are also appropriate, using the standard assembly pattern for error propagation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of chain IDs to ensure consistency
rg -g '*.sol' -g '*.js' -g '*.ts' '1337|31337'
# Search for any deployment configurations that might reference these chain IDs
fd -g 'deploy*.{js,ts,json}' --exec cat {} \; | grep -A 5 -B 5 'chainId'
Length of output: 4206
validator-cli/src/utils/ethers.ts (3)
12-13: LGTM! Import changes are well-organized
The new factory imports are properly placed alongside other similar imports and correspond to the new provider functions.
68-70: LGTM! Provider function follows established patterns
The implementation is consistent with other provider functions in the file.
84-84: LGTM! Exports are properly updated
The new functions are correctly exported and maintain consistent ordering with similar exports.
Also applies to: 86-86
contracts/deploy/03-routers/03-arb-to-gnosis-router.ts (1)
71-71: Verify contract addresses in router deployment arguments
The router deployment uses multiple contract addresses. Consider adding validation to ensure all addresses are valid.
contracts/deployments/arbitrumSepolia/VeaInboxArbToGnosisTestnet.json (1)
2-2: Verify router address configuration
The contract is deployed with:
- Router address:
0x22d70804d4Ef5BB206C6B39e3267DFe8a0f97d27 - Epoch period: 3600 seconds (1 hour)
Also applies to: 294-295
✅ Verification successful
Let me gather more information about the router contract to verify its configuration.
Let me gather more information about the VeaInbox contract to verify its configuration.
Based on the gathered information, I can now provide the final response:
Router configuration is correctly set up
The VeaInbox contract at 0x854374483572FFcD4d0225290346279d0718240b is properly configured with:
- Router address:
0x22d70804d4Ef5BB206C6B39e3267DFe8a0f97d27(matches the deployment) - Epoch period: 3600 seconds (1 hour)
The router contract has the expected interface with the route function and is referenced consistently across the deployment artifacts for the Arbitrum-to-Gnosis bridge system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if the router address is correctly configured and has the expected interface
# Check if the router contract exists and has the expected interface
ast-grep --pattern $'contract $_ {
$$$
function route(uint256 _epoch, bytes32 _stateRoot, uint256 _gasLimit, Claim memory _claim) external {
$$$
}
$$$
}'
# Check if there are any other contracts using this router address
rg -l "0x22d70804d4Ef5BB206C6B39e3267DFe8a0f97d27"
Length of output: 409
Script:
#!/bin/bash
# Check the router contract deployment details and interface
cat contracts/deployments/sepolia/RouterArbToGnosisTestnet.json
# Check if there are any related configuration files
fd -e json -e yaml -e toml -e config router
Length of output: 50710
Script:
#!/bin/bash
# Check VeaInbox contract deployment details
cat contracts/deployments/arbitrumSepolia/VeaInboxArbToGnosisTestnet.json
# Check if there are any related configuration files
fd -e json -e yaml -e toml -e config inbox
Length of output: 49103
contracts/deployments/sepolia/RouterArbToGnosisTestnet.json (5)
121-184: Review security considerations for cross-chain message routing
The route function implementation includes important security checks:
- Validates msg.sender is the bridge contract
- Verifies l2ToL1Sender is the veaInbox
- Uses AMB's maxGasPerTx for gas limit capping
This follows security best practices for cross-chain message passing.
345-394: Review storage layout
The contract has an efficient storage layout:
- sequencerDelayLimit (slot 0)
- sequencerDelayLimitDecreaseRequest (slots 1-2)
The storage layout is optimized and follows best practices for minimizing storage costs.
251-266: Verify deployment transaction status
The deployment transaction (0x6ce9144ad70f3773de005f80c3995eaec001593d90a4760cd44b3c0271614930) was successful:
- Status: 1 (Success)
- Block: 6696213
- Gas Used: 848,386
✅ Verification successful
Deployment verification successful with proper cross-chain integration
The deployment transaction has been verified successfully with proper integration across the bridge components:
- Contract deployed at
0x22d70804d4Ef5BB206C6B39e3267DFe8a0f97d27is correctly referenced in:- Arbitrum Sepolia's VeaInbox configuration
- Gnosis Chiado's VeaOutbox configuration
- Transaction status is successful (1) with expected gas consumption
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the deployment transaction details
# Search for any references to this transaction hash
rg "0x6ce9144ad70f3773de005f80c3995eaec001593d90a4760cd44b3c0271614930"
Length of output: 469
Script:
#!/bin/bash
# Let's verify the contract deployment details more thoroughly
# 1. Check if this contract address is referenced elsewhere
rg "0x22d70804d4Ef5BB206C6B39e3267DFe8a0f97d27"
# 2. Look for any router deployment configurations
fd -e json -e js -e ts "router" --exec grep -l "RouterArbToGnosisTestnet"
# 3. Check for any deployment scripts that might reference this router
fd -e js -e ts "deploy" --exec grep -l "RouterArbToGnosisTestnet"
Length of output: 662
1-2: Verify contract deployment status
The contract has been successfully deployed to address 0x22d70804d4Ef5BB206C6B39e3267DFe8a0f97d27 on Sepolia testnet.
✅ Verification successful
Contract deployment and configuration verified successfully
The contract has been properly deployed and configured:
- Successfully deployed at
0x22d70804d4Ef5BB206C6B39e3267DFe8a0f97d27on Sepolia testnet - Correctly referenced by VeaInbox on Arbitrum Sepolia and VeaOutbox on Chiado
- Constructor parameters properly set:
- bridge: 0x38f918D0E9F1b721EDaA41302E399fa1B79333a9
- amb: 0xf2546D6648BD2af6a008A7e7C1542BB240329E11
- veaInboxArbToGnosis: 0x72c7d51647cBeaca636d0E20A66ca2F682da3539
- veaOutboxArbToGnosis: 0xa3C6608539693C13434e4E29c9aB53Dd029178BE
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the contract deployment and code match
# Check if the contract is verified on Etherscan
gh api graphql -f query='
{
repository(owner: "madhurMongia", name: "vea") {
object(expression: "HEAD") {
... on Commit {
history(path: "contracts/deployments/sepolia/RouterArbToGnosis.json") {
nodes {
messageBody
oid
}
}
}
}
}
}'
Length of output: 586
Script:
#!/bin/bash
# Let's verify the contract deployment file and its references
# First, check if this file exists and its content
cat contracts/deployments/sepolia/RouterArbToGnosisTestnet.json
# Check for any related contract source files
fd -e sol -e json Router
# Look for any references to this contract address
rg -i "0x22d70804d4Ef5BB206C6B39e3267DFe8a0f97d27"
# Check for any deployment scripts that might have deployed this contract
fd -e js -e ts deploy
Length of output: 51454
267-271: Verify constructor parameters
The contract was initialized with the following addresses:
- bridge: 0x38f918D0E9F1b721EDaA41302E399fa1B79333a9 (Arbitrum Bridge)
- amb: 0xf2546D6648BD2af6a008A7e7C1542BB240329E11 (AMB Bridge)
- veaInboxArbToGnosis: 0x72c7d51647cBeaca636d0E20A66ca2F682da3539
- veaOutboxArbToGnosis: 0xa3C6608539693C13434e4E29c9aB53Dd029178BE
✅ Verification successful
Based on the code analysis, I can now provide a final response as the verification is complete.
Constructor parameters are correctly configured for Sepolia testnet
The provided constructor parameters in the deployment file match the expected configuration:
bridge(0x38f918D0E9F1b721EDaA41302E399fa1B79333a9): Arbitrum Bridge on Sepoliaamb(0xf2546D6648BD2af6a008A7e7C1542BB240329E11): AMB Bridge for Gnosis Chain on Sepolia- The VeaInbox and VeaOutbox addresses are fetched from companion networks (arbitrumSepolia and chiado respectively)
The contract implementation in RouterArbToGnosis.sol confirms these parameters are used correctly:
bridgefor Arbitrum message verificationambfor message passing to Gnosis ChainveaInboxandveaOutboxfor cross-chain communication
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the addresses are valid contracts on Sepolia
# Search for contract interactions with these addresses
rg -A 5 "0x38f918D0E9F1b721EDaA41302E399fa1B79333a9|0xf2546D6648BD2af6a008A7e7C1542BB240329E11|0x72c7d51647cBeaca636d0E20A66ca2F682da3539|0xa3C6608539693C13434e4E29c9aB53Dd029178BE"
Length of output: 469
Script:
#!/bin/bash
# Let's search for these addresses in deployment files and contract source code
rg -l "0x38f918D0E9F1b721EDaA41302E399fa1B79333a9|0xf2546D6648BD2af6a008A7e7C1542BB240329E11|0x72c7d51647cBeaca636d0E20A66ca2F682da3539|0xa3C6608539693C13434e4E29c9aB53Dd029178BE"
# Let's also look for the contract implementation to verify the constructor parameters
fd RouterArbToGnosisTestnet --extension sol
Length of output: 887
Script:
#!/bin/bash
# Let's check the contract source code to verify constructor parameters
fd Router --extension sol | xargs cat
# Also check the deployment script specifically for this router
cat contracts/deploy/03-routers/03-arb-to-gnosis-router.ts
Length of output: 30440
madhurMongia
force-pushed
the
feat/arbSep-chaido-deploy
branch
from
74643ae to
f1d765f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
validator-cli/.env.dist(1 hunks)validator-cli/src/utils/ethers.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- validator-cli/.env.dist
🔇 Additional comments (3)
validator-cli/src/utils/ethers.ts (3)
12-13: LGTM!
The new imports are properly organized and necessary for the added functionality.
68-70: LGTM!
The function follows the established pattern and best practices by accepting the router address as a parameter.
84-86: LGTM!
The exports are properly updated to include the new provider functions.
madhurMongia
force-pushed
the
feat/arbSep-chaido-deploy
branch
2 times, most recently
from
c0bb829 to
e197be5
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (3)
43-47: Consider using chain IDs as keys for network mappingThe current implementation uses string enum keys which could be fragile if network names change. Consider using chain IDs directly as keys for better maintainability.
const routerNetworks = { - ARBITRUM: config.networks.mainnet, - ARBITRUM_SEPOLIA: config.networks.sepolia, - HARDHAT: config.networks.localhost, + [SenderChains.ARBITRUM]: config.networks.mainnet, + [SenderChains.ARBITRUM_SEPOLIA]: config.networks.sepolia, + [SenderChains.HARDHAT]: config.networks.localhost, };
Line range hint
53-85: Consider implementing deployment guardsThe deployment sequence relies on exact nonce calculations and assumes contracts will be deployed in perfect order. This is fragile and could lead to incorrect contract linking if any deployment fails.
Consider:
- Adding deployment status checks between steps
- Implementing a rollback mechanism for failed deployments
- Storing intermediate addresses in a deployment manifest
Would you like assistance in implementing these safeguards?
93-98: Make contract name selection more explicitThe conditional contract naming could be clearer and more maintainable.
- await deploy("VeaInboxArbToGnosis" + (chainId === 42161 ? "" : "Testnet"), { + const contractName = chainId === SenderChains.ARBITRUM + ? "VeaInboxArbToGnosis" + : "VeaInboxArbToGnosisTestnet"; + await deploy(contractName, { contract: "VeaInboxArbToGnosis", from: deployer, args: [epochPeriod, routerAddress], log: true, });validator-cli/src/ArbToEth/watcherArbToGnosis.ts (1)
29-77: Consider using TypeScript enums for status fields.The
ChallengeProgressinterface uses string literals for status fields. Consider using TypeScript enums for better type safety and maintainability.+enum TransactionStatus { + NONE = "none", + PENDING = "pending", + MINED = "mined" +} +enum ChallengeStatus { + UNCLAIMED = "Unclaimed", + CLAIMED = "Claimed", + CHALLENGED = "Challenged", + CHALLENGE_PENDING = "ChallengePending", + // ... other statuses +} interface ChallengeProgress { challenge: { txHash: string; timestamp: number; finalized: boolean; - status: "mined" | "pending" | "none"; + status: TransactionStatus; }; // ... other fields - status: "Unclaimed" | "Claimed" | "Challenged" | /* ... */; + status: ChallengeStatus; }contracts/deployments/sepolia/RouterArbToGnosisTestnet.json (2)
257-257: Consider gas optimization opportunitiesThe contract deployment consumed 848,386 gas. Consider optimizing the constructor to reduce deployment costs.
Some optimization suggestions:
- Consider using immutable variables instead of storage for constructor-set values
- Optimize the initialization logic in the constructor
- Consider splitting the contract if it's too large
279-280: Enhance documentation with deployment environment detailsThe contract documentation states it's deployed on Ethereum, but the file path suggests it's for Sepolia testnet. Consider updating the documentation to clearly specify the deployment environment.
- "details": "Router from Arbitrum to Gnosis Chain. Note: This contract is deployed on Ethereum.", + "details": "Router from Arbitrum to Gnosis Chain. Note: This contract is deployed on Sepolia testnet.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts(1 hunks)contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts(3 hunks)contracts/deployments/arbitrumSepolia/VeaInboxArbToGnosisTestnet.json(1 hunks)contracts/deployments/chiado/VeaInboxGnosisToArbTestnet.json(0 hunks)contracts/deployments/sepolia/RouterArbToGnosisTestnet.json(1 hunks)contracts/src/canonical/gnosis-chain/IAMB.sol(1 hunks)relayer-cli/package.json(1 hunks)relayer-cli/src/devnetRelayExample.ts(3 hunks)validator-cli/.env.dist(1 hunks)validator-cli/package.json(2 hunks)validator-cli/src/ArbToEth/watcherArbToGnosis.ts(14 hunks)validator-cli/src/utils/arbMsgExecutor.ts(1 hunks)validator-cli/src/utils/ethers.ts(3 hunks)
💤 Files with no reviewable changes (1)
- contracts/deployments/chiado/VeaInboxGnosisToArbTestnet.json
✅ Files skipped from review due to trivial changes (1)
- contracts/deployments/arbitrumSepolia/VeaInboxArbToGnosisTestnet.json
🚧 Files skipped from review as they are similar to previous changes (7)
- contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts
- contracts/src/canonical/gnosis-chain/IAMB.sol
- relayer-cli/package.json
- relayer-cli/src/devnetRelayExample.ts
- validator-cli/.env.dist
- validator-cli/package.json
- validator-cli/src/utils/arbMsgExecutor.ts
🔇 Additional comments (8)
validator-cli/src/utils/ethers.ts (4)
12-13: LGTM!
The new factory imports are correctly added and follow the existing import pattern.
68-70: LGTM!
The function follows the established pattern of other provider functions and maintains consistency in parameter handling.
72-74: Referring to previous review comment
The previous review comment about refactoring this function to follow established patterns and add validation is still applicable.
84-84: LGTM!
The new functions are correctly exported, maintaining the module's interface consistency.
Also applies to: 86-86
contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (1)
Line range hint 13-23: Verify impact of epochPeriod changes
The epochPeriod has been set to 1 hour (3600 seconds) for both ARBITRUM and ARBITRUM_SEPOLIA. Please verify that this change:
- Aligns with the network's block time
- Doesn't affect existing contracts or integrations
✅ Verification successful
Based on the search results, I can now generate the final response as I have sufficient information about the epochPeriod usage across the codebase:
epochPeriod configuration is correctly aligned with network requirements
The 1-hour (3600 seconds) epochPeriod for ARBITRUM and ARBITRUM_SEPOLIA is consistent with the system's design and requirements:
- It aligns with the network's block time as it's a reasonable duration for L2 networks
- It's properly integrated across the codebase:
- Used consistently in validator-cli for epoch calculations and state synchronization
- Matches the sequencer delay considerations in the watcher implementations
- Compatible with the challenge periods and timeouts in the outbox contracts
- Properly handled in the subgraph indexing logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for epochPeriod usage across the codebase
rg -A 3 "epochPeriod" --type ts
Length of output: 47866
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (1)
19-24: LGTM: Contract imports are properly structured.
The imports for IAMB, RouterArbToGnosis, and VEA contracts are correctly organized.
contracts/deployments/sepolia/RouterArbToGnosisTestnet.json (2)
1-395: LGTM! The deployment artifact looks well-structured
The contract deployment was successful with proper configuration:
- Transaction status: 1 (success)
- Proper ABI and bytecode included
- Comprehensive documentation
- Clear storage layout
267-271: Verify the constructor arguments for testnet deployment
The contract is deployed with the following addresses:
- Bridge: 0x38f918D0E9F1b721EDaA41302E399fa1B79333a9
- AMB: 0xf2546D6648BD2af6a008A7e7C1542BB240329E11
- VeaInbox: 0x72c7d51647cBeaca636d0E20A66ca2F682da3539
- VeaOutbox: 0xa3C6608539693C13434e4E29c9aB53Dd029178BE
✅ Verification successful
Constructor arguments verified for testnet deployment
The addresses are correctly referenced across the codebase:
- Bridge (0x38f918D0E9F1b721EDaA41302E399fa1B79333a9): Official Arbitrum bridge address for testnet as per deployment scripts
- AMB (0xf2546D6648BD2af6a008A7e7C1542BB240329E11): Gnosis Chain AMB bridge contract for testnet
- VeaInbox (0x72c7d51647cBeaca636d0E20A66ca2F682da3539): Deployed on Arbitrum Sepolia
- VeaOutbox (0xa3C6608539693C13434e4E29c9aB53Dd029178BE): Deployed on Chiado (Gnosis testnet)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the constructor arguments are valid testnet contract addresses
# Check if the addresses have contract code
for addr in "0x38f918D0E9F1b721EDaA41302E399fa1B79333a9" "0xf2546D6648BD2af6a008A7e7C1542BB240329E11" "0x72c7d51647cBeaca636d0E20A66ca2F682da3539" "0xa3C6608539693C13434e4E29c9aB53Dd029178BE"; do
# Search for contract references in the codebase
echo "Searching references for $addr..."
rg -i "$addr"
done
Length of output: 4270
| const averageArbitrumBlocktime = 0.26; | ||
| const estimatedArbBlocks = Math.ceil((arbLatest.timestamp - challengeBlock.timestamp) / averageArbitrumBlocktime); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded Arbitrum block time with dynamic calculation.
The average Arbitrum block time is hardcoded to 0.26 seconds. This should be calculated dynamically as block times can vary.
- const averageArbitrumBlocktime = 0.26;
- const estimatedArbBlocks = Math.ceil((arbLatest.timestamp - challengeBlock.timestamp) / averageArbitrumBlocktime);
+ const blockTimeWindow = 100; // Calculate average over last 100 blocks
+ const oldBlock = await providerArb.getBlock(arbLatest.number - blockTimeWindow);
+ const averageArbitrumBlocktime = (arbLatest.timestamp - oldBlock.timestamp) / blockTimeWindow;
+ const estimatedArbBlocks = Math.ceil((arbLatest.timestamp - challengeBlock.timestamp) / averageArbitrumBlocktime);📝 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 averageArbitrumBlocktime = 0.26; | |
| const estimatedArbBlocks = Math.ceil((arbLatest.timestamp - challengeBlock.timestamp) / averageArbitrumBlocktime); | |
| const blockTimeWindow = 100; // Calculate average over last 100 blocks | |
| const oldBlock = await providerArb.getBlock(arbLatest.number - blockTimeWindow); | |
| const averageArbitrumBlocktime = (arbLatest.timestamp - oldBlock.timestamp) / blockTimeWindow; | |
| const estimatedArbBlocks = Math.ceil((arbLatest.timestamp - challengeBlock.timestamp) / averageArbitrumBlocktime); |
madhurMongia
force-pushed
the
feat/arbSep-chaido-deploy
branch
from
e197be5 to
a5ffcb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
relayer-cli/src/devnetRelayExample.ts (2)
37-37: Enhance error message for chain ID validationConsider providing a more descriptive error message that includes the supported chain IDs.
- if (!chain_ids.includes(chain_id)) throw new Error("Invalid chainid"); + if (!chain_ids.includes(chain_id)) throw new Error(`Invalid chain ID ${chain_id}. Supported chain IDs: ${chain_ids.join(', ')}`);
Line range hint
71-77: Review lock file cleanup logicThe function is unlinking lock files for all chain IDs, which could interfere with other running relay processes. Consider unlinking only the lock file for the current chain_id.
async function updateStateFile(chain_id: number, createdTimestamp: number, nonceFrom: number) { const chain_state_file = "./src/state/" + chain_id + ".json"; const json = { ts: createdTimestamp, nonce: nonceFrom, }; fs.writeFileSync(chain_state_file, JSON.stringify(json), { encoding: "utf8" }); - for (const chain_id of chain_ids) { - const lock_file_name = "./src/state/" + chain_id + ".pid"; - if (fs.existsSync(lock_file_name)) { - fs.unlinkSync(lock_file_name); - } - } + const lock_file_name = "./src/state/" + chain_id + ".pid"; + if (fs.existsSync(lock_file_name)) { + fs.unlinkSync(lock_file_name); + } }validator-cli/src/ArbToEth/watcherArbToGnosis.ts (4)
29-77: Add JSDoc documentation for the ChallengeProgress interface.The interface is well-structured but lacks documentation explaining the purpose of each field and the state machine transitions.
Add documentation like this:
/** * Tracks the progress of a challenge through various states in the cross-chain verification process. * The challenge moves through states: Unclaimed -> Claimed -> Challenged -> SnapshotSent -> Routed -> AMBMessageSent -> Completed */ interface ChallengeProgress { /** Challenge submission status on Gnosis chain */ challenge: { txHash: string; // Transaction hash of the challenge timestamp: number; // Block timestamp when challenge was submitted finalized: boolean; // Whether the challenge block is finalized status: "mined" | "pending" | "none"; }; // ... (similar docs for other fields) }
629-738: Optimize claim retrieval with batch requests.The
getClaimForEpochfunction makes multiple separate RPC calls. Consider using batch requests to reduce network overhead.+ // Batch the initial RPC calls + const [claimHash, claimedEvents] = await Promise.all([ + retryOperation(() => veaOutbox.claimHashes(epoch), 1000, 10), + retryOperation( + () => + providerGnosis.getLogs({ + ...veaOutbox.filters.Claimed(null, epoch, null), + fromBlock: blockNumberOutboxLowerBound, + toBlock: "latest", + }), + 1000, + 10 + ), + ]);
793-809: Make MAX_PENDING_TIME configurable.The
needsRetryfunction uses a hardcoded timeout of 1 hour. This should be configurable based on network conditions.+const DEFAULT_MAX_PENDING_TIME = 3600; // 1 hour + function needsRetry(current: ChallengeProgress, previous: ChallengeProgress | undefined): boolean { if (!previous) return false; - const MAX_PENDING_TIME = 3600; // 1 hour + const MAX_PENDING_TIME = process.env.MAX_PENDING_TIME + ? parseInt(process.env.MAX_PENDING_TIME) + : DEFAULT_MAX_PENDING_TIME;
978-981: Improve error message for missing messages.The error message when no child-to-parent messages are found could be more descriptive.
const messages = await messageReceipt.getChildToParentMessages(parentSigner); const childToParentMessage = messages[0]; if (!childToParentMessage) { - throw new Error("No child-to-parent messages found"); + throw new Error( + `No child-to-parent messages found in transaction ${snapshotTxnReceipt.transactionHash} for epoch ${epoch}` + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
relayer-cli/src/devnetRelayExample.ts(3 hunks)validator-cli/.env.dist(1 hunks)validator-cli/src/ArbToEth/watcherArbToGnosis.ts(14 hunks)validator-cli/src/utils/ethers.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- validator-cli/.env.dist
- validator-cli/src/utils/ethers.ts
🔇 Additional comments (2)
relayer-cli/src/devnetRelayExample.ts (1)
25-25: Verify sender address configuration and permissions
The lightbulb switch address should be properly funded and have the correct permissions on Arbitrum Sepolia. Also ensure this address is consistently used across deployment files.
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (1)
104-105: Verify AMB and Router contract addresses.
The code initializes new providers for AMB and Router contracts. Ensure these addresses are correctly set in the environment variables.
✅ Verification successful
Based on the search results, I can now generate the final response:
Contract addresses are correctly configured in deployment files
The AMB and Router addresses are properly defined in the deployment configuration:
- Chiado AMB address:
0x8448E15d0e706C0298dECA99F0b4744030e59d7d - VeaRouter Arb-to-Gnosis address:
0x9481b3A49ac67d03D9022E6200eFD81850BADDB4(devnet) - VeaRouter Arb-to-Gnosis address:
0x931FA807020231bCE1340Be8E1e5054207BbAFEd(testnet)
The addresses are properly documented in the contracts README.md and used in deployment scripts, indicating they are valid and correctly configured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if AMB and Router addresses are set and valid
echo "Checking environment variables..."
grep -E "VEAROUTER_ARB_TO_GNOSIS_ADDRESS|GNOSIS_AMB_ADDRESS" .env.dist
# Verify the addresses are valid Ethereum addresses
rg -A 1 'VEAROUTER_ARB_TO_GNOSIS_ADDRESS|GNOSIS_AMB_ADDRESS' .env* | grep -E '^[0-9a-fA-F]{40}$'
Length of output: 339
Script:
#!/bin/bash
# Let's search for environment variable declarations in the codebase
echo "Searching for environment variable declarations..."
rg -l "VEAROUTER_ARB_TO_GNOSIS_ADDRESS|GNOSIS_AMB_ADDRESS"
# Check how these addresses are used in the code
echo "Checking address usage..."
rg "veaRouterAddress|gnosisAMBAddress" -A 2
# Look for any configuration or constant files
echo "Looking for config files..."
fd -e ts -e js -e json "config|constants"
Length of output: 1983
Script:
#!/bin/bash
# Let's check the ecosystem config file for environment variables
echo "Checking ecosystem config..."
cat validator-cli/ecosystem.config.js
# Let's also check for any example env files
echo "Looking for example env files..."
fd ".env" --type f
# Check if addresses are defined in any deployment scripts or constants
echo "Checking deployment files..."
rg -A 2 "0x[a-fA-F0-9]{40}" contracts/
Length of output: 156815
madhurMongia
force-pushed
the
feat/arbSep-chaido-deploy
branch
from
a5ffcb1 to
b233445
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
validator-cli/src/utils/ethers.ts (1)
68-74: Add input validation to the new provider functions.While the implementation follows the established pattern correctly, consider adding input validation for the address parameters to prevent runtime errors:
function getVeaRouterArbToGnosisProvider(veaRouterAddress: string, privateKey: string, rpc: JsonRpcProvider) { + if (!veaRouterAddress) { + throw new Error("Router address is required"); + } return RouterArbToGnosis__factory.connect(veaRouterAddress, getWalletRPC(privateKey, rpc)); } function getAMBProvider(ambAddress: string, privateKey: string, rpc: JsonRpcProvider) { + if (!ambAddress) { + throw new Error("AMB address is required"); + } return IAMB__factory.connect(ambAddress, getWalletRPC(privateKey, rpc)); }validator-cli/src/ArbToEth/watcherArbToGnosis.ts (2)
29-77: Consider enhancing the ChallengeProgress interface.The interface is well-structured but could benefit from some improvements:
- Consider using an enum for the status field instead of string literals
- Add JSDoc comments to document the purpose of each field
+/** Represents the progress of a challenge through various stages */ interface ChallengeProgress { + /** Challenge initiation details */ challenge: { txHash: string; timestamp: number; finalized: boolean; status: "mined" | "pending" | "none"; }; + /** Snapshot details on the target chain */ snapshot: { txHash: string; timestamp: number; finalized: boolean; status: "mined" | "pending" | "none"; }; // ... rest of the interface + /** Overall status of the challenge process */ + type ChallengeStatus = + | "Unclaimed" + | "Claimed" + | "Challenged" + | "ChallengePending" + | "SnapshotSent" + | "SnapshotPending" + | "Routed" + | "RoutePending" + | "AMBMessageSent" + | "AMBMessagePending" + | "WithdrawalPending" + | "Completed"; - status: - | "Unclaimed" - | "Claimed" - | "Challenged" - | "ChallengePending" - | "SnapshotSent" - | "SnapshotPending" - | "Routed" - | "RoutePending" - | "AMBMessageSent" - | "AMBMessagePending" - | "WithdrawalPending" - | "Completed"; + status: ChallengeStatus;
793-809: Make the pending transaction timeout configurable.The
MAX_PENDING_TIMEis hardcoded to 1 hour. This should be configurable via environment variables to allow for different network conditions.+const MAX_PENDING_TIME = parseInt(process.env.MAX_PENDING_TIME || '3600'); // Default to 1 hour function needsRetry(current: ChallengeProgress, previous: ChallengeProgress | undefined): boolean { if (!previous) return false; // Check if any pending transaction has been pending too long - const MAX_PENDING_TIME = 3600; // 1 hour const now = Math.floor(Date.now() / 1000);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
relayer-cli/src/devnetRelayExample.ts(3 hunks)validator-cli/.env.dist(1 hunks)validator-cli/src/ArbToEth/watcherArbToGnosis.ts(14 hunks)validator-cli/src/utils/ethers.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- relayer-cli/src/devnetRelayExample.ts
- validator-cli/.env.dist
🔇 Additional comments (2)
validator-cli/src/utils/ethers.ts (2)
12-13: LGTM!
The new imports follow the existing pattern and are correctly placed within the import block.
84-86: LGTM!
The new exports are correctly placed and maintain consistent formatting with the existing pattern.
| @@ -151,6 +212,7 @@ const watch = async () => { | |||
|
|
|||
| const veaEpochOutboxClaimableNowOld = veaEpochOutboxClaimableNow; | |||
| veaEpochOutboxClaimableNow = Math.floor(timeGnosis / epochPeriod) - 1; | |||
| // TODO: sometimes veaEpochOutboxClaimableNow is 1 epoch behind veaEpochOutboxClaimableNowOld | |||
There was a problem hiding this comment.
I added a new condition to solve this issue, i.e. to concat new epochs when veaEpochOutboxClaimableNow > veaEpochOutboxClaimableNowOld
| amb, | ||
| previousProgress | ||
| ); | ||
| challenges.set(index, challengeProgress); |
There was a problem hiding this comment.
I think it would be better to use veaEpochOutboxCheck as the key value for challenges instead of index. As index will represent different epochs in different iterations.
| ); | ||
| claim.timestampVerification = verificationBlock.timestamp; | ||
| claim.blocknumberVerification = verificationBlock.number; | ||
| claim.challenger = constants.AddressZero; |
There was a problem hiding this comment.
claim.challenger could possibly be overwritten here resulting in wrong calculatedHash, as earlier its set using challenge logs on line 697
There was a problem hiding this comment.
a claim cannot be in challenged and under verification at the same time.
| other.challengeBlock = challengedEvents[0].blockNumber; | ||
| } | ||
|
|
||
| calculatedHash = await retryOperation(() => veaOutbox.hashClaim(claim), 1000, 10); |
| challenger: constants.AddressZero, | ||
| }; | ||
| let other = {} as any; | ||
| let calculatedHash = await retryOperation(() => veaOutbox.hashClaim(claim), 1000, 10); |
There was a problem hiding this comment.
A local hash function can be used to reduce contract calls.
| claim.challenger = constants.AddressZero; | ||
| } | ||
|
|
||
| calculatedHash = await retryOperation(() => veaOutbox.hashClaim(claim), 1000, 10); |
| if (calculatedHash == claimHash) return claim; | ||
|
|
||
| const [claimBridgerHonest, claimChallengerHonest] = await Promise.all([ | ||
| retryOperation(() => veaOutbox.hashClaim({ ...claim, honest: 1 }), 1000, 10) as any, |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (1)
811-1094: Consider implementing an event-driven architecture.The
reconstructChallengeProgressfunction, while thorough, follows a sequential polling approach. Consider refactoring to an event-driven architecture where:
- Subscribe to relevant events from each chain
- Update challenge progress state when events are received
- Use a state machine to manage transitions
This would:
- Reduce RPC calls
- Improve responsiveness
- Make the code more maintainable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts(14 hunks)
🧰 Additional context used
📓 Learnings (1)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (1)
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:454-457
Timestamp: 2024-11-25T09:05:40.703Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToGnosis.ts`, when calculating `arbAverageBlockTime`, it's acceptable not to add error handling for division by zero when `blockLatestArb.timestamp` equals `blockoldArb.timestamp` because if it occurs, it indicates a major issue that requires investigation.
🔇 Additional comments (2)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (2)
1111-1113: LGTM! Good use of retry mechanism.
The script execution is properly wrapped in an IIFE with retry logic to handle potential failures.
24-24:
Fix incorrect import path for ClaimStruct.
The import path should be updated to match the Arbitrum to Gnosis bridge implementation.
-import { ClaimStruct } from "@kleros/vea-contracts/typechain-types/arbitrumToEth/VeaInboxArbToEth";
+import { ClaimStruct } from "@kleros/vea-contracts/typechain-types/arbitrumToGnosis/VeaInboxArbToGnosis";Likely invalid or redundant comment.
madhurMongia
force-pushed
the
feat/arbSep-chaido-deploy
branch
from
12ee67e to
1357620
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts(14 hunks)
🧰 Additional context used
📓 Learnings (1)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (2)
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:454-457
Timestamp: 2024-11-25T09:05:40.703Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToGnosis.ts`, when calculating `arbAverageBlockTime`, it's acceptable not to add error handling for division by zero when `blockLatestArb.timestamp` equals `blockoldArb.timestamp` because if it occurs, it indicates a major issue that requires investigation.
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:342-351
Timestamp: 2024-11-25T09:05:35.087Z
Learning: The `retryOperation` function in `watcherArbToGnosis.ts` acts as the retry mechanism for the challenge transaction, so additional retry logic is unnecessary.
🔇 Additional comments (3)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (3)
24-24:
Fix incorrect import path for ClaimStruct.
The import path should be from arbitrumToGnosis since this file handles the Arbitrum to Gnosis bridge.
-import { ClaimStruct } from "@kleros/vea-contracts/typechain-types/arbitrumToEth/VeaInboxArbToEth";
+import { ClaimStruct } from "@kleros/vea-contracts/typechain-types/arbitrumToGnosis/VeaInboxArbToGnosis";Likely invalid or redundant comment.
573-574:
Add error handling in catch blocks.
Empty catch blocks hide potential issues and make debugging difficult.
- .catch((e) => {})) as [BigNumber] & { batch: BigNumber };
+ .catch((e) => {
+ console.error('Error finding batch containing block:', e);
+ return undefined;
+ })) as [BigNumber] & { batch: BigNumber };Likely invalid or redundant comment.
343-344:
Implement profitability calculation before challenging.
The profitability calculation is commented out. This should be implemented to ensure challenges are economically viable.
Uncomment and complete the profitability calculation:
-//TODO : check profitablity of the whole dispute resolution
-//const profitablity = await calculateDisputeResolutionProfitability(veaEpochOutboxCheck,claim,veaOutbox,veaInbox,providerGnosis,providerArb,providerEth);
+const profitability = await calculateDisputeResolutionProfitability(
+ veaEpochOutboxCheck,
+ claim,
+ veaOutbox,
+ veaInbox,
+ providerGnosis,
+ providerArb,
+ providerEth
+);
+if (!profitability.profitable) {
+ console.log(`Challenge for epoch ${veaEpochOutboxCheck} is not profitable. Skipping...`);
+ continue;
+}Likely invalid or redundant comment.
madhurMongia
force-pushed
the
feat/arbSep-chaido-deploy
branch
from
1357620 to
9c12102
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts(14 hunks)
🧰 Additional context used
📓 Learnings (1)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (1)
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:454-457
Timestamp: 2024-11-25T09:05:40.703Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToGnosis.ts`, when calculating `arbAverageBlockTime`, it's acceptable not to add error handling for division by zero when `blockLatestArb.timestamp` equals `blockoldArb.timestamp` because if it occurs, it indicates a major issue that requires investigation.
🔇 Additional comments (3)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (3)
24-24:
Fix incorrect import path for ClaimStruct.
The ClaimStruct is imported from the arbitrumToEth contract, but this file handles the Arbitrum to Gnosis bridge.
-import { ClaimStruct } from "@kleros/vea-contracts/typechain-types/arbitrumToEth/VeaInboxArbToEth";
+import { ClaimStruct } from "@kleros/vea-contracts/typechain-types/arbitrumToGnosis/VeaInboxArbToGnosis";Likely invalid or redundant comment.
343-344:
Implement profitability calculation for dispute resolution.
The profitability calculation is commented out, but it's crucial for making informed decisions about whether to proceed with challenges.
Uncomment and complete the profitability calculation:
-//TODO : check profitablity of the whole dispute resolution
-//const profitablity = await calculateDisputeResolutionProfitability(veaEpochOutboxCheck,claim,veaOutbox,veaInbox,providerGnosis,providerArb,providerEth);
+const profitability = await calculateDisputeResolutionProfitability(
+ veaEpochOutboxCheck,
+ claim,
+ veaOutbox,
+ veaInbox,
+ providerGnosis,
+ providerArb,
+ providerEth
+);
+if (!profitability.profitable) {
+ console.log(`Challenge for epoch ${veaEpochOutboxCheck} is not profitable. Skipping...`);
+ continue;
+}Likely invalid or redundant comment.
804-805:
Implement L2 to L1 message execution gas cost calculation.
The TODO comment indicates missing implementation for calculating L2 to L1 message execution costs, which is crucial for accurate profitability calculation.
Add the L2 to L1 message execution gas cost calculation:
+ // 3. Costs on Ethereum (for Arbitrum -> Ethereum message)
+ const l2ToL1GasEstimate = await childToParentMessage.estimateL1Gas(providerArb);
+ const ethGasPrice = await providerEth.getGasPrice();
+ const l2ToL1Cost = l2ToL1GasEstimate.mul(ethGasPrice);
+
+ if (l2ToL1Cost.gt(maximumAllowableCost)) {
+ return { profitable: false, estimatedProfit: constants.Zero };
+ }
+ totalCost = totalCost.add(l2ToL1Cost);
+
+ const estimatedProfit = totalReward.sub(totalCost);
+ return {
+ profitable: estimatedProfit.gte(minimumProfit),
+ estimatedProfit
+ };Likely invalid or redundant comment.
madhurMongia
force-pushed
the
feat/arbSep-chaido-deploy
branch
from
9c12102 to
c7a9e5d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts(14 hunks)
🧰 Additional context used
📓 Learnings (1)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (2)
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:829-1112
Timestamp: 2024-11-26T08:37:47.591Z
Learning: Refactoring the `reconstructChallengeProgress` function in `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` into smaller functions is considered unnecessary abstraction.
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:454-457
Timestamp: 2024-11-25T09:05:40.703Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToGnosis.ts`, when calculating `arbAverageBlockTime`, it's acceptable not to add error handling for division by zero when `blockLatestArb.timestamp` equals `blockoldArb.timestamp` because if it occurs, it indicates a major issue that requires investigation.
🔇 Additional comments (8)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (8)
29-77: Well-structured ChallengeProgress interface.
The interface provides comprehensive tracking of challenge states with clear status indicators and timestamps.
88-89: Well-documented environment variables and constants.
The new environment variables for VEA router and Gnosis AMB addresses are properly defined, and the constants are well-documented with source references.
104-105: Good provider initialization with error handling.
The initialization of VEA router and AMB providers is well-implemented with proper error handling using retryOperation.
216-223: Good epoch array management.
The logic correctly handles the addition of new epochs when veaEpochOutboxClaimableNow increases, with safe array manipulation.
1114-1127: Correct implementation of claim hashing.
The hashClaim function correctly implements the claim hashing using solidityKeccak256 with proper parameter ordering.
24-24:
Fix incorrect import path for ClaimStruct.
The import path should be from arbitrumToGnosis since this file handles the Arbitrum to Gnosis bridge.
-import { ClaimStruct } from "@kleros/vea-contracts/typechain-types/arbitrumToEth/VeaInboxArbToEth";
+import { ClaimStruct } from "@kleros/vea-contracts/typechain-types/arbitrumToGnosis/VeaInboxArbToGnosis";Likely invalid or redundant comment.
343-344:
Implement profitability calculation before proceeding with challenges.
The TODO comment indicates missing profitability calculation which is crucial for determining whether to proceed with challenges.
Uncomment and implement the profitability calculation to ensure challenges are economically viable:
-//TODO : check profitablity of the whole dispute resolution
-//const profitablity = await calculateDisputeResolutionProfitability(veaEpochOutboxCheck,claim,veaOutbox,veaInbox,providerGnosis,providerArb,providerEth);
+const profitability = await calculateDisputeResolutionProfitability(
+ veaEpochOutboxCheck,
+ claim,
+ veaOutbox,
+ veaInbox,
+ providerGnosis,
+ providerArb,
+ providerEth
+);
+if (!profitability.profitable) {
+ console.log(`Challenge for epoch ${veaEpochOutboxCheck} is not profitable. Skipping...`);
+ continue;
+}Likely invalid or redundant comment.
803-804:
Implement L2 to L1 message execution gas cost calculation.
The TODO comment indicates missing implementation for calculating L2 to L1 message execution costs, which is crucial for accurate profitability calculation.
Add the L2 to L1 message execution gas cost calculation:
+ // 3. Costs on Ethereum (for Arbitrum -> Ethereum message)
+ const l2ToL1GasEstimate = await childToParentMessage.estimateL1Gas(providerArb);
+ const ethGasPrice = await providerEth.getGasPrice();
+ const l2ToL1Cost = l2ToL1GasEstimate.mul(ethGasPrice);
+
+ if (l2ToL1Cost.gt(maximumAllowableCost)) {
+ return { profitable: false, estimatedProfit: constants.Zero };
+ }
+ totalCost = totalCost.add(l2ToL1Cost);
+
+ const estimatedProfit = totalReward.sub(totalCost);
+ return {
+ profitable: estimatedProfit.gte(minimumProfit),
+ estimatedProfit
+ };Likely invalid or redundant comment.
madhurMongia
force-pushed
the
feat/arbSep-chaido-deploy
branch
from
c7a9e5d to
095f09a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (2)
29-77: Add JSDoc documentation for the ChallengeProgress interface.The interface is well-structured but lacks documentation explaining the purpose of each field and the state flow.
+/** + * Tracks the progress of a challenge through various states across multiple chains. + * @property challenge - Challenge transaction details on Gnosis chain + * @property snapshot - Snapshot transaction details on Arbitrum chain + * @property L2toL1Message - Status of the L2 to L1 message + * @property route - Route transaction details on Ethereum + * @property AMB - Arbitrary Message Bridge details + * @property withdrawal - Withdrawal transaction details + * @property status - Current state of the challenge process + */ interface ChallengeProgress {
Line range hint
1-1128: Add architectural documentation for the multi-chain interaction flow.Consider adding a high-level documentation section explaining:
- The flow of operations across Arbitrum, Gnosis, and Ethereum chains
- State transitions in the challenge process
- Retry mechanisms and their purposes
- Error handling strategies
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
validator-cli/package.json(2 hunks)validator-cli/src/ArbToEth/watcherArbToGnosis.ts(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- validator-cli/package.json
🧰 Additional context used
📓 Learnings (1)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (4)
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:804-804
Timestamp: 2024-11-27T10:16:33.692Z
Learning: When the scope is to resurrect the testnet, it does not include making sure the validator is always profitable.
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:829-1112
Timestamp: 2024-11-26T08:37:47.591Z
Learning: Refactoring the `reconstructChallengeProgress` function in `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` into smaller functions is considered unnecessary abstraction.
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:454-457
Timestamp: 2024-11-25T09:05:40.703Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToGnosis.ts`, when calculating `arbAverageBlockTime`, it's acceptable not to add error handling for division by zero when `blockLatestArb.timestamp` equals `blockoldArb.timestamp` because if it occurs, it indicates a major issue that requires investigation.
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:342-351
Timestamp: 2024-11-25T09:05:35.087Z
Learning: The `retryOperation` function in `watcherArbToGnosis.ts` acts as the retry mechanism for the challenge transaction, so additional retry logic is unnecessary.
🔇 Additional comments (4)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (4)
473-476: LGTM! Block time calculation is correctly implemented.
The implementation aligns with the intended design where a division by zero would indicate a major system issue requiring investigation.
590-603: Well-implemented error handling and fallback logic.
The code properly handles errors when finding batch containing block and includes a robust fallback mechanism.
758-809: LGTM! Profitability calculation implementation.
The implementation is sufficient for the current scope of testnet resurrection.
24-24:
Fix incorrect import path for ClaimStruct.
The import path references arbitrumToEth but this file handles the Arbitrum to Gnosis bridge.
-import { ClaimStruct } from "@kleros/vea-contracts/typechain-types/arbitrumToEth/VeaInboxArbToEth";
+import { ClaimStruct } from "@kleros/vea-contracts/typechain-types/arbitrumToGnosis/VeaInboxArbToGnosis";Likely invalid or redundant comment.
madhurMongia
dismissed
coderabbitai[bot]’s stale review
all the reviews requested have been resolved , you are referring to the commits which do not exist anymore
madhurMongia
dismissed stale reviews from jaybuidl and mani99brar
via
7588346
madhurMongia
force-pushed
the
feat/arbSep-chaido-deploy
branch
from
095f09a to
7588346
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (1)
43-47: Add type safety to network configurationConsider adding TypeScript interfaces to improve type safety and maintainability of the network configuration.
+interface NetworkConfig { + url: string; + [key: string]: any; +} + +type RouterNetworks = { + [K in keyof typeof SenderChains]: NetworkConfig; +}; + - const routerNetworks = { + const routerNetworks: RouterNetworks = { ARBITRUM: config.networks.mainnet, ARBITRUM_SEPOLIA: config.networks.sepolia, HARDHAT: config.networks.localhost, };contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts (2)
Line range hint
164-164: Consider using dynamic gas pricingInstead of hardcoding
gasPrice: ethers.utils.parseUnits("1", "gwei"), consider using dynamic gas pricing with a multiplier:- gasPrice: ethers.utils.parseUnits("1", "gwei"), // chiado rpc response underprices gas + gasPrice: async (provider) => { + const gasPrice = await provider.getGasPrice(); + return gasPrice.mul(120).div(100); // 20% buffer for underpriced gas + },
Nonce offsets need to be updated to match the deployment sequence
The current nonce offsets in the code don't match the actual deployment sequence:
- The code uses
nonce + 10for router address, but the deployment sequence shows router is deployed atnonce + 6- The code uses
nonce + 6for sender gateway, but this doesn't match the sequence of deploymentsThe nonce offsets should be corrected to match the actual deployment order:
- AMB Mock
- WETH Mock
- VeaOutbox
- ReceiverGateway
- ArbSysMock
- VeaInbox
- SenderGateway
- Router
🔗 Analysis chain
Line range hint
93-98: Verify the updated nonce offsets for contract address predictionThe nonce offsets have been modified:
- Router address: nonce + 10
- SenderGateway address: nonce + 6
While using
getContractAddress()is acceptable for testnet resurrection, please verify these new offset values match the actual deployment sequence to avoid address prediction errors.
Let me analyze the deployment sequence to verify the nonce offsets. I need to check the order of contract deployments in the specific file.
Let me check the deployment sequence in the inbox file to complete the verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the deployment sequence matches the nonce offsets # Check the deployment order in the repository rg -A 5 "deploy\(" contracts/deploy/Length of output: 24407
Script:
#!/bin/bash # Check the deployment sequence in the specific file rg -A 10 "deploy\(" contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.tsLength of output: 1280
Script:
#!/bin/bash # Check the deployment sequence in the inbox file rg -A 10 "deploy\(" contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.tsLength of output: 1429
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (3)
88-91: Add specific type for the Map key.Consider adding a type alias for the epoch number to make the code more maintainable:
+type EpochNumber = number; -const challenges = new Map<number, ChallengeProgress>(); +const challenges = new Map<EpochNumber, ChallengeProgress>();
216-223: Optimize array concatenation using spread operator.The current implementation using array creation and push could be simplified:
-const veaEpochsOutboxClaimableNew: number[] = new Array( - veaEpochOutboxClaimableNow - veaEpochOutboxClaimableNowOld -) - .fill(veaEpochOutboxClaimableNowOld + 1) - .map((el, i) => el + i); -veaEpochOutboxCheckClaimsRangeArray.push(...veaEpochsOutboxClaimableNew); +veaEpochOutboxCheckClaimsRangeArray.push( + ...Array.from( + { length: veaEpochOutboxClaimableNow - veaEpochOutboxClaimableNowOld }, + (_, i) => veaEpochOutboxClaimableNowOld + 1 + i + ) +);
Line range hint
244-254: Use epoch number instead of index for challenge deletion.Using array index for challenge deletion could be problematic as the index might not correspond to the correct epoch in subsequent iterations:
-if (challenges.has(veaEpochOutboxCheck)) challenges.delete(veaEpochOutboxCheck); +const epochToDelete = veaEpochOutboxCheck; +if (challenges.has(epochToDelete)) challenges.delete(epochToDelete);contracts/deployments/arbitrumSepolia/VeaInboxArbToGnosisTestnet.json (1)
391-448: Document storage constraints and limitations.The contract has the following storage constraints:
- Fixed inbox array size of 64 elements limits the merkle tree height
- uint64 count limits total messages to 2^64-1
Consider:
- Adding these limitations to the contract documentation
- Documenting the rationale for choosing these specific sizes
contracts/deployments/sepolia/RouterArbToGnosisTestnet.json (1)
1-395: Consider adding emergency pause mechanismWhile the contract implements good security controls, consider adding an emergency pause mechanism to halt operations in case of detected anomalies or attacks. This would provide an additional safety layer for this critical cross-chain infrastructure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts(1 hunks)contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts(3 hunks)contracts/deployments/arbitrumSepolia/VeaInboxArbToGnosisTestnet.json(1 hunks)contracts/deployments/chiado/VeaInboxGnosisToArbTestnet.json(0 hunks)contracts/deployments/sepolia/RouterArbToGnosisTestnet.json(1 hunks)contracts/src/canonical/gnosis-chain/IAMB.sol(1 hunks)relayer-cli/package.json(1 hunks)validator-cli/.env.dist(2 hunks)validator-cli/package.json(2 hunks)validator-cli/src/ArbToEth/watcherArbToGnosis.ts(14 hunks)validator-cli/src/utils/ethers.ts(3 hunks)
💤 Files with no reviewable changes (1)
- contracts/deployments/chiado/VeaInboxGnosisToArbTestnet.json
🚧 Files skipped from review as they are similar to previous changes (4)
- contracts/src/canonical/gnosis-chain/IAMB.sol
- relayer-cli/package.json
- validator-cli/.env.dist
- validator-cli/package.json
🧰 Additional context used
📓 Learnings (2)
contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts (1)
Learnt from: madhurMongia
PR: kleros/vea#359
File: contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts:93-98
Timestamp: 2024-11-20T10:49:25.392Z
Learning: In `contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts`, when resurrecting testnets, it's acceptable to use `getContractAddress()` for contract address prediction.
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (5)
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:24-24
Timestamp: 2024-11-27T10:48:48.433Z
Learning: In the `validator-cli` codebase, both `arbitrumToEth` and `arbitrumToGnosis` modules use the same `ClaimStruct`, so importing `ClaimStruct` from either module is acceptable.
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:804-804
Timestamp: 2024-11-27T10:16:33.692Z
Learning: When the scope is to resurrect the testnet, it does not include making sure the validator is always profitable.
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:829-1112
Timestamp: 2024-11-26T08:37:47.591Z
Learning: Refactoring the `reconstructChallengeProgress` function in `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` into smaller functions is considered unnecessary abstraction.
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:454-457
Timestamp: 2024-11-25T09:05:40.703Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToGnosis.ts`, when calculating `arbAverageBlockTime`, it's acceptable not to add error handling for division by zero when `blockLatestArb.timestamp` equals `blockoldArb.timestamp` because if it occurs, it indicates a major issue that requires investigation.
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:342-351
Timestamp: 2024-11-25T09:05:35.087Z
Learning: The `retryOperation` function in `watcherArbToGnosis.ts` acts as the retry mechanism for the challenge transaction, so additional retry logic is unnecessary.
🔇 Additional comments (14)
validator-cli/src/utils/ethers.ts (2)
12-13: LGTM! New imports align with added functionality.
The imports for RouterArbToGnosis__factory and IAMB__factory are correctly added and follow the existing pattern.
84-84: LGTM! Exports are properly updated.
The new functions are correctly exported and maintain the alphabetical order.
Also applies to: 86-86
contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (3)
86-92:
Add provider cleanup and error handling
The provider instance needs proper cleanup and error handling to prevent resource leaks and handle network issues gracefully.
const liveDeployer = async () => {
const routerChainProvider = new providers.JsonRpcProvider(routerNetworks[SenderChains[chainId]].url);
- let nonceRouter = await routerChainProvider.getTransactionCount(deployer);
-
- const routerAddress = getContractAddress(deployer, nonceRouter);
- console.log("calculated future router for nonce %d: %s", nonceRouter, routerAddress);
+ try {
+ let nonceRouter = await routerChainProvider.getTransactionCount(deployer);
+ const routerAddress = getContractAddress(deployer, nonceRouter);
+ console.log("calculated future router for nonce %d: %s", nonceRouter, routerAddress);
+
+ // Rest of the deployment logic
+ } catch (error) {
+ console.error('Provider operation failed:', error);
+ throw error;
+ } finally {
+ routerChainProvider.removeAllListeners();
+ }Likely invalid or redundant comment.
90-91: Verify router address existence
The calculated router address should be verified to ensure it doesn't already exist on the chain.
93-98: Add deployment verification
The contract deployment should be verified to ensure it was successful and the correct parameters were used.
contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts (2)
29-29: Verify the impact of parameter changes for GNOSIS_CHIADO
The changes to GNOSIS_CHIADO parameters raise some concerns:
- Reducing
epochPeriodto 1 hour will double the frequency of state updates - Setting
numEpochTimeoutandmaxMissingBlocksto 1000000 effectively disables these safety checks
While this might be acceptable for initial testing, consider using values that better reflect production behavior to catch potential issues early.
Line range hint 99-110: LGTM! Mock contracts deployment follows best practices
The mock contracts (MockAMB and MockWETH) are correctly deployed only in the hardhat environment (chainId === 31337), following best practices for test environment setup.
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (2)
29-77: LGTM! Well-structured interface definition.
The ChallengeProgress interface provides a comprehensive structure for tracking challenge states with clear status types and nested objects for different stages of the challenge process.
650-756: LGTM! Well-implemented claim reconstruction.
The getClaimForEpoch function has good error handling and properly reconstructs claims by checking multiple events. The progressive hash checking approach is particularly robust.
contracts/deployments/arbitrumSepolia/VeaInboxArbToGnosisTestnet.json (2)
161-167: Consider implementing explicit access control.
The saveSnapshot function lacks explicit access control. While it may be implicitly protected by the protocol's design, consider adding explicit access control using OpenZeppelin's AccessControl.
2-2: Verify the router address configuration.
The contract is deployed with:
- Address:
0x854374483572FFcD4d0225290346279d0718240b - Router:
0x22d70804d4Ef5BB206C6B39e3267DFe8a0f97d27
Also applies to: 294-295
✅ Verification successful
Based on the results, I can now generate the final response:
Router address configuration is correct and properly integrated
The router address 0x22d70804d4Ef5BB206C6B39e3267DFe8a0f97d27 is:
- Deployed as RouterArbToGnosisTestnet on Sepolia
- Referenced correctly in VeaOutboxArbToGnosisTestnet on Chiado
- Implements the expected IRouterToGnosis interface with the
routefunction
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the router address exists and has the expected interface
# Check if the router address has contract code
rg -A 5 "0x22d70804d4Ef5BB206C6B39e3267DFe8a0f97d27"
# Search for router interface implementation
ast-grep --pattern $'interface IRouterToGnosis {
$$$
function route(uint256 _epoch, bytes32 _stateRoot, uint256 _gasLimit, Claim memory _claim) external;
$$$
}'
Length of output: 32984
contracts/deployments/sepolia/RouterArbToGnosisTestnet.json (3)
31-87: LGTM! Well-structured events for tracking state changes.
The events provide comprehensive tracking of:
- Message routing with indexed epoch for efficient filtering
- Sequencer delay limit changes and cross-chain communication
- Decrease requests for auditability
114-223: LGTM! Well-implemented functions with proper controls.
The contract implements:
- Secure message routing with bridge authentication
- Safe sequencer delay limit management with delayed decrease mechanism
- Proper event emissions for state changes
267-271: Verify the constructor parameters for production deployment
Please verify these addresses are the correct production contracts for:
- Bridge:
0x38f918D0E9F1b721EDaA41302E399fa1B79333a9 - AMB:
0xf2546D6648BD2af6a008A7e7C1542BB240329E11 - VeaInbox:
0x72c7d51647cBeaca636d0E20A66ca2F682da3539 - VeaOutbox:
0xa3C6608539693C13434e4E29c9aB53Dd029178BE
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (3)
29-77: Enhance type safety with TypeScript literals and readonly properties.The
ChallengeProgressinterface could benefit from TypeScript's type system:
- Use literal types for status values
- Make properties readonly to prevent accidental mutations
interface ChallengeProgress { - challenge: { + readonly challenge: { txHash: string; timestamp: number; finalized: boolean; - status: "mined" | "pending" | "none"; + status: TransactionStatus; }; + type TransactionStatus = "mined" | "pending" | "none"; + type ChallengeStatus = + | "Unclaimed" + | "Claimed" + | "Challenged" + | "ChallengePending" + | "SnapshotSent" + | "SnapshotPending" + | "Routed" + | "RoutePending" + | "AMBMessageSent" + | "AMBMessagePending" + | "WithdrawalPending" + | "Completed"; - status: - | "Unclaimed" - | "Claimed" - | "Challenged" - | "ChallengePending" - | "SnapshotSent" - | "SnapshotPending" - | "Routed" - | "RoutePending" - | "AMBMessageSent" - | "AMBMessagePending" - | "WithdrawalPending" - | "Completed"; + status: ChallengeStatus; }
91-91: Use string keys in the challenges Map for numerical safety.JavaScript numbers can have precision issues when used as Map keys. Convert epoch numbers to strings when using them as keys.
-const challenges = new Map<number, ChallengeProgress>(); +const challenges = new Map<string, ChallengeProgress>();Then update all Map operations to use string keys:
challenges.set(veaEpochOutboxCheck.toString(), challengeProgress); challenges.get(veaEpochOutboxCheck.toString()); challenges.has(veaEpochOutboxCheck.toString()); challenges.delete(veaEpochOutboxCheck.toString());
216-223: Improve clarity of epoch concatenation logic.The logic for adding new epochs could be more explicit and self-documenting.
-if (veaEpochOutboxClaimableNow > veaEpochOutboxClaimableNowOld) { - const veaEpochsOutboxClaimableNew: number[] = new Array( - veaEpochOutboxClaimableNow - veaEpochOutboxClaimableNowOld - ) - .fill(veaEpochOutboxClaimableNowOld + 1) - .map((el, i) => el + i); - veaEpochOutboxCheckClaimsRangeArray.push(...veaEpochsOutboxClaimableNew); -} +const newEpochsCount = veaEpochOutboxClaimableNow - veaEpochOutboxClaimableNowOld; +if (newEpochsCount > 0) { + const newEpochs = Array.from( + { length: newEpochsCount }, + (_, i) => veaEpochOutboxClaimableNowOld + 1 + i + ); + veaEpochOutboxCheckClaimsRangeArray.push(...newEpochs); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts(14 hunks)
🧰 Additional context used
📓 Learnings (1)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (5)
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:24-24
Timestamp: 2024-11-27T10:48:48.433Z
Learning: In the `validator-cli` codebase, both `arbitrumToEth` and `arbitrumToGnosis` modules use the same `ClaimStruct`, so importing `ClaimStruct` from either module is acceptable.
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:804-804
Timestamp: 2024-11-27T10:16:33.692Z
Learning: When the scope is to resurrect the testnet, it does not include making sure the validator is always profitable.
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:829-1112
Timestamp: 2024-11-26T08:37:47.591Z
Learning: Refactoring the `reconstructChallengeProgress` function in `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` into smaller functions is considered unnecessary abstraction.
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:454-457
Timestamp: 2024-11-25T09:05:40.703Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToGnosis.ts`, when calculating `arbAverageBlockTime`, it's acceptable not to add error handling for division by zero when `blockLatestArb.timestamp` equals `blockoldArb.timestamp` because if it occurs, it indicates a major issue that requires investigation.
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:342-351
Timestamp: 2024-11-25T09:05:35.087Z
Learning: The `retryOperation` function in `watcherArbToGnosis.ts` acts as the retry mechanism for the challenge transaction, so additional retry logic is unnecessary.
rabbit is refering to commits which do not exists due to force push
PR-Codex overview
This PR focuses on enhancing the
VeaInboxArbToGnosiscontract and related components, including the addition of new functions and events, adjustments to epoch periods, and updates to package dependencies.Detailed summary
event AffirmationCompletedinIAMB.sol.validator-cli/src/utils/ethers.tsfor connecting toRouterArbToGnosisandIAMB.01-arb-to-gnosis-outbox.ts..env.dist.watcherArbToGnosis.ts.VeaInboxArbToGnosisTestnet.package.jsonfiles to includets-nodeindevDependencies.Summary by CodeRabbit
Release Notes
New Features
AffirmationCompletedadded to the Gnosis interface for tracking message affirmation statuses.Bug Fixes
Documentation
Chores