chore: remap chains #821
chore: remap chains #821
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughVersion bump to 4.3.9 with viem upgrade to ^2.38.3 and pnpm package manager specification. Refactors chain configuration from array to mapping-based structure with defineChain calls. Moves token slots logic from imported function to private method in LayerZeroGatewayClient. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @slavastartsev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request streamlines the SDK's chain configuration by transitioning from a simple array to a more structured mapping object, which enhances flexibility for defining and managing supported blockchain networks. This refactoring includes specific customization for the Ethereum mainnet's RPC endpoint and is accompanied by updates to core dependencies to leverage the latest improvements and maintain compatibility. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates dependencies, including viem, and refactors the chain configuration management in sdk/src/gateway/utils/common.ts. The change from an array of supported chains to a mapping object is a good improvement, allowing for custom chain names and configurations, such as a dedicated RPC for Ethereum. My feedback includes a couple of suggestions to remove redundant code for improved clarity and maintainability.
| const chainNameToChainIdMapping = Object.entries(supportedChainsMapping).reduce( | ||
| (acc, [name, chain]) => { | ||
| acc[name.toLowerCase()] = chain.id; | ||
| return acc; | ||
| }, | ||
| {} as Record<ViemChain['name'], ViemChain['id']> |
There was a problem hiding this comment.
The keys of supportedChainsMapping are already in lowercase, so the call to .toLowerCase() on the name is redundant. You can simplify this by using name directly. Additionally, the type assertion can be made more accurate to reflect that the keys are strings, not necessarily ViemChain['name'].
| const chainNameToChainIdMapping = Object.entries(supportedChainsMapping).reduce( | |
| (acc, [name, chain]) => { | |
| acc[name.toLowerCase()] = chain.id; | |
| return acc; | |
| }, | |
| {} as Record<ViemChain['name'], ViemChain['id']> | |
| const chainNameToChainIdMapping = Object.entries(supportedChainsMapping).reduce( | |
| (acc, [name, chain]) => { | |
| acc[name] = chain.id; | |
| return acc; | |
| }, | |
| {} as Record<string, ViemChain['id']> | |
| ); |
| if (!chainId) { | ||
| throw new Error( | ||
| `Chain id for "${chainName}" not found. Allowed values ${supportedChains.map((chain) => chain.name)}` | ||
| `Chain id for "${chainName}" not found. Allowed values ${Object.keys(supportedChainsMapping).map((chainName) => chainName.toLocaleLowerCase())}` |
There was a problem hiding this comment.
The keys of supportedChainsMapping are already in lowercase, so the call to .toLocaleLowerCase() is redundant here. You can simplify the error message generation by removing it.
| `Chain id for "${chainName}" not found. Allowed values ${Object.keys(supportedChainsMapping).map((chainName) => chainName.toLocaleLowerCase())}` | |
| `Chain id for "${chainName}" not found. Allowed values ${Object.keys(supportedChainsMapping)}` |
![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
🧹 Nitpick comments (3)
sdk/src/gateway/utils/common.ts (3)
171-177: Type the name→id map to reflect lowercase keys; simpler construction.Use Lowercase and fromEntries to better model runtime keys.
-const chainNameToChainIdMapping = Object.entries(supportedChainsMapping).reduce( - (acc, [name, chain]) => { - acc[name.toLowerCase()] = chain.id; - return acc; - }, - {} as Record<ViemChain['name'], ViemChain['id']> -); +const chainNameToChainIdMapping = Object.fromEntries( + Object.entries(supportedChainsMapping).map(([name, chain]) => [name.toLowerCase(), chain.id] as const), +) as Record<Lowercase<keyof typeof supportedChainsMapping>, ViemChain['id']>;
183-183: Unify casing and improve error readability.Use toLowerCase (not locale‑aware) consistently and join for readability; also avoid shadowing chainName in the callback param.
-`Chain id for "${chainName}" not found. Allowed values ${Object.keys(supportedChainsMapping).map((chainName) => chainName.toLocaleLowerCase())}` +`Chain id for "${chainName}" not found. Allowed values ${Object.keys(supportedChainsMapping).map((n) => n.toLowerCase()).join(', ')}`
193-193: Join allowed IDs for clearer error output.Current template prints a raw array. Join for readability.
-`Chain id for "${chainId}" not found. Allowed values ${Object.values(supportedChainsMapping).map((chain) => chain.id)}` +`Chain id for "${chainId}" not found. Allowed values ${Object.values(supportedChainsMapping).map((c) => c.id).join(', ')}`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
sdk/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
sdk/package.json(2 hunks)sdk/src/gateway/utils/common.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
sdk/src/gateway/utils/common.ts (1)
sdk/src/gateway/client.ts (1)
chainId(181-183)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (4)
sdk/package.json (2)
3-3: Version bump looks good; remember release hygiene.Add a CHANGELOG entry and tag 4.3.9 on merge. Also confirm no breaking surface changes slipped in with the chain rename (see common.ts comment).
54-54: Viem upgrade to ^2.38.3 — run compat checks.Looks fine. Please run the SDK tests and a quick smoke against a forked mainnet to catch any subtle client defaults (e.g., experimental_* flags, multicall tweaks). Based on learnings.
sdk/src/gateway/utils/common.ts (2)
5-5: Import of defineChain — OK.No issues with the new import.
163-169: ID→config map construction — OK.Simple and clear; no order assumptions.
| const supportedChainsMapping = { | ||
| bob, | ||
| mainnet, | ||
| ethereum: defineChain({ | ||
| ...mainnet, | ||
| rpcUrls: { | ||
| default: { | ||
| http: ['https://ethereum-rpc.publicnode.com'], | ||
| }, | ||
| }, | ||
| }), | ||
| sonic, | ||
| bsc, | ||
| unichain, | ||
| berachain, | ||
| bera: berachain, | ||
| sei, | ||
| avalanche, | ||
| base, | ||
| soneium, | ||
| optimism, | ||
| ] as const; | ||
| } as const; |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid breaking rename (berachain→bera) and preserve mainnet RPC fallbacks.
- Renaming the key breaks callers using "berachain". Keep an alias to avoid a breaking change in a patch release.
- Your ethereum override discards mainnet’s built-in public/default RPCs (and ws), reducing resiliency. Merge instead of replace.
Apply this diff:
-const supportedChainsMapping = {
- bob,
- ethereum: defineChain({
- ...mainnet,
- rpcUrls: {
- default: {
- http: ['https://ethereum-rpc.publicnode.com'],
- },
- },
- }),
- sonic,
- bsc,
- unichain,
- bera: berachain,
- sei,
- avalanche,
- base,
- soneium,
- optimism,
-} as const;
+const supportedChainsMapping = {
+ bob,
+ ethereum: defineChain({
+ ...mainnet,
+ rpcUrls: {
+ ...mainnet.rpcUrls,
+ default: {
+ ...mainnet.rpcUrls.default,
+ http: ['https://ethereum-rpc.publicnode.com', ...mainnet.rpcUrls.default.http],
+ },
+ },
+ }),
+ sonic,
+ bsc,
+ unichain,
+ // Keep old key for backward compatibility.
+ berachain: berachain,
+ bera: berachain,
+ sei,
+ avalanche,
+ base,
+ soneium,
+ optimism,
+} as const;🏁 Script executed:
#!/bin/bash
# Find external references that might still use "berachain"
rg -nP --type=ts -C2 '\b(getChainConfig|getChainIdByName)\s*\(\s*["'\'']berachain["'\'']\s*\)'Length of output: 0
I need to run additional verification scripts to better understand the scope of this change and validate the concerns raised.
Now let me search for broader usage of "berachain" in the codebase and verify the current state of the code:
🏁 Script executed:
#!/bin/bash
# Find any reference to "berachain" in TypeScript/TSX files
rg -n "berachain" --type=ts --type=tsxLength of output: 27
Let me correct the search and also check the actual file content:
🏁 Script executed:
#!/bin/bash
# Find any reference to "berachain" in TypeScript files (broader search)
rg -n "berachain" --type=ts
# Also find the supportedChainsMapping section to see current state
echo "---"
rg -n -A 25 "const supportedChainsMapping" --type=tsLength of output: 1669
Let me search for how supportedChainsMapping is accessed and check if there are references to the old key:
🏁 Script executed:
#!/bin/bash
# Find all usages of supportedChainsMapping to see how it's accessed
rg -n -B2 -A2 "supportedChainsMapping" --type=ts | head -100
echo "---"
# Search for any code accessing .berachain property or ['berachain'] key
rg -n "\.berachain\b|supportedChainsMapping\s*\[\s*['\"]berachain" --type=tsLength of output: 1707
Let me check if there are any calls to chain lookup functions and verify the mainnet structure concern:
🏁 Script executed:
#!/bin/bash
# Search for calls to functions that take chain names (getChainIdByName, getChainConfig, etc.)
rg -n "getChainIdByName|getChainConfig" --type=ts -A1 -B1
echo "---"
# Check the full common.ts file for all function definitions that use chainNameToChainIdMapping
rg -n "function\s+get|const\s+get" sdk/src/gateway/utils/common.ts -A5Length of output: 3086
Let me search for any hardcoded "berachain" strings and check the scope more broadly:
🏁 Script executed:
#!/bin/bash
# Search for any hardcoded 'berachain' or "berachain" strings (config, constants, tests, etc.)
rg -n "[\'\"]berachain[\'\"]" --type=ts | grep -v "import"
echo "---"
# Check if there are any exports or re-exports of berachain key
rg -n "export.*berachain|berachain.*export" --type=ts
echo "---"
# Look for test files that might test chain name lookups
fd -e test.ts -e spec.ts -e test.tsx | head -20Length of output: 348
Let me retry with a simpler pattern and check test files:
🏁 Script executed:
#!/bin/bash
# Search for berachain string (simpler pattern)
rg -n "berachain" --type=ts | grep -v "import { " | grep -v "const berachain"
echo "---"
# Check test files specifically for chain name usage
rg -n "berachain\|getChainConfig\|getChainIdByName" sdk/test/gateway.test.ts sdk/test/utils.test.ts 2>/dev/null | head -30Length of output: 60
Let me check the git history and also search for information about viem's mainnet structure:
🏁 Script executed:
#!/bin/bash
# Check if berachain key existed in previous versions using git
git log -p sdk/src/gateway/utils/common.ts 2>/dev/null | grep -A5 -B5 "berachain\|bera:" | head -50
echo "---"
# Also check current branch/status to understand context
git log --oneline -10 2>/dev/null
git diff HEAD~1 sdk/src/gateway/utils/common.ts 2>/dev/null | head -80Length of output: 3139
Now let me verify what viem's berachain chain name is:
Based on my verification, I can now generate the rewritten review comment. The analysis confirms both concerns are valid.
Preserve backward compatibility for berachain lookup and merge Ethereum RPC URLs instead of replacing.
The refactor from array to object mapping has introduced two breaking changes:
-
Breaking chain name lookup: The old code supported lookup by
chain.name.toLowerCase()(e.g., "berachain"), but the new mapping keys-based lookup only supports the key name "bera". Code callinggetChainConfig('berachain')will fail. Add a backward-compatible alias. -
Lost RPC redundancy: The Ethereum override discards mainnet's built-in public/default RPCs (and potentially webSocket URLs), reducing resiliency. Merge the custom RPC into mainnet's existing URLs instead of replacing them.
Apply:
- Add
berachain: berachainkey alongsidebera: berachainfor backward compatibility - Spread
mainnet.rpcUrls.defaultand prepend the custom URL:[...mainnet.rpcUrls.default.http, 'https://ethereum-rpc.publicnode.com']to maintain fallback RPCs
🤖 Prompt for AI Agents
In sdk/src/gateway/utils/common.ts around lines 142 to 161, the refactor changed
the mapping keys and replaced mainnet RPCs causing two regressions: add a
backward-compatible alias key "berachain" that points to the existing berachain
value alongside "bera", and change the ethereum entry to merge its custom RPC
into mainnet.rpcUrls instead of overwriting — preserve existing
mainnet.rpcUrls.default.http (and any webSocket entries) by prepending or
concatenating the custom 'https://ethereum-rpc.publicnode.com' URL to
mainnet.rpcUrls.default.http and include existing webSocket urls from
mainnet.rpcUrls where present.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sdk/src/gateway/tokens.ts (2)
76-117: Remove this duplicated WBTC (OFT) block (use layerZeroTokens instead).Keeps one canonical definition and avoids inconsistent edits later.
(Apply the removal diff shown in the previous comment.)
577-585: Fix symbol lookup, type, and missing chain guard in getTokenAddress.
- Keys in SYMBOL_LOOKUP are lowercase; input isn’t normalized.
- Returning a string where Address is expected breaks types.
- Accessing SYMBOL_LOOKUP[chainId] may be undefined (e.g., unsupported chain), causing a crash.
Apply:
export function getTokenAddress(chainId: number, token: string): Address { - if (isAddress(token)) { - return token; - } else if (SYMBOL_LOOKUP[chainId][token]) { - return SYMBOL_LOOKUP[chainId][token].address; - } else { - throw new Error('Unknown output token'); - } + if (isAddress(token)) { + return token as Address; + } + const key = token.toLowerCase(); + const symbols = SYMBOL_LOOKUP[chainId]; + if (symbols && symbols[key]) { + return symbols[key].address; + } + throw new Error('Unknown output token'); }
♻️ Duplicate comments (3)
sdk/src/gateway/utils/common.ts (3)
178-184: Fix map typing and remove redundant lowercasing.Keys are custom ids ("bob", "ethereum", etc.), not ViemChain['name']. Lowercasing is unnecessary since keys are already lowercase.
-const chainNameToChainIdMapping = Object.entries(supportedChainsMapping).reduce( - (acc, [name, chain]) => { - acc[name.toLowerCase()] = chain.id; - return acc; - }, - {} as Record<ViemChain['name'], ViemChain['id']> -); +type SupportedChainKey = keyof typeof supportedChainsMapping; +const chainNameToChainIdMapping = Object.entries(supportedChainsMapping).reduce( + (acc, [name, chain]) => { + acc[name as SupportedChainKey] = chain.id; + return acc; + }, + {} as Record<SupportedChainKey, ViemChain['id']> +);
186-194: Simplify allowed‑values message; avoid locale‑dependent lowercasing.Use the keys directly.
- `Chain id for "${chainName}" not found. Allowed values ${Object.keys(supportedChainsMapping).map((chainName) => chainName.toLocaleLowerCase())}` + `Chain id for "${chainName}" not found. Allowed values ${Object.keys(supportedChainsMapping)}`
142-168: Preserve RPC fallbacks and WebSocket endpoints; add alias for berachain.Overwriting rpcUrls drops mainnet/optimism defaults (and ws), reducing resiliency. Also, renaming the key to "bera" is breaking for callers using "berachain". Merge RPCs and keep a backward‑compatible alias.
Apply:
const supportedChainsMapping = { bob, - ethereum: defineChain({ - ...mainnet, - rpcUrls: { - default: { - http: ['https://ethereum-rpc.publicnode.com'], - }, - }, - }), + ethereum: defineChain({ + ...mainnet, + rpcUrls: { + ...mainnet.rpcUrls, + default: { + ...mainnet.rpcUrls.default, + http: ['https://ethereum-rpc.publicnode.com', ...mainnet.rpcUrls.default.http], + }, + }, + }), sonic, bsc, unichain, - bera: berachain, + // Keep old key for compatibility; new alias stays too. + berachain: berachain, + bera: berachain, sei, avalanche, base, soneium, - optimism: defineChain({ - ...optimism, - rpcUrls: { - default: { - http: ['https://optimism-rpc.publicnode.com'], - }, - }, - }), + optimism: defineChain({ + ...optimism, + rpcUrls: { + ...optimism.rpcUrls, + default: { + ...optimism.rpcUrls.default, + http: ['https://optimism-rpc.publicnode.com', ...optimism.rpcUrls.default.http], + }, + }, + }), } as const;Based on learnings.
🧹 Nitpick comments (1)
sdk/src/gateway/tokens.ts (1)
495-508: Unify chain naming with utils/common.ts.tokens.ts uses "berachain"; common.ts mapping uses "bera". You added an ad‑hoc normalization later; prefer a central alias map to avoid future drift.
- Export a normalizeChainName from utils/common.ts (e.g., mapping { bera: 'berachain' }).
- Reuse it here and elsewhere; remove local special‑case where possible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
sdk/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
sdk/package.json(2 hunks)sdk/src/gateway/tokens.ts(6 hunks)sdk/src/gateway/utils/common.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/package.json
🧰 Additional context used
🧬 Code graph analysis (2)
sdk/src/gateway/utils/common.ts (1)
sdk/src/gateway/client.ts (1)
chainId(181-183)
sdk/src/gateway/tokens.ts (1)
sdk/src/gateway/types/token.ts (1)
Token(7-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Tests
🔇 Additional comments (2)
sdk/src/gateway/tokens.ts (2)
592-596: Good: alias normalization for LZ naming (“bera” → “berachain”).This guards token slot lookups against external “bera” inputs.
If we adopt a central normalizeChainName util (suggested above), we can replace this local rule with that helper.
558-565: Should L0/OFT tokens be in tokenToStrategyTypeMap?layerZeroTokens aren’t included; downstream strategy selection may fail/misdirect for OFT tokens.
Would you like me to add entries for layerZeroTokens with the correct strategy label? If yes, share the canonical label (e.g., 'layerzero' or similar), and I’ll provide a patch.
We need this change due to the |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
sdk/src/gateway/utils/common.ts (3)
142-182: [Previously flagged] Preserve backward compatibility for berachain lookup and merge Ethereum RPC URLs.This was already flagged in previous reviews. The refactor introduces two breaking changes:
- Breaking chain name lookup: The rename from
berachaintoberabreaks code callinggetChainConfig('berachain').- Lost RPC redundancy: The Ethereum override discards mainnet's built-in public/default RPCs, reducing resiliency.
See the previous review comment for the detailed fix.
192-197: [Previously flagged] Redundant.toLowerCase()call on mapping keys.The keys in
supportedChainsMappingare already lowercase, so callingname.toLowerCase()at line 194 is redundant.This was already noted in a previous review comment.
204-204: [Previously flagged] Redundant.toLocaleLowerCase()call in error message.The keys of
supportedChainsMappingare already lowercase, so the.toLocaleLowerCase()call is redundant.This was already noted in a previous review comment.
🧹 Nitpick comments (1)
sdk/src/gateway/layerzero.ts (1)
719-727: Consider validating the chain name and documenting slot value sources.The implementation has several concerns:
Case sensitivity: The comparison
originChain === 'ethereum'is case-sensitive. If chain names are normalized elsewhere (e.g.,.toLowerCase()), this comparison may fail. Consider usingoriginChain.toLowerCase() === 'ethereum'.No validation: Unknown chain names silently fall through to the default slots (6n, 5n). Consider throwing an error or logging a warning for unexpected chain values.
Hardcoded magic numbers: The slot values (2n, 0n, 6n, 5n) lack documentation. Add a comment explaining their source (e.g., "WBTC ERC20 storage layout" or "WBTC OFT storage layout").
Temporary solution: Per the PR comments, this is work-in-progress. Consider adding a TODO comment referencing the planned refactor to the LayerZero client.
Apply this diff to improve robustness:
private getTokenSlots(originChain: string) { - // slots for WBTC ERC20 token on ethereum - if (originChain === 'ethereum') return { allowanceSlot: 2n, balanceSlot: 0n }; + // TODO: Move to LayerZero client with token-specific slot mappings (see PR #821 discussion) + // Slots for WBTC ERC20 token on Ethereum mainnet + // Source: WBTC token contract storage layout + if (originChain.toLowerCase() === 'ethereum') return { allowanceSlot: 2n, balanceSlot: 0n }; + // Slots for WBTC OFT tokens on other chains + // Source: LayerZero OFT token contract storage layout return { allowanceSlot: 6n, balanceSlot: 5n, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
sdk/package.json(2 hunks)sdk/src/gateway/layerzero.ts(4 hunks)sdk/src/gateway/utils/common.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/package.json
🧰 Additional context used
🧬 Code graph analysis (1)
sdk/src/gateway/utils/common.ts (1)
sdk/src/gateway/client.ts (1)
chainId(181-183)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
sdk/src/gateway/layerzero.ts (1)
21-21: I need to verify that the private method implementation in layerzero.ts is correct and consistent with the removed import dependency.
The original concern is not substantiated. The
getTokenSlotsfunction remains exported from./tokens(line 469 in tokens.ts) and continues to be used byclient.ts(imported on line 27 and called on line 276). Removing the import fromlayerzero.tsrepresents a localized refactoring—moving the logic to a private method within the class—and poses no risk to other files. No action required.Likely an incorrect or invalid review comment.
Summary by CodeRabbit
Release Notes