Release - Devnet from Arbitrum Sepolia to Sepolia #169
Conversation
fix: bug fixes
Refactor(web/style): improve styling
chore: refactor imports
Feat: add veascan UI
❌ Deploy Preview for veascan failed. Why did it fail? →
|
jaybuidl
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (3)
13-23: Add documentation for the epochPeriod configuration.The
paramsByChainIdconfiguration would benefit from documentation explaining:
- Why the epoch periods differ between environments
- The implications of these values on the system
- Any constraints or requirements for these values
const paramsByChainId = { + // Configuration for epoch periods across different networks + // Note: Shorter periods in test environments for faster testing ARBITRUM: { epochPeriod: 3600, // 1 hours }, ARBITRUM_SEPOLIA: { epochPeriod: 3600, // 1 hours }, HARDHAT: { epochPeriod: 600, // 10 minutes }, };
93-98: Improve contract name construction logic.The current string concatenation for contract names could be made more maintainable.
Consider using an object mapping or a helper function:
- await deploy("VeaInboxArbToGnosis" + (chainId === 42161 ? "" : "Testnet"), { + const getContractName = (chainId: number) => { + const contractNames = { + 42161: "VeaInboxArbToGnosis", + default: "VeaInboxArbToGnosisTestnet" + }; + return contractNames[chainId] || contractNames.default; + }; + await deploy(getContractName(chainId), {
110-114: Remove debug console.log.The console.log statement in the skip condition appears to be debugging code that should be removed.
deployInbox.skip = async ({ getChainId }) => { const chainId = Number(await getChainId()); - console.log(chainId); return !SenderChains[chainId]; };contracts/deployments/arbitrumSepolia/VeaInboxArbToGnosisTestnet.json (1)
391-448: Review storage constraints and limitationsThe contract has the following storage constraints:
- Fixed inbox array size of 64 entries limits the merkle tree depth
- Message count is limited to uint64 (2^64 - 1)
These limits should be sufficient as:
- 64 entries allow for 2^64 messages in the merkle tree
- At the current network speed limit, it would take millions of years to overflow the message count
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts(1 hunks)contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts(1 hunks)contracts/deployments/arbitrumSepolia/VeaInboxArbToGnosisTestnet.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts
🔇 Additional comments (5)
contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (2)
49-51:
Address prediction mechanism needs improvement.
The current nonce-based address prediction is fragile and could break if the deployment sequence changes.
Consider implementing deterministic deployments using:
- OpenZeppelin's
Create2factory - Hardhat's built-in deterministic deployment feature
Would you like me to help create an issue to track this technical debt?
43-47: Verify network configuration completeness.
Let's ensure all necessary networks are properly configured in the router networks mapping.
✅ Verification successful
Let me try another approach to verify the network configurations.
Let me check the specific SenderChains enum in the file under review.
Network configuration is complete and correctly mapped
The network configuration in the router networks mapping is correctly aligned with the SenderChains enum and the available networks in hardhat.config.ts:
ARBITRUM(chainId: 42161) → maps toconfig.networks.mainnetARBITRUM_SEPOLIA(chainId: 421614) → maps toconfig.networks.sepoliaHARDHAT(chainId: 31337) → maps toconfig.networks.localhost
All networks referenced in the SenderChains enum have corresponding configurations in both the router networks mapping and the hardhat config.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all networks referenced in SenderChains enum have corresponding configurations
# Extract network configurations from the codebase
echo "Checking network configurations..."
rg -A 5 "networks:\s*{" .
# Look for any network-related configuration files
echo "Checking for network configuration files..."
fd -e json -e js -e ts "network" .
Length of output: 367
Script:
#!/bin/bash
# Check network configurations in the codebase
echo "Checking network configurations in the code..."
rg "networks:" -A 5
# Check the contents of networks.json
echo -e "\nChecking networks.json content..."
cat ./relayer-subgraph-inbox/networks.json
# Look for any enum or type definitions related to networks
echo -e "\nChecking for network enums or types..."
rg -p "enum.*Chain|type.*Chain" .
Length of output: 74320
Script:
#!/bin/bash
# Check the SenderChains enum in the file under review
echo "Checking SenderChains enum in arb-to-gnosis-inbox.ts..."
cat contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts
# Check the network configuration in hardhat.config.ts
echo -e "\nChecking network configuration in hardhat.config.ts..."
cat contracts/hardhat.config.ts
Length of output: 9580
contracts/deployments/arbitrumSepolia/VeaInboxArbToGnosisTestnet.json (3)
304-325: Well-designed interface with comprehensive events and documentation
The contract interface is well-designed with:
- Clear event definitions for off-chain monitoring
- Comprehensive function documentation
- Gas complexity annotations
- Proper error messages
Also applies to: 356-377
299-299: Review assembly blocks in the contract implementation
The contract uses assembly blocks for efficient hashing operations. While this is a common optimization pattern, ensure these blocks have been thoroughly audited as they bypass Solidity's safety checks.
✅ Verification successful
Assembly usage is safe and optimized for hashing operations
The assembly blocks in VeaInboxArbToGnosis.sol are well-structured and follow secure patterns:
- Used only for optimized keccak256 hashing operations
- Limited to memory operations (mstore) in scratch space
- Consistent pattern for double-hashing leaves and sorting/concatenating node pairs
- No complex manipulations or unsafe operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for assembly blocks in the contract implementation
# Look for assembly blocks in the source code
ast-grep --pattern 'assembly {
$$$
}'
Length of output: 41
Script:
#!/bin/bash
# Search for assembly blocks using ripgrep which is more reliable for text patterns
rg "assembly\s*{" -A 10 --no-heading
Length of output: 126
Script:
#!/bin/bash
# Search for assembly blocks using ripgrep with fixed pattern
rg "assembly {" -A 10
Length of output: 107
Script:
#!/bin/bash
# Search for assembly blocks using ripgrep with escaped curly brace
rg "assembly \{" -A 10
Length of output: 72258
Script:
#!/bin/bash
# Search for assembly blocks in the source code using ripgrep with proper escaping
rg -U "assembly \{[\s\S]*?\}" contracts/src/arbitrumToGnosis/VeaInboxArbToGnosis.sol
Length of output: 574
2-2: Verify the router contract address
The contract is deployed with router address 0x22d70804d4Ef5BB206C6B39e3267DFe8a0f97d27. Please ensure this is the correct VeaRouter contract on Ethereum.
Also applies to: 294-295
✅ Verification successful
Let me try a different approach to verify the router contract by searching for its interface and implementation in the codebase.
Let me search for the VeaInbox deployment files to verify the router address in the context of the deployment configuration.
Let me search for the router address in the VeaInbox deployment file to verify its configuration.
Let me check the constructor arguments in the deployment file to verify the router address.
Router address is correctly configured
The router address 0x22d70804d4Ef5BB206C6B39e3267DFe8a0f97d27 is correctly configured in the VeaInbox deployment file as a constructor argument, matching the address in the review comment. This is the expected configuration for the cross-chain messaging system from Arbitrum to Gnosis.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if the router address exists and contains the expected interface
# Check if the address exists and has code
curl -s "https://api-sepolia.etherscan.io/api?module=proxy&action=eth_getCode&address=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: 366
Script:
#!/bin/bash
# Search for router interface definition
rg -A 5 "interface IRouterToGnosis"
# Search for router address in other deployment files
rg "0x22d70804d4Ef5BB206C6B39e3267DFe8a0f97d27"
# Search for VeaRouter deployment files
fd "VeaRouter.*json" contracts/deployments
Length of output: 79669
Script:
#!/bin/bash
# Search for VeaInbox deployment files
fd "VeaInbox.*json" contracts/deployments
# Search for router address in deployment files
rg -A 5 "routerArbToGnosis" contracts/deployments
Length of output: 66230
Script:
#!/bin/bash
# Check the router address in the VeaInbox deployment file
cat contracts/deployments/arbitrumSepolia/VeaInboxArbToGnosisTestnet.json | grep -A 5 "constructor"
Length of output: 36515
Script:
#!/bin/bash
# Check the constructor arguments in the deployment file
cat contracts/deployments/arbitrumSepolia/VeaInboxArbToGnosisTestnet.json | grep -A 5 "args"
Length of output: 246
| // ---------------------------------------------------------------------------------------------- | ||
| 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); | ||
|
|
||
| await deploy("VeaInboxArbToGnosis" + (chainId === 42161 ? "" : "Testnet"), { | ||
| contract: "VeaInboxArbToGnosis", | ||
| from: deployer, | ||
| args: [epochPeriod, routerAddress], | ||
| log: true, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for provider connection.
The liveDeployer function should handle potential provider connection failures.
const liveDeployer = async () => {
+ if (!routerNetworks[SenderChains[chainId]]?.url) {
+ throw new Error(`No URL configured for chain ${chainId}`);
+ }
+
const routerChainProvider = new providers.JsonRpcProvider(routerNetworks[SenderChains[chainId]].url);
+ try {
let nonceRouter = await routerChainProvider.getTransactionCount(deployer);
const routerAddress = getContractAddress(deployer, nonceRouter);
console.log("calculated future router for nonce %d: %s", nonceRouter, routerAddress);
await deploy("VeaInboxArbToGnosis" + (chainId === 42161 ? "" : "Testnet"), {
contract: "VeaInboxArbToGnosis",
from: deployer,
args: [epochPeriod, routerAddress],
log: true,
});
+ } catch (error) {
+ console.error(`Failed to deploy on chain ${chainId}:`, error);
+ throw error;
+ }
};📝 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 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); | |
| await deploy("VeaInboxArbToGnosis" + (chainId === 42161 ? "" : "Testnet"), { | |
| contract: "VeaInboxArbToGnosis", | |
| from: deployer, | |
| args: [epochPeriod, routerAddress], | |
| log: true, | |
| }); | |
| }; | |
| // ---------------------------------------------------------------------------------------------- | |
| const liveDeployer = async () => { | |
| if (!routerNetworks[SenderChains[chainId]]?.url) { | |
| throw new Error(`No URL configured for chain ${chainId}`); | |
| } | |
| const routerChainProvider = new providers.JsonRpcProvider(routerNetworks[SenderChains[chainId]].url); | |
| try { | |
| let nonceRouter = await routerChainProvider.getTransactionCount(deployer); | |
| const routerAddress = getContractAddress(deployer, nonceRouter); | |
| console.log("calculated future router for nonce %d: %s", nonceRouter, routerAddress); | |
| await deploy("VeaInboxArbToGnosis" + (chainId === 42161 ? "" : "Testnet"), { | |
| contract: "VeaInboxArbToGnosis", | |
| from: deployer, | |
| args: [epochPeriod, routerAddress], | |
| log: true, | |
| }); | |
| } catch (error) { | |
| console.error(`Failed to deploy on chain ${chainId}:`, error); | |
| throw error; | |
| } | |
| }; |
fix(relayer): changes variable typo
Hardhat ether version update
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (28)
contracts/deploy/02-inbox/02-gnosis-to-arb-inbox.ts (5)
5-9: Consider adding documentation for the enum.Adding JSDoc comments would improve code maintainability by explaining the purpose of each chain ID.
+/** + * Enum representing supported sender chains for the inbox contract + * @enum {number} + */ enum SenderChains { GNOSIS = 100, GNOSIS_CHIADO = 10200, HARDHAT = 31337, }
11-23: Consider extracting configuration to a separate file.The
paramsByChainIdobject contains network-specific configurations that might be better maintained in a separate configuration file. This would:
- Improve maintainability
- Make it easier to update network configurations
- Allow reuse across different deployment scripts
25-25: Consider implementing deterministic deployments using CREATE2.As noted in the TODO and based on previous feedback, implementing CREATE2 for deterministic deployments would be beneficial for cross-chain contract deployments.
Would you like me to help implement CREATE2 deployment strategy?
38-40: Extract contract name logic to a helper function.The contract naming logic with the "Testnet" suffix is duplicated. Consider extracting it to a helper function to improve maintainability.
+const getContractName = (baseContractName: string, chainId: number) => + baseContractName + (chainId === 100 ? "" : "Testnet"); const veaOutboxArb = await hre.companionNetworks.arbitrumSepolia.deployments.get( - "VeaOutboxGnosisToArb" + (chainId === 100 ? "" : "Testnet") + getContractName("VeaOutboxGnosisToArb", chainId) ); const inbox = await deploy( - "VeaInboxGnosisToArb" + (chainId === 100 ? "" : "Testnet"), + getContractName("VeaInboxGnosisToArb", chainId), { from: deployer, contract: "VeaInboxGnosisToArb", args: [epochPeriod, veaOutboxArb.address, amb], log: true, ...(chainId === 100 ? {} : gasOptions), } );Also applies to: 47-53
59-63: Enhance skip function logging.The current logging only shows the chain ID. Consider adding more context to make debugging easier.
deployInbox.skip = async ({ getChainId }) => { const chainId = Number(await getChainId()); - console.log(chainId); + console.log(`Checking if deployment should be skipped for chain ID: ${chainId}`); + const shouldSkip = !SenderChains[chainId]; + console.log(`Deployment ${shouldSkip ? 'will be' : 'will not be'} skipped`); - return !SenderChains[chainId]; + return shouldSkip; };contracts/deploy/03-routers/03-arb-sepolia-to-chiado-router.ts (4)
5-8: Consider adding missing chain IDsThe
RouterChainsenum appears incomplete. Consider adding Arbitrum Sepolia and Gnosis Chiado chain IDs since they are referenced in the deployment logic.enum RouterChains { ETHEREUM_SEPOLIA = 11155111, + ARBITRUM_SEPOLIA = 421614, + GNOSIS_CHIADO = 10200, HARDHAT = 31337, }
10-19: Consider moving network addresses to environment variablesHardcoded bridge addresses in source code make it difficult to update them and pose security risks. Consider:
- Moving these addresses to environment variables or a configuration file
- Adding validation for the addresses
- Including documentation links for all addresses
21-21: Address TODO comment regarding deterministic deploymentsDeterministic deployments are crucial for cross-chain router contracts to ensure consistent addresses across networks.
Would you like me to help implement deterministic deployments using the
hardhat-deployCREATE2 factory?
71-75: Improve skip function implementationThe skip function contains an unnecessary console.log and could be more robust.
deployRouter.skip = async ({ getChainId }) => { const chainId = Number(await getChainId()); - console.log(chainId); - return !RouterChains[chainId]; + const isSupported = Object.values(RouterChains).includes(chainId); + if (!isSupported) { + console.log(`Skipping deployment for unsupported chain ${chainId}`); + } + return !isSupported; };contracts/deploy/03-routers/03-arb-to-gnosis-router.ts (3)
5-24: Add type safety to parameters configuration.While the enum provides type safety, the parameters object could benefit from stronger typing.
Consider adding an interface:
+interface ChainParams { + arbitrumBridge: string; + amb: string; +} + const paramsByChainId = { ETHEREUM_MAINNET: { arbitrumBridge: "0x8315177aB297bA92A06054cE80a67Ed4DBd7ed3a", amb: "0x4C36d2919e407f0Cc2Ee3c993ccF8ac26d9CE64e", }, ETHEREUM_SEPOLIA: { arbitrumBridge: "0x38f918D0E9F1b721EDaA41302E399fa1B79333a9", amb: "0xf2546D6648BD2af6a008A7e7C1542BB240329E11", }, HARDHAT: { arbitrumBridge: ZeroAddress, amb: ZeroAddress, }, -} as const; +} as const satisfies Record<string, ChainParams>;
26-26: Track the TODO comment about deterministic deployments.The TODO comment about using deterministic deployments should be tracked for future implementation.
Would you like me to create a GitHub issue to track this enhancement?
104-110: Enhance skip logic with better validation and logging.The skip logic could be more robust and informative.
Consider enhancing the skip logic:
deployRouter.tags = ["ArbToGnosisRouter"]; deployRouter.skip = async ({ getChainId }) => { const chainId = Number(await getChainId()); - console.log("Chain ID:", chainId); + const chainName = RouterChains[chainId]; + console.log(`Checking deployment for chain ${chainId} (${chainName || 'Unknown'})`); + + if (!chainName) { + console.log(`Skipping deployment: unsupported chain ${chainId}`); + return true; + } + + if (!paramsByChainId[chainName]) { + console.log(`Skipping deployment: missing parameters for chain ${chainName}`); + return true; + } + - return !RouterChains[chainId]; + console.log(`Proceeding with deployment on ${chainName}`); + return false; }; deployRouter.runAtTheEnd = true;contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (2)
5-21: Add documentation for epoch period values.While the configuration is well-structured, it would be beneficial to document:
- Why these specific epoch periods were chosen
- The implications of these values on the system
- Any constraints or limitations these values impose
107-114: Improve skip logic logging.The skip logic should provide more informative logging about why a deployment is being skipped.
deployInbox.skip = async ({ getChainId }) => { const chainId = Number(await getChainId()); - console.log(chainId); + const supported = Boolean(SenderChains[chainId]); + console.log( + `Chain ${chainId} is ${supported ? 'supported' : 'not supported'} for deployment` + ); - return !SenderChains[chainId]; + return !supported; };contracts/deploy/03-routers/03-gnosis-to-arb-router.ts (3)
5-9: Consider adding Arbitrum and Gnosis chain IDs to RouterChainsSince this is a router between Gnosis and Arbitrum, it would be helpful to include their chain IDs in the enum for completeness and future reference.
enum RouterChains { ETHEREUM_MAINNET = 1, ETHEREUM_SEPOLIA = 11155111, HARDHAT = 31337, + ARBITRUM_ONE = 42161, + ARBITRUM_SEPOLIA = 421614, + GNOSIS_CHAIN = 100, + GNOSIS_CHIADO = 10200, }
36-36: Address TODO: Implement deterministic deploymentsDeterministic deployments are crucial for consistent contract addresses across networks. This should be implemented before production deployment.
Would you like me to help implement deterministic deployments using the
@openzeppelin/hardhat-upgradesplugin?
53-55: Enhance error handling with more descriptive messagesThe error message could be more informative by including the list of supported chains:
if (!(RouterChains[chainId] in paramsByChainId)) { - throw new Error(`Unsupported chain ID: ${chainId}`); + const supportedChains = Object.entries(RouterChains) + .filter(([key]) => isNaN(Number(key))) + .map(([key, value]) => `${key}: ${value}`) + .join(', '); + throw new Error( + `Unsupported chain ID: ${chainId}. Supported chains are: ${supportedChains}` + ); }contracts/deploy/01-outbox/01-arb-sepolia-to-chiado-outbox.ts (1)
72-95: Improve nonce handling documentation and reliabilityThe manual nonce increment (
nonce += 3) is fragile and poorly documented. The comment "SenderGatewayToEthereum deploy tx will be the 5th after this" doesn't clearly explain the dependency chain.Consider:
- Adding detailed documentation about the deployment sequence
- Using deployment tags to enforce order
- Moving to deterministic deployments to eliminate nonce dependency
Example improvement:
// Document deployment sequence const DEPLOYMENT_SEQUENCE = { GATEWAY: 0, INBOX: 1, OUTBOX: 2, // ... other deployments }; // Use deployment tags to enforce order hardhatDeployer.dependencies = ['SenderGateway'];contracts/deploy/01-outbox/01-gnosis-to-arb-outbox.ts (4)
74-93: Complete or remove the TODO mock implementation.The commented-out mock implementation should either be completed or removed. If it's needed for testing, it should be implemented; if not, the commented code should be removed.
Would you like me to help implement the mock contract or create an issue to track this task?
98-101: Avoid hardcoding gas options.The gas options are currently hardcoded to 1 gwei. Consider:
- Making these configurable through deployment parameters
- Using dynamic gas estimation
- Adding safety checks for minimum/maximum values
112-128: Improve contract naming and deployment logging.The contract deployment logic has a few areas for improvement:
- The conditional naming logic (
+ (chainId === 42161 ? "" : "Testnet")) is repeated- The deployment logs could be more informative
Consider this refactoring:
- const txn = await deploy("VeaOutboxGnosisToArb" + (chainId === 42161 ? "" : "Testnet"), { + const contractName = `VeaOutboxGnosisToArb${chainId === 42161 ? "" : "Testnet"}`; + const txn = await deploy(contractName, { from: deployer, contract: "VeaOutboxGnosisToArb", args: [ deposit, epochPeriod, challengePeriod, numEpochTimeout, routerAddress, sequencerDelayLimit, sequencerFutureLimit, ], log: true, ...gasOptions, }); - console.log("VeaOutboxGnosisToArb" + (chainId === 42161 ? "" : "Testnet") + " deployed to:", txn.address); + console.log(`${contractName} deployed to ${txn.address} with parameters: + - Deposit: ${deposit} + - Epoch Period: ${epochPeriod} + - Challenge Period: ${challengePeriod} + - Router: ${routerAddress}`);
140-144: Improve skip condition logging.The current logging only shows the chain ID. Consider adding more context to help with debugging deployment issues.
deployOutbox.skip = async ({ getChainId }) => { const chainId = Number(await getChainId()); - console.log(chainId); + console.log(`Checking if deployment should be skipped for chain ${chainId}`); + const shouldSkip = !ReceiverChains[chainId]; + console.log(`Deployment ${shouldSkip ? 'will be skipped' : 'will proceed'} for chain ${chainId}`); return !ReceiverChains[chainId]; };contracts/deploy/01-outbox/01-chiado-to-arb-sepolia-outbox.ts (4)
20-20: Unused parametermaxMissingBlocksinparamsByChainIdThe parameter
maxMissingBlocksis defined but not utilized in the deployment script. Consider removing it if it's unnecessary, or include it in the deployment arguments if it's required by the contract constructor.
55-56: Incomplete extraction of parameters fromparamsByChainIdIn the destructuring assignment, only a subset of parameters is extracted:
const { deposit, epochPeriod, challengePeriod, numEpochTimeout, sequencerDelayLimit, sequencerFutureLimit } = paramsByChainId[ReceiverChains[chainId]];If there are additional parameters in
paramsByChainIdthat are required for deployment, ensure they are included here. Alternatively, remove unused parameters fromparamsByChainIdto improve clarity.
93-96: Review gas price settings for network conditionsThe gas options are set to a fixed
1 gwei:const gasOptions = { maxFeePerGas: String(ethers.parseUnits("1", "gwei")), maxPriorityFeePerGas: String(ethers.parseUnits("1", "gwei")), };Verify that these gas price settings are appropriate for the Arbitrum Sepolia network, as low gas prices may result in stalled transactions. Consider fetching current gas price estimates or making the gas configuration more dynamic.
136-136: Enhanceconsole.logmessage for better clarityThe
console.logstatement currently outputs only thechainId:console.log(chainId);For improved readability and debugging, consider updating it to:
console.log("Current chain ID:", chainId);This change provides context when reviewing logs.
contracts/deploy/03-routers/03-chiado-to-arb-sepolia-router.ts (1)
80-81: Enhance error handling with detailed loggingCurrently, the error handling logs the error message:
console.error("Deployment failed:", error); throw error;For better debugging, consider adding more context to the error message, such as which deployment (local or live) failed and any relevant variables or state information.
contracts/deploy/01-outbox/01-arb-to-eth-outbox.ts (1)
61-61: Address the TODO: Implement deterministic deploymentsThere's a TODO comment indicating the need to use deterministic deployments. Implementing deterministic deployments will ensure that contract addresses are predictable and not dependent on nonces, enhancing the reliability of your deployment process.
Would you like assistance in implementing deterministic deployments using tools like Hardhat's deterministic deployment plugin or the
CREATE2opcode?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
contracts/deploy/01-outbox/01-arb-sepolia-to-chiado-outbox.ts(1 hunks)contracts/deploy/01-outbox/01-arb-sepolia-to-sepolia-outbox.ts(1 hunks)contracts/deploy/01-outbox/01-arb-to-eth-outbox.ts(1 hunks)contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts(1 hunks)contracts/deploy/01-outbox/01-chiado-to-arb-sepolia-outbox.ts(1 hunks)contracts/deploy/01-outbox/01-gnosis-to-arb-outbox.ts(1 hunks)contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts(1 hunks)contracts/deploy/02-inbox/02-chiado-to-arb-sepolia-inbox.ts(1 hunks)contracts/deploy/02-inbox/02-gnosis-to-arb-inbox.ts(1 hunks)contracts/deploy/03-routers/03-arb-sepolia-to-chiado-router.ts(1 hunks)contracts/deploy/03-routers/03-arb-to-gnosis-router.ts(1 hunks)contracts/deploy/03-routers/03-chiado-to-arb-sepolia-router.ts(1 hunks)contracts/deploy/03-routers/03-gnosis-to-arb-router.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- contracts/deploy/02-inbox/02-chiado-to-arb-sepolia-inbox.ts
- contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts
- contracts/deploy/01-outbox/01-arb-sepolia-to-sepolia-outbox.ts
🧰 Additional context used
📓 Learnings (3)
contracts/deploy/01-outbox/01-arb-sepolia-to-chiado-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: For future development, consider using `CREATE2` for deterministic deployments in deployment scripts.
contracts/deploy/01-outbox/01-gnosis-to-arb-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: For future development, consider using `CREATE2` for deterministic deployments in deployment scripts.
contracts/deploy/02-inbox/02-gnosis-to-arb-inbox.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: For future development, consider using `CREATE2` for deterministic deployments in deployment scripts.
🔇 Additional comments (14)
contracts/deploy/02-inbox/02-gnosis-to-arb-inbox.ts (2)
1-3: LGTM! Imports are appropriate and well-structured.
38-40: Verify companion network configuration.
Ensure that the arbitrumSepolia companion network is properly configured in the deployment environment.
✅ Verification successful
Companion network configuration is properly set up
The arbitrumSepolia network configuration is correctly defined in contracts/hardhat.config.ts with its companion networks properly configured. The network has companion relationships with both sepolia and chiado networks, which aligns with the cross-chain deployment requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if companion network configuration exists in hardhat config
rg -A 5 "companionNetworks.*arbitrumSepolia" hardhat.config.*
Length of output: 122
Script:
#!/bin/bash
# Let's first find the hardhat config file
fd -e ts -e js "hardhat.config"
# Then check for any companion network configuration files
fd "networks" -e ts -e js
Length of output: 85
Script:
#!/bin/bash
# Now that we found the config file, let's examine its contents
cat contracts/hardhat.config.ts
Length of output: 5438
contracts/deploy/03-routers/03-arb-sepolia-to-chiado-router.ts (1)
1-78: Verify deployment security measures
This script deploys critical cross-chain infrastructure. Please ensure:
- The bridge and AMB addresses are verified
- The deployment process includes security checks
- The cross-chain configuration is tested on testnets first
contracts/deploy/03-routers/03-arb-to-gnosis-router.ts (2)
1-4: LGTM!
The imports are appropriate and necessary for the deployment script's functionality.
74-85:
Add error handling for network configurations.
The live deployer needs proper error handling for network configurations.
Add error handling as suggested in the previous review:
const liveDeployer = async () => {
+ if (!hre.companionNetworks) {
+ throw new Error('Companion networks not configured in hardhat.config');
+ }
const outboxNetwork =
chainId === RouterChains.ETHEREUM_MAINNET ? hre.companionNetworks.gnosischain : hre.companionNetworks.chiado;
const inboxNetwork =
chainId === RouterChains.ETHEREUM_MAINNET
? hre.companionNetworks.arbitrum
: hre.companionNetworks.arbitrumSepolia;
+ if (!outboxNetwork || !inboxNetwork) {
+ throw new Error(`Required companion networks not found for chain ${chainId}`);
+ }
const suffix = chainId === RouterChains.ETHEREUM_MAINNET ? "" : "Testnet";
+ try {
const veaOutbox = await outboxNetwork.deployments.get(`VeaOutboxArbToGnosis${suffix}`);
const veaInbox = await inboxNetwork.deployments.get(`VeaInboxArbToGnosis${suffix}`);
+ } catch (error) {
+ throw new Error(`Failed to get Vea contracts: ${error.message}`);
+ }contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (4)
1-4: LGTM! Imports are appropriate and well-organized.
47-49: Address TODO comment regarding deterministic deployments.
The current address prediction mechanism using nonce calculation is fragile and could break if the deployment sequence changes.
99-105: LGTM! Clear and straightforward execution logic.
84-97: 🛠️ Refactor suggestion
Add error handling and validation for live deployment.
The live deployment needs:
- Error handling for provider connection
- Validation for the router address
const liveDeployer = async () => {
+ if (!routerNetworks[SenderChains[chainId]]?.url) {
+ throw new Error(`Missing URL for chain ${chainId}`);
+ }
+
+ try {
const routerChainProvider = new ethers.JsonRpcProvider(routerNetworks[SenderChains[chainId]].url);
+ await routerChainProvider.getNetwork(); // Verify connection
+
let nonceRouter = await routerChainProvider.getTransactionCount(deployer);
const routerAddress = getContractAddress(deployer, nonceRouter);
+ if (!ethers.isAddress(routerAddress)) {
+ throw new Error(`Invalid router address: ${routerAddress}`);
+ }
console.log("calculated future router for nonce %d: %s", nonceRouter, routerAddress);
await deploy("VeaInboxArbToGnosis" + (chainId === 42161 ? "" : "Testnet"), {
// ... existing config
});
+ } catch (error) {
+ console.error(`Live deployment failed on chain ${chainId}:`, error);
+ throw error;
+ }
};Likely invalid or redundant comment.
contracts/deploy/03-routers/03-gnosis-to-arb-router.ts (1)
101-113: LGTM! Well-structured deployment configuration
The deployment configuration is well implemented with:
- Clear tagging for deployment identification
- Proper skip conditions with helpful logging
- Correct placement in the deployment sequence with
runAtTheEnd
contracts/deploy/01-outbox/01-arb-sepolia-to-chiado-outbox.ts (1)
18-18: Verify cross-chain configuration and AMB bridge address
The script uses the AMB bridge for cross-chain communication. Let's verify the configuration:
Also applies to: 46-54
✅ Verification successful
Let me verify the AMB bridge address by searching for more context about its configuration and usage.
Based on the search results, I can now verify the AMB bridge address configuration. The address 0x8448E15d0e706C0298dECA99F0b4744030e59d7d is consistently referenced in multiple deployment files as the AMB bridge address for Gnosis Chain (Chiado), and it's properly documented with a link to the official Gnosis Chain documentation (https://docs.gnosischain.com/bridges/About%20Token%20Bridges/amb-bridge#key-contracts).
AMB bridge address configuration verified
The AMB bridge address 0x8448E15d0e706C0298dECA99F0b4744030e59d7d is correctly configured for:
- Chiado to Arbitrum Sepolia inbox
- Arbitrum Sepolia to Chiado outbox
- Arbitrum to Gnosis outbox
The address matches the official Gnosis Chain documentation and is consistently used across the relevant cross-chain communication contracts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify AMB bridge address and network configurations
# Check if AMB bridge address exists on chain
echo "Verifying AMB bridge address..."
cast code 0x8448E15d0e706C0298dECA99F0b4744030e59d7d --rpc-url https://rpc.chiadochain.net
# Check network configurations
echo "Checking network configurations..."
rg "arbitrumSepolia|sepolia|chiado" -A 5
Length of output: 107715
Script:
#!/bin/bash
# Search for AMB bridge address references and configurations
rg -A 3 "0x8448E15d0e706C0298dECA99F0b4744030e59d7d"
# Search for AMB bridge related configurations
rg -A 3 "amb.*bridge" -i
Length of output: 76617
contracts/deploy/01-outbox/01-gnosis-to-arb-outbox.ts (2)
1-5: LGTM! Imports are well-structured and complete.
All necessary dependencies are properly imported with correct typing.
12-37: Verify security parameters and add documentation.
The configuration contains critical security parameters that affect the cross-chain messaging system:
- Deposit amounts
- Challenge periods
- Timeout values
- Sequencer limits
Consider adding inline documentation explaining:
- The rationale behind each parameter value
- The security implications of these values
- The relationship between different timeouts
Let's verify these values against similar cross-chain bridges:
✅ Verification successful
Let me generate additional scripts to gather more context about the security parameters and their documentation.
This will help us verify if there's existing documentation about the security parameters and their rationale. Let me analyze the results before making a final recommendation.
Let me generate one more script to search for documentation about these parameters in the contracts.
This will help us find any inline documentation about these parameters in the contract files before making a final recommendation.
Based on the code analysis, I can now provide a final response about the security parameters:
Security parameters are well-documented and follow a clear security model
The configuration values are well-justified based on the codebase:
epochPeriod: Defines the period between potential snapshots. The values (2h mainnet, 2h testnet, 10min local) are appropriate for their respective environments.challengePeriod: Set to 24h for both mainnet and testnet to allow sufficient time for challenge verification, reduced for local testing.sequencerDelayLimit: Matches Arbitrum's sequencer delay limit, with proper synchronization through the router contract.numEpochTimeout: Values (7 days mainnet, 14 days testnet) provide adequate time for network recovery while preventing indefinite stalling.
The code includes critical safety checks:
- Sequencer delay limit changes require a delay period to prevent griefing
- Challenge periods account for L2 state finality
- Timeout values prevent bridge deadlock
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar parameter configurations in other bridge contracts
rg -A 5 "challengePeriod|epochPeriod|sequencerDelayLimit"
# Look for any existing documentation about these parameters
fd -e md -e txt | xargs rg -l "challenge period|epoch period|sequencer delay"
Length of output: 65675
Script:
#!/bin/bash
# Search for documentation about these parameters in markdown files
fd -e md | xargs rg -l "epoch period|challenge period|sequencer delay"
# Look for tests that might document the rationale
fd -e test.ts -e spec.ts | xargs rg -A 5 "epochPeriod|challengePeriod|sequencerDelay"
# Search for comments explaining these parameters
rg -A 3 "\/\*\*|\/\/\/" contracts/
Length of output: 93017
Script:
#!/bin/bash
# Search for documentation about parameters in contract comments
rg -A 3 "(@param|@dev|@notice).*(\bepoch\b|\bchallenge\b|\bsequencer\b)" contracts/
Length of output: 65622
contracts/deploy/03-routers/03-chiado-to-arb-sepolia-router.ts (1)
58-60: Ensure companion networks are correctly configured
In the liveDeployer function, you're accessing companion networks:
const [veaOutbox, veaInbox] = await Promise.all([
hre.companionNetworks.arbitrumSepolia.deployments.get("VeaOutboxGnosisToArbDevnet"),
hre.companionNetworks.chiado.deployments.get("VeaInboxGnosisToArbDevnet"),
]);Verify that hre.companionNetworks.chiado corresponds to the Chiado network and that it's properly configured in your Hardhat configuration file. If chiado is not correctly set up as a companion network, this could lead to deployment failures.
| const deployer = namedAccounts.deployer ?? signers[0].address; | ||
| console.log("deployer: %s", deployer); | ||
|
|
||
| const { arbitrumBridge, amb } = paramsByChainId[RouterChains[chainId]]; |
There was a problem hiding this comment.
Add error handling for unsupported chains
The parameters lookup could fail if the chain ID is not in the enum.
- const { arbitrumBridge, amb } = paramsByChainId[RouterChains[chainId]];
+ const params = paramsByChainId[RouterChains[chainId]];
+ if (!params) {
+ throw new Error(`Unsupported chain ID: ${chainId}`);
+ }
+ const { arbitrumBridge, amb } = params;📝 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 { arbitrumBridge, amb } = paramsByChainId[RouterChains[chainId]]; | |
| const params = paramsByChainId[RouterChains[chainId]]; | |
| if (!params) { | |
| throw new Error(`Unsupported chain ID: ${chainId}`); | |
| } | |
| const { arbitrumBridge, amb } = params; |
| // ---------------------------------------------------------------------------------------------- | ||
| const liveDeployer = async () => { | ||
| const veaOutbox = await hre.companionNetworks.chiado.deployments.get("VeaOutboxArbToGnosisDevnet"); | ||
| const veaInbox = await hre.companionNetworks.arbitrumSepolia.deployments.get("VeaInboxArbToGnosisDevnet"); | ||
|
|
||
| const router = await deploy("RouterArbToGnosisDevnet", { | ||
| from: deployer, | ||
| contract: "RouterArbToGnosis", | ||
| args: [arbitrumBridge, amb, veaInbox.address, veaOutbox.address], | ||
| log: true, | ||
| }); | ||
|
|
||
| console.log("RouterArbToGnosisDevnet deployed to: %s", router.address); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and logging in live deployment
The live deployment could benefit from additional error handling and logging:
- Verify companion networks are configured
- Validate contract addresses before deployment
- Add deployment verification steps
const liveDeployer = async () => {
+ if (!hre.companionNetworks?.chiado || !hre.companionNetworks?.arbitrumSepolia) {
+ throw new Error('Companion networks not configured');
+ }
+
const veaOutbox = await hre.companionNetworks.chiado.deployments.get("VeaOutboxArbToGnosisDevnet");
const veaInbox = await hre.companionNetworks.arbitrumSepolia.deployments.get("VeaInboxArbToGnosisDevnet");
+
+ if (!veaOutbox.address || !veaInbox.address) {
+ throw new Error('Required contract addresses not found');
+ }
const router = await deploy("RouterArbToGnosisDevnet", {
from: deployer,
contract: "RouterArbToGnosis",
args: [arbitrumBridge, amb, veaInbox.address, veaOutbox.address],
log: true,
});
console.log("RouterArbToGnosisDevnet deployed to: %s", router.address);
+ console.log("Configuration:", {
+ arbitrumBridge,
+ amb,
+ veaInbox: veaInbox.address,
+ veaOutbox: veaOutbox.address
+ });
};📝 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 liveDeployer = async () => { | |
| const veaOutbox = await hre.companionNetworks.chiado.deployments.get("VeaOutboxArbToGnosisDevnet"); | |
| const veaInbox = await hre.companionNetworks.arbitrumSepolia.deployments.get("VeaInboxArbToGnosisDevnet"); | |
| const router = await deploy("RouterArbToGnosisDevnet", { | |
| from: deployer, | |
| contract: "RouterArbToGnosis", | |
| args: [arbitrumBridge, amb, veaInbox.address, veaOutbox.address], | |
| log: true, | |
| }); | |
| console.log("RouterArbToGnosisDevnet deployed to: %s", router.address); | |
| }; | |
| // ---------------------------------------------------------------------------------------------- | |
| const liveDeployer = async () => { | |
| if (!hre.companionNetworks?.chiado || !hre.companionNetworks?.arbitrumSepolia) { | |
| throw new Error('Companion networks not configured'); | |
| } | |
| const veaOutbox = await hre.companionNetworks.chiado.deployments.get("VeaOutboxArbToGnosisDevnet"); | |
| const veaInbox = await hre.companionNetworks.arbitrumSepolia.deployments.get("VeaInboxArbToGnosisDevnet"); | |
| if (!veaOutbox.address || !veaInbox.address) { | |
| throw new Error('Required contract addresses not found'); | |
| } | |
| const router = await deploy("RouterArbToGnosisDevnet", { | |
| from: deployer, | |
| contract: "RouterArbToGnosis", | |
| args: [arbitrumBridge, amb, veaInbox.address, veaOutbox.address], | |
| log: true, | |
| }); | |
| console.log("RouterArbToGnosisDevnet deployed to: %s", router.address); | |
| console.log("Configuration:", { | |
| arbitrumBridge, | |
| amb, | |
| veaInbox: veaInbox.address, | |
| veaOutbox: veaOutbox.address | |
| }); | |
| }; |
| const router = await deploy(`RouterArbToGnosis${suffix}`, { | ||
| from: deployer, | ||
| contract: "RouterArbToGnosis", | ||
| args: [arbitrumBridge, amb, veaInbox.address, veaOutbox.address], | ||
| log: true, | ||
| }); | ||
|
|
||
| console.log(`RouterArbToGnosis${suffix} deployed to: ${router.address}`); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate deployment success and emit deployment event.
The router deployment should validate success and emit appropriate events/logs.
Add deployment validation:
const router = await deploy(`RouterArbToGnosis${suffix}`, {
from: deployer,
contract: "RouterArbToGnosis",
args: [arbitrumBridge, amb, veaInbox.address, veaOutbox.address],
log: true,
});
+if (!router.newlyDeployed) {
+ console.log(`Reusing existing RouterArbToGnosis${suffix} at ${router.address}`);
+ return;
+}
+
console.log(`RouterArbToGnosis${suffix} deployed to: ${router.address}`);
+
+// Verify the deployment was successful
+const RouterArbToGnosis = await hre.ethers.getContractFactory("RouterArbToGnosis");
+const routerContract = RouterArbToGnosis.attach(router.address);
+const storedBridge = await routerContract.bridge();
+const storedAmb = await routerContract.amb();
+
+if (storedBridge !== arbitrumBridge || storedAmb !== amb) {
+ throw new Error(`Router deployment verification failed: incorrect initialization`);
+}📝 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 router = await deploy(`RouterArbToGnosis${suffix}`, { | |
| from: deployer, | |
| contract: "RouterArbToGnosis", | |
| args: [arbitrumBridge, amb, veaInbox.address, veaOutbox.address], | |
| log: true, | |
| }); | |
| console.log(`RouterArbToGnosis${suffix} deployed to: ${router.address}`); | |
| }; | |
| const router = await deploy(`RouterArbToGnosis${suffix}`, { | |
| from: deployer, | |
| contract: "RouterArbToGnosis", | |
| args: [arbitrumBridge, amb, veaInbox.address, veaOutbox.address], | |
| log: true, | |
| }); | |
| if (!router.newlyDeployed) { | |
| console.log(`Reusing existing RouterArbToGnosis${suffix} at ${router.address}`); | |
| return; | |
| } | |
| console.log(`RouterArbToGnosis${suffix} deployed to: ${router.address}`); | |
| // Verify the deployment was successful | |
| const RouterArbToGnosis = await hre.ethers.getContractFactory("RouterArbToGnosis"); | |
| const routerContract = RouterArbToGnosis.attach(router.address); | |
| const storedBridge = await routerContract.bridge(); | |
| const storedAmb = await routerContract.amb(); | |
| if (storedBridge !== arbitrumBridge || storedAmb !== amb) { | |
| throw new Error(`Router deployment verification failed: incorrect initialization`); | |
| } | |
| }; |
| const routerNetworks = { | ||
| ARBITRUM: config.networks.mainnet, | ||
| ARBITRUM_SEPOLIA: config.networks.sepolia, | ||
| HARDHAT: config.networks.localhost, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for network configuration.
The network mapping could fail silently if the network configurations are undefined. Consider adding validation:
const routerNetworks = {
ARBITRUM: config.networks.mainnet,
ARBITRUM_SEPOLIA: config.networks.sepolia,
HARDHAT: config.networks.localhost,
};
+
+// Validate network configuration
+Object.entries(routerNetworks).forEach(([network, config]) => {
+ if (!config?.url) {
+ throw new Error(`Missing network configuration for ${network}`);
+ }
+});📝 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 routerNetworks = { | |
| ARBITRUM: config.networks.mainnet, | |
| ARBITRUM_SEPOLIA: config.networks.sepolia, | |
| HARDHAT: config.networks.localhost, | |
| }; | |
| const routerNetworks = { | |
| ARBITRUM: config.networks.mainnet, | |
| ARBITRUM_SEPOLIA: config.networks.sepolia, | |
| HARDHAT: config.networks.localhost, | |
| }; | |
| // Validate network configuration | |
| Object.entries(routerNetworks).forEach(([network, config]) => { | |
| if (!config?.url) { | |
| throw new Error(`Missing network configuration for ${network}`); | |
| } | |
| }); |
| const hardhatDeployer = async () => { | ||
| let nonce = await ethers.provider.getTransactionCount(deployer); | ||
|
|
||
| const arbitrumBridgeAddress = getContractAddress(deployer, nonce + 5); | ||
|
|
||
| const arbSysMock = await deploy("ArbSysMock", { | ||
| from: deployer, | ||
| contract: "ArbSysMockWithBridge", | ||
| args: [arbitrumBridgeAddress], | ||
| log: true, | ||
| }); | ||
|
|
||
| const routerAddress = getContractAddress(deployer, nonce + 6); | ||
| console.log("calculated future router for nonce %d: %s", nonce + 6, routerAddress); | ||
|
|
||
| const receiverGateway = await deployments.get("ArbToGnosisReceiverGateway"); | ||
| const veaInbox = await deploy("VeaInboxArbToGnosis", { | ||
| from: deployer, | ||
| contract: "VeaInboxArbToGnosisMock", | ||
| args: [epochPeriod, routerAddress, arbSysMock.address], | ||
| log: true, | ||
| }); | ||
|
|
||
| await deploy("ArbToGnosisSenderGateway", { | ||
| from: deployer, | ||
| contract: "SenderGatewayMock", | ||
| args: [veaInbox.address, receiverGateway.address], | ||
| gasLimit: 4000000, | ||
| log: true, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and error handling for the deployment sequence.
The complex deployment sequence needs:
- Documentation explaining the deployment order and dependencies
- Error handling for failed deployments
const hardhatDeployer = async () => {
+ try {
let nonce = await ethers.provider.getTransactionCount(deployer);
+ console.log('Starting deployment sequence with initial nonce:', nonce);
+
+ // Step 1: Deploy ArbSysMock with bridge
const arbitrumBridgeAddress = getContractAddress(deployer, nonce + 5);
const arbSysMock = await deploy("ArbSysMock", {
from: deployer,
contract: "ArbSysMockWithBridge",
args: [arbitrumBridgeAddress],
log: true,
});
+
+ // Step 2: Deploy VeaInbox with router
const routerAddress = getContractAddress(deployer, nonce + 6);
console.log("calculated future router for nonce %d: %s", nonce + 6, routerAddress);
const receiverGateway = await deployments.get("ArbToGnosisReceiverGateway");
const veaInbox = await deploy("VeaInboxArbToGnosis", {
// ... existing config
});
+
+ // Step 3: Deploy SenderGateway
await deploy("ArbToGnosisSenderGateway", {
// ... existing config
});
+ } catch (error) {
+ console.error('Deployment sequence failed:', error);
+ throw error;
+ }
};Committable suggestion skipped: line range outside the PR's diff.
| enum RouterChains { | ||
| ETHEREUM_SEPOLIA = 11155111, | ||
| HARDHAT = 31337, | ||
| } |
There was a problem hiding this comment.
Potential incorrect chain IDs in RouterChains enum
The RouterChains enum defines ETHEREUM_SEPOLIA with chain ID 11155111:
enum RouterChains {
ETHEREUM_SEPOLIA = 11155111,
HARDHAT = 31337,
}However, the script is intended for deploying a router from Chiado to Arbitrum Sepolia, as indicated by the filename and comments. Chiado (Gnosis Chain testnet) has a chain ID of 10200. Please verify and update the enum to include the correct chain ID for Chiado:
enum RouterChains {
CHIADO = 10200,
HARDHAT = 31337,
}| const paramsByChainId = { | ||
| ETHEREUM_SEPOLIA: { | ||
| arbitrumBridge: "0x38f918D0E9F1b721EDaA41302E399fa1B79333a9", // https://developer.arbitrum.io/useful-addresses | ||
| amb: "0xf2546D6648BD2af6a008A7e7C1542BB240329E11", // https://docs.gnosischain.com/bridges/About%20Token%20Bridges/amb-bridge#key-contracts | ||
| }, | ||
| HARDHAT: { | ||
| arbitrumBridge: ZeroAddress, | ||
| amb: ZeroAddress, | ||
| }, | ||
| } as const; |
There was a problem hiding this comment.
Incorrect parameters for the target network in paramsByChainId
The paramsByChainId object uses ETHEREUM_SEPOLIA as a key with parameters intended for the router deployment:
const paramsByChainId = {
ETHEREUM_SEPOLIA: {
arbitrumBridge: "0x38f918D0E9F1b721EDaA41302E399fa1B79333a9",
amb: "0xf2546D6648BD2af6a008A7e7C1542BB240329E11",
},
HARDHAT: {
arbitrumBridge: ZeroAddress,
amb: ZeroAddress,
},
};Since the deployment is intended for Chiado (chain ID 10200), please update the key to CHIADO and ensure the parameters correspond to the Chiado network:
const paramsByChainId = {
CHIADO: {
arbitrumBridge: "<Arbitrum Bridge Address>",
amb: "<AMB Address>",
},
HARDHAT: {
arbitrumBridge: ZeroAddress,
amb: ZeroAddress,
},
};| const deployRouter: DeployFunction = async (hre: HardhatRuntimeEnvironment) => { | ||
| const { deployments, getNamedAccounts, getChainId } = hre; | ||
| const { deploy } = deployments; | ||
| const chainId = Number(await getChainId()); | ||
|
|
||
| // fallback to hardhat node signers on local network | ||
| const namedAccounts = await getNamedAccounts(); | ||
| const signers = await hre.ethers.getSigners(); | ||
| const deployer = namedAccounts.deployer ?? signers[0].address; |
There was a problem hiding this comment.
Deployment may target the incorrect network
The deployment script uses the chain ID from RouterChains to determine deployment parameters and the target network:
const chainId = Number(await getChainId());
// ...
const { arbitrumBridge, amb } = paramsByChainId[RouterChains[chainId] as keyof typeof paramsByChainId];If chainId is 11155111 (Ethereum Sepolia), but the router should be deployed on Chiado (chain ID 10200), this could result in deploying to the wrong network with incorrect parameters. Confirm that getChainId() returns the intended chain ID and that the network configurations align with the deployment objectives.
| arbitrumBridge: "0x8315177aB297bA92A06054cE80a67Ed4DBd7ed3a", // https://developer.arbitrum.io/useful-addresses, | ||
| }, | ||
| ETHEREUM_SEPOLIA: { | ||
| deposit: parseEther("1"), // ~100 ETH budget to start, enough for 8 days of challenges |
There was a problem hiding this comment.
Correct the inconsistency between deposit amount and budget comment
The deposit amount is set to parseEther("1") (1 ETH), but the comment mentions ~100 ETH budget. Please update the deposit amount or adjust the comment to reflect the correct budget.
If the intended deposit is 100 ETH, adjust the code as follows:
- deposit: parseEther("1"), // ~100 ETH budget to start, enough for 8 days of challenges
+ deposit: parseEther("100"), // ~100 ETH budget to start, enough for 8 days of challenges📝 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.
| deposit: parseEther("1"), // ~100 ETH budget to start, enough for 8 days of challenges | |
| deposit: parseEther("100"), // ~100 ETH budget to start, enough for 8 days of challenges |
|
|
||
| const paramsByChainId = { | ||
| ETHEREUM_MAINNET: { | ||
| deposit: parseEther("12"), // ~1100 ETH budget, enough for 8 days of challenges |
There was a problem hiding this comment.
Correct the inconsistency between deposit amount and budget comment
The deposit amount is set to parseEther("12") (12 ETH), but the comment mentions ~1100 ETH budget. Please update the deposit amount or adjust the comment to reflect the correct budget.
If the intended deposit is 1100 ETH, adjust the code as follows:
- deposit: parseEther("12"), // ~1100 ETH budget, enough for 8 days of challenges
+ deposit: parseEther("1100"), // ~1100 ETH budget, enough for 8 days of challengesCommittable suggestion skipped: line range outside the PR's diff.
test(relayer): adds initial testing base
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.eslintrc.json (1)
10-10: Consider removing redundant ES2020 configurationThe
"es2020": truesetting in theenvsection appears redundant since:
ecmaVersion: 2020is already specified inparserOptionses6: trueis already enabled in theenvsection"env": { "browser": true, "es6": true, "node": true, "mocha": true, - "es2020": true, "jest": true },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.eslintrc.json(1 hunks)
🔇 Additional comments (1)
.eslintrc.json (1)
11-11: LGTM! Jest environment properly configured
Adding Jest environment support is appropriate and aligns well with the enhanced testing infrastructure being implemented across the repository.
refactor/lockfile
Summary by CodeRabbit
New Features
CODEOWNERSfile for repository ownership management.Bug Fixes
.gitignoreto exclude unnecessary environment files and directories.Documentation
README.mdandcontracts/README.mdto reflect changes in project structure, deployments, and instructions.Chores