chore: gateway addresses #414
Conversation
fadeev
requested review from
andresaiello,
brewmaster012,
lumtis,
CharlieMc0,
skosito and
fbac
as code owners
📝 WalkthroughWalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #414 +/- ##
=======================================
Coverage 84.27% 84.27%
=======================================
Files 8 8
Lines 388 388
Branches 123 123
=======================================
Hits 327 327
Misses 61 61 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
v2/lib/types.ts (1)
3-3: Consider using consistent naming conventions.The new types use snake_case (
custody_implementation,gateway_implementation) while existing types use camelCase (erc20Custody,uniswapV2Factory). Consider maintaining consistency:-export type ParamType = "connector" | "erc20Custody" | "tss" | "uniswapV2Factory" | "uniswapV2Router02" | "uniswapV3Factory" | "uniswapV3Router" | "weth9" | "zetaToken" | "fungibleModule" | "systemContract" | "zrc20" | "custody" | "custody_implementation" | "gateway" | "gateway_implementation"; +export type ParamType = "connector" | "erc20Custody" | "tss" | "uniswapV2Factory" | "uniswapV2Router02" | "uniswapV3Factory" | "uniswapV3Router" | "weth9" | "zetaToken" | "fungibleModule" | "systemContract" | "zrc20" | "custody" | "custodyImplementation" | "gateway" | "gatewayImplementation";v2/tasks/addresses.testnet.json (1)
8-119: Consider adding documentation for address deployment strategy.Given the pattern of address reuse across different testnets, it would be helpful to document:
- The rationale behind using the same addresses across different testnets
- The deployment strategy for these contracts
- Any security considerations for testnet deployments
Would you like me to help create a documentation template for this?
v2/tasks/addresses.ts (2)
144-144: LGTM! Consider adding ethers.js version constraint.The migration to the new JsonRpcProvider syntax is correct. However, to prevent future compatibility issues, consider adding a specific version constraint for ethers.js in package.json.
- "ethers": "^6.0.0" + "ethers": "~6.8.0"
Line range hint
207-273: Consider refactoring to reduce code duplication between V2 and V3 factory fetching.The fetchFactoryV2 and fetchFactoryV3 functions share similar structure and could be consolidated into a single function to improve maintainability.
Consider this refactor:
interface RouterConfig { type: string; abi: any; version: 'v2' | 'v3'; } const fetchFactory = async (addresses: any, network: Network, config: RouterConfig) => { const routers = addresses.filter((a: any) => a.type === config.type); for (const router of routers) { const rpc = getEndpoints("evm", router.chain_name)[0]?.url; if (!rpc) { console.error(`No RPC URL found for chain ${router.chain_name}`); continue; } const provider = new ethers.JsonRpcProvider(rpc); const routerContract = new ethers.Contract(router.address, config.abi, provider); try { const wethAddress = await routerContract[config.version === 'v2' ? 'WETH' : 'WETH9'](); const factoryAddress = await routerContract.factory(); // Skip ZetaChain check only for V2 if (config.version === 'v2' && (router.chain_id === 7000 || router.chain_id === 7001)) { continue; } const wethObj = { address: wethAddress, category: "messaging", chain_id: router.chain_id, chain_name: router.chain_name, type: "weth9", }; // V3 specific deduplication if (config.version === 'v3' && !addresses.some((e: any) => isEqual(e, wethObj))) { addresses.push(wethObj); } else if (config.version === 'v2') { addresses.push(wethObj); } addresses.push({ address: factoryAddress, category: "messaging", chain_id: router.chain_id, chain_name: router.chain_name, type: `uniswap${config.version.toUpperCase()}Factory`, }); } catch (error) { console.error(`Error fetching factory for router ${config.version} ${router.address}:`, error); } } }; // Usage: await fetchFactory(addresses, network, { type: "uniswapV2Router02", abi: uniswapV2Router.abi, version: 'v2' }); await fetchFactory(addresses, network, { type: "uniswapV3Router", abi: SwapRouter.abi, version: 'v3' });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (89)
v2/data/addresses.testnet.jsonis excluded by!v2/data/**v2/pkg/address.sol/address.gois excluded by!v2/pkg/**v2/pkg/beaconproxy.sol/beaconproxy.gois excluded by!v2/pkg/**v2/pkg/core.sol/core.gois excluded by!v2/pkg/**v2/pkg/defender.sol/defender.gois excluded by!v2/pkg/**v2/pkg/defenderdeploy.sol/defenderdeploy.gois excluded by!v2/pkg/**v2/pkg/erc1967proxy.sol/erc1967proxy.gois excluded by!v2/pkg/**v2/pkg/erc1967utils.sol/erc1967utils.gois excluded by!v2/pkg/**v2/pkg/erc20custody.sol/erc20custody.gois excluded by!v2/pkg/**v2/pkg/erc20custody.t.sol/erc20custodytest.gois excluded by!v2/pkg/**v2/pkg/erc20custodyupgradetest.sol/erc20custodyupgradetest.gois excluded by!v2/pkg/**v2/pkg/errors.sol/errors.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.sol/gatewayevm.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevm.t.sol/gatewayevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmupgradetest.sol/gatewayevmupgradetest.gois excluded by!v2/pkg/**v2/pkg/gatewayevmzevm.t.sol/gatewayevmzevmtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.sol/gatewayzevm.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevminboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevm.t.sol/gatewayzevmoutboundtest.gois excluded by!v2/pkg/**v2/pkg/gatewayzevmupgradetest.sol/gatewayzevmupgradetest.gois excluded by!v2/pkg/**v2/pkg/ierc1363.sol/ierc1363.gois excluded by!v2/pkg/**v2/pkg/ierc20permit.sol/ierc20permit.gois excluded by!v2/pkg/**v2/pkg/interfaces/ierc165.sol/ierc165.gois excluded by!v2/pkg/**v2/pkg/interfaces/ierc20.sol/ierc20.gois excluded by!v2/pkg/**v2/pkg/math.sol/math.gois excluded by!v2/pkg/**v2/pkg/panic.sol/panic.gois excluded by!v2/pkg/**v2/pkg/proxyadmin.sol/proxyadmin.gois excluded by!v2/pkg/**v2/pkg/receiverevm.sol/receiverevm.gois excluded by!v2/pkg/**v2/pkg/safecast.sol/safecast.gois excluded by!v2/pkg/**v2/pkg/safeerc20.sol/safeerc20.gois excluded by!v2/pkg/**v2/pkg/senderzevm.sol/senderzevm.gois excluded by!v2/pkg/**v2/pkg/signedmath.sol/signedmath.gois excluded by!v2/pkg/**v2/pkg/storageslot.sol/storageslot.gois excluded by!v2/pkg/**v2/pkg/strings.sol/strings.gois excluded by!v2/pkg/**v2/pkg/testerc20.sol/testerc20.gois excluded by!v2/pkg/**v2/pkg/testuniversalcontract.sol/testuniversalcontract.gois excluded by!v2/pkg/**v2/pkg/transparentupgradeableproxy.sol/itransparentupgradeableproxy.gois excluded by!v2/pkg/**v2/pkg/transparentupgradeableproxy.sol/transparentupgradeableproxy.gois excluded by!v2/pkg/**v2/pkg/upgrades.sol/unsafeupgrades.gois excluded by!v2/pkg/**v2/pkg/upgrades.sol/upgrades.gois excluded by!v2/pkg/**v2/pkg/uupsupgradeable.sol/uupsupgradeable.gois excluded by!v2/pkg/**v2/pkg/zeta.non-eth.sol/zetanoneth.gois excluded by!v2/pkg/**v2/pkg/zetaconnectorbase.sol/zetaconnectorbase.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.sol/zetaconnectornative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornative.t.sol/zetaconnectornativetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornativeupgradetest.sol/zetaconnectornativeupgradetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.sol/zetaconnectornonnative.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnative.t.sol/zetaconnectornonnativetest.gois excluded by!v2/pkg/**v2/pkg/zetaconnectornonnativeupgradetest.sol/zetaconnectornonnativeupgradetest.gois excluded by!v2/pkg/**v2/pkg/zrc20.t.sol/zrc20test.gois excluded by!v2/pkg/**v2/types/ERC1967Utils.tsis excluded by!v2/types/**v2/types/Errors.sol/Errors.tsis excluded by!v2/types/**v2/types/Errors.sol/index.tsis excluded by!v2/types/**v2/types/IERC1363.tsis excluded by!v2/types/**v2/types/SafeCast.tsis excluded by!v2/types/**v2/types/TransparentUpgradeableProxy.sol/ITransparentUpgradeableProxy.tsis excluded by!v2/types/**v2/types/factories/Address__factory.tsis excluded by!v2/types/**v2/types/factories/BeaconProxy__factory.tsis excluded by!v2/types/**v2/types/factories/ERC1967Proxy__factory.tsis excluded by!v2/types/**v2/types/factories/ERC1967Utils__factory.tsis excluded by!v2/types/**v2/types/factories/ERC20CustodyUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/ERC20Custody__factory.tsis excluded by!v2/types/**v2/types/factories/Errors.sol/Errors__factory.tsis excluded by!v2/types/**v2/types/factories/Errors.sol/index.tsis excluded by!v2/types/**v2/types/factories/GatewayEVMUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayEVM__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayZEVMUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/GatewayZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/IERC1363__factory.tsis excluded by!v2/types/**v2/types/factories/ProxyAdmin__factory.tsis excluded by!v2/types/**v2/types/factories/ReceiverEVM__factory.tsis excluded by!v2/types/**v2/types/factories/SafeCast__factory.tsis excluded by!v2/types/**v2/types/factories/SafeERC20__factory.tsis excluded by!v2/types/**v2/types/factories/SenderZEVM__factory.tsis excluded by!v2/types/**v2/types/factories/Strings__factory.tsis excluded by!v2/types/**v2/types/factories/TestERC20__factory.tsis excluded by!v2/types/**v2/types/factories/TestUniversalContract__factory.tsis excluded by!v2/types/**v2/types/factories/TransparentUpgradeableProxy.sol/ITransparentUpgradeableProxy__factory.tsis excluded by!v2/types/**v2/types/factories/TransparentUpgradeableProxy.sol/TransparentUpgradeableProxy__factory.tsis excluded by!v2/types/**v2/types/factories/UUPSUpgradeable__factory.tsis excluded by!v2/types/**v2/types/factories/Zeta.non-eth.sol/ZetaNonEth__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorBase__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNativeUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNative__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNonNativeUpgradeTest__factory.tsis excluded by!v2/types/**v2/types/factories/ZetaConnectorNonNative__factory.tsis excluded by!v2/types/**v2/types/factories/index.tsis excluded by!v2/types/**v2/types/index.tsis excluded by!v2/types/**
📒 Files selected for processing (3)
- v2/lib/types.ts (1 hunks)
- v2/tasks/addresses.testnet.json (1 hunks)
- v2/tasks/addresses.ts (3 hunks)
🔇 Additional comments (5)
v2/lib/types.ts (1)
3-3: LGTM! The new types support upgradeable smart contract patterns.The addition of implementation-specific types (
custody_implementation,gateway_implementation) alongside their proxy counterparts (custody,gateway) follows good smart contract development patterns for upgradeability.v2/tasks/addresses.testnet.json (4)
10-10: Verify the reuse of gateway implementation address across chains.The same gateway implementation address
0x4f8b2c28a670ae47049fae0a114af0204752df0dis being used across all chains. While this could be intentional for testnet environments, it's worth verifying if this is the expected behavior.#!/bin/bash # Description: Verify if the gateway implementation address is consistently used across different environments # Expected: The address should be different in production/mainnet configurations # Check if this address appears in any production/mainnet config files rg -l "0x4f8b2c28a670ae47049fae0a114af0204752df0d" | grep -v "testnet"Also applies to: 17-17, 38-38, 66-66, 94-94
31-35: Verify custody address consistency across chains.The custody address
0xd80be3710f08d280f51115e072e5d2a778946cd7is reused across all chains. Similar to the gateway implementation, please verify if this is intentional for testnet environments.#!/bin/bash # Description: Check if the custody address is used differently in other environments # Expected: The address should be different in production/mainnet configurations # Search for custody address references rg -l "0xd80be3710f08d280f51115e072e5d2a778946cd7" | grep -v "testnet"Also applies to: 59-63, 87-91, 115-119
10-119: Validate Ethereum addresses checksum.All Ethereum addresses should follow the EIP-55 mixed-case checksum format. Let's verify the checksum for these addresses:
- 0x4f8b2c28a670ae47049fae0a114af0204752df0d
- 0x0c487a766110c85d301d96e33579c5b317fa4995
- 0x61ed1f2d85f95182740fa1f6618d4542fedb7468
- 0xd80be3710f08d280f51115e072e5d2a778946cd7
#!/bin/bash # Description: Verify if addresses follow EIP-55 checksum format # Expected: Addresses should be properly checksummed # Extract and validate Ethereum addresses rg -o "0x[a-fA-F0-9]{40}" | sort -u | while read -r addr; do echo "Address: $addr" # Note: In a real environment, we would use web3/ethereum libraries to validate checksums done
12-13: Verify chain IDs against public chain documentation.Please verify the chain IDs against official documentation:
- Base Sepolia: 84532
- Sepolia: 11155111
- BSC Testnet: 97
- Amoy Testnet: 80002
Also applies to: 19-20, 40-41, 68-69, 96-97
v2/tasks/addresses.ts
Outdated
| @@ -241,7 +241,7 @@ const fetchFactoryV3 = async (addresses: any, network: Network) => { | |||
|
|
|||
| for (const router of routers) { | |||
| const rpc = getEndpoints("evm", router.chain_name)[0]?.url; | |||
| const provider = new ethers.providers.JsonRpcProvider(rpc); | |||
| const provider = new ethers.JsonRpcProvider(rpc); | |||
There was a problem hiding this comment.
Add RPC URL validation to prevent runtime errors.
Similar to fetchFactoryV2, the RPC URL needs validation before use.
Add validation before creating the provider:
const rpc = getEndpoints("evm", router.chain_name)[0]?.url;
+ if (!rpc) {
+ console.error(`No RPC URL found for chain ${router.chain_name}`);
+ continue;
+ }
const provider = new ethers.JsonRpcProvider(rpc);📝 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 provider = new ethers.JsonRpcProvider(rpc); | |
| const rpc = getEndpoints("evm", router.chain_name)[0]?.url; | |
| if (!rpc) { | |
| console.error(`No RPC URL found for chain ${router.chain_name}`); | |
| continue; | |
| } | |
| const provider = new ethers.JsonRpcProvider(rpc); |
v2/tasks/addresses.ts
Outdated
| @@ -205,7 +205,7 @@ | |||
|
|
|||
| for (const router of routers) { | |||
| const rpc = getEndpoints("evm", router.chain_name)[0]?.url; | |||
| const provider = new ethers.providers.JsonRpcProvider(rpc); | |||
| const provider = new ethers.JsonRpcProvider(rpc); | |||
There was a problem hiding this comment.
Add RPC URL validation to prevent runtime errors.
The RPC URL obtained from getEndpoints is used without validation. A null/undefined RPC URL would cause a runtime error.
Add validation before creating the provider:
const rpc = getEndpoints("evm", router.chain_name)[0]?.url;
+ if (!rpc) {
+ console.error(`No RPC URL found for chain ${router.chain_name}`);
+ continue;
+ }
const provider = new ethers.JsonRpcProvider(rpc);Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
v2/tasks/addresses.testnet.json (1)
10-63: Ensure addresses are in checksum format.The Ethereum addresses should be in checksum format for better security. Consider using web3.js or ethers.js to convert these addresses to their checksum format.
Example using web3.js:
const Web3 = require('web3'); const web3 = new Web3(); const gatewayAddress = web3.utils.toChecksumAddress('0x0c487a766110c85d301d96e33579c5b317fa4995'); const custodyAddress = web3.utils.toChecksumAddress('0xd80be3710f08d280f51115e072e5d2a778946cd7');
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
v2/data/addresses.testnet.jsonis excluded by!v2/data/**v2/docs/src/README.mdis excluded by!v2/docs/**v2/docs/src/index.mdis excluded by!v2/docs/**
📒 Files selected for processing (2)
- v2/lib/types.ts (1 hunks)
- v2/tasks/addresses.testnet.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/lib/types.ts
🔇 Additional comments (3)
v2/tasks/addresses.testnet.json (3)
10-14: Verify the reuse of gateway address across different chains.The same gateway address
0x0c487a766110c85d301d96e33579c5b317fa4995is being used across all EVM chains (base_sepolia, sepolia_testnet, bsc_testnet, and amoy_testnet). This is highly unusual and could potentially be risky. Please verify if this is intentional.#!/bin/bash # Description: Check if this gateway address is referenced in any documentation or deployment scripts # to validate its intentional reuse across chains echo "Searching for gateway address references..." rg -i "0x0c487a766110c85d301d96e33579c5b317fa4995" echo -e "\nSearching for deployment scripts..." fd -e js -e ts -e sol "deploy|migration" --exec grep -l "gateway"Also applies to: 24-28, 38-42, 52-56
12-13: Verify chain IDs for the test networks.Please confirm the chain IDs are correct:
- base_sepolia: 84532
- sepolia_testnet: 11155111
- bsc_testnet: 97
- amoy_testnet: 80002
#!/bin/bash # Description: Search for chain ID references in the codebase to verify consistency echo "Searching for chain ID references..." rg "chain_id.*84532|chain_id.*11155111|chain_id.*97|chain_id.*80002" echo -e "\nSearching for chain configuration files..." fd -e json -e ts -e js "chain|network" --exec grep -l "chain_id"Also applies to: 26-27, 40-41, 54-55
17-21: Verify the reuse of custody address across different chains.Similarly, the custody address
0xd80be3710f08d280f51115e072e5d2a778946cd7is reused across all EVM chains. Please confirm if this is the intended configuration.Also applies to: 31-35, 45-49, 59-63
✅ Verification successful
The previous search didn't show us the actual custody addresses. Let's examine the content of these JSON files to verify if the custody address is indeed reused across different chains.
The custody address is intentionally reused across EVM chains
The verification confirms that the custody address
0xd80be3710f08d280f51115e072e5d2a778946cd7is consistently used across all EVM chains in the testnet configuration:
- Base Sepolia (chain_id: 84532)
- Sepolia Testnet (chain_id: 11155111)
- BSC Testnet (chain_id: 97)
- Amoy Testnet (chain_id: 80002)
This appears to be an intentional design choice for the omnichain custody system, as it's consistently implemented across both the
v2/tasksandv2/dataconfigurations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if this custody address is referenced in any documentation or deployment scripts # to validate its intentional reuse across chains echo "Searching for custody address references..." rg -i "0xd80be3710f08d280f51115e072e5d2a778946cd7" echo -e "\nSearching for deployment scripts..." fd -e js -e ts -e sol "deploy|migration" --exec grep -l "custody"Length of output: 5046
Script:
#!/bin/bash # Let's check the content of the addresses.testnet.json files to verify the custody addresses echo "Checking v2/tasks/addresses.testnet.json..." cat v2/tasks/addresses.testnet.json echo -e "\nChecking v2/data/addresses.testnet.json..." cat v2/data/addresses.testnet.jsonLength of output: 11927
I've removed implementation addresses. |
Added them manually since there is no way to fetch them from anywhere.
Summary by CodeRabbit
New Features
ParamTypedefinitions to include new values:"custody"and"gateway".Bug Fixes