feat/arb-sep-to-sep-testnet #344
Conversation
WalkthroughThe changes introduce a new smart contract, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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. |
✅ 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: 2
🧹 Outside diff range and nitpick comments (3)
validator-cli/src/ArbToEth/watcherArbToEth.ts (2)
Line range hint
1-624: Consider refactoring for improved maintainability and readability.While the changes related to the SDK update are correct, there are several areas where the overall code structure could be improved:
- The
watchfunction is quite long and complex. Consider breaking it down into smaller, more manageable functions.- Replace magic numbers and hardcoded values with named constants. For example,
300inconst finalityBuffer = 300;could beconst FINALITY_BUFFER_SECONDS = 300;.- The extensive use of
retryOperationsuggests that error handling could be centralized or improved. Consider implementing a more robust error handling strategy.- Replace console.log statements with a proper logging system that supports different log levels and can be easily configured for different environments.
Here's an example of how you might start refactoring the
watchfunction:const FINALITY_BUFFER_SECONDS = 300; const MAX_FINALITY_TIME_SECONDS_ETH = (slotsPerEpochEth * 3 - 1) * secondsPerSlotEth; async function watch() { const providers = await initializeProviders(); const contracts = await initializeContracts(providers); const params = await getSequencerParams(contracts); while (true) { const [blockArbFoundOnL1, blockFinalizedEth, finalityIssueFlagEth] = await getBlocksAndCheckFinality( providers.eth, providers.arb, contracts.sequencer, params.maxDelaySeconds ); if (!blockArbFoundOnL1) { console.error("Critical Error: Arbitrum block is not found on L1."); return; } await processEpochs(blockArbFoundOnL1, blockFinalizedEth, finalityIssueFlagEth, contracts, params); await wait(3000); // 3 second delay } } // Implement other functions like initializeProviders, initializeContracts, getSequencerParams, processEpochs, etc.This refactoring suggestion provides a starting point. You would need to implement the new functions and adjust the logic accordingly.
Line range hint
1-624: Summary: SDK update changes are correct, but consider overall code improvements.The changes related to the
@arbitrum/sdkupdate (import and usage ofgetArbitrumNetwork) are correct and align with the PR objectives. However, the file could benefit from refactoring to improve maintainability and readability. Consider breaking down complex functions, replacing magic numbers with named constants, improving error handling, and implementing a proper logging system.These improvements would make the code easier to understand, maintain, and debug in the long run.
validator-cli/src/utils/arbMsgExecutor.ts (1)
9-9: Rename 'trnxHash' to 'txnHash' for clarityThe parameter name
trnxHashcan be renamed totxnHashfor better readability and consistency, astxnis a common abbreviation for "transaction".Apply this diff to rename the parameter:
- trnxHash: string, + txnHash: string,Also, update all occurrences of
trnxHashwithin the function accordingly.
📜 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 (5)
- contracts/deployments/arbitrumSepolia/VeaInboxArbToEthTestnet.json (1 hunks)
- contracts/test/integration/ArbToEth.ts (8 hunks)
- validator-cli/package.json (1 hunks)
- validator-cli/src/ArbToEth/watcherArbToEth.ts (2 hunks)
- validator-cli/src/utils/arbMsgExecutor.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (16)
validator-cli/package.json (1)
18-18: Approve the dependency update with a recommendation for thorough testing.The update of
@arbitrum/sdkfrom^3.1.2to4.0.1is a major version bump, which could potentially introduce breaking changes. While the PR objectives indicate that related code has been modified to align with the SDK changes, it's crucial to ensure comprehensive testing across the entire project.To verify the impact of this change, please run the following script:
This script will help identify areas of the codebase that might be affected by the SDK update. Please review the results and ensure all necessary adjustments have been made.
Would you like assistance in reviewing any specific areas of the codebase that might be affected by this update?
validator-cli/src/ArbToEth/watcherArbToEth.ts (1)
3-3: LGTM! Verify impact on dependent code.The update from
getL2NetworktogetArbitrumNetworkaligns with the@arbitrum/sdkpackage upgrade mentioned in the PR objectives. This change appears to be correct and more specific to the Arbitrum network.To ensure this change doesn't affect other parts of the codebase, run the following script:
Also applies to: 35-35
✅ Verification successful
Verification Successful: No conflicting usages of
getL2Networkdetected outsidewatcherArbToGnosis.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of getL2Network rg "getL2Network" --type ts # Search for new usage of getArbitrumNetwork rg "getArbitrumNetwork" --type tsLength of output: 461
contracts/deployments/arbitrumSepolia/VeaInboxArbToEthTestnet.json (4)
3-270: ABI structure looks correct and comprehensiveThe ABI includes the following key components:
- Constructor with parameters
_epochPeriodand_veaOutboxArbToEth- Events:
MessageSent,SnapshotSaved, andSnapshotSent- Functions:
count,epochAt,epochFinalized,epochNow,epochPeriod,saveSnapshot,sendMessage,sendSnapshot,snapshots, andveaOutboxArbToEthThe function and event signatures appear to be correct and align with the expected functionality of a VeaInbox contract. Please verify that all required functions and events are present and correctly defined.
272-286: Deployment transaction details look correctThe contract was deployed with the following details:
- Transaction hash: 0x6e499d37ad3867970cef46cfd3cf46eda955b36197f9e8703e2ed7ecb546c5f1
- From: 0xFa00D29d378EDC57AA1006946F0fc6230a5E3288
- To: null (contract creation)
- Contract address: 0xE12daFE59Bc3A996362d54b37DFd2BA9279cAd06
- Block number: 77452741
- Gas used: 7438794
- Status: 1 (Success)
The deployment was successful. Please verify these details on the Arbitrum Sepolia block explorer to ensure everything is correct.
#!/bin/bash # Verify the deployment transaction on Arbitrum Sepolia explorer echo "Please manually verify the deployment transaction 0x6e499d37ad3867970cef46cfd3cf46eda955b36197f9e8703e2ed7ecb546c5f1 on the Arbitrum Sepolia explorer: https://sepolia.arbiscan.io/tx/0x6e499d37ad3867970cef46cfd3cf46eda955b36197f9e8703e2ed7ecb546c5f1"
2-2: Verify the contract addressThe contract is deployed at address
0xE12daFE59Bc3A996362d54b37DFd2BA9279cAd06. Please ensure this address matches the intended deployment address on the Arbitrum Sepolia testnet.
288-291: Verify constructor argumentsThe contract was deployed with the following constructor arguments:
epochPeriod: 7200 (2 hours in seconds)veaOutboxArbToEth: "0x209BFdC6B7c66b63A8382196Ba3d06619d0F12c9"Please confirm that these values are correct for the Arbitrum Sepolia testnet deployment. Ensure that the
epochPeriodmatches the corresponding VeaOutboxArbToEth contract on Ethereum and that theveaOutboxArbToEthaddress is correct.validator-cli/src/utils/arbMsgExecutor.ts (1)
44-44: Verify that the correct provider is passed toexecuteThe
executemethod is currently called withchildProvider, but for executing a child-to-parent message, it should typically be invoked on the parent chain usingparentProvider. Please verify that the correct provider is being used according to the Arbitrum SDK documentation.Run the following script to check the usage of
executein the codebase:✅ Verification successful
Verification Successful: The
childProvideris correctly used with theexecutemethod for child-to-parent messages.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify which provider is passed to the `execute` method for child-to-parent messages. # Test: Search for `childToParentMessage.execute` and inspect the provider used. rg --type ts 'childToParentMessage\.execute\(' -A 2Length of output: 282
Script:
#!/bin/bash # Description: Locate the definition and usage of `childProvider` and `parentProvider` to verify correct usage. # Search for the definition of `childProvider` rg --type ts 'const\s+childProvider\s*=' # Search for all usages of `parentProvider` with the `execute` method rg --type ts 'parentProvider\.execute\(' -A 2Length of output: 192
Script:
#!/bin/bash # Description: Locate the definition and all usages of `parentProvider` to verify correct usage. # Search for the definition of `parentProvider` rg --type ts 'const\s+parentProvider\s*=' # Search for all usages of `parentProvider` in the codebase rg --type ts 'parentProvider'Length of output: 385
contracts/test/integration/ArbToEth.ts (9)
29-30: LGTM for updated constantsThe updated
EPOCH_PERIODandCHALLENGE_PERIODconstants set to 600 seconds (10 minutes) are appropriate for Hardhat testing environments.
563-577: Duplicate of previous comment regarding challenge parameters
668-682: Duplicate of previous comment regarding challenge parameters
800-816: Duplicate of previous comment regarding challenge parameters
870-979: Test correctly verifies state updates after dispute resolutionThe test case effectively verifies that the
latestVerifiedEpochandstateRootare updated correctly after dispute resolution, ensuring the contract behaves as expected in this scenario.
981-1105: Test ensures correct state after failed dispute resolutionThe test case appropriately confirms that after a disputed and failed claim, the
latestVerifiedEpochandstateRootare updated to reflect the subsequent valid epoch, maintaining contract integrity.
781-790: Ensure verification parameters match expected typesVerify that the parameters provided to the
startVerificationfunction align with expected types, particularly the timestamp fields, to avoid potential type issues.Run the following script to verify the parameter types:
#!/bin/bash # Description: Verify the parameter types of the `startVerification` function in the VeaOutbox contract. # Test: Display the `startVerification` function definition to confirm parameter types. # Expect: Parameter types in the contract match those used in the test. rg --type solidity -A 10 'function startVerification'
525-539: Ensure challenge parameters match expected typesVerify that the parameters provided to the
challengefunction match the expected types, especially the timestamp fields, to prevent potential type mismatches or overflows.Run the following script to verify the parameter types:
#!/bin/bash # Description: Verify the parameter types of the `challenge` function in the VeaOutbox contract. # Test: Display the `challenge` function definitions to confirm parameter types. # Expect: Parameter types in the contract match those used in the test. rg --type solidity -A 15 'function challenge'
136-136: Verify that the error message matches the contractEnsure that the
claimfunction in the contract reverts with "Invalid epoch." when an invalid epoch is provided, so the test assertion remains accurate.Run the following script to verify the error message:
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Outside diff range and nitpick comments (5)
relayer-cli/src/devnetRelayExample.ts (1)
28-28: Fix typo in the commentThere's a typo in the comment: "libghtbulb" should be "lightbulb".
Apply this diff to correct the typo:
- // This is libghtbulb switch address in arbitrum sepolia + // This is lightbulb switch address in Arbitrum Sepoliarelayer-cli/src/utils/relayerHelpers.ts (2)
22-22: Remove debug logging or make it conditionalThe
console.log(process.cwd());statement is used for debugging purposes. If left in production code, it may clutter the logs with unnecessary information. Consider removing it or making it conditional based on a debug flag.Option 1: Remove the debug statement:
- console.log(process.cwd());Option 2: Make it conditional:
+ if (process.env.DEBUG) { console.log(process.cwd()); + }
6-9: Provide more informative error messagesWhen the lock file exists, the error thrown is generic:
throw new Error("Already running");. Providing a more informative error message can help with debugging and understanding the issue.Enhance the error message:
- throw new Error("Already running"); + throw new Error(`Process already running for chain ${chain_id} on network ${network}. Lock file exists at ${lock_file_name}`);relayer-cli/src/utils/relay.ts (2)
13-13: Update documentation forgetCountfunction signatureThe
getCountfunction now accepts eitherVeaOutboxArbToEthorVeaOutboxArbToGnosis. Please ensure that any associated documentation or comments are updated to reflect this change for clarity.
71-71: Fix typo in error messageThere's a typo in the error message on line 71:
"Unable to execute messgae batch..."should be corrected to"Unable to execute message batch...".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- relayer-cli/package.json (1 hunks)
- relayer-cli/src/devnetRelayExample.ts (2 hunks)
- relayer-cli/src/testnet/arbSepToChiadoRelayer.ts (1 hunks)
- relayer-cli/src/testnet/arbSepToSepRelayer.ts (1 hunks)
- relayer-cli/src/utils/ethers.ts (3 hunks)
- relayer-cli/src/utils/relay.ts (4 hunks)
- relayer-cli/src/utils/relayerHelpers.ts (1 hunks)
🧰 Additional context used
🪛 Biome
relayer-cli/src/testnet/arbSepToChiadoRelayer.ts
[error] 24-24: Unexpected constant condition.
(lint/correctness/noConstantCondition)
relayer-cli/src/testnet/arbSepToSepRelayer.ts
[error] 25-25: Unexpected constant condition.
(lint/correctness/noConstantCondition)
🔇 Additional comments (13)
relayer-cli/package.json (2)
13-13: LGTM: Syntax correctionThe added comma is a necessary syntax correction to accommodate the new script entry.
14-14: New script added for Sepolia testnetThe new script
start-testnet-sepoliahas been added to run the Arbitrum Sepolia to Sepolia relayer. This aligns with the PR objectives of updating the code to work with the new Arbitrum SDK version and focusing on testnet updates.Let's verify if the referenced TypeScript file exists:
✅ Verification successful
Verification Successful: TypeScript file exists as referenced.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of the new TypeScript file if [ -f "./src/testnet/arbSepToSepRelayer.ts" ]; then echo "File ./src/testnet/arbSepToSepRelayer.ts exists." else echo "File ./src/testnet/arbSepToSepRelayer.ts does not exist." fiLength of output: 171
Script:
#!/bin/bash # Search for the TypeScript file in the codebase fd "arbSepToSepRelayer\.ts" . --extension tsLength of output: 94
relayer-cli/src/testnet/arbSepToSepRelayer.ts (1)
30-30: 🛠️ Refactor suggestionClarify the delay calculation and explain the extra 100 seconds
The delay calculation adds an extra
100 * 1000milliseconds (100 seconds) to the delay amount. If this is intentional to account for network delays or synchronization buffers, consider adding a comment to explain the reason.Add a comment to clarify:
const delayAmount = (epochPeriod - (currentTS % epochPeriod)) * 1000 + 100 * 1000; + // Added extra 100 seconds to ensure the next epoch has startedIf the extra delay is not necessary, adjust the calculation accordingly.
relayer-cli/src/testnet/arbSepToChiadoRelayer.ts (1)
31-31: Verify the delay calculation and additional 100 secondsIn the delay calculation, an extra 100 seconds (
100 * 1000milliseconds) is added after computing the time until the next epoch. Please verify whether this additional time is intentional, as it could cause the relayer to wait longer than necessary between epochs.relayer-cli/src/devnetRelayExample.ts (2)
3-3: LGTMThe import of
initializeandupdateStateFilefromrelayerHelpersis appropriate and improves modularity.
27-27: Ensure all calls toinitializeinclude the newnetworkparameterThe
initializefunction now acceptsnetworkas a second parameter. Verify that all other invocations ofinitializein the codebase are updated accordingly to prevent unexpected behavior.Run the following script to identify any calls to
initializemissing thenetworkparameter:relayer-cli/src/utils/relayerHelpers.ts (1)
3-31: 🛠️ Refactor suggestionRemove unnecessary 'async' from 'initialize' function
The
initializefunction is marked asasyncbut only performs synchronous operations, except for the call toupdateStateFile. SinceupdateStateFiledoesn't contain anyawaitstatements, theasynckeyword is unnecessary. Removing it can simplify the code and prevent potential confusion about asynchronous behavior.Apply this diff to remove the
asynckeyword:- async function initialize(chain_id: number, network: string): Promise<number> { + function initialize(chain_id: number, network: string): number {Also, remove the
awaitkeyword when callingupdateStateFile:- await updateStateFile(chain_id, tsnow, 0, network); + updateStateFile(chain_id, tsnow, 0, network);Likely invalid or redundant comment.
relayer-cli/src/utils/ethers.ts (2)
7-8: Imports added for Gnosis factories are appropriateThe addition of
VeaInboxArbToGnosis__factoryandVeaOutboxArbToGnosis__factoryimports is correct and necessary for handling connections to Gnosis-related contracts.
63-67: Updated exports are correct and ensure availabilityThe export statements have been updated appropriately, making the new functions available for import elsewhere in the codebase.
relayer-cli/src/utils/relay.ts (4)
2-4: Imports updated to reflect SDK changes correctlyThe import statements have been updated to include
getVeaOutbox,VeaOutboxArbToEth, andVeaOutboxArbToGnosis, aligning with the updated SDK version4.0.1.
33-33: EnsuregetVeaOutboxhandleschainidparameter correctlyVerify that the
getVeaOutboxfunction correctly utilizes thechainidparameter to instantiate the appropriateveaOutboxinstance. This ensures correct contract interactions based on the network.
90-90: Confirm correct initialization ofveaOutboxwithchainidEnsure that the
veaOutboxinstance is correctly initialized with thechainidparameter in therelayAllFromfunction. This is crucial for accurate network-specific operations.
51-74:⚠️ Potential issueReview batch relay logic for correctness and efficiency
The new batching mechanism introduced in the
relayBatchfunction processes messages based onmaxBatchSize. Please verify the following:
Nonce Increment Logic: The line
nonce += batchMessages + 1;may result in skipping nonces. Ensure thatnonceis incremented correctly bybatchMessageswithout the additional+ 1to prevent skipping messages.Batch Size Control: Confirm that the conditions in the
forloop correctly control the batch size and thatbatchMessagesaccurately reflects the number of messages processed.Error Handling: In the
catchblock, consider implementing a retry mechanism or logging additional context to facilitate troubleshooting.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (13)
relayer-cli/src/testnet/arbSepToChiadoRelayer.ts (4)
5-5: Consider making the contract path configurable.The hardcoded contract path makes the code less flexible and harder to maintain. Consider:
- Moving it to a configuration file or environment variable
- Using dynamic import instead of require
-const _contract = require("@kleros/vea-contracts/deployments/chiado/VeaOutboxArbToGnosisTestnet.json"); +import contractConfig from process.env.CONTRACT_PATH || "@kleros/vea-contracts/deployments/chiado/VeaOutboxArbToGnosisTestnet.json";
7-10: Consider making configuration values environment-dependent.These hardcoded values should be configurable based on the environment to improve flexibility and maintainability.
-const chainId = 10200; -const epochPeriod = 7200; // 3 hrs -const batchSize = 10; // 10 messages per batch -const network = "testnet"; +const chainId = parseInt(process.env.CHAIN_ID || "10200"); +const epochPeriod = parseInt(process.env.EPOCH_PERIOD || "7200"); // 3 hrs +const batchSize = parseInt(process.env.BATCH_SIZE || "10"); // messages per batch +const network = process.env.NETWORK || "testnet";
13-20: Enhance cleanup logic with better error handling.The cleanup logic could be more robust by:
- Adding error handling for file operations
- Ensuring synchronous cleanup before exit
process.on(signal, async () => { - console.log("exit"); + console.log(`Received ${signal} signal, cleaning up...`); const lock_file_name = "./src/state/" + chainId + ".pid"; - if (fs.existsSync(lock_file_name)) { - fs.unlinkSync(lock_file_name); + try { + if (fs.existsSync(lock_file_name)) { + fs.unlinkSync(lock_file_name); + console.log('Lock file removed successfully'); + } + } catch (error) { + console.error('Error during cleanup:', error); } process.exit(0); })
29-32: Consider improving delay precision.The current delay calculation adds a fixed 100 seconds buffer, which might be excessive. Consider:
- Making the buffer configurable
- Using a more precise timing mechanism
const currentTS = Math.floor(Date.now() / 1000); - const delayAmount = (epochPeriod - (currentTS % epochPeriod)) * 1000 + 100 * 1000; + const bufferSeconds = parseInt(process.env.EPOCH_BUFFER || "30"); + const delayAmount = (epochPeriod - (currentTS % epochPeriod)) * 1000 + bufferSeconds * 1000; console.log("waiting for the next epoch. . .", Math.floor(delayAmount / 1000), "seconds"); await delay(delayAmount);relayer-cli/src/utils/relay.ts (4)
Line range hint
13-29: Add error handling for subgraph queries.While the function implementation is correct, consider adding error handling for the GraphQL request to handle potential network issues or API failures gracefully.
const getCount = async (veaOutbox: VeaOutboxArbToEth | VeaOutboxArbToGnosis, chainid: number): Promise<number> => { const subgraph = getInboxSubgraph(chainid); const stateRoot = await veaOutbox.stateRoot(); + try { const result = await request( `https://api.studio.thegraph.com/query/${subgraph}`, `{ snapshotSaveds(first: 1, where: { stateRoot: "${stateRoot}" }) { count } }` ); if (result["snapshotSaveds"].length == 0) return 0; return Number(result["snapshotSaveds"][0].count); + } catch (error) { + console.error(`Failed to fetch count from subgraph: ${error.message}`); + throw error; + } };🧰 Tools
🪛 eslint
[error] 13-13: Parsing error: Unexpected token :
Line range hint
31-40: Add error handling for contract interactions.The relay function should handle potential contract interaction failures gracefully.
const relay = async (chainid: number, nonce: number) => { const routeParams = getBridgeConfig(chainid); const veaOutbox = getVeaOutbox(routeParams.veaOutbox, process.env.PRIVATE_KEY, routeParams.rpcOutbox, chainid); - const count = await getCount(veaOutbox, chainid); + try { + const count = await getCount(veaOutbox, chainid); - const proof = await getProofAtCount(chainid, nonce, count); - const [to, data] = await getMessageDataToRelay(chainid, nonce); + const proof = await getProofAtCount(chainid, nonce, count); + const [to, data] = await getMessageDataToRelay(chainid, nonce); - const txn = await veaOutbox.sendMessage(proof, nonce, to, data); - await txn.wait(); + const txn = await veaOutbox.sendMessage(proof, nonce, to, data); + await txn.wait(); + } catch (error) { + console.error(`Failed to relay message at nonce ${nonce}: ${error.message}`); + throw error; + } };
43-78: Enhance batch processing robustness.While the new batching logic is an improvement, consider these enhancements:
- Add input validation for maxBatchSize
- Improve error message formatting
- Add gas estimation checks before batch execution
const relayBatch = async (chainid: number, nonce: number, maxBatchSize: number, _contract: any) => { + if (maxBatchSize <= 0 || maxBatchSize > 100) { + throw new Error('maxBatchSize must be between 1 and 100'); + } const routeParams = getBridgeConfig(chainid); const web3 = new Web3(routeParams.rpcOutbox); const batchedSend = _batchedSend(web3, routeParams.batcher, process.env.PRIVATE_KEY, 0); const contract = new web3.eth.Contract(_contract.abi, routeParams.veaOutbox); const veaOutbox = getVeaOutbox(routeParams.veaOutbox, process.env.PRIVATE_KEY, routeParams.rpcOutbox, chainid); const count = await getCount(veaOutbox, chainid); while (nonce < count) { let batchMessages = 0; let txns = []; while (batchMessages < maxBatchSize && nonce < count) { const isMsgRelayed = await veaOutbox.isMsgRelayed(nonce); if (isMsgRelayed) { nonce++; continue; } const proof = await getProofAtCount(chainid, nonce, count); const [to, data] = await getMessageDataToRelay(chainid, nonce); txns.push({ args: [proof, nonce, to, data], method: contract.methods.sendMessage, to: contract.options.address, }); batchMessages += 1; nonce++; } if (batchMessages > 0) { try { + // Estimate gas for the batch + const gasEstimate = await batchedSend.estimateGas(txns); + console.log(`Estimated gas for batch: ${gasEstimate}`); await batchedSend(txns); } catch (error) { - console.error(`Unable to execute messgae batch(${batchMessages} msgs) from nonce:${nonce} `, error); + console.error( + `Failed to execute message batch (size: ${batchMessages}, start nonce: ${nonce - batchMessages}): ${error.message}` + ); + throw error; } } } return nonce; };
Line range hint
81-115: Add error handling and improve code organization.The function needs error handling for batch transactions and could benefit from better code organization.
const relayAllFrom = async (chainid: number, nonce: number, msgSender: string, _contract: any): Promise<number> => { const routeParams = getBridgeConfig(chainid); const web3 = new Web3(routeParams.rpcOutbox); - const batchedSend = _batchedSend( - web3, // Your web3 object. - // The address of the transaction batcher contract you wish to use. The addresses for the different networks are listed below. If the one you need is missing, feel free to deploy it yourself and make a PR to save the address here for others to use. - routeParams.batcher, - process.env.PRIVATE_KEY, // The private key of the account you want to send transactions from. - 0 // The debounce timeout period in milliseconds in which transactions are batched. - ); + const batchedSend = _batchedSend(web3, routeParams.batcher, process.env.PRIVATE_KEY, 0); const contract = new web3.eth.Contract(_contract.abi, routeParams.veaOutbox); const veaOutbox = getVeaOutbox(routeParams.veaOutbox, process.env.PRIVATE_KEY, routeParams.rpcOutbox, chainid); - const count = await getCount(veaOutbox, chainid); + try { + const count = await getCount(veaOutbox, chainid); + if (!count) return null; - if (!count) return null; + const nonces = await getNonceFrom(chainid, nonce, msgSender); + if (!nonces.length) { + console.log(`No messages found from sender ${msgSender} starting at nonce ${nonce}`); + return null; + } - let txns = []; + const txns = []; - const nonces = await getNonceFrom(chainid, nonce, msgSender); for (const x of nonces) { const proof = await getProofAtCount(chainid, x, count); const [to, data] = await getMessageDataToRelay(chainid, x); txns.push({ args: [proof, x, to, data], method: contract.methods.sendMessage, to: contract.options.address, }); } - await batchedSend(txns); + await batchedSend(txns); + return nonces[nonces.length - 1]; + } catch (error) { + console.error(`Failed to relay messages from ${msgSender}: ${error.message}`); + throw error; + } - return nonces[nonces.length - 1]; };Also, consider moving the batchedSend configuration details to the documentation.
relayer-cli/src/testnet/arbSepToSepRelayer.ts (3)
25-25: Suppress linter error for constant condition in the while loopThe
while (true)loop triggers a linting error: "Unexpected constant condition." Since the infinite loop is intentional, you can suppress the linter warning by adding an inline comment.Apply this diff to suppress the linter warning:
+ // eslint-disable-next-line no-constant-condition while (true) {🧰 Tools
🪛 eslint
[error] 25-25: Unexpected constant condition.
(no-constant-condition)
16-18: Use camelCase for variable namesIt's recommended to use camelCase for variable names in JavaScript/TypeScript. Rename
lock_file_nametolockFileNamefor consistency.Apply this diff:
- const lock_file_name = "./src/state/" + chainId + ".pid"; - if (fs.existsSync(lock_file_name)) { - fs.unlinkSync(lock_file_name); + const lockFileName = "./src/state/" + chainId + ".pid"; + if (fs.existsSync(lockFileName)) { + fs.unlinkSync(lockFileName);
6-6: Use a descriptive variable name instead of_contractUsing a leading underscore in variable names is unconventional and may lead to confusion. Consider renaming
_contractto a more descriptive name likeveaOutboxContract.Apply this diff:
-const _contract = require("@kleros/vea-contracts/deployments/sepolia/VeaOutboxArbToEthTestnet.json"); +const veaOutboxContract = require("@kleros/vea-contracts/deployments/sepolia/VeaOutboxArbToEthTestnet.json");Ensure to update all references to
_contractaccordingly.relayer-cli/src/utils/relayerHelpers.ts (1)
6-9: Consider handling stale lock files to improve robustnessThe current implementation checks for the existence of a lock file to prevent concurrent processes. However, if a previous process terminated unexpectedly without deleting the lock file, subsequent executions will fail unnecessarily. To improve robustness, consider verifying whether the process ID stored in the lock file corresponds to an active process before throwing an error. If the process is not running, the lock file can be safely removed.
relayer-cli/src/utils/ethers.ts (1)
20-27: Consider refactoring repetitive contract connection functionsThe functions
getVeaInbox,getVeaInboxProvider,getVeaOutbox, andgetVeaOutboxProviderhave similar structures and logic, differing mainly in contract factories and providers. Refactoring these into generic helper functions would reduce code duplication and enhance maintainability.Here is a suggested refactor:
// Generic function to connect to a Vea contract using provider URL function getVeaContract( contractFactories: { [key: number]: any }, address: string, privateKey: string, providerURL: string, chainId: number ) { const wallet = getWallet(privateKey, providerURL); const contractFactory = contractFactories[chainId]; if (!contractFactory) { throw new Error(`Unsupported chainId: ${chainId}`); } return contractFactory.connect(address, wallet); } // Generic function to connect to a Vea contract using JsonRpcProvider function getVeaContractWithProvider( contractFactories: { [key: number]: any }, address: string, privateKey: string, rpc: JsonRpcProvider, chainId: number ) { const wallet = getWalletRPC(privateKey, rpc); const contractFactory = contractFactories[chainId]; if (!contractFactory) { throw new Error(`Unsupported chainId: ${chainId}`); } return contractFactory.connect(address, wallet); }Then, replace the individual functions:
-function getVeaInbox(veaInboxAddress: string, privateKey: string, web3ProviderURL: string, chainId: number) { - if (chainId == 11155111) { - return VeaInboxArbToEth__factory.connect(veaInboxAddress, getWallet(privateKey, web3ProviderURL)); - } else if (chainId == 10200) { - return VeaInboxArbToGnosis__factory.connect(veaInboxAddress, getWallet(privateKey, web3ProviderURL)); - } else { - throw new Error(`Unsupported chainId: ${chainId}`); - } +function getVeaInbox(address: string, privateKey: string, web3ProviderURL: string, chainId: number) { + const contractFactories = { + 11155111: VeaInboxArbToEth__factory, + 10200: VeaInboxArbToGnosis__factory, + }; + return getVeaContract(contractFactories, address, privateKey, web3ProviderURL, chainId); } -function getVeaInboxProvider(veaInboxAddress: string, privateKey: string, rpc: JsonRpcProvider, chainId: number) { - if (chainId == 11155111) { - return VeaInboxArbToEth__factory.connect(veaInboxAddress, getWalletRPC(privateKey, rpc)); - } else if (chainId == 10200) { - return VeaInboxArbToGnosis__factory.connect(veaInboxAddress, getWalletRPC(privateKey, rpc)); - } +function getVeaInboxProvider(address: string, privateKey: string, rpc: JsonRpcProvider, chainId: number) { + const contractFactories = { + 11155111: VeaInboxArbToEth__factory, + 10200: VeaInboxArbToGnosis__factory, + }; + return getVeaContractWithProvider(contractFactories, address, privateKey, rpc, chainId); } -function getVeaOutbox(veaInboxAddress: string, privateKey: string, web3ProviderURL: string, chainId: number) { - if (chainId == 11155111) { - return VeaOutboxArbToEth__factory.connect(veaInboxAddress, getWallet(privateKey, web3ProviderURL)); - } else if (chainId == 10200) { - return VeaOutboxArbToGnosis__factory.connect(veaInboxAddress, getWallet(privateKey, web3ProviderURL)); - } else { - throw new Error(`Unsupported chainId: ${chainId}`); - } +function getVeaOutbox(address: string, privateKey: string, web3ProviderURL: string, chainId: number) { + const contractFactories = { + 11155111: VeaOutboxArbToEth__factory, + 10200: VeaOutboxArbToGnosis__factory, + }; + return getVeaContract(contractFactories, address, privateKey, web3ProviderURL, chainId); } -function getVeaOutboxProvider(veaOutboxAddress: string, privateKey: string, rpc: JsonRpcProvider, chainId: number) { - if (chainId == 11155111) { - return VeaOutboxArbToEth__factory.connect(veaOutboxAddress, getWalletRPC(privateKey, rpc)); - } else if (chainId == 10200) { - return VeaOutboxArbToGnosis__factory.connect(veaOutboxAddress, getWalletRPC(privateKey, rpc)); - } else { - throw new Error(`Unsupported chainId: ${chainId}`); - } +function getVeaOutboxProvider(address: string, privateKey: string, rpc: JsonRpcProvider, chainId: number) { + const contractFactories = { + 11155111: VeaOutboxArbToEth__factory, + 10200: VeaOutboxArbToGnosis__factory, + }; + return getVeaContractWithProvider(contractFactories, address, privateKey, rpc, chainId); }Also applies to: 30-35, 38-45, 48-55
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
relayer-cli/src/testnet/arbSepToChiadoRelayer.ts(1 hunks)relayer-cli/src/testnet/arbSepToSepRelayer.ts(1 hunks)relayer-cli/src/utils/ethers.ts(3 hunks)relayer-cli/src/utils/relay.ts(4 hunks)relayer-cli/src/utils/relayerHelpers.ts(1 hunks)
🧰 Additional context used
🪛 eslint
relayer-cli/src/testnet/arbSepToChiadoRelayer.ts
[error] 24-24: Unexpected constant condition.
(no-constant-condition)
relayer-cli/src/testnet/arbSepToSepRelayer.ts
[error] 25-25: Unexpected constant condition.
(no-constant-condition)
relayer-cli/src/utils/relay.ts
[error] 13-13: Parsing error: Unexpected token :
relayer-cli/src/utils/relayerHelpers.ts
[error] 3-3: Parsing error: Unexpected token :
🔇 Additional comments (1)
relayer-cli/src/utils/relay.ts (1)
2-4: LGTM! Import changes align with multi-network support.
The changes to imports and type definitions appropriately support the generalization of the outbox functionality across different networks.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
validator-cli/.env.dist (2)
3-6: Consider adding backup RPC endpoints and documenting empty variables.While the RPC endpoints are correctly configured, consider:
- Adding fallback RPC endpoints to prevent single point of failure
- Documenting the expected format for
RPC_SEPOLIAExample documentation to add:
# Devnet RPCs +# Format: https://rpc-endpoint-url +# For improved reliability, consider using multiple RPC endpoints separated by comma RPC_CHIADO=https://rpc.chiadochain.net RPC_ARB_SEPOLIA=https://sepolia-rollup.arbitrum.io/rpc RPC_SEPOLIA=
8-11: Document the required RPC endpoints for production.The empty RPC variables need documentation to guide users on which networks they should point to.
Add documentation:
# Testnet or Mainnet RPCs +# RPC_ARB: Arbitrum One mainnet RPC endpoint +# RPC_ETH: Ethereum mainnet RPC endpoint RPC_ARB= RPC_ETH=validator-cli/src/utils/arbMsgExecutor.ts (2)
13-13: Add JSDoc documentation for the exported function.Since this is an exported function, it should have proper documentation explaining its purpose, parameters, return value, and possible errors.
+/** + * Executes a child-to-parent message from Arbitrum to Ethereum + * @param trnxHash - The transaction hash containing the message to execute + * @param childRpc - The RPC URL for the Arbitrum network + * @param parentRpc - The RPC URL for the Ethereum network + * @returns Promise resolving to the executed transaction + * @throws Error if message execution fails or prerequisites are not met + */ async function messageExecutor(
1-87: Consider architectural improvements for better maintainability.
- Consider extracting provider initialization into a reusable utility function to avoid duplication between
messageExecutorandgetMessageStatus.- Implement a consistent error handling strategy across both functions, preferably using custom error classes for better error categorization.
- Consider adding retry mechanisms for transient failures, especially for RPC calls.
Example implementation of a provider utility:
interface ProviderConfig { childRpc: string; parentRpc: string; privateKey: string; } interface NetworkProviders { childProvider: ArbitrumProvider; parentProvider: JsonRpcProvider; parentSigner: Signer; } function initializeProviders(config: ProviderConfig): NetworkProviders { const childJsonRpc = new JsonRpcProvider(config.childRpc); const childProvider = new ArbitrumProvider(childJsonRpc); const parentProvider = new JsonRpcProvider(config.parentRpc); const parentSigner = new Wallet(config.privateKey, parentProvider); return { childProvider, parentProvider, parentSigner }; }validator-cli/src/ArbToEth/watcherArbToEth.ts (1)
19-24: Consider implementing monitoring and alerting system.The watcher script handles critical cross-chain message verification. While the code comments explain the core flow well, the system would benefit from:
- Structured logging with severity levels
- Metrics collection for:
- Number of claims processed
- Challenge success/failure rates
- Gas usage patterns
- Health check endpoints
- Alert triggers for:
- Failed message executions
- Gas price anomalies
- Network issues
Would you like assistance in implementing a monitoring solution?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
validator-cli/.env.dist(1 hunks)validator-cli/package.json(1 hunks)validator-cli/src/ArbToEth/watcherArbToEth.ts(13 hunks)validator-cli/src/utils/arbMsgExecutor.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- validator-cli/package.json
🔇 Additional comments (6)
validator-cli/.env.dist (2)
Line range hint 1-24: LGTM! Well-organized environment template.
The environment template is well-structured with clear sections for different networks and purposes. The new variables are properly integrated into the existing organization.
12-15: Verify the contract addresses match the deployed contracts.
The contract addresses need to be verified against the deployment artifacts to ensure they match the actual deployed contracts.
✅ Verification successful
Based on the deployment artifacts, I can verify that the contract addresses match correctly:
- VeaInbox (arbitrumSepolia): 0xE12daFE59Bc3A996362d54b37DFd2BA9279cAd06
- VeaOutbox (sepolia): 0x209BFdC6B7c66b63A8382196Ba3d06619d0F12c9
The contracts are correctly linked to each other:
- VeaInbox constructor was called with VeaOutbox address
- VeaOutbox constructor was called with VeaInbox address
The addresses in the .env.dist file match the deployed contract addresses.
Contract addresses are correctly configured and match the deployment artifacts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract addresses match deployment artifacts
# Search for contract addresses in deployment files
echo "Searching for VeaInbox contract address..."
rg -l "0xE12daFE59Bc3A996362d54b37DFd2BA9279cAd06" contracts/deployments/
echo "Searching for VeaOutbox contract address..."
rg -l "0x209BFdC6B7c66b63A8382196Ba3d06619d0F12c9" contracts/deployments/
Length of output: 593
Script:
#!/bin/bash
# Let's examine the actual content of these deployment files to verify the addresses
echo "Content of VeaOutboxArbToEthTestnet.json in sepolia:"
cat contracts/deployments/sepolia/VeaOutboxArbToEthTestnet.json
echo -e "\nContent of VeaInboxArbToEthTestnet.json in arbitrumSepolia:"
cat contracts/deployments/arbitrumSepolia/VeaInboxArbToEthTestnet.json
Length of output: 186109
validator-cli/src/utils/arbMsgExecutor.ts (2)
1-11: LGTM! Imports are well-organized and necessary.
All imported modules are utilized effectively in the implementation.
87-87: LGTM! Clean and clear exports.
The named exports are well-structured.
validator-cli/src/ArbToEth/watcherArbToEth.ts (2)
1-11: LGTM: Import statements correctly updated for new Arbitrum SDK.
The changes align with the PR objective of updating to @arbitrum/sdk v4.0.1, particularly the change from getL2Network to getArbitrumNetwork.
475-488: Review gas fee calculation logic.
The gas fee calculation logic has potential issues:
- The
maxFeePerGasProfitablecalculation divides the deposit bygasEstimate.mul(6), which might result in very low gas fees during high network congestion. - The hardcoded priority fee of 6667 gwei might be too high or too low depending on network conditions.
Consider implementing a more dynamic gas fee calculation:
- Use an oracle or gas price feed
- Implement exponential backoff for retries with increasing gas prices
- Add configurable minimum/maximum gas price bounds
| async function getMessageStatus( | ||
| trnxHash: string, | ||
| childRpc: string, | ||
| parentRpc: string | ||
| ): Promise<ChildToParentMessageStatus> { | ||
| 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); | ||
| } catch (error) { | ||
| throw new Error(`Failed to get child transaction receipt: ${error.message}`); | ||
| } | ||
| if (!childReceipt) { | ||
| throw new Error(`Transaction receipt not found for hash: ${trnxHash}`); | ||
| } | ||
| const messageReceipt = new ChildTransactionReceipt(childReceipt); | ||
| const parentSigner: Signer = new Wallet(PRIVATE_KEY, parentProvider); | ||
| const messages = await messageReceipt.getChildToParentMessages(parentSigner); | ||
| const childToParentMessage = messages[0]; | ||
| if (!childToParentMessage) { | ||
| console.error("No child-to-parent messages found"); | ||
| } | ||
| try { | ||
| const status = await childToParentMessage.status(childProvider); | ||
| return status; | ||
| } catch (error) { | ||
| console.error(`Failed to get message status: ${error.message}`); | ||
| return undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error handling consistency and type safety.
The function has several issues that need attention:
- Inconsistent error handling (mixing throws and console.error)
- Return type doesn't match implementation (can return undefined)
- Missing array length validation
Apply these improvements:
async function getMessageStatus(
trnxHash: string,
childRpc: string,
parentRpc: string
-): Promise<ChildToParentMessageStatus> {
+): Promise<ChildToParentMessageStatus | undefined> {
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);
} catch (error) {
throw new Error(`Failed to get child transaction receipt: ${error.message}`);
}
if (!childReceipt) {
throw new Error(`Transaction receipt not found for hash: ${trnxHash}`);
}
const messageReceipt = new ChildTransactionReceipt(childReceipt);
const parentSigner: Signer = new Wallet(PRIVATE_KEY, parentProvider);
const messages = await messageReceipt.getChildToParentMessages(parentSigner);
+ if (!messages || messages.length === 0) {
+ throw new Error("No child-to-parent messages found");
+ }
const childToParentMessage = messages[0];
- if (!childToParentMessage) {
- console.error("No child-to-parent messages found");
- }
try {
const status = await childToParentMessage.status(childProvider);
return status;
} catch (error) {
- console.error(`Failed to get message status: ${error.message}`);
- return undefined;
+ throw new Error(`Failed to get message status: ${error.message}`);
}
}📝 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.
| async function getMessageStatus( | |
| trnxHash: string, | |
| childRpc: string, | |
| parentRpc: string | |
| ): Promise<ChildToParentMessageStatus> { | |
| 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); | |
| } catch (error) { | |
| throw new Error(`Failed to get child transaction receipt: ${error.message}`); | |
| } | |
| if (!childReceipt) { | |
| throw new Error(`Transaction receipt not found for hash: ${trnxHash}`); | |
| } | |
| const messageReceipt = new ChildTransactionReceipt(childReceipt); | |
| const parentSigner: Signer = new Wallet(PRIVATE_KEY, parentProvider); | |
| const messages = await messageReceipt.getChildToParentMessages(parentSigner); | |
| const childToParentMessage = messages[0]; | |
| if (!childToParentMessage) { | |
| console.error("No child-to-parent messages found"); | |
| } | |
| try { | |
| const status = await childToParentMessage.status(childProvider); | |
| return status; | |
| } catch (error) { | |
| console.error(`Failed to get message status: ${error.message}`); | |
| return undefined; | |
| } | |
| } | |
| async function getMessageStatus( | |
| trnxHash: string, | |
| childRpc: string, | |
| parentRpc: string | |
| ): Promise<ChildToParentMessageStatus | undefined> { | |
| 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); | |
| } catch (error) { | |
| throw new Error(`Failed to get child transaction receipt: ${error.message}`); | |
| } | |
| if (!childReceipt) { | |
| throw new Error(`Transaction receipt not found for hash: ${trnxHash}`); | |
| } | |
| const messageReceipt = new ChildTransactionReceipt(childReceipt); | |
| const parentSigner: Signer = new Wallet(PRIVATE_KEY, parentProvider); | |
| const messages = await messageReceipt.getChildToParentMessages(parentSigner); | |
| if (!messages || messages.length === 0) { | |
| throw new Error("No child-to-parent messages found"); | |
| } | |
| const childToParentMessage = messages[0]; | |
| try { | |
| const status = await childToParentMessage.status(childProvider); | |
| return status; | |
| } catch (error) { | |
| throw new Error(`Failed to get message status: ${error.message}`); | |
| } | |
| } |
There was a problem hiding this comment.
Will update the message array check. For failed status fetch I think its better to return undefined because it could be caused by a temporary finality issue, what do you think?
There was a problem hiding this comment.
@mani99brar, that makes sense. Returning undefined for failed status fetches due to potential temporary finality issues is reasonable. Thank you for updating the message array check.
✏️ Learnings added
Learnt from: mani99brar
PR: kleros/vea#344
File: validator-cli/src/utils/arbMsgExecutor.ts:52-85
Timestamp: 2024-11-21T05:40:34.773Z
Learning: In the `getMessageStatus` function, returning `undefined` for failed status fetches is acceptable because failures may be due to temporary finality issues.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@mani99brar if the operation is a find or get like, in which the existance of the requested item is not assumed, I think it is reasonable to return a null (I know it does not exist) or, as second option, as undefined (I have no knowledge about the entity state, which is not as true as we know it does not exist)
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@fcanela The code which calls this function ensures that the message exists. It's not a case where we are uncertain; we always expect to retrieve the message status unless there's an error from the sdk call. It would be more wise to let it fail here?
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
There was a problem hiding this comment.
Good job mate!
I made a quick review. I am not deeply familiar with Vea yet, so there may be issues that I was not able to catch. Logic-wise, it seemed reasonable for someone with my limited context.
I did a quick check on practices that we are following in other parts of our codebase. The intention is normalizing how the codebases are, but I understand the feedback can be quite... intense. Please do not assume the feedback is correct and feel free to ask any clarifications questions or annotations as you want.
If you need help implementing any, please ping!
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (16)
relayer-cli/.env.dist (1)
9-11: Clean up consecutive empty linesConsider removing one of the consecutive empty lines after
VEAOUTBOX_CHAIN_ID.VEAOUTBOX_CHAIN_ID=1115111 - VEAINBOX_ARBSEPOLIA_TO_SEPOLIA_ADDRESS=relayer-cli/src/testnetRelayer.ts (1)
6-11: Consider improving configuration flexibility and validation.The current implementation has several areas that could be enhanced:
- The batch size is hardcoded to 10, which might not be optimal for all scenarios
- Chain ID is used without validation
- Network value is hardcoded to "testnet"
Consider applying these improvements:
+const DEFAULT_BATCH_SIZE = 10; + export async function start(shutdownManager: ShutdownManager = new ShutdownManager()) { const network = "testnet"; const chainId = parseInt(process.env.VEAOUTBOX_CHAIN_ID); + if (isNaN(chainId)) { + throw new Error("Invalid chain ID in environment variables"); + } const epochPeriod = getEpochPeriod(chainId); - const batchSize = 10; // 10 messages per batch + const batchSize = parseInt(process.env.BATCH_SIZE) || DEFAULT_BATCH_SIZE;relayer-cli/src/consts/bridgeRoutes.ts (3)
3-4: Consider making contract imports more flexible.The hardcoded imports for specific testnet deployments might make it difficult to switch between different environments (e.g., mainnet, other testnets).
Consider implementing a dynamic import mechanism based on the environment:
const getContractArtifact = (chain: string, contract: string) => import(`@kleros/vea-contracts/deployments/${chain}/${contract}.json`);
8-14: Improve type safety for contract interfaces.The
veaOutboxContractis typed asany, which bypasses TypeScript's type checking benefits.Consider creating proper type definitions:
import { Contract } from 'ethers'; interface VeaOutboxContract extends Contract { // Add specific contract methods and properties } interface IBridge { // ... other properties veaOutboxContract: VeaOutboxContract; }
50-58: Standardize error handling across getter functions.The error handling in
getInboxSubgraphdiffers from other getters. Consider using a consistent approach.const getInboxSubgraph = (chainId: number): string => { + if (!(chainId in bridges)) { + throw new Error(`Unsupported chainId: ${chainId}`); + } switch (chainId) { case 11155111: return process.env.VEAINBOX_ARBSEPOLIA_TO_SEPOLIA_SUBGRAPH; case 10200: return process.env.VEAINBOX_ARBSEPOLIA_TO_CHIADO_SUBGRAPH; default: - throw new Error("Invalid chainId"); + throw new Error(`Unsupported chainId: ${chainId}`); } };relayer-cli/src/utils/ethers.ts (4)
20-21: Improve comment clarity for chainId usageThe current comment could be more explicit. Consider:
-// Using destination chainId as identifier, Ex: Arbitrum One (42161) -> Ethereum Mainnet (1): Use "1" as chainId +// chainId represents the destination chain identifier +// Example: For Arbitrum One (42161) -> Ethereum Mainnet (1) bridge, use chainId=1 +// Example: For Arbitrum Sepolia -> Ethereum Sepolia bridge, use chainId=11155111
36-37: Maintain consistent variable namingThe variable name
bridgesshould be singular to maintain consistency withgetVeaInboxfunction.- const bridges = getBridgeConfig(chainId); - switch (bridges.chain) { + const bridge = getBridgeConfig(chainId); + switch (bridge.chain) {
64-65: Maintain consistent variable namingThe variable name
bridgesshould be singular for consistency across all functions.- const bridges = getBridgeConfig(chainId); - switch (bridges.chain) { + const bridge = getBridgeConfig(chainId); + switch (bridge.chain) {
21-74: Extract chain IDs to constantsAs suggested in previous reviews, chain IDs should be defined as constants to improve maintainability and avoid magic numbers.
Consider adding a constants file or extending the bridge configuration:
// constants/chains.ts export const CHAIN_IDS = { ETH_MAINNET: 1, ETH_SEPOLIA: 11155111, GNOSIS_MAINNET: 100, GNOSIS_CHIADO: 10200, } as const;relayer-cli/src/utils/relay.ts (4)
Line range hint
10-26: Add error handling and improve type safety.Consider these improvements:
- Add error handling for the GraphQL request
- Use strict equality (
===) instead of loose equality (==)const getCount = async (veaOutbox: VeaOutboxArbToEth | VeaOutboxArbToGnosis, chainId: number): Promise<number> => { const subgraph = getInboxSubgraph(chainId); const stateRoot = await veaOutbox.stateRoot(); + try { const result = await request( `https://api.studio.thegraph.com/query/${subgraph}`, `{ snapshotSaveds(first: 1, where: { stateRoot: "${stateRoot}" }) { count } }` ); - if (result["snapshotSaveds"].length == 0) return 0; + if (result["snapshotSaveds"].length === 0) return 0; return Number(result["snapshotSaveds"][0].count); + } catch (error) { + console.error(`Failed to fetch count from subgraph: ${error}`); + throw error; + } };
42-75: Improve batch processing robustness and monitoring.Consider these improvements:
- Add progress logging for better monitoring
- Implement memory management for large batches
- Add error handling for batch transactions
const relayBatch = async (chainid: number, nonce: number, maxBatchSize: number) => { + if (!process.env.PRIVATE_KEY) { + throw new Error('PRIVATE_KEY environment variable is not set'); + } + const routeParams = getBridgeConfig(chainid); const web3 = new Web3(routeParams.rpcOutbox); const batchedSend = initializeBatchedSend(web3, routeParams.batcher, process.env.PRIVATE_KEY, 0); const veaOutboxInstance = new web3.eth.Contract(routeParams.veaOutboxContract.abi, routeParams.veaOutboxAddress); const veaOutbox = getVeaOutbox(routeParams.veaOutboxAddress, process.env.PRIVATE_KEY, routeParams.rpcOutbox, chainid); const count = await getCount(veaOutbox, chainid); + console.log(`Starting batch relay from nonce ${nonce} to ${count}`); while (nonce < count) { let batchMessages = 0; let txns = []; while (batchMessages < maxBatchSize && nonce < count) { const isMsgRelayed = await veaOutbox.isMsgRelayed(nonce); if (isMsgRelayed) { + console.log(`Message ${nonce} already relayed, skipping`); nonce++; continue; } const [proof, [to, data]] = await Promise.all([ getProofAtCount(chainid, nonce, count), getMessageDataToRelay(chainid, nonce), ]); txns.push({ args: [proof, nonce, to, data], method: veaOutboxInstance.methods.sendMessage, to: veaOutboxInstance.options.address, }); batchMessages += 1; nonce++; } if (batchMessages > 0) { + console.log(`Sending batch of ${batchMessages} messages`); + try { await batchedSend(txns); + console.log(`Successfully relayed batch of ${batchMessages} messages`); + } catch (error) { + console.error(`Failed to relay batch: ${error}`); + throw error; + } + // Clear memory + txns = []; } } return nonce; };
Line range hint
77-115: Fix inconsistent return values and add error handling.The function needs:
- Consistent return values (return 0 instead of null)
- Error handling for batch transactions
- Environment variable validation
const relayAllFrom = async (chainid: number, nonce: number, msgSender: string): Promise<number> => { + if (!process.env.PRIVATE_KEY) { + throw new Error('PRIVATE_KEY environment variable is not set'); + } + const routeParams = getBridgeConfig(chainid); const web3 = new Web3(routeParams.rpcOutbox); const batchedSend = initializeBatchedSend( web3, routeParams.batcher, process.env.PRIVATE_KEY, 0 ); const contract = new web3.eth.Contract(routeParams.veaOutboxContract.abi, routeParams.veaOutboxAddress); const veaOutbox = getVeaOutbox(routeParams.veaOutboxAddress, process.env.PRIVATE_KEY, routeParams.rpcOutbox, chainid); const count = await getCount(veaOutbox, chainid); - if (!count) return null; + if (!count) return 0; let txns = []; const nonces = await getNonceFrom(chainid, nonce, msgSender); + if (!nonces || nonces.length === 0) return 0; for (const x of nonces) { const [proof, [to, data]] = await Promise.all([ getProofAtCount(chainid, x, count), getMessageDataToRelay(chainid, x), ]); txns.push({ args: [proof, x, to, data], method: contract.methods.sendMessage, to: contract.options.address, }); } + try { await batchedSend(txns); + } catch (error) { + console.error(`Failed to relay messages: ${error}`); + throw error; + } return nonces[nonces.length - 1]; };
Line range hint
117-136: Fix error handling and type safety issues.Critical issues to address:
- Don't swallow errors
- Add return type specification
- Ensure consistent return value
-const getNonceFrom = async (chainid: number, nonce: number, msgSender: string) => { +const getNonceFrom = async (chainid: number, nonce: number, msgSender: string): Promise<number[]> => { try { const subgraph = getInboxSubgraph(chainid); const result = await request( `https://api.studio.thegraph.com/query/${subgraph}`, `{ messageSents(first: 1000, where: {nonce_gte: ${nonce}, msgSender_: {id: "${msgSender}"}}, orderBy: nonce, orderDirection: asc) { nonce } }` ); return result[`messageSents`].map((a) => a.nonce); } catch (e) { - console.log(e); + console.error(`Failed to fetch nonces from subgraph: ${e}`); + throw e; } };validator-cli/src/ArbToEth/watcherArbToEth.ts (2)
16-18: Consider adding detailed documentation for timing constants.Add JSDoc comments explaining the significance of these Ethereum consensus constants and their source.
+/** + * Constants from Prysm's mainnet config for Ethereum consensus timing + * @see https://github.com/prysmaticlabs/prysm/blob/master/config/params/mainnet_config.go#L103 + */ const slotsPerEpochEth = 32; const secondsPerSlotEth = 12;
91-91: Consider using a Set instead of Map for tracking epochs.The
challengeTxnHashesmap is used primarily for existence checks. A Set would be more appropriate for this use case. Additionally, when adding new epochs, consider using array spread operator for better readability.-const challengeTxnHashes = new Map<number, string>(); +const challengedEpochs = new Set<number>(); -const veaEpochsOutboxClaimableNew: number[] = new Array( - veaEpochOutboxClaimableNow - veaEpochOutboxClaimableNowOld -) - .fill(veaEpochOutboxClaimableNowOld + 1) - .map((el, i) => el + i); -veaEpochOutboxCheckClaimsRangeArray.push(...veaEpochsOutboxClaimableNew); +const newEpochs = [...Array(veaEpochOutboxClaimableNow - veaEpochOutboxClaimableNowOld)] + .map((_, i) => veaEpochOutboxClaimableNowOld + 1 + i); +veaEpochOutboxCheckClaimsRangeArray.push(...newEpochs);Also applies to: 123-130
relayer-cli/src/devnetRelayExample.ts (1)
11-11: Clarify the comment describing thesenderaddressThe current comment may not clearly convey the purpose of the
senderaddress. Consider rephrasing it for better clarity.Suggested change:
- // This is lightbulb switch address in arbitrum sepolia + // Sender address used for relaying transactions in Arbitrum Sepolia network
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
relayer-cli/.env.dist(1 hunks)relayer-cli/package.json(1 hunks)relayer-cli/src/consts/bridgeRoutes.ts(1 hunks)relayer-cli/src/devnetRelayExample.ts(1 hunks)relayer-cli/src/state/11155111.json(0 hunks)relayer-cli/src/testnetRelayer.ts(1 hunks)relayer-cli/src/utils/ethers.ts(3 hunks)relayer-cli/src/utils/relay.ts(3 hunks)relayer-cli/src/utils/relayerHelpers.ts(1 hunks)validator-cli/src/ArbToEth/watcherArbToEth.ts(13 hunks)validator-cli/src/utils/arbMsgExecutor.ts(1 hunks)
💤 Files with no reviewable changes (1)
- relayer-cli/src/state/11155111.json
🚧 Files skipped from review as they are similar to previous changes (2)
- relayer-cli/package.json
- validator-cli/src/utils/arbMsgExecutor.ts
🧰 Additional context used
📓 Learnings (1)
validator-cli/src/ArbToEth/watcherArbToEth.ts (1)
Learnt from: fcanela
PR: kleros/vea#344
File: validator-cli/src/ArbToEth/watcherArbToEth.ts:411-414
Timestamp: 2024-11-19T10:59:44.618Z
Learning: In the TypeScript codebase, particularly in `validator-cli/src/ArbToEth/watcherArbToEth.ts`, when there is no error fallback operation, errors should not be caught; they should be allowed to bubble up to enable proper error propagation and handling at higher levels.
🔇 Additional comments (12)
relayer-cli/.env.dist (2)
4-4: Verify the duplicate address usage
The address 0x906dE43dBef27639b1688Ac46532a16dc07Ce410 is used both as the DEVNET_SENDER and as contract addresses for VEAINBOX and VEAOUTBOX. This could indicate a potential misconfiguration or security concern.
Also applies to: 11-14
9-9: Verify Sepolia chain ID configuration
The VEAOUTBOX_CHAIN_ID=1115111 corresponds to the Sepolia testnet. This aligns with the PR objectives for Arbitrum-Sepolia integration.
✅ Verification successful
Let me gather more information about chain ID configurations and validations in the codebase.
Let me search for more specific information about Sepolia chain ID configurations.
Based on the gathered information, I can now provide the final response:
Chain ID configuration verified for Arbitrum-Sepolia integration
The VEAOUTBOX_CHAIN_ID=1115111 in .env.dist is correctly configured for Sepolia testnet. This is verified by:
- The chain ID is defined in
bridgeRoutes.tsmapping as11155111for Sepolia - The configuration includes proper Sepolia-specific settings (RPC, subgraph, addresses)
- The chain ID is used consistently in the relayer code for testnet operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify chain ID usage in the codebase
# Search for chain ID references
echo "Searching for Sepolia chain ID usage..."
rg "1115111"
# Look for chain ID validation
ast-grep --pattern 'chainId === $_ || chainId == $_'
Length of output: 156
Script:
#!/bin/bash
# Search for chain ID configurations and constants
echo "Searching for chain ID configurations..."
rg -i "chain.?id"
# Search for Sepolia-related configurations
echo -e "\nSearching for Sepolia references..."
rg -i "sepolia"
# Look for environment variable definitions related to chain IDs
echo -e "\nSearching for chain ID environment variables..."
fd ".env" --exec cat {}
Length of output: 135369
Script:
#!/bin/bash
# Search for Sepolia chain ID in bridge configurations
echo "Searching for Sepolia chain ID in bridge configurations..."
rg -A 5 -B 5 "sepolia|11155111" relayer-cli/src/consts/
# Search for environment variable usage
echo -e "\nSearching for environment variable usage..."
rg "VEAOUTBOX_CHAIN_ID" relayer-cli/src/
Length of output: 2961
relayer-cli/src/testnetRelayer.ts (2)
1-5: LGTM! Well-organized imports and dependencies.
The imports are logically grouped and provide all necessary functionality for the relayer implementation.
1-28: Verify relayer's interaction with blockchain contracts.
Let's ensure the relayer correctly interacts with the expected contracts and handles chain-specific configurations properly.
✅ Verification successful
Based on the gathered information, I can now provide a final response about the relayer's interaction with blockchain contracts:
Relayer's contract interactions are properly implemented and secure
The implementation shows a well-structured interaction with the VeaOutbox contracts:
- Proper contract initialization using typed contracts (VeaOutboxArbToEth/VeaOutboxArbToGnosis)
- Secure message relay mechanism with merkle proof verification
- Environment-based configuration for contract addresses and chain IDs
- Batch processing capability with proper state management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract interactions and configurations
# Check for contract addresses in environment files
echo "Checking for contract configurations..."
rg -l "VEAOUTBOX|VEAINBOX" .env*
# Check for contract interface implementations
echo "Checking for contract interfaces..."
rg -A 5 "interface.*Outbox|interface.*Inbox" contracts/
# Check for chain-specific configurations
echo "Checking for chain configurations..."
rg "chainId.*testnet" .
Length of output: 132007
Script:
#!/bin/bash
# Let's verify the relayer's contract interactions more specifically
# Check for relay function implementations
echo "Checking relay implementations..."
ast-grep --pattern 'function relayBatch($$$) {
$$$
}'
# Check for contract imports and configurations
echo "Checking contract imports..."
rg -A 3 "import.*contract" relayer-cli/
# Check for environment variable usage
echo "Checking environment configurations..."
rg "VEAOUTBOX_CHAIN_ID|INBOX|OUTBOX" relayer-cli/
Length of output: 2106
Script:
#!/bin/bash
# Let's check the relay implementation details
# Check relay function implementation
echo "Checking relay function..."
rg -A 10 "export async function relayBatch" relayer-cli/
# Check for ethers provider and contract initialization
echo "Checking contract initialization..."
rg -A 5 "getVeaOutbox" relayer-cli/
# Check for proof generation and message relay logic
echo "Checking proof and message relay..."
rg -A 5 "getProofAtCount|getMessageDataToRelay" relayer-cli/
Length of output: 7212
relayer-cli/src/consts/bridgeRoutes.ts (1)
21-27: 🛠️ Refactor suggestion
Validate environment variables and consider making epoch periods configurable.
The bridge configurations have hardcoded epoch periods and rely heavily on environment variables that should be validated at startup.
Let's verify the environment variables are properly documented:
Consider adding environment variable validation:
const validateEnvVars = () => {
const required = [
'VEAINBOX_ARBSEPOLIA_TO_SEPOLIA_ADDRESS',
'VEAOUTBOX_ARBSEPOLIA_TO_SEPOLIA_ADDRESS',
// ... add all required vars
];
const missing = required.filter(var => !process.env[var]);
if (missing.length > 0) {
throw new Error(`Missing required environment variables: ${missing.join(', ')}`);
}
};Also applies to: 31-37
relayer-cli/src/utils/relayerHelpers.ts (2)
3-17: LGTM! Well-structured shutdown manager class.
The class follows good TypeScript practices with proper encapsulation and type safety.
89-91: LGTM! Well-implemented delay function.
The function correctly implements a promise-based delay.
relayer-cli/src/utils/ethers.ts (3)
7-10: LGTM: Import changes align with multi-chain support
The addition of Gnosis-related factories and bridge configuration imports properly supports the new multi-chain functionality.
88-92: LGTM: Exports are properly updated
The exports correctly include all the new multi-chain support functions while maintaining the existing devnet exports.
21-32: Verify bridge configurations for all supported chains
The chain selection logic looks good, but let's verify the bridge configurations for all supported chains.
✅ Verification successful
Let me gather more specific information about the bridge configurations.
Based on the gathered information, I can now provide the final response:
Bridge configurations are correctly implemented and aligned with the switch cases
The bridge configurations in bridgeRoutes.ts perfectly match the chain selection logic in the getVeaInbox function. The implementation supports:
- Sepolia (chainId: 11155111) using VeaInboxArbToEth
- Chiado (chainId: 10200) using VeaInboxArbToGnosis
The configurations are consistently used across the codebase, with proper contract factory connections and environment-specific settings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify bridge configurations for all supported chains
# Expected: Find all chain configurations to ensure they match the switch cases
# Search for bridge configuration definitions
echo "Searching for bridge configurations..."
rg -l "chain.*(?:sepolia|mainnet|chiado|gnosis)"
# Search for chainId usage
echo "Searching for chainId references..."
ast-grep --pattern 'chainId = $_'
Length of output: 1274
Script:
#!/bin/bash
# Let's check the bridge configuration definitions in the relevant files
echo "Checking bridge configurations in relayer-cli..."
cat relayer-cli/src/consts/bridgeRoutes.ts
echo -e "\nChecking bridge configurations in veascan-web..."
cat veascan-web/src/consts/bridges.ts
echo -e "\nChecking getBridgeConfig implementation..."
rg -A 10 "getBridgeConfig" relayer-cli/src/
Length of output: 13056
relayer-cli/src/utils/relay.ts (1)
1-8: LGTM! Import statements are well-organized.
The imports correctly reflect the architectural changes, particularly the addition of VeaOutboxArbToGnosis support and the new getVeaOutbox utility.
validator-cli/src/ArbToEth/watcherArbToEth.ts (1)
709-730: Fix binary search edge cases in findLatestL2BatchAndBlock.
The function has the following issues:
- Returns
[undefined, undefined]which doesn't match the return typePromise<[number, number]> - No validation for
fromArbBlockbeing less thanlatestBlockNumber
This was previously flagged in an earlier review. Please apply the suggested fix:
const findLatestL2BatchAndBlock = async (
nodeInterface: NodeInterface,
fromArbBlock: number,
latestBlockNumber: number
): Promise<[number, number]> => {
+ if (fromArbBlock > latestBlockNumber) {
+ throw new Error('fromArbBlock must be less than latestBlockNumber');
+ }
let low = fromArbBlock;
let high = latestBlockNumber;
while (low <= high) {
const mid = Math.floor((low + high) / 2);
try {
(await nodeInterface.functions.findBatchContainingBlock(mid, { blockTag: "latest" })) as any;
low = mid + 1;
} catch (e) {
high = mid - 1;
}
}
- if (high < low) return [undefined, undefined];
+ if (high < low) return [-1, -1]; // Return sentinel values instead of undefined
const result = (await nodeInterface.functions.findBatchContainingBlock(high, { blockTag: "latest" })) as any;
return [result.batch.toNumber(), high];
};
There was a problem hiding this comment.
I have limited knowledge about Vea so I can not validate that all the logic works well or that there are no unhandled error conditions. Code looks great and the effort spent in improving the codebase can be appreciated clearly. Great job, @mani99brar !
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
relayer-cli/src/devnetRelayExample.ts (3)
6-6: Consider documenting the epoch period calculationWhile the comment indicates it's "30 min", it would be helpful to document why this specific period was chosen and any implications it might have.
12-12: Improve the comment clarity for the sender addressThe current comment "This is libghtbulb switch address in arbitrum sepolia" contains a typo and could be more descriptive about the purpose of this address.
Consider updating to:
- // This is libghtbulb switch address in arbitrum sepolia + // Address of the lightbulb switch contract deployed on Arbitrum Sepolia that initiates the relay
23-26: Document the ShutdownManager parameterThe
falseparameter passed toShutdownManagerconstructor should be documented to explain its purpose.Consider adding a comment:
+ // Initialize ShutdownManager with isShuttingDown set to false const shutdownManager = new ShutdownManager(false);relayer-cli/src/utils/relayerHelpers.ts (1)
3-17: Consider adding readonly modifier to isShuttingDownThe implementation looks good and follows proper encapsulation. Consider adding the
readonlymodifier to prevent accidental reassignment of the private field outside the constructor.- private isShuttingDown: boolean; + private readonly isShuttingDown: boolean;relayer-cli/src/utils/relay.ts (4)
Line range hint
10-26: Add error handling for GraphQL requestThe GraphQL request could fail for various reasons (network issues, invalid responses, etc.), but there's no error handling in place.
const getCount = async (veaOutbox: VeaOutboxArbToEth | VeaOutboxArbToGnosis, chainId: number): Promise<number> => { const subgraph = getInboxSubgraph(chainId); const stateRoot = await veaOutbox.stateRoot(); + try { const result = await request( `https://api.studio.thegraph.com/query/${subgraph}`, `{ snapshotSaveds(first: 1, where: { stateRoot: "${stateRoot}" }) { count } }` ); if (result["snapshotSaveds"].length == 0) return 0; return Number(result["snapshotSaveds"][0].count); + } catch (error) { + throw new Error(`Failed to fetch count from subgraph: ${error.message}`); + } };
Line range hint
14-19: Extract GraphQL query into a constantThe GraphQL query string should be extracted into a named constant for better maintainability and reusability.
+const GET_SNAPSHOT_COUNT_QUERY = ` + query GetSnapshotCount($stateRoot: String!) { + snapshotSaveds(first: 1, where: { stateRoot: $stateRoot }) { + count + } + } +`; const getCount = async (veaOutbox: VeaOutboxArbToEth | VeaOutboxArbToGnosis, chainId: number): Promise<number> => { const subgraph = getInboxSubgraph(chainId); const stateRoot = await veaOutbox.stateRoot(); const result = await request( `https://api.studio.thegraph.com/query/${subgraph}`, - `{ - snapshotSaveds(first: 1, where: { stateRoot: "${stateRoot}" }) { - count - } - }` + GET_SNAPSHOT_COUNT_QUERY, + { stateRoot } );
82-89: Move implementation details to documentationThe inline comments explaining batchedSend parameters should be moved to JSDoc or external documentation.
- const batchedSend = initializeBatchedSend( - web3, // Your web3 object. - // The address of the transaction batcher contract you wish to use. The addresses for the different networks are listed below. If the one you need is missing, feel free to deploy it yourself and make a PR to save the address here for others to use. - routeParams.batcher, - process.env.PRIVATE_KEY, // The private key of the account you want to send transactions from. - 0 // The debounce timeout period in milliseconds in which transactions are batched. - ); + const batchedSend = initializeBatchedSend( + web3, + routeParams.batcher, + process.env.PRIVATE_KEY, + 0 + );
Line range hint
117-132: Improve error handling in getNonceFromThe function currently swallows errors and doesn't return anything in the error case. This could lead to undefined behavior.
+const MAX_MESSAGES_PER_QUERY = 1000; + const getNonceFrom = async (chainId: number, nonce: number, msgSender: string) => { try { const subgraph = getInboxSubgraph(chainId); const result = await request( `https://api.studio.thegraph.com/query/${subgraph}`, `{ - messageSents(first: 1000, where: {nonce_gte: ${nonce}, msgSender_: {id: "${msgSender}"}}, orderBy: nonce, orderDirection: asc) { + messageSents(first: ${MAX_MESSAGES_PER_QUERY}, where: {nonce_gte: ${nonce}, msgSender_: {id: "${msgSender}"}}, orderBy: nonce, orderDirection: asc) { nonce } }` ); return result[`messageSents`].map((a) => a.nonce); } catch (e) { - console.log(e); + throw new Error(`Failed to fetch nonces from subgraph: ${e.message}`); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
relayer-cli/src/devnetRelayExample.ts(1 hunks)relayer-cli/src/testnetRelayer.ts(1 hunks)relayer-cli/src/utils/relay.ts(3 hunks)relayer-cli/src/utils/relayerHelpers.ts(1 hunks)validator-cli/src/utils/arbMsgExecutor.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- relayer-cli/src/testnetRelayer.ts
- validator-cli/src/utils/arbMsgExecutor.ts
🧰 Additional context used
📓 Learnings (1)
relayer-cli/src/devnetRelayExample.ts (1)
Learnt from: fcanela
PR: kleros/vea#344
File: relayer-cli/src/devnetRelayExample.ts:9-14
Timestamp: 2024-11-26T11:30:35.723Z
Learning: In `relayer-cli/src/devnetRelayExample.ts`, error handling is managed by the upper layer via `ShutdownManager`'s error signal handler, so adding a `try...catch` block in the main loop is unnecessary.
🔇 Additional comments (5)
relayer-cli/src/devnetRelayExample.ts (2)
1-2: LGTM! Well-organized imports
The imports are properly organized and consistently use ES module syntax.
15-19: Verify the epoch delay calculation
The delay calculation includes an additional 100 seconds (100 * 1000 ms) on top of the remaining time until the next epoch. Consider documenting why this buffer is necessary.
relayer-cli/src/utils/relayerHelpers.ts (3)
99-101: LGTM!
The delay function implementation is correct and follows standard practices.
19-47:
Add validation for STATE_DIR environment variable
The function relies on STATE_DIR without validating its existence or format. This was previously raised but not addressed.
async function initialize(chainId: number, network: string): Promise<number> {
+ if (!process.env.STATE_DIR) {
+ throw new Error("STATE_DIR environment variable is not set");
+ }
+ const stateDir = process.env.STATE_DIR.endsWith("/")
+ ? process.env.STATE_DIR
+ : process.env.STATE_DIR + "/";
const lockFileName = "./state/" + network + "_" + chainId + ".pid";49-61:
Add error handling and use consistent path construction
The function has similar path inconsistency issues and lacks error handling for file operations.
async function updateStateFile(chainId: number, createdTimestamp: number, nonceFrom: number, network: string) {
+ if (!process.env.STATE_DIR) {
+ throw new Error("STATE_DIR environment variable is not set");
+ }
+ const stateDir = process.env.STATE_DIR.endsWith("/")
+ ? process.env.STATE_DIR
+ : process.env.STATE_DIR + "/";
- const chain_state_file = "./state/" + network + "_" + chainId + ".json";
+ const chain_state_file = stateDir + network + "_" + chainId + ".json";
const json = {
ts: createdTimestamp,
nonce: nonceFrom,
};
- fs.writeFileSync(chain_state_file, JSON.stringify(json), { encoding: "utf8" });
+ try {
+ fs.writeFileSync(chain_state_file, JSON.stringify(json), { encoding: "utf8" });
+ } catch (error) {
+ console.error(`Failed to write state file: ${error.message}`);
+ throw error;
+ }
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
relayer-cli/src/utils/ethers.ts (1)
20-32: Consider defining chain names as constantsThe function implementation looks good with proper error handling and clear chain-specific logic. However, the chain names ("sepolia", "mainnet", etc.) should be defined as constants to prevent typos and improve maintainability.
+const SUPPORTED_CHAINS = { + SEPOLIA: "sepolia", + MAINNET: "mainnet", + CHIADO: "chiado", + GNOSIS: "gnosis", +} as const; function getVeaInbox(veaInboxAddress: string, privateKey: string, web3ProviderURL: string, chainId: number) { const bridge = getBridgeConfig(chainId); switch (bridge.chain) { - case "sepolia": - case "mainnet": + case SUPPORTED_CHAINS.SEPOLIA: + case SUPPORTED_CHAINS.MAINNET: return VeaInboxArbToEth__factory.connect(veaInboxAddress, getWallet(privateKey, web3ProviderURL)); - case "chiado": - case "gnosis": + case SUPPORTED_CHAINS.CHIADO: + case SUPPORTED_CHAINS.GNOSIS: return VeaInboxArbToGnosis__factory.connect(veaInboxAddress, getWallet(privateKey, web3ProviderURL)); default: throw new Error(`Unsupported chainId: ${chainId}`); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
relayer-cli/src/utils/ethers.ts(3 hunks)
🔇 Additional comments (5)
relayer-cli/src/utils/ethers.ts (5)
7-8: LGTM: Import statements for Gnosis factories
The addition of Gnosis-specific factory imports aligns with the new multi-chain support functionality.
35-46: Fix variable naming inconsistency
The variable name bridges should be singular (bridge) for consistency with other functions.
function getVeaInboxProvider(veaInboxAddress: string, privateKey: string, rpc: JsonRpcProvider, chainId: number) {
- const bridges = getBridgeConfig(chainId);
- switch (bridges.chain) {
+ const bridge = getBridgeConfig(chainId);
+ switch (bridge.chain) {63-74: Fix variable naming inconsistency
The variable name bridges should be singular (bridge) for consistency with other functions.
function getVeaOutboxProvider(veaOutboxAddress: string, privateKey: string, rpc: JsonRpcProvider, chainId: number) {
- const bridges = getBridgeConfig(chainId);
- switch (bridges.chain) {
+ const bridge = getBridgeConfig(chainId);
+ switch (bridge.chain) {88-92: LGTM: Export statements are properly updated
The exports correctly include all the new functions while maintaining the existing ones.
49-60:
Critical: Fix incorrect parameter usage in connect calls
The function is using veaInboxAddress instead of the provided veaOutboxAddress parameter in the connect calls.
function getVeaOutbox(veaOutboxAddress: string, privateKey: string, web3ProviderURL: string, chainId: number) {
const bridge = getBridgeConfig(chainId);
switch (bridge.chain) {
case "sepolia":
case "mainnet":
- return VeaOutboxArbToEth__factory.connect(veaInboxAddress, getWallet(privateKey, web3ProviderURL));
+ return VeaOutboxArbToEth__factory.connect(veaOutboxAddress, getWallet(privateKey, web3ProviderURL));
case "chiado":
case "gnosis":
- return VeaOutboxArbToGnosis__factory.connect(veaInboxAddress, getWallet(privateKey, web3ProviderURL));
+ return VeaOutboxArbToGnosis__factory.connect(veaOutboxAddress, getWallet(privateKey, web3ProviderURL));
default:
throw new Error(`Unsupported chainId: ${chainId}`);
}
}
PR-Codex overview
This PR introduces significant updates to the
relayer-cliandvalidator-cliprojects, enhancing functionality for both devnet and testnet operations. It adds new scripts, improves state management, and updates dependencies, particularly focusing on bridging and message relaying capabilities.Detailed summary
start-testnet-relayerscript torelayer-cli/package.json.DEVNET_SENDERandVEAOUTBOX_CHAIN_IDto.env.dist.relayer-cli/src/testnetRelayer.tsfor testnet operations.@arbitrum/sdkdependency to version4.0.1.bridgeRoutes.tsto include epoch management.getVeaInboxandgetVeaOutboxfunctions for better chain handling.validator-cli/src/utils/arbMsgExecutor.ts.relayer-cli/src/utils/relay.ts.contracts/test/integration/ArbToEth.tsfor better coverage.watcherArbToEth.tsto improve claim monitoring and dispute resolution.Summary by CodeRabbit
New Features
VeaInboxArbToEthon the Arbitrum Sepolia testnet.ShutdownManagerclass to manage shutdown states in the relayer.Bug Fixes
Documentation
Chores