fix: allow buffer, use correct oft, check viem clients #749
Conversation
|
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. Caution Review failedThe pull request is closed. WalkthroughUpdates LayerZero gateway to use WBTC OFT addresses for bitcoin routes, adds a configurable LayerZero fee buffer (l0FeeBuffer), extends Offramp extraOptions and origin-chain validations, bumps SDK version to 4.2.4, and skips two LayerZero integration tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant SDK as SDK LayerZero Gateway
participant Chains as Chain Registry
participant L0 as LayerZero
participant Dex as Pricing Engine
User->>SDK: getQuote({from: bitcoin, to: bob, l0FeeBuffer?})
SDK->>Chains: getEidForChain(bob) / getOftAddressForChain(WBTC, bob)
SDK-->>User: error if OFT or EID missing
SDK->>L0: quoteSend(token=WBTC_OFT, dstEid)
L0-->>SDK: nativeFee
SDK->>SDK: apply l0FeeBuffer (default 500 BPS)
SDK->>Dex: quoteExactOutputSingle(tokenIn=WBTC_OFT, amountOut=layerZeroFeeWithBuffer)
Dex-->>SDK: amountIn
SDK-->>User: quote (amountIn, nativeFee, bufferedFee, params)
sequenceDiagram
autonumber
actor User
participant SDK as SDK LayerZero Gateway
participant Client as Origin Public Client
participant Chains as Chain Registry
participant L0 as LayerZero
User->>SDK: executeQuote({from: bob, to: bitcoin, ...})
SDK->>Client: assert public client is origin chain
SDK->>Chains: getOftAddressForChain(WBTC, origin)
SDK-->>User: error if OFT missing or wrong client
SDK->>L0: quoteSend(token=WBTC_OFT, dstEid, extraOptions++)
L0-->>SDK: nativeFee
SDK->>L0: send(..., extended Executor/IOFT options)
L0-->>User: tx receipt / result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
✨ 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 focuses on refining the LayerZero integration within the SDK. It addresses critical fixes related to the correct usage of Omnichain Fungible Token (OFT) addresses and introduces a new configurable option for LayerZero fee buffers. Additionally, it enhances the reliability of client interactions by adding a validation check for the public client's chain configuration.
Highlights
- LayerZero OFT Address Correction: Ensured the correct WBTC Omnichain Fungible Token (OFT) address is consistently used for LayerZero operations, specifically for quoting send fees and Uniswap quotes on the BOB chain.
- Configurable LayerZero Fee Buffer: Introduced an optional
l0FeeBufferparameter in theGatewayQuoteParamsto allow consumers to specify a custom percentage buffer for LayerZero fees, overriding the default 5%. - Viem Client Validation: Added a validation check to ensure that the
publicClient(likely a Viem client) is correctly configured for the origin chain when quoting LayerZero send fees, enhancing robustness. - SDK Version Update: The SDK package version has been incremented from 4.2.3 to 4.2.4.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| 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 issue 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. ↩
gregdhill
force-pushed
the
fix/l0-client
branch
from
57e6d51 to
8a229b2
Compare
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
gregdhill
force-pushed
the
fix/l0-client
branch
from
8a229b2 to
68e0ac8
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces several fixes, including allowing a configurable buffer for LayerZero fees, ensuring the correct wBTC OFT address is used on BOB, and adding a check for the viem public client's chain. A new l0FeeBuffer parameter is added to GatewayQuoteParams for this purpose. The changes are a good improvement, but there are a few areas that could be enhanced. Some error messages could be more descriptive to aid in debugging, and the test file contains leftover console.log statements and skipped tests that should be addressed before merging.
| if (!wbtcOftAddress) { | ||
| throw new Error(`WBTC OFT not found for chain: ${fromChain}`); | ||
| } |
There was a problem hiding this comment.
The error message here is a bit misleading. You are fetching the OFT address for 'bob', but if it's not found, the error message refers to ${fromChain}. This could cause confusion during debugging. Consider changing it to explicitly mention 'bob'.
| if (!wbtcOftAddress) { | |
| throw new Error(`WBTC OFT not found for chain: ${fromChain}`); | |
| } | |
| if (!wbtcOftAddress) { | |
| throw new Error(`WBTC OFT not found for chain: bob`); | |
| } |
| if (publicClient.chain?.name.toLowerCase() !== fromChain) { | ||
| throw new Error(`Public client must be origin chain`); | ||
| } |
There was a problem hiding this comment.
This is a great check to ensure the public client is configured for the correct chain. To make debugging easier, consider making the error message more descriptive by including the actual and expected chain names.
| if (publicClient.chain?.name.toLowerCase() !== fromChain) { | |
| throw new Error(`Public client must be origin chain`); | |
| } | |
| if (publicClient.chain?.name.toLowerCase() !== fromChain) { | |
| throw new Error(`Public client is on the wrong chain. Expected '${fromChain}', but got '${publicClient.chain?.name.toLowerCase()}'.`); | |
| } |
| }, 120000); | ||
|
|
There was a problem hiding this comment.
This empty line and console.log statement appear to be for debugging purposes. They should be removed before merging.
| it('should get an onramp quote and execute it', async () => { | ||
| it.skip('should get an onramp quote and execute it', async () => { | ||
| const client = new LayerZeroGatewayClient(bob.id); | ||
|
|
There was a problem hiding this comment.
This test case is currently skipped using it.skip. Skipped tests can hide regressions. Please either enable and fix the test, or remove it if it's no longer relevant.
| it('should get an onramp quote and execute it', async () => { |
| it('should get an offramp quote and execute it', async () => { | ||
| it.skip('should get an offramp quote and execute it', async () => { | ||
| const client = new LayerZeroGatewayClient(bob.id); | ||
| const layerZeroClient = new LayerZeroClient(); |
There was a problem hiding this comment.
This test case is also skipped. As with the other skipped test, it should be re-enabled or removed to ensure full test coverage and prevent hidden issues.
| const layerZeroClient = new LayerZeroClient(); | |
| it('should get an offramp quote and execute it', async () => { |
Summary by CodeRabbit
New Features
Improvements
Tests
Chores