Hardhat ether version update #379
Conversation
madhurMongia
requested review from
jaybuidl,
mani99brar and
alcercu
as code owners
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
contracts/hardhat.config.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@typescript-eslint/parser' declared in 'contracts/.eslintrc.js': Cannot find module '@typescript-eslint/parser'
WalkthroughThis pull request encompasses a series of updates across multiple TypeScript files within the contracts and relayer-cli directories, primarily focusing on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
❌ Deploy Preview for veascan failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (15)
contracts/deploy/02-inbox/02-chiado-to-arb-sepolia-inbox.ts (1)
Line range hint
21-21: Consider implementing deterministic deploymentsThe TODO comment indicates that deterministic deployments should be implemented. This would be a good addition to ensure consistent contract addresses across different environments.
Would you like me to help implement deterministic deployments using the
@openzeppelin/hardhat-upgradesplugin?contracts/deploy/03-routers/03-gnosis-to-arb-router.ts (2)
31-34: Ensure proper handling of unsupported chain IDs in getNetworkConfigThe
getNetworkConfigfunction currently differentiates betweenETHEREUM_MAINNETand other chain IDs by defaulting to testnet configurations. Consider adding explicit handling or validations for unsupported chain IDs to prevent potential misconfigurations in different environments.Apply this diff to add validation:
const getNetworkConfig = (chainId: number): NetworkConfig => { + if (chainId !== RouterChains.ETHEREUM_MAINNET && chainId !== RouterChains.ETHEREUM_SEPOLIA) { + throw new Error(`Unsupported chain ID: ${chainId}`); + } return { outboxNetwork: chainId === RouterChains.ETHEREUM_MAINNET ? "arbitrum" : "arbitrumSepolia", inboxNetwork: chainId === RouterChains.ETHEREUM_MAINNET ? "gnosischain" : "chiado", }; };
105-110: Improve chain ID check in skip functionUsing
!RouterChains[chainId]to determine if the chain ID is supported may not correctly handle cases where the chain ID is zero or another falsy value but valid. Consider using a more reliable method to check if the chain ID is among the supported ones.Apply this diff to enhance the chain ID check:
const shouldSkip = !RouterChains[chainId]; +// Improved check for supported chain IDs +const shouldSkip = !(chainId in RouterChains);validator-cli/src/ArbToEth/watcherArbToEth.ts (3)
Line range hint
109-109: Use 'bigint' instead of 'BigInt' for type consistencyThe type 'BigInt' should be replaced with the lowercase 'bigint' to maintain consistency with TypeScript's primitive types.
Apply this fix:
- const balance = (await retryOperation(() => weth.balanceOf(watcherAddress), 1000, 10)) as BigInt; + const balance = (await retryOperation(() => weth.balanceOf(watcherAddress), 1000, 10)) as bigint;
Line range hint
110-110: Use 'bigint' instead of 'BigInt' for type consistencySimilar to the previous line, replace 'BigInt' with 'bigint' for the 'allowance' variable.
Apply this fix:
- const allowance = (await retryOperation(() => weth.allowance(watcherAddress, veaOutboxAddress), 1000, 10)) as BigInt; + const allowance = (await retryOperation(() => weth.allowance(watcherAddress, veaOutboxAddress), 1000, 10)) as bigint;
Line range hint
115-115: Use 'bigint' instead of 'BigInt' for type consistencyUpdate the type casting to use 'bigint' instead of 'BigInt' when casting the second element of
maxTimeVariation().Apply this fix:
- const maxDelaySeconds = Number((await retryOperation(() => sequencer.maxTimeVariation(), 1000, 10))[1] as BigInt); + const maxDelaySeconds = Number((await retryOperation(() => sequencer.maxTimeVariation(), 1000, 10))[1] as bigint);validator-cli/src/ArbToEth/watcherArbToGnosis.ts (1)
582-582: Avoid detailed error logging in production codeLogging detailed error messages, including parsing JSON errors, can expose sensitive information or clutter logs.
Consider simplifying the error logging:
- console.error("Error finding batch containing block:", JSON.parse(JSON.stringify(e)).error.body); + console.error("Error finding batch containing block:", e);This provides sufficient error information without overcomplicating the log output.
contracts/test/integration/ArbToEth.ts (3)
60-61: Update contract address references to '.address'Ensure consistency by using '.address' instead of '.target' when referencing contract addresses.
Apply this fix:
- expect(await senderGateway.veaInbox()).to.equal(veaInbox.target); + expect(await senderGateway.veaInbox()).to.equal(veaInbox.address); - expect(await senderGateway.receiverGateway()).to.equal(receiverGateway.target); + expect(await senderGateway.receiverGateway()).to.equal(receiverGateway.address);
64-66: Update contract address references to '.address'Similarly, update the contract address references for consistency.
Apply this fix:
- expect(await veaInbox.arbSys()).to.equal(arbsysMock.target); + expect(await veaInbox.arbSys()).to.equal(arbsysMock.address); - expect(await veaInbox.veaOutboxArbToEth()).to.equal(veaOutbox.target); + expect(await veaInbox.veaOutboxArbToEth()).to.equal(veaOutbox.address);
72-75: Update contract address references to '.address'Continue updating contract address references for consistency and clarity.
Apply this fix:
- expect(await veaOutbox.veaInboxArbToEth()).to.equal(veaInbox.target); - expect(await veaOutbox.bridge()).to.equal(bridge.target); - expect(await receiverGateway.veaOutbox()).to.equal(veaOutbox.target); + expect(await veaOutbox.veaInboxArbToEth()).to.equal(veaInbox.address); + expect(await veaOutbox.bridge()).to.equal(bridge.address); + expect(await receiverGateway.veaOutbox()).to.equal(veaOutbox.address);contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (1)
Ethers imports and patterns need standardization across the codebase
- Multiple files are using mixed imports from both
ethersand@ethersproject/*packages:
validator-cli/src/ArbToEth/watcherArbToGnosis.tsvalidator-cli/src/ArbToEth/watcherArbToEth.tsvalidator-cli/src/utils/arbMsgExecutor.ts- Some files use the older
JsonRpcProviderfrom@ethersproject/providerswhile others use it fromethers- Inconsistent import patterns:
- Direct imports:
import { ethers } from "ethers"- Specific imports:
import { ZeroAddress } from "ethers"- Legacy imports:
import { JsonRpcProvider } from "@ethersproject/providers"The codebase should standardize on the new ethers v6 patterns consistently across all files.
🔗 Analysis chain
Line range hint
1-1: Verify cross-file ethers version consistencyLet's ensure all files are using the same ethers version and patterns consistently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining old ethers patterns rg "ethers\.constants\." --type ts rg "providers\.JsonRpcProvider" --type ts rg "import.*from.*ethers" --type ts -A 1Length of output: 6228
validator-cli/src/utils/ethers.ts (1)
79-90: Update exports to reflect new function namesThe exports section still includes both old and new function names. This should be cleaned up to maintain a consistent API.
export { getVeaOutboxArbToEth, - getWalletRPC, getWallet, - getVeaOutboxArbToEthDevnetProvider, - getVeaInboxArbToEthProvider, - getVeaOutboxArbToEthProvider, getVeaOutboxArbToGnosis, getVeaInboxArbToGnosis, getVeaRouterArbToGnosis, getWETH, getAMB, };contracts/deploy/03-routers/03-arb-to-gnosis-router.ts (1)
75-81: Consider extracting network configuration logicThe network selection logic could be moved to a separate configuration utility for better maintainability.
// networks.config.ts export const getNetworkConfig = (chainId: number) => ({ outboxNetwork: chainId === RouterChains.ETHEREUM_MAINNET ? 'gnosischain' : 'chiado', inboxNetwork: chainId === RouterChains.ETHEREUM_MAINNET ? 'arbitrum' : 'arbitrumSepolia' });contracts/deploy/01-outbox/01-arb-sepolia-to-chiado-outbox.ts (1)
100-101: Consider using BigInt for gas valuesWhile converting to string works, consider using BigInt literals for better type safety and readability.
- maxFeePerGas: String(ethers.parseUnits("1", "gwei")), - maxPriorityFeePerGas: String(ethers.parseUnits("1", "gwei")), + maxFeePerGas: BigInt(1_000_000_000), // 1 gwei + maxPriorityFeePerGas: BigInt(1_000_000_000), // 1 gweirelayer-cli/src/utils/relay.ts (1)
38-45: Consider removing redundant error handlingBased on previous feedback for this file, we should avoid adding try-catch blocks that only log and rethrow errors, as they add unnecessary complexity without providing additional value.
- try { const txn = await veaOutbox.sendMessage(proof, nonce, to, data); const receipt = await txn.wait(); return receipt; - } catch (error) { - console.error("Transaction failed:", error); - throw error; - }
📜 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 (25)
contracts/deploy/01-outbox/01-arb-sepolia-to-chiado-outbox.ts(4 hunks)contracts/deploy/01-outbox/01-arb-sepolia-to-sepolia-outbox.ts(3 hunks)contracts/deploy/01-outbox/01-arb-to-eth-outbox.ts(3 hunks)contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts(4 hunks)contracts/deploy/01-outbox/01-chiado-to-arb-sepolia-outbox.ts(4 hunks)contracts/deploy/01-outbox/01-gnosis-to-arb-outbox.ts(3 hunks)contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts(1 hunks)contracts/deploy/02-inbox/02-chiado-to-arb-sepolia-inbox.ts(2 hunks)contracts/deploy/02-inbox/02-gnosis-to-arb-inbox.ts(2 hunks)contracts/deploy/03-routers/03-arb-sepolia-to-chiado-router.ts(4 hunks)contracts/deploy/03-routers/03-arb-to-gnosis-router.ts(5 hunks)contracts/deploy/03-routers/03-chiado-to-arb-sepolia-router.ts(3 hunks)contracts/deploy/03-routers/03-gnosis-to-arb-router.ts(4 hunks)contracts/hardhat.config.ts(1 hunks)contracts/package.json(2 hunks)contracts/test/integration/ArbToEth.ts(48 hunks)contracts/test/integration/ArbToGnosis.ts(26 hunks)contracts/test/merkle/MerkleTree.ts(1 hunks)contracts/test/merkle/index.ts(2 hunks)relayer-cli/src/utils/ethers.ts(3 hunks)relayer-cli/src/utils/relay.ts(3 hunks)validator-cli/package.json(1 hunks)validator-cli/src/ArbToEth/watcherArbToEth.ts(12 hunks)validator-cli/src/ArbToEth/watcherArbToGnosis.ts(12 hunks)validator-cli/src/utils/ethers.ts(3 hunks)
🧰 Additional context used
📓 Learnings (3)
relayer-cli/src/utils/relay.ts (1)
Learnt from: fcanela
PR: kleros/vea#344
File: relayer-cli/src/utils/relay.ts:28-40
Timestamp: 2024-11-26T11:30:36.180Z
Learning: In `relayer-cli/src/utils/relay.ts`, avoid suggesting adding try/catch blocks that only log errors and rethrow in the `relay` function.
validator-cli/src/ArbToEth/watcherArbToEth.ts (2)
Learnt from: mani99brar
PR: kleros/vea#344
File: validator-cli/src/ArbToEth/watcherArbToEth.ts:732-745
Timestamp: 2024-11-27T04:18:05.872Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToEth.ts`, input validation inside the `hashClaim` function is unnecessary because the upper layer ensures valid input before calling it.
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:829-1112
Timestamp: 2024-11-26T08:37:47.591Z
Learning: Refactoring the `reconstructChallengeProgress` function in `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` into smaller functions is considered unnecessary abstraction.
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (2)
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:829-1112
Timestamp: 2024-11-26T08:37:47.591Z
Learning: Refactoring the `reconstructChallengeProgress` function in `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` into smaller functions is considered unnecessary abstraction.
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:829-840
Timestamp: 2024-12-02T10:16:56.825Z
Learning: In the `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` file, avoid adding redundant error handling in functions like `reconstructChallengeProgress` when error handling is already adequately managed.
🪛 Biome (1.9.4)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts
[error] 109-109: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
[error] 110-110: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
[error] 115-115: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
🔇 Additional comments (54)
contracts/deploy/02-inbox/02-chiado-to-arb-sepolia-inbox.ts (2)
16-16: LGTM: Correct migration of ethers zero address constant
The update from ethers.constants.AddressZero to ethers.ZeroAddress correctly follows the ethers v6 API changes.
37-38: Verify gas options handling pattern
The conversion of parsed units to strings is a change in how gas parameters are handled. Let's verify this pattern is consistent across the codebase and aligns with ethers v6 best practices.
✅ Verification successful
Gas options pattern is consistent across deployment scripts
The verification shows that all deployment scripts consistently use the same pattern:
- Converting gas values to strings using
String(ethers.parseUnits("1", "gwei")) - No deprecated
ethers.utils.parseUnitsusage found - Pattern is uniformly applied across both inbox and outbox deployments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other gas options patterns in deployment scripts
echo "Checking for other gas options patterns in deployment scripts:"
rg -A 2 "maxFeePerGas|maxPriorityFeePerGas" --type ts contracts/deploy/
# Check if there are any remaining uses of ethers.utils.parseUnits
echo "Checking for remaining uses of deprecated ethers.utils.parseUnits:"
rg "ethers\.utils\.parseUnits" --type ts
Length of output: 2234
contracts/deploy/03-routers/03-gnosis-to-arb-router.ts (5)
3-3: Consistency in importing ZeroAddress
Good job updating the import statement to use ZeroAddress from ethers, ensuring consistency across the codebase.
21-22: Appropriate use of ZeroAddress for Hardhat configuration
Using ZeroAddress for arbitrumBridge and amb in the HARDHAT configuration is appropriate for local testing environments.
26-29: Introduction of NetworkConfig interface improves type safety
Defining the NetworkConfig interface enhances type safety and improves code readability by clearly specifying the expected structure of network configurations.
43-46: Improved deployer selection logic enhances clarity
Refactoring the deployer assignment enhances clarity by first attempting to use namedAccounts.deployer before falling back to the default signer's address. This ensures the correct deployer account is used in various environments.
74-91: Good practice: Added try-catch block for error handling in liveDeployer
Implementing error handling in liveDeployer improves the robustness of the deployment script by catching and logging deployment errors, which aids in debugging and ensures that unexpected issues are properly surfaced.
validator-cli/src/ArbToEth/watcherArbToEth.ts (1)
461-465: Validate 'maxPriorityFeePerGasMEV' against 'maxFeePerGasProfitable'
Ensure that the hardcoded value for maxPriorityFeePerGasMEV is appropriate and does not exceed maxFeePerGasProfitable.
Run the following script to validate the gas fee parameters:
✅ Verification successful
The code already handles the maxPriorityFeePerGasMEV validation appropriately
The code includes proper validation and adjustment logic:
- It checks if
maxPriorityFeePerGasMEVexceedsmaxFeePerGasProfitable - If it does, it automatically adjusts
maxPriorityFeePerGasMEVto be equal tomaxFeePerGasProfitable - The adjustment is logged with a warning message for transparency
- The adjusted values are then used correctly in the transaction parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that maxPriorityFeePerGasMEV does not exceed maxFeePerGasProfitable
# Expected: maxPriorityFeePerGasMEV should be less than or equal to maxFeePerGasProfitable
grep -E 'let maxPriorityFeePerGasMEV = BigInt\((\d+)\);' validator-cli/src/ArbToEth/watcherArbToEth.ts | while read -r line; do
value=$(echo "$line" | grep -oP '\d+')
if [ "$value" -gt 100000000000 ]; then
echo "maxPriorityFeePerGasMEV is set to an unusually high value: $value"
fi
done
Length of output: 119
Script:
#!/bin/bash
# Let's examine the context around maxFeePerGasProfitable and maxPriorityFeePerGasMEV
rg "maxFeePerGasProfitable|maxPriorityFeePerGasMEV" validator-cli/src/ArbToEth/watcherArbToEth.ts -B 2 -A 2
Length of output: 1196
contracts/test/integration/ArbToEth.ts (1)
21-24: Migration to BigInt literals is appropriate
The usage of BigInt literals improves performance and readability. Constants are correctly defined.
validator-cli/package.json (1)
23-23: Dependency update to '@typechain/ethers-v6' is appropriate
Updating to '@typechain/ethers-v6' aligns with the upgrade to Ethers v6 and ensures compatibility.
contracts/deploy/02-inbox/02-gnosis-to-arb-inbox.ts (2)
3-3: LGTM: Import statement updated for ethers v6
The change from hardhat to direct ethers import aligns with the ethers v6 migration.
43-44: Verify gas options compatibility with network
The conversion to String is necessary as ethers v6 returns BigInt which isn't JSON-RPC compatible. However, we should verify if these gas values are appropriate for the Gnosis chain.
✅ Verification successful
Let me gather more information about the gas prices on Gnosis chain.
Let me check for any hardhat or network configuration files that might contain gas price settings.
Based on the gathered information, I can now provide the final response:
Gas values are appropriate for Gnosis chain deployment
The 1 gwei values for both maxFeePerGas and maxPriorityFeePerGas are consistent with other deployment scripts in the codebase and are appropriate for Gnosis chain. This is supported by:
- The same gas values are used across all Gnosis/Chiado deployment scripts
- A comment in
contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.tsconfirms "1 gwei" as an intentional value to handle RPC response underpricing - The hardhat config shows proper network configurations for both Gnosis mainnet (chainId: 100) and Chiado testnet (chainId: 10200)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if similar gas values are used in other deployment scripts for Gnosis chain
rg -A 2 "maxFeePerGas.*gwei" --type ts
Length of output: 1585
Script:
#!/bin/bash
# Look for any configuration files or constants related to gas prices
rg -i "gas.*price.*gnosis|gas.*price.*chiado" --type ts
# Check for any documentation about gas prices
fd -e md | xargs rg -i "gas.*price.*gnosis|gas.*price.*chiado"
Length of output: 371
Script:
#!/bin/bash
# Look for hardhat config files and network settings
fd "hardhat.config" | xargs cat
# Check for any network configuration files
fd -e ts -e js | xargs rg -l "networks.*gnosis|networks.*chiado"
Length of output: 5723
contracts/deploy/03-routers/03-arb-sepolia-to-chiado-router.ts (3)
3-3: LGTM: Updated to ethers v6 ZeroAddress constant
The change from ethers.constants.AddressZero to ZeroAddress aligns with ethers v6's reorganized constants.
Also applies to: 16-17
28-30: LGTM: Improved deployer address fallback logic
The new implementation properly handles cases where named accounts might not be available by falling back to the first signer.
63-66: LGTM: Enhanced type safety with RouterChains enum
Using RouterChains.HARDHAT instead of the magic number 31337 improves code maintainability and type safety.
contracts/test/merkle/index.ts (4)
Line range hint 20-25: LGTM: Simplified reduce callback signature
Removed unused currentIndex parameter from the reduce callback, making the code more concise without affecting functionality.
33-34: LGTM: Added explicit type annotations
Added TypeScript type annotations improve code clarity and help catch potential type-related errors early.
39-44: LGTM: Updated deployment comments for ethers v6
The comment accurately reflects that deployed() is no longer needed in ethers v6 as the deploy() promise now resolves after deployment is complete.
29-30: LGTM: Removed unnecessary async
Removed unnecessary async keyword from describe blocks as they don't contain any direct async operations.
relayer-cli/src/utils/ethers.ts (3)
1-1: LGTM: Import statement aligns with newer ethers version
The consolidated import from 'ethers' follows the updated package structure.
76-78: LGTM: Provider type generalization
The change from JsonRpcProvider to Provider in getVeaOutboxArbToEthDevnetProvider allows for more flexibility in provider types while maintaining type safety.
11-13: Verify consistent usage of the new wallet initialization pattern
The new getWallet function and updated getWalletRPC signature improve type safety by explicitly returning Wallet. However, we should verify that all callers have been updated to handle the new Provider type.
Also applies to: 15-16
✅ Verification successful
Let me verify the usage of getWalletRPC and its type compatibility by checking the Provider type declarations and imports.
Provider type compatibility is maintained across the codebase
The verification shows that both codebases (relayer-cli and validator-cli) handle Provider types correctly:
- In relayer-cli:
getWalletRPCaccepts the genericProvidertype, making it more flexible while maintaining type safety - All callers use either
JsonRpcProviderorProvidertypes consistently - The
getWalletfunction's usage ofJsonRpcProvideris compatible with both implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all getWalletRPC calls to verify they pass compatible Provider types
rg "getWalletRPC\(" --type ts -A 2
Length of output: 2880
Script:
#!/bin/bash
# Check Provider type imports and declarations
rg "Provider" --type ts -B 2 relayer-cli/src/utils/ethers.ts validator-cli/src/utils/ethers.ts
# Check JsonRpcProvider imports and usage
rg "JsonRpcProvider" --type ts -B 2 relayer-cli/src/utils/ethers.ts validator-cli/src/utils/ethers.ts
Length of output: 11195
contracts/deploy/03-routers/03-chiado-to-arb-sepolia-router.ts (3)
3-3: LGTM: Updated ethers constant usage
Replacing ethers.constants.AddressZero with ZeroAddress aligns with the new ethers version.
Also applies to: 16-17
40-43: Performance improvement with Promise.all
Good use of Promise.all for parallel deployment fetching in both local and live environments.
Also applies to: 57-60
73-81: LGTM: Enhanced error handling
The addition of try-catch block with detailed error logging improves deployment reliability.
contracts/deploy/02-inbox/02-arb-to-gnosis-inbox.ts (1)
85-85: LGTM: Updated JsonRpcProvider instantiation
The change from providers.JsonRpcProvider to ethers.JsonRpcProvider aligns with the new ethers version's API.
contracts/deploy/03-routers/03-arb-to-gnosis-router.ts (1)
21-22: Verify ZeroAddress usage in test environment
The change from AddressZero to ZeroAddress is correct for the updated ethers version. However, ensure this doesn't affect any test cases that might depend on the old constant.
✅ Verification successful
ZeroAddress usage is consistent across the codebase
The codebase shows consistent usage of ZeroAddress from ethers, with no instances of the deprecated AddressZero. All test cases and deployment scripts are already using the updated constant correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining AddressZero usage
rg "AddressZero" .
# Check for ZeroAddress usage consistency
rg "ZeroAddress" .
Length of output: 4569
contracts/deploy/01-outbox/01-chiado-to-arb-sepolia-outbox.ts (1)
94-95: Verify gas option string conversion
Converting gas values to strings using parseUnits might affect how they're processed. Ensure this change is compatible with the contract deployment process.
✅ Verification successful
Gas option string conversion is consistently implemented
The string conversion of gas values using ethers.parseUnits is consistently implemented across all inbox and outbox deployment scripts, with the same values being used (1 gwei). This pattern is safe and compatible with the contract deployment process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent gas option handling across deployment scripts
rg -A 2 "maxFeePerGas.*parseUnits" .
rg -A 2 "maxPriorityFeePerGas.*parseUnits" .
Length of output: 2944
contracts/deploy/01-outbox/01-arb-sepolia-to-chiado-outbox.ts (3)
1-1: LGTM: Import statement updated for newer ethers version
The import statement has been correctly updated to use the new path from ethers v6.
29-29: LGTM: Updated zero address constant usage
Correctly replaced deprecated ethers.constants.AddressZero with ethers.ZeroAddress for both amb and WETH parameters.
Also applies to: 33-33
104-104: LGTM: Updated provider instantiation
Correctly updated to use new JsonRpcProvider syntax from ethers v6.
Also applies to: 107-107
contracts/deploy/01-outbox/01-gnosis-to-arb-outbox.ts (2)
99-100: LGTM: Consistent updates with other files
The changes to gas options and provider instantiation are consistent with updates in other files, following the ethers v6 patterns.
Also applies to: 103-103, 106-106
84-84: Verify mock deployment parameters
The mock deployment is currently commented out. Ensure all parameters are properly updated when uncommenting.
✅ Verification successful
Mock deployment parameters are consistent across files
The mock deployment parameters in 01-gnosis-to-arb-outbox.ts follow the same pattern as other files in the codebase, with identical parameter ordering and usage of ethers.ZeroAddress. All parameters are properly aligned with the contract's requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any other mock deployments in the codebase for reference
rg -l "VeaOutboxGnosisMock" --type ts
Length of output: 215
Script:
#!/bin/bash
# Check the mock deployment parameters across different files
rg "VeaOutboxGnosisMock" -A 10 -B 2 --type ts
Length of output: 3290
contracts/deploy/01-outbox/01-arb-sepolia-to-sepolia-outbox.ts (3)
29-29: LGTM: Updated zero address constant
Correctly replaced deprecated ethers.constants.AddressZero with ethers.ZeroAddress for arbitrumBridge parameter.
100-100: LGTM: Updated provider instantiation
Correctly updated to use new JsonRpcProvider syntax from ethers v6.
1-1: Verify parseUnits usage
The file imports parseUnits but it's not used in the code. Consider removing if unused.
relayer-cli/src/utils/relay.ts (3)
17-20: LGTM! Improved GraphQL query formatting
The query formatting changes enhance readability while maintaining the same functionality.
130-141: LGTM! Enhanced GraphQL query structure
The query formatting improvements make the code more maintainable and easier to read.
144-147: LGTM! Added proper error handling
The error handling in getNonceFrom is appropriate as it provides a meaningful return value (undefined) when the operation fails.
contracts/package.json (2)
86-94: Verify hardhat plugin compatibility
The updates to hardhat and its plugins are significant:
- hardhat: 2.22.15
- hardhat-deploy: 0.14.0
- hardhat-deploy-ethers: 0.4.2
- hardhat-gas-reporter: 2.2.1
- hardhat-tracer: 3.1.0
Please ensure all hardhat plugins are compatible with the specified hardhat version.
70-76: Verify compatibility between major version updates
Several major version updates have been made:
- Migration from @nomiclabs to @NomicFoundation for Hardhat ethers
- Update to ethers v6
- Update to @typechain/ethers-v6
Please ensure that:
- All dependencies are compatible with ethers v6
- Breaking changes in ethers v6 have been addressed across the codebase
✅ Verification successful
The previous results showed some usage of BigNumber.from() which is a v5 pattern. Let's gather more context about the ethers usage to better assess the migration status and potential compatibility issues.
Based on the results, I can now provide a final response. The codebase shows proper migration to ethers v6 patterns with:
- Usage of
parseUnitsandparseEtherfrom ethers v6 in deployment scripts - Some remaining
BigNumber.from()usage in validator-cli, but this is outside the contracts directory and not affected by the package.json changes - No usage of deprecated ethers v5 patterns like
ethers/lib/utilsorethers.constants - Event handling using standard patterns that work in both v5 and v6
The dependency updates are compatible and properly implemented
The codebase has been correctly migrated to use ethers v6 patterns within the contracts directory where the package.json changes are made. The remaining v5 patterns are in a separate validator-cli directory which has its own dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential incompatibilities with ethers v6
echo "Checking for ethers v5 specific imports..."
rg "ethers/lib/utils"
rg "ethers\.constants\."
rg "BigNumber\.from"
echo "Checking for potential type issues..."
rg "providers\."
Length of output: 725
Script:
#!/bin/bash
# Let's check for ethers v6 specific imports and patterns to verify migration
echo "Checking for ethers v6 specific patterns..."
rg "parseUnits|parseEther|formatUnits|formatEther"
echo "Checking for Contract instantiation patterns..."
rg "new ethers\.Contract|getContractAt"
echo "Checking test files for ethers usage..."
fd "test" --type f --exec rg "ethers\." {} \;
echo "Checking for potential breaking changes in event handling..."
rg "queryFilter|on\(.*event"
Length of output: 123329
contracts/hardhat.config.ts (1)
4-4: LGTM! Updated Hardhat ethers plugin import
The import statement has been correctly updated to use the new @NomicFoundation namespace, which is consistent with the package.json changes.
contracts/deploy/01-outbox/01-arb-to-eth-outbox.ts (2)
1-1: LGTM: Import statement updated for ethers v6
The import of parseEther directly from "ethers" follows the new pattern in ethers v6 where utilities are available from the main package.
122-122: LGTM: Provider instantiation updated for ethers v6
The provider instantiation has been correctly updated to use ethers.JsonRpcProvider instead of the v5 pattern.
contracts/deploy/01-outbox/01-arb-to-gnosis-outbox.ts (2)
45-46: LGTM: Constants updated for ethers v6
The use of ethers.ZeroAddress correctly replaces the deprecated ethers.constants.AddressZero from v5.
163-163: Verify gas price specification
The gas price has been changed to use a string representation (String(10 ** 9)). While this works, ensure this value is appropriate for the target network.
✅ Verification successful
Let me search for more context about gas prices in the codebase.
Let me check the hardhat configuration file for network-specific gas price settings.
Gas price specification is appropriate for Chiado network
The hardcoded gas price of 1 gwei (10^9 wei) is appropriate for the Chiado network (Gnosis Chain testnet) as indicated by the comment. The code explicitly mentions that "chiado rpc response underprices gas," which explains why a fixed gas price is being used instead of relying on the RPC's gas price estimation. This is a valid approach for testnet deployments where gas prices are typically stable and low.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other deployment scripts use similar gas price patterns
rg -A 1 'gasPrice:' --type ts
Length of output: 225
Script:
#!/bin/bash
# Search for gas price configurations or constants in the codebase
rg -i 'gas.?price' --type ts -A 2
# Check for hardhat config or network settings that might define gas prices
fd -e ts -e js hardhat.config
Length of output: 1124
Script:
#!/bin/bash
# Check hardhat config for network settings
cat contracts/hardhat.config.ts
Length of output: 5542
contracts/test/merkle/MerkleTree.ts (1)
38-39: LGTM: Hash function updated for ethers v6
The update from ethers.utils.keccak256 to ethers.keccak256 correctly follows the ethers v6 API changes while maintaining the same functionality.
contracts/test/integration/ArbToGnosis.ts (7)
3-3: LGTM: Import and constant updates align with ethers upgrade
The changes correctly update the imports to use the new paths and replace BigNumber with native BigInt, which provides better type safety and performance.
Also applies to: 17-17, 21-21
51-51: LGTM: Helper functions updated correctly
The changes properly migrate from deprecated ethers constants to their new equivalents:
ethers.constants.AddressZero→ethers.ZeroAddresscontract.address→contract.target
Also applies to: 65-66
113-114: LGTM: Test setup correctly uses BigInt
The WETH setup correctly uses BigInt notation for numeric operations, maintaining type safety.
175-175: Good addition of null checks for block operations
The added null checks for block operations improve type safety and prevent potential runtime errors.
Also applies to: 209-209, 222-222, 270-270, 283-283, 322-322, 335-335, 482-482, 493-493, 513-513, 523-523, 874-874, 883-883
573-575: LGTM: Withdrawal amount calculations use BigInt correctly
The withdrawal amount calculations properly use BigInt arithmetic operations.
Also applies to: 777-779
630-630: LGTM: Message relay uses contract.target properly
The message relay operations correctly use contract.target instead of deprecated address access.
Also applies to: 808-808
412-412: LGTM: Updated ethers utility function usage
The code correctly uses the new ethers utility functions:
ethers.encodeBytes32Stringethers.keccak256ethers.ZeroHash
Also applies to: 420-420, 474-474, 509-509, 649-649
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
contracts/deploy/03-routers/03-gnosis-to-arb-router.ts (3)
26-34: Add chain ID validation in getNetworkConfigWhile the function handles the mainnet/testnet distinction, it should validate the input chain ID to prevent undefined behavior.
Consider this improvement:
const getNetworkConfig = (chainId: number): NetworkConfig => { + if (!(chainId in RouterChains)) { + throw new Error(`Unsupported chain ID: ${chainId}`); + } return { outboxNetwork: chainId === RouterChains.ETHEREUM_MAINNET ? "arbitrum" : "arbitrumSepolia", inboxNetwork: chainId === RouterChains.ETHEREUM_MAINNET ? "gnosischain" : "chiado", }; };
59-71: Consider adding gas optimization optionsWhile the parallel deployment retrieval is efficient, consider adding gas optimization options for the contract deployment.
Consider adding these options:
const router = await deploy(deploymentName, { from: deployer, contract: contractBaseName, args: [arbitrumBridge, amb, veaInbox.address, veaOutbox.address, chainId], log: true, + gasLimit: 5000000, // Adjust based on contract size + gasPrice: await hre.ethers.provider.getGasPrice(), });
78-95: Enhance error logging in catch blockWhile the error handling is good, consider adding more detailed error logging to help with debugging deployment issues.
Consider this improvement:
} catch (error) { - console.error("Failed to deploy:", error); + console.error("Failed to deploy:", { + error: error instanceof Error ? error.message : String(error), + chainId, + deploymentName, + outboxNetwork, + inboxNetwork + }); throw error; }validator-cli/src/ArbToEth/watcherArbToGnosis.ts (1)
749-800: Consider implementing the profitability calculationThe commented out
calculateDisputeResolutionProfitabilityfunction contains valuable logic for determining if a challenge would be profitable. Consider implementing this to optimize the challenger's operations.Would you like me to help implement this function or create a GitHub issue to track this enhancement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
contracts/deploy/01-outbox/01-arb-sepolia-to-sepolia-outbox.ts(3 hunks)contracts/deploy/03-routers/03-gnosis-to-arb-router.ts(4 hunks)validator-cli/src/ArbToEth/watcherArbToGnosis.ts(12 hunks)validator-cli/src/utils/devnet.ts(2 hunks)validator-cli/src/utils/ethers.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/deploy/01-outbox/01-arb-sepolia-to-sepolia-outbox.ts
🧰 Additional context used
📓 Learnings (1)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (2)
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:829-1112
Timestamp: 2024-11-26T08:37:47.591Z
Learning: Refactoring the `reconstructChallengeProgress` function in `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` into smaller functions is considered unnecessary abstraction.
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:829-840
Timestamp: 2024-12-02T10:16:56.825Z
Learning: In the `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` file, avoid adding redundant error handling in functions like `reconstructChallengeProgress` when error handling is already adequately managed.
🪛 Biome (1.9.4)
validator-cli/src/ArbToEth/watcherArbToGnosis.ts
[error] 115-115: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
validator-cli/src/utils/devnet.ts
[error] 10-10: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
[error] 10-10: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
[error] 44-44: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
[error] 46-46: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
[error] 47-47: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
[error] 55-55: Don't use 'BigInt' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'bigint' instead
(lint/complexity/noBannedTypes)
🔇 Additional comments (10)
contracts/deploy/03-routers/03-gnosis-to-arb-router.ts (5)
Line range hint 3-24: LGTM! Import and constants updated correctly for ethers v6
The changes correctly implement the ethers v6 migration by:
- Using
ZeroAddressinstead ofethers.constants.AddressZero - Properly typing the constants with
as const
43-46: LGTM! Robust deployer address handling
The changes improve reliability by:
- Properly handling named accounts
- Providing a fallback to hardhat signers
- Adding helpful logging
48-55: LGTM! Validation added for unsupported chain IDs
The added validation properly addresses the previous review comment about handling undefined parameters for unsupported chain IDs.
Line range hint 99-103: LGTM! Clear deployment execution logic
The conditional execution based on network type is clear and well-structured.
109-114: LGTM! Improved skip logic with helpful logging
The changes enhance the skip logic by:
- Adding clear logging of the current chain ID
- Providing informative messages for skipped deployments
validator-cli/src/utils/ethers.ts (2)
Line range hint 13-20: Consider consolidating duplicate wallet initialization functions
The file has two similar functions getWallet and getWalletRPC that essentially do the same thing with different parameter types.
Apply this diff to consolidate the functions:
-function getWallet(privateKey: string, web3ProviderURL: string) {
- return new Wallet(privateKey, new JsonRpcProvider(web3ProviderURL));
-}
-
-function getWalletRPC(privateKey: string, rpc: JsonRpcProvider) {
- return new Wallet(privateKey, rpc);
-}
+function getWallet(privateKey: string, provider: string | JsonRpcProvider) {
+ const rpcProvider = typeof provider === 'string' ? new JsonRpcProvider(provider) : provider;
+ return new Wallet(privateKey, rpcProvider);
+}53-63: 🛠️ Refactor suggestion
Remove redundant export after consolidating wallet functions
After consolidating the wallet functions, getWalletRPC should be removed from exports.
Apply this diff:
export {
getVeaOutboxArbToEth,
- getWalletRPC,
getWallet,
getVeaOutboxArbToEthDevnet,
getVeaInboxArbToEth,
getVeaOutboxArbToGnosis,
getVeaInboxArbToGnosis,
getVeaRouterArbToGnosis,
getWETH,
getAMB,
};Likely invalid or redundant comment.
validator-cli/src/utils/devnet.ts (1)
36-37: LGTM! Correct usage of ethers v6 APIs
The code correctly uses:
- ethers.AbiCoder.defaultAbiCoder() for decoding
- Number() for safe BigInt conversions
Also applies to: 68-68, 78-78
validator-cli/src/ArbToEth/watcherArbToGnosis.ts (2)
236-236: LGTM! Correct usage of ethers v6 APIs
The code correctly uses:
- ethers.ZeroHash for zero hash checks
- ethers.ZeroAddress for zero address checks
- ethers.solidityPackedKeccak256 for hashing
Also applies to: 336-336, 651-651, 1106-1117
15-15: LGTM! Consistent type usage with ethers v6
The code correctly uses:
- 'bigint' for numeric values
- ContractTransactionResponse type from ethers
- Proper type casting for contract interactions
Also applies to: 109-110, 118-120, 130-130, 133-133, 136-136
madhurMongia
dismissed stale reviews from mani99brar and coderabbitai[bot]
via
6295b39
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
validator-cli/src/ArbToEth/watcherArbToEth.ts (1)
456-463: Consider making gas fee parameters configurableThe maxPriorityFeePerGas is hardcoded to 6667 gwei, which might not be optimal across different network conditions. Consider making this configurable through environment variables or configuration files.
- let maxPriorityFeePerGasMEV = BigInt(6667000000000); // 6667 gwei + const MAX_PRIORITY_FEE_PER_GAS = BigInt(process.env.MAX_PRIORITY_FEE_PER_GAS || "6667000000000"); // Default: 6667 gwei + let maxPriorityFeePerGasMEV = MAX_PRIORITY_FEE_PER_GAS;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
validator-cli/src/ArbToEth/watcherArbToEth.ts(12 hunks)validator-cli/src/ArbToEth/watcherArbToGnosis.ts(12 hunks)validator-cli/src/utils/devnet.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- validator-cli/src/utils/devnet.ts
- validator-cli/src/ArbToEth/watcherArbToGnosis.ts
🧰 Additional context used
📓 Learnings (1)
validator-cli/src/ArbToEth/watcherArbToEth.ts (1)
Learnt from: mani99brar
PR: kleros/vea#344
File: validator-cli/src/ArbToEth/watcherArbToEth.ts:732-745
Timestamp: 2024-11-27T04:18:05.872Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToEth.ts`, input validation inside the `hashClaim` function is unnecessary because the upper layer ensures valid input before calling it.
🔇 Additional comments (2)
validator-cli/src/ArbToEth/watcherArbToEth.ts (2)
7-7: LGTM: Correct type updates for ethers v6
The changes correctly update the transaction type from ContractTransaction to ContractTransactionResponse to align with ethers v6 API.
Also applies to: 315-315, 382-382, 484-484
181-188: LGTM: Consistent use of queryFilter for event retrieval
The changes correctly implement event querying using queryFilter across all event listeners, which is the recommended approach in ethers v6.
Also applies to: 243-247, 285-289, 354-358
|
|
||
| console.log("RouterGnosisToArb" + (chainId === 1 ? "" : "Testnet") + " deployed to: %s", router.address); | ||
| console.log(`${deploymentName} deployed to: ${router.address}`); | ||
| } catch (error) { |
There was a problem hiding this comment.
This try/catch is not needed. Just let the error pop up, and you save yourself one execution branch that you will need to test in the future
relayer-cli/src/utils/relay.ts
Outdated
| const txn = await veaOutbox.sendMessage(proof, nonce, to, data); | ||
| const receipt = await txn.wait(); | ||
| return receipt; | ||
| } catch (error) { |
There was a problem hiding this comment.
the try/catch can be removed so the code is simpler and there is one branch less to test, just ensure that the function is small and the name is easy to find so we can quickly understand what is failing from the stack trace
| return { profitable: false, estimatedProfit: constants.Zero }; | ||
| } | ||
| } | ||
| // async function calculateDisputeResolutionProfitability( |
There was a problem hiding this comment.
If the code is not executing, just remove it. If we want it in the future we can always go back in the git history.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
contracts/deploy/03-routers/03-gnosis-to-arb-router.ts (1)
26-34: Consider using constants for network namesWhile the implementation is functional, consider extracting the network names into constants for better maintainability and to avoid magic strings.
+const NETWORK_NAMES = { + MAINNET: { + outbox: "arbitrum", + inbox: "gnosischain" + }, + TESTNET: { + outbox: "arbitrumSepolia", + inbox: "chiado" + } +} as const; const getNetworkConfig = (chainId: number): NetworkConfig => ({ - outboxNetwork: chainId === RouterChains.ETHEREUM_MAINNET ? "arbitrum" : "arbitrumSepolia", - inboxNetwork: chainId === RouterChains.ETHEREUM_MAINNET ? "gnosischain" : "chiado", + outboxNetwork: chainId === RouterChains.ETHEREUM_MAINNET ? NETWORK_NAMES.MAINNET.outbox : NETWORK_NAMES.TESTNET.outbox, + inboxNetwork: chainId === RouterChains.ETHEREUM_MAINNET ? NETWORK_NAMES.MAINNET.inbox : NETWORK_NAMES.TESTNET.inbox, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
contracts/deploy/03-routers/03-gnosis-to-arb-router.ts(4 hunks)contracts/test/merkle/index.ts(2 hunks)relayer-cli/src/utils/relay.ts(3 hunks)validator-cli/src/ArbToEth/watcherArbToGnosis.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- relayer-cli/src/utils/relay.ts
- contracts/test/merkle/index.ts
- validator-cli/src/ArbToEth/watcherArbToGnosis.ts
🔇 Additional comments (6)
contracts/deploy/03-routers/03-gnosis-to-arb-router.ts (6)
3-3: LGTM: Proper usage of ethers v6 ZeroAddress
The changes correctly implement the ethers v6 update by replacing AddressZero with ZeroAddress and using proper type assertions.
Also applies to: 21-22, 24-24
43-51: Handle potential undefined parameters for unsupported chain IDs
The issue from the previous review still applies. Accessing paramsByChainId[RouterChains[chainId]] without validation could lead to runtime errors.
59-71: LGTM: Efficient local deployment implementation
The hardhat deployer function is well-structured with proper parallel deployment retrieval and informative logging.
Line range hint 94-98: LGTM: Clear deployment logic
The deployment logic clearly distinguishes between local and live environments.
104-109: LGTM: Improved skip logic with better logging
The skip logic is well-implemented with clear logging that helps understand deployment decisions.
76-90: 🛠️ Refactor suggestion
Add error handling for deployment failures
Consider adding try-catch blocks to handle potential deployment failures gracefully.
const liveDeployer = async () => {
const { outboxNetwork, inboxNetwork } = getNetworkConfig(chainId);
+ try {
const [veaOutbox, veaInbox] = await Promise.all([
hre.companionNetworks[outboxNetwork].deployments.get(`VeaOutboxGnosisToArb${suffix}`),
hre.companionNetworks[inboxNetwork].deployments.get(`VeaInboxGnosisToArb${suffix}`),
]);
const router = await deploy(deploymentName, {
from: deployer,
contract: contractBaseName,
args: [arbitrumBridge, amb, veaInbox.address, veaOutbox.address, chainId],
log: true,
});
console.log(`${deploymentName} deployed to: ${router.address}`);
+ } catch (error) {
+ console.error(`Failed to deploy ${deploymentName}:`, error);
+ throw error;
+ }
};Likely invalid or redundant comment.
madhurMongia
dismissed stale reviews from coderabbitai[bot] and mani99brar
via
13db49b
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
relayer-cli/package.json (1)
Line range hint
17-17: Update @typechain/ethers-v5 to @typechain/ethers-v6This dependency needs to be updated to align with the PR's objective of upgrading to ethers v6.
Apply this diff:
- "@typechain/ethers-v5": "^10.2.0", + "@typechain/ethers-v6": "^0.1.0",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
relayer-cli/package.json(1 hunks)validator-cli/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- validator-cli/package.json
🔇 Additional comments (1)
relayer-cli/package.json (1)
22-22: Verify compatibility with web3 v4 upgrade
The upgrade from web3 v1 to v4 is a major version jump that might introduce breaking changes. Let's verify the compatibility with other dependencies.
✅ Verification successful
Let me gather more information about the actual usage of web3 in the codebase to verify compatibility.
Let me check the specific usage of web3 in the relay.ts file to verify compatibility with v4.
Web3 v4 upgrade appears compatible with current usage
The codebase uses Web3.js in a way that's compatible with v4:
- Only uses stable APIs: Contract creation, provider initialization, and basic eth methods
- web3-batched-send v1.0.3 works with Web3 v4 as it's a wrapper that uses basic Web3 functionality
- No deprecated methods or breaking changes from v4 are being used in the implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential compatibility issues with web3 v4
# Check if web3-batched-send is compatible with web3 v4
curl -s https://registry.npmjs.org/web3-batched-send | jq '.versions | keys_unsorted[-5:]'
# Look for web3 peer dependency requirements in package.json files
rg -l '"web3":\s*"[^"]*"' | xargs cat
Length of output: 1785
Script:
#!/bin/bash
# Search for web3 usage patterns in the codebase
rg -l "web3" --type ts
# Check specific usage of Web3 class and methods
ast-grep --pattern 'new Web3($$$)'
# Look for imports of web3
rg 'import.*web3' -g '*.ts'
# Check for web3-batched-send usage
rg 'web3-batched-send' -g '*.ts'
Length of output: 838
Script:
#!/bin/bash
# Check the specific Web3 methods being used in relay.ts
cat relayer-cli/src/utils/relay.ts
Length of output: 4974
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
contracts/package.json (1)
Line range hint
70-100: Consider adding resolutions for potential dependency conflictsWith multiple major version updates, consider adding a
resolutionsfield to package.json to ensure consistent versions across the dependency tree.Add a resolutions field to handle potential conflicts:
{ "devDependencies": { // ... existing dependencies }, + "resolutions": { + "ethers": "^6.13.4", + "@openzeppelin/contracts": "^5.1.0" + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
contracts/hardhat.config.ts(1 hunks)contracts/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/hardhat.config.ts
🔇 Additional comments (3)
contracts/package.json (3)
100-100: Verify solidity-coverage configuration
The update to solidity-coverage@^0.8.13 might require adjustments to the coverage configuration.
Let's check the coverage setup:
✅ Verification successful
No coverage configuration adjustments needed
The solidity-coverage plugin is properly configured in the hardhat.config.ts file using the default settings, which are sufficient for version 0.8.13. The plugin is correctly imported and no special configuration is required for this version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify coverage configuration
# Check for .solcover.js config file
fd -t f ".solcover.js"
# Check for coverage-specific excludes in config
rg -l "coverageExclude|solidity-coverage"
Length of output: 127
Script:
#!/bin/bash
# Check hardhat.config.ts for coverage configuration
cat contracts/hardhat.config.ts
# Check if there are any coverage-specific settings in package.json
cat contracts/package.json | jq '.scripts | select(. | tostring | contains("coverage"))'
Length of output: 5529
74-74: Review OpenZeppelin contracts upgrade impact
The upgrade to @openzeppelin/contracts@^5.1.0 is a major version bump that may include breaking changes.
Let's check for potential impacts:
70-71: Verify Hardhat ecosystem compatibility
The updates to Hardhat and its ecosystem packages are substantial:
@nomicfoundation/hardhat-chai-matchers→ ^2.0.8@nomicfoundation/hardhat-ethers→ ^3.0.8 (replacing nomiclabs)hardhat→ 2.22.15- Various hardhat plugins updated
These updates require verification of the hardhat configuration and plugin usage.
Let's check for potential compatibility issues:
Also applies to: 87-93
✅ Verification successful
Let me check the hardhat configuration file to understand how the plugins are being used.
Let me check the test files to verify the ethers usage patterns and imports.
No compatibility issues found with Hardhat ecosystem updates
The codebase is already using the updated Hardhat ecosystem packages correctly:
- Using
@nomicfoundation/hardhat-ethersin hardhat.config.ts - All imports are using the new package paths
- No deprecated
@nomiclabsimports found - Proper usage of ethers imports from both 'hardhat' and 'ethers' packages
- Correct usage of HardhatUserConfig and other Hardhat types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Hardhat configuration and plugin usage
# Check hardhat config for deprecated plugin usage
rg -l "@nomiclabs/hardhat-ethers"
# Check for old plugin imports in test files
rg -l "require\(['\"]@nomiclabs"
# Check for hardhat-specific type imports that might need updating
rg -l "HardhatRuntimeEnvironment|HardhatUserConfig"
Length of output: 1068
Script:
#!/bin/bash
# Check hardhat.config.ts content
cat contracts/hardhat.config.ts
# Check for ethers imports/usage patterns
rg -l "ethers\." contracts/
Length of output: 6472
Script:
#!/bin/bash
# Check test files for ethers imports and usage
rg -A 5 "import.*ethers" contracts/test/
rg -A 5 "require.*ethers" contracts/test/
# Check deploy files for ethers imports and usage
rg -A 5 "import.*ethers" contracts/deploy/
rg -A 5 "require.*ethers" contracts/deploy/
Length of output: 11213
PR-Codex overview
This PR focuses on upgrading dependencies, specifically transitioning from
ethersversion 5 to version 6, and making related code adjustments across various files.Detailed summary
web3from1.10.4to4.16.0.@typechain/ethers-v5to@typechain/ethers-v6.ethersto reflect v6 changes.String()for values.ethersAPI.Summary by CodeRabbit
Release Notes
New Features
etherslibrary for improved clarity and consistency across deployment scripts.Bug Fixes
etherslibrary standards.Documentation
Chores
package.jsonto the latest versions, ensuring compatibility and access to new features.