chore: update Base & Polygon ERC-20 custody addresses #430
Conversation
fadeev
requested review from
andresaiello,
brewmaster012,
lumtis,
CharlieMc0,
skosito and
fbac
as code owners
📝 WalkthroughWalkthroughThe changes in this pull request involve updates to two exported type declarations in the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@fadeev I think you need to upgrade Foundry with |
fadeev
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #430 +/- ##
=======================================
Coverage 84.06% 84.06%
=======================================
Files 8 8
Lines 389 389
Branches 124 124
=======================================
Hits 327 327
Misses 62 62 ☔ View full report in Codecov by Sentry. |
Done! |
|
@fadeev maybe let's update Ethereum and BSC in this same PR at this point? Let's get it merged today |
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
v2/data/addresses.mainnet.jsonis excluded by!v2/data/**v2/data/addresses.testnet.jsonis excluded by!v2/data/**
📒 Files selected for processing (1)
v2/lib/types.ts(1 hunks)
🔇 Additional comments (1)
v2/lib/types.ts (1)
1-2: Verify corresponding updates in configuration files
Since these type changes are part of custody address updates, ensure that any related configuration files (e.g., network configs, deployment scripts) are updated to handle the new "sBTC" symbol and "empty" chain name.
✅ Verification successful
Type changes are properly aligned with configuration
The new symbols "sBTC" and chain name "empty" are correctly reflected in the testnet configuration file (v2/data/addresses.testnet.json):
- "sBTC" is defined as a ZRC20 token with corresponding configuration
- "empty" chain name is used for chain_id 18333 with TSS configuration
The changes are consistent with the custody address updates and properly configured in the system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related configuration files that might need updates
# Search for configuration files with network/chain definitions
echo "Searching for network configuration files:"
fd -e json -e yaml -e toml | xargs rg -l "chain|network|custody"
# Search for deployment scripts
echo -e "\nSearching for deployment scripts:"
fd -e ts -e js | xargs rg -l "deploy.*custody"
Length of output: 5452
Script:
#!/bin/bash
# Let's check the content of address configuration files for any references to the new symbols
echo "Checking mainnet addresses configuration:"
cat v2/data/addresses.mainnet.json
echo -e "\nChecking testnet addresses configuration:"
cat v2/data/addresses.testnet.json
# Let's also check if these new types are already being used in the codebase
echo -e "\nChecking usage of new symbols:"
rg "sBTC|empty" --type ts
Length of output: 21717
| @@ -1,4 +1,4 @@ | |||
| export type ParamSymbol = "USDC.BSC" | "USDC.ETH" | "BTC.BTC" | "ETH.BASE" | "PEPE.ETH" | "BNB.BSC" | "SHIB.ETH" | "USDT.ETH" | "USDT.BSC" | "USDC.BASE" | "POL.POLYGON" | "DAI.ETH" | "ULTI.BSC" | "ETH.ETH" | "USDT.POL" | "ULTI.ETH" | "USDC.POL" | "sETH.SEPOLIA" | "ETH.BASESEPOLIA" | "tBTC" | "MATIC.AMOY" | "USDC" | "SOL.SOLANA" | "USDC.SEPOLIA" | "tBNB"; | |||
| export type ParamChainName = "eth_mainnet" | "bsc_mainnet" | "polygon_mainnet" | "zeta_mainnet" | "btc_mainnet" | "base_mainnet" | "bsc_testnet" | "solana_devnet" | "zeta_testnet" | "btc_testnet" | "amoy_testnet" | "base_sepolia" | "sepolia_testnet"; | |||
| export type ParamSymbol = "USDC.BSC" | "USDC.ETH" | "BTC.BTC" | "ETH.BASE" | "PEPE.ETH" | "BNB.BSC" | "SHIB.ETH" | "USDT.ETH" | "USDT.BSC" | "USDC.BASE" | "POL.POLYGON" | "DAI.ETH" | "ULTI.BSC" | "ETH.ETH" | "USDT.POL" | "ULTI.ETH" | "USDC.POL" | "sETH.SEPOLIA" | "ETH.BASESEPOLIA" | "tBTC" | "MATIC.AMOY" | "USDC" | "SOL.SOLANA" | "USDC.SEPOLIA" | "tBNB" | "sBTC"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Add network suffix to "sBTC" for consistency
The codebase follows a consistent pattern where test tokens (prefixed with 's' or 't') include their network suffix (e.g., "sETH.SEPOLIA"). The newly added "sBTC" lacks this network designation, breaking this established pattern. This inconsistency could lead to confusion or potential issues in network-specific handling.
- Consider updating "sBTC" to include its network, e.g., "sBTC.SEPOLIA" if it's meant for the Sepolia network
- Other test tokens in the codebase: "sETH.SEPOLIA", "tBTC", "tBNB", "tMATIC" all maintain consistent naming
🔗 Analysis chain
Verify the naming pattern for "sBTC"
The newly added "sBTC" symbol deviates from the established pattern where tokens include their network (e.g., "USDC.BSC", "sETH.SEPOLIA"). While this might be intentional, we should verify its usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how "sBTC" is used in the codebase and verify if this naming is consistent
# Search for any references to sBTC in the codebase
echo "Searching for sBTC usage:"
rg "sBTC" --type ts
# Search for similar test token patterns to compare
echo -e "\nSearching for similar test token patterns:"
rg "s[A-Z]+\.[A-Z]+" --type ts
Length of output: 1628
| @@ -3,2 +1,4 @@ | |||
| export type ParamSymbol = "USDC.BSC" | "USDC.ETH" | "BTC.BTC" | "ETH.BASE" | "PEPE.ETH" | "BNB.BSC" | "SHIB.ETH" | "USDT.ETH" | "USDT.BSC" | "USDC.BASE" | "POL.POLYGON" | "DAI.ETH" | "ULTI.BSC" | "ETH.ETH" | "USDT.POL" | "ULTI.ETH" | "USDC.POL" | "sETH.SEPOLIA" | "ETH.BASESEPOLIA" | "tBTC" | "MATIC.AMOY" | "USDC" | "SOL.SOLANA" | "USDC.SEPOLIA" | "tBNB" | "sBTC"; | |||
| export type ParamChainName = "eth_mainnet" | "bsc_mainnet" | "polygon_mainnet" | "zeta_mainnet" | "btc_mainnet" | "base_mainnet" | "bsc_testnet" | "solana_devnet" | "zeta_testnet" | "btc_testnet" | "empty" | "amoy_testnet" | "base_sepolia" | "sepolia_testnet"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the implementation of getAddress and its usage, I can now provide a final response:
The "empty" chain type could lead to runtime errors
The getAddress function directly uses the network parameter to query addresses from a network list by matching chain_name. Using "empty" as a chain name will always result in undefined address since there's no corresponding network configuration with chain_name === "empty". This could lead to runtime errors when the function is called.
- The
getAddressimplementation inv2/lib/addresses.tsuseschain_name === networkfor lookups - No special handling exists for the "empty" case
- This will cause
addressto be undefined, leading to potential runtime errors
Consider these alternatives:
- Use
undefined | nullfor uninitialized states instead of "empty" - Add proper handling in
getAddressif "empty" is a valid special case - Use a discriminated union if this represents a distinct state:
type ParamChainName = NetworkChain | { type: "uninitialized" }
🔗 Analysis chain
Consider a more type-safe alternative to "empty"
The addition of "empty" to ParamChainName deviates from the established network naming pattern and might introduce potential type-safety issues. Consider these alternatives:
- Use a separate type union with undefined/null if this represents an uninitialized state
- Create a discriminated union if this represents a special case
Example:
type NetworkChainName = "eth_mainnet" | "bsc_mainnet" | /* ... */;
type ParamChainName = NetworkChainName | { type: "empty" };Let me gather more context about how this type is used in the codebase.
Let me search for the actual usage of this type in function calls to understand how "empty" is being used.
Let me check one more thing - looking at how this type is used in the codebase, particularly in the getAddress function implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how "empty" chain name is used to understand its purpose
# Search for usage of empty chain name
echo "Searching for empty chain usage:"
rg '"empty"' --type ts -C 3
# Look for similar patterns that might indicate uninitialized states
echo -e "\nSearching for similar initialization patterns:"
ast-grep --pattern 'type $_ = $_ | "empty" | $_'
Length of output: 1198
Script:
#!/bin/bash
# Search for ParamChainName usage to understand the context
echo "Searching for ParamChainName usage:"
rg "ParamChainName" --type ts -C 4
# Search for any function or variable that handles chain initialization
echo -e "\nSearching for chain initialization patterns:"
rg "chain.*(?:init|empty)" --type ts -C 4
Length of output: 5068
Script:
#!/bin/bash
# Search for getAddress function calls to see how the network parameter is used
echo "Searching for getAddress function calls:"
ast-grep --pattern 'getAddress($_, $network, $_)'
# Search for any string literals containing "empty" in typescript files
echo -e "\nSearching for 'empty' string literals in ts files:"
rg '"empty"' --type ts -A 2 -B 2
# Look for test files that might explain the usage
echo -e "\nSearching for test files containing 'empty':"
fd -e test.ts -e spec.ts -x rg -l "empty" {}
Length of output: 1392
Script:
#!/bin/bash
# Look at the implementation of getAddress to see how it handles the network parameter
echo "Searching for getAddress implementation:"
rg -A 15 "export const getAddress = \(.*\)" --type ts
# Check if there are any other files that might use "empty" chain
echo -e "\nSearching for files importing ParamChainName:"
rg "import.*ParamChainName" --type ts
Length of output: 3542
Updating ERC-20 custody addresses following the migration.
Summary by CodeRabbit
"sBTC"."empty"to the list.