chore: check fromChain ID in L0 client #767
Conversation
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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. WalkthroughUpdates onramp response typing and OnrampOrder shape (introduces Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant LZ as LayerZeroGatewayClient
participant LC as LayerZeroClient
participant API as MetadataAPI
participant PC as publicClient
Caller->>LZ: getSupportedChainsInfo()
LZ->>LC: getSupportedChainsInfo()
LC->>API: fetch chain deployments & WBTC deployments
API-->>LC: chain deployments + WBTC addresses
LC-->>LZ: array of LayerZeroChainInfo{name,eid,oftAddress,nativeChainId}
LZ-->>Caller: supported chains info
sequenceDiagram
autonumber
actor Caller
participant LZ as LayerZeroGatewayClient
participant PC as publicClient
Caller->>LZ: executeQuote(params)
LZ->>LZ: maybeFromChainId = params.params.fromChain
alt fromChain is number
LZ->>PC: read publicClient.chain?.id
alt id matches fromChain
LZ-->>Caller: proceed with quote
else id mismatch
LZ-->>Caller: throw "Public client must be origin chain"
end
else not a number
LZ-->>Caller: proceed without ID check
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @gregdhill, 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 enhances the robustness of the LayerZero client by implementing a more precise validation mechanism for the fromChain identifier. The change shifts from name-based comparison to numerical chain ID comparison, mitigating potential inconsistencies arising from differing naming conventions. Additionally, it includes minor but important type definition updates within the SDK to ensure accurate representation and handling of onramp order details.
Highlights
- Improved Chain ID Validation: The LayerZero client now validates the
fromChainID using the numerical chain ID instead of the chain name, which can vary between different systems like L0 and viem. - Type Definition Refinement: Adjustments were made to the
OnrampOrderResponseandOnrampOrdertypes to correctly handleorderDetails, ensuring better type consistency. - SDK Version Update: The SDK package version has been incremented from
4.2.10to4.2.11.
Using Gemini Code Assist
The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of the LayerZero client by checking the fromChain ID instead of its name, which can be inconsistent. It also includes several type corrections, notably for OnrampOrderResponse and OnrampOrder to better reflect the data structures from the API and after processing. While the changes are generally good, there is an issue with the updated OnrampOrder type definition that needs to be addressed.
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
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/layerzero.ts (1)
160-168: Pad to bytes32 fortofield insendParam.
tois ABI-encoded asbytes32. Pad the EVM address to 32 bytes to avoid encoding errors; also fix the comment typo.Apply:
- to: padHex(params.toUserAddress as Hex) as Hex, + to: padHex(params.toUserAddress as Hex, { size: 32 }) as Hex, - minAmountLD: BigInt(0), // will be added inside the strategyzz + minAmountLD: BigInt(0), // will be added inside the strategysdk/src/gateway/client.ts (1)
1021-1085:OnrampOrdertype hidesorderDetails, but the returned object still exposes it.This leaks a field intentionally omitted from the public type and can cause consumer reliance on a non-API property.
Apply:
- const orderDetails = order.orderDetails - ? convertOrderDetailsRawToOrderDetails(order.orderDetails) - : undefined; - return { ...order, - orderDetails, gasRefill: order.satsToConvertToEth, baseToken: ADDRESS_LOOKUP[chainId][order.baseTokenAddress], outputToken: ADDRESS_LOOKUP[chainId][order.outputTokenAddress!],If you need
orderDetailsinternally later, keep it local-only (don’t include it in the returned shape).
🧹 Nitpick comments (3)
sdk/src/gateway/layerzero.ts (2)
182-186: Zero buffer should be allowed.
params.l0FeeBuffer ? ...treats0as falsy and forces default 5%. Use explicit undefined check.Apply:
- const buffer = params.l0FeeBuffer ? BigInt(params.l0FeeBuffer) : BigInt(500); // 5% buffer + const buffer = params.l0FeeBuffer !== undefined ? BigInt(params.l0FeeBuffer) : 500n; // 5% buffer
318-321: Good: chain-id based origin validation; add a guard for missing chain.If
publicClient.chainis unset, fail fast; keep id comparison for numbers.Apply:
- const maybeFromChainId = executeQuoteParams.params.fromChain; - if (typeof maybeFromChainId === 'number' && publicClient.chain?.id !== maybeFromChainId) { - // avoid matching on name since L0 and viem may have different naming conventions - throw new Error(`Public client must be origin chain`); - } + const maybeFromChainId = executeQuoteParams.params.fromChain; + if (typeof maybeFromChainId === 'number') { + if (publicClient.chain?.id === undefined) { + throw new Error('Public client must be configured with a chain.'); + } + if (publicClient.chain.id !== maybeFromChainId) { + // avoid matching on name since L0 and viem may have different naming conventions + throw new Error('Public client must be origin chain'); + } + }sdk/src/gateway/types.ts (1)
318-318: Good shift to transport/raw typing for orderDetails; please document and confirm normalization pathUsing
OrderDetailsRawat the IO boundary is sensible. Add a brief JSDoc note that this is the raw/transport shape (string-encoded quantities) and confirm that client code reliably converts it toOrderDetailsbefore exposure/usage.Can you confirm
getOnrampOrders(or equivalent) performs theOrderDetailsRaw -> OrderDetailsnormalization and that no public return types expose the raw shape?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sdk/package.json(1 hunks)sdk/src/gateway/client.ts(2 hunks)sdk/src/gateway/layerzero.ts(1 hunks)sdk/src/gateway/types.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-17T11:18:53.195Z
Learnt from: slavastartsev
PR: bob-collective/bob#634
File: sdk/src/gateway/client.ts:136-140
Timestamp: 2025-06-17T11:18:53.195Z
Learning: The codebase has a requirement not to introduce discriminated unions in the Gateway SDK client methods, including the getQuote method return types in sdk/src/gateway/client.ts.
Applied to files:
sdk/src/gateway/layerzero.ts
🧬 Code graph analysis (1)
sdk/src/gateway/client.ts (1)
sdk/src/gateway/types.ts (1)
OnrampOrderResponse(285-319)
⏰ 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 (3)
sdk/package.json (1)
3-3: Version bump looks good.No concerns from me for 4.2.11.
sdk/src/gateway/client.ts (1)
46-46: ImportingOnrampOrderResponseis correct.Aligns payload typing with API response.
sdk/src/gateway/types.ts (1)
346-354: Avoid intersect-then-omit; simplify OnrampOrder and treat removal of orderDetails as a breaking change.Verification: sdk/package.json → 4.2.11; ripgrep reported "unrecognized file type: tsx" and returned no matches — search inconclusive, do not assume consumers are unaffected. Either keep orderDetails (deprecated) for one minor release or bump the major version.
Apply this diff to simplify the type and avoid the risky merge:
-export type OnrampOrder = Omit< - OnrampOrderResponse & { - /** @description The gas refill in satoshis */ - gasRefill: number; - /** @description V4 order details */ - orderDetails?: OrderDetails; - }, - 'satsToConvertToEth' | 'orderDetails' -> & { +export type OnrampOrder = Omit<OnrampOrderResponse, 'satsToConvertToEth' | 'orderDetails'> & { + /** @description The gas refill in satoshis */ + gasRefill: number; +} & {
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
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 (1)
sdk/src/gateway/layerzero.ts (1)
211-214: Padtoto bytes32 explicitly for ABI(bytes32 to).Without
{ size: 32 },padHexmay not produce a 32‑byte value; ABI encoders often require exact size. Also fix comment typo.Apply:
- to: padHex(params.toUserAddress as Hex) as Hex, - amountLD: BigInt(0), // will be added inside the strategy - minAmountLD: BigInt(0), // will be added inside the strategyzz + to: padHex(params.toUserAddress as Hex, { size: 32 }) as Hex, + amountLD: BigInt(0), // will be added inside the strategy + minAmountLD: BigInt(0), // will be added inside the strategy
♻️ Duplicate comments (1)
sdk/src/gateway/types.ts (1)
345-351: Correct property override pattern for OnrampOrder.This fixes the earlier issue with
Omit<A & { prop }, 'prop'>. New form is canonical.
🧹 Nitpick comments (9)
sdk/src/gateway/layerzero.ts (6)
41-56: Fix tuple typing for deployments; use arrays.
deployments?: [ { ... } ]is a 1‑element tuple, not “array of”. This narrows types and can break when multiple deployments are returned.Apply:
- return this.getJson<{ - [chainKey: string]: { - deployments?: [ - { - version: number; - eid: string; - }, - ]; - chainKey: string; - chainDetails?: { - nativeChainId: number; - }; - }; - }>(`${this.basePath}`); + return this.getJson<{ + [chainKey: string]: { + deployments?: Array<{ + version: number; + eid: string; + }>; + chainKey: string; + chainDetails?: { + nativeChainId: number; + }; + }; + }>(`${this.basePath}`);
63-74: Harden typing and null‑guard WBTC deployments.Same tuple issue here; also guard for missing data to avoid
Cannot read properties of undefinedonWBTC[0].Apply:
- const data = await this.getJson<{ - WBTC: [{ deployments: { [chainKey: string]: { address: string } } }]; - }>(`${this.basePath}/experiment/ofts/list?${params.toString()}`); - - return data.WBTC[0].deployments; + const data = await this.getJson<{ + WBTC: Array<{ deployments: Record<string, { address: string }> }>; + }>(`${this.basePath}/experiment/ofts/list?${params.toString()}`); + + const first = data.WBTC?.[0]; + if (!first) { + throw new Error('WBTC deployments missing from LayerZero metadata API'); + } + return first.deployments;
85-96: Remove unused_binding; it triggers static‑analysis warning.Use elision to avoid “defined but never used”.
Apply:
- const chainLookup = Object.fromEntries( - Object.entries(chains).map(([_, chainData]) => [ + const chainLookup = Object.fromEntries( + Object.entries(chains).map(([, chainData]) => [ chainData.chainKey, { eid: chainData.deployments?.find((item) => item.version === 2)?.eid, nativeChainId: chainData.chainDetails?.nativeChainId, }, ]) );
97-107: OK, but consider memoizing metadata to cut HTTP calls.Optional: cache results inside LayerZeroClient to reuse across calls in a process.
109-115: Type assertion nit: cast JSON toT, notPromise<T>.
await response.json()yields a value, not a Promise. Current cast is misleading.Apply:
- return (await response.json()) as Promise<T>; + return (await response.json()) as T;
308-309: Reuse the existing client instance.Avoid extra network state and duplicate caches by reusing
this.l0Client.Apply:
- const layerZeroClient = new LayerZeroClient(); + const layerZeroClient = this.l0Client;sdk/test/layerzero.test.ts (3)
16-19: Network‑dependent test; consider mocking to reduce flakiness.Calls live metadata API; CI can fail on network/hot updates. Consider stubbing
LayerZeroClient.getSupportedChainsInfowith fixtures.Do you want a quick vitest mock setup + fixture generator?
38-52: Hard‑coding EIDs may be brittle.EIDs can change or new versions could alter the mapping. If the intent is smoke‑testing structure, assert presence/type rather than exact values; keep exact checks in a snapshot that you update intentionally.
53-69: Address checks OK; consider casing normalizer.Comparisons are case‑sensitive; if API casing changes, this breaks. Normalizing to lowercase could help.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sdk/src/gateway/layerzero.ts(4 hunks)sdk/src/gateway/types.ts(2 hunks)sdk/test/gateway.test.ts(0 hunks)sdk/test/layerzero.test.ts(2 hunks)
💤 Files with no reviewable changes (1)
- sdk/test/gateway.test.ts
🧰 Additional context used
🪛 GitHub Check: Tests
sdk/src/gateway/layerzero.ts
[warning] 88-88:
'_' is defined but never used
⏰ 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 (8)
sdk/src/gateway/layerzero.ts (6)
27-32: Type addition looks good.LayerZeroChainInfo shape is clear and matches usage across the file.
58-61: Getter logic is fine.Version filter and nullable fallback are appropriate.
75-79: Supported chains via deployments keys: LGTM.
80-84: OFT address fetch: LGTM.Null fallback is appropriate.
144-147: Gateway wrapper passthrough: LGTM.Keeps API parity while introducing the richer info method.
155-167: Deprecation docs are clear.Good to keep shims while tests migrate.
sdk/test/layerzero.test.ts (1)
19-34: Subset assertion is fine; verifycontainSubsetis available.Ensure chai‑subset (or equivalent) is registered with Vitest; otherwise switch to a stable assertion.
Would you like me to refactor this to
assert.includeMemberswith a curated minimal set?sdk/src/gateway/types.ts (1)
318-319: Raw vs enriched orderDetails split: LGTM.Clear separation avoids accidental misuse of raw types downstream.
| // we're quoting on the origin chain, so public client must be configured correctly | ||
| if (publicClient.chain?.name.toLowerCase() !== fromChain) { | ||
| const maybeFromChainId = executeQuoteParams.params.fromChain; | ||
| if (typeof maybeFromChainId === 'number' && publicClient.chain?.id !== maybeFromChainId) { | ||
| // avoid matching on name since L0 and viem may have different naming conventions | ||
| throw new Error(`Public client must be origin chain`); | ||
| } |
There was a problem hiding this comment.
Strengthen origin‑chain check when fromChain is a string.
Currently only enforced for numeric IDs. If a string slug is provided, publicClient could point to the wrong chain and the tx would be simulated/submitted on the wrong network.
Apply:
- const maybeFromChainId = executeQuoteParams.params.fromChain;
- if (typeof maybeFromChainId === 'number' && publicClient.chain?.id !== maybeFromChainId) {
- // avoid matching on name since L0 and viem may have different naming conventions
- throw new Error(`Public client must be origin chain`);
- }
+ const fromParam = executeQuoteParams.params.fromChain;
+ if (typeof fromParam === 'number') {
+ if (publicClient.chain?.id !== fromParam) {
+ throw new Error(`Public client must be origin chain`);
+ }
+ } else {
+ // Resolve slug -> nativeChainId via metadata and verify by numeric id to avoid name mismatches.
+ const info = await this.l0Client.getSupportedChainsInfo();
+ const nativeId = info.find((c) => c.name === fromParam)?.nativeChainId;
+ if (nativeId && publicClient.chain?.id !== nativeId) {
+ throw new Error(`Public client must be origin chain`);
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In sdk/src/gateway/layerzero.ts around lines 366 to 371, the origin-chain guard
only checks numeric IDs; extend it to also validate when
executeQuoteParams.params.fromChain is a string by performing a case-insensitive
comparison against the publicClient.chain's identifying fields (e.g., chain.name
and chain.network or chain.slug if available) and throw the same error if none
match; keep the existing numeric ID check, ensure null/undefined
publicClient.chain is handled safely, and normalize strings before comparing.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores