feat: SDK changes Offramp V2 #841
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @nakul1010, 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 introduces a comprehensive set of changes to the SDK to facilitate the integration of Offramp V2. The modifications primarily involve updating the data structures for offramp quotes and orders, refactoring the methods for fetching and processing order details to leverage a new API endpoint, and streamlining the way offramp registry addresses are resolved. These updates ensure compatibility with the new Offramp V2 contract, enhance the flexibility of fee handling, and improve the efficiency of order management within the SDK. 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
|
|
Warning Rate limit exceeded@nakul1010 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
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. WalkthroughIntroduces a versioned offramp ABI ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant SDK as Gateway SDK
participant API as Offramp API
participant Mapper as Order Mapper
User->>SDK: fetchOfframpOrder(orderId, registryAddress?)
SDK->>API: GET /orders?orderId®istryAddress
API-->>SDK: OfframpRawOrder
SDK->>Mapper: mapRawOrderToOfframpOrder(OfframpRawOrder)
Mapper-->>SDK: OfframpOrder (affiliate fields, registryVersion, unlockable)
SDK-->>User: OfframpOrder
sequenceDiagram
autonumber
participant User
participant SDK as Gateway SDK
participant Registry as Registry Resolver
participant API as Offramp API
User->>SDK: fetchOfframpQuote(..., affiliateFeeRecipient?)
SDK->>Registry: getTargetOfframpRegistryAddress(chainId)
Registry-->>SDK: registryAddress
SDK->>API: request quote (affiliate_fee, affiliateFeeRecipient)
API-->>SDK: OfframpQuote
SDK-->>User: OfframpQuote
sequenceDiagram
autonumber
participant User
participant SDK as Gateway SDK
participant ABI as ABI Descriptor
participant Chain as Chain/Contract
User->>SDK: createOfframpOrderV2(args...)
SDK->>ABI: use offrampCallerV2 descriptor
SDK->>Chain: writeContract.createOrderV2(args: satSolverFeeMax, satAffiliateFee, affiliateFeeRecipient, ...)
Chain-->>SDK: txReceipt
SDK-->>User: txReceipt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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.
Code Review
This pull request introduces significant changes for the Offramp V2 SDK, including support for affiliate fees, refactoring order detail fetching to use an API instead of direct contract calls, and simplifying fee bump logic on the client side. The changes are well-structured and align with the goal of a V2 upgrade. My review includes a few suggestions to improve robustness and test reliability, such as enhancing type safety when parsing API responses and addressing issues in the test suite related to mock data and unmocked network calls. Overall, this is a solid step forward for the SDK.
…eat/offramp-v2 # Conflicts: # sdk/src/gateway/client.ts
nakul1010
changed the title
![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
♻️ Duplicate comments (1)
sdk/src/gateway/client.ts (1)
199-228: Type assertions bypass TypeScript safety.As noted in a previous review, this function uses multiple type assertions (
order.status as OfframpOrderStatus,order.offrampRegistryAddress as Address, etc.) which can lead to runtime errors if the API returns unexpected data.Consider using Zod or custom type guards to validate the API response at runtime:
import { z } from 'zod'; const OfframpRawOrderSchema = z.object({ orderId: z.string(), token: z.string().refine(isAddress), status: z.enum(['Active', 'Accepted', 'Processed', 'Refunded']), offrampRegistryAddress: z.string().refine(isAddress), // ... other fields }); public async mapRawOrderToOfframpOrder(order: OfframpRawOrder): Promise<OfframpOrder> { const validated = OfframpRawOrderSchema.parse(order); // Throws if invalid // Now TypeScript knows the types are correct ... }
🧹 Nitpick comments (2)
sdk/src/gateway/client.ts (2)
195-197: Consider adding version validation.The method defaults to V2 for any version other than 1. While this may be intentional (forward compatibility), it could mask bugs if an invalid version is passed.
Consider adding a check:
private getOfframpAbi(version: number) { - return version === 1 ? offrampCallerV1 : offrampCallerV2; + if (version === 1) return offrampCallerV1; + if (version === 2) return offrampCallerV2; + throw new Error(`Unsupported offramp registry version: ${version}`); }
536-590: LGTM with minor naming inconsistency.The affiliate fee handling is correct, with proper validation and normalization to
zeroAddresswhen not provided. Note that the parameter is namedaffiliate_fee(line 542) here butaffiliateFeeSatsinGatewayQuoteParams(line 52 of quote.ts) - consider standardizing the naming for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
sdk/src/gateway/abi.ts(2 hunks)sdk/src/gateway/client.ts(17 hunks)sdk/src/gateway/types/offramp.ts(7 hunks)sdk/src/gateway/types/quote.ts(2 hunks)sdk/test/gateway.test.ts(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-17T11:18:53.195Z
Learnt from: slavastartsev
Repo: bob-collective/bob PR: 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/types/quote.tssdk/test/gateway.test.tssdk/src/gateway/types/offramp.tssdk/src/gateway/client.ts
🧬 Code graph analysis (3)
sdk/test/gateway.test.ts (1)
sdk/src/gateway/types/offramp.ts (1)
OfframpRawOrder(148-165)
sdk/src/gateway/types/offramp.ts (1)
sdk/src/gateway/abi.ts (1)
offrampCallerV2(80-172)
sdk/src/gateway/client.ts (2)
sdk/src/gateway/abi.ts (2)
offrampCallerV1(41-78)offrampCallerV2(80-172)sdk/src/gateway/types/offramp.ts (5)
OfframpRawOrder(148-165)OfframpOrder(86-121)OfframpOrderStatus(13-13)BumpFeeParams(173-176)UnlockOrderParams(178-182)
🪛 Gitleaks (8.29.0)
sdk/test/gateway.test.ts
[high] 691-691: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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 (15)
sdk/src/gateway/types/quote.ts (1)
1-1: LGTM! Clean addition of affiliate fee support.The import of
Addressand the two new optional fields (affiliateFeeSatsandaffiliateFeeRecipient) are well-typed and maintain backward compatibility.Also applies to: 51-54
sdk/src/gateway/abi.ts (2)
41-78: LGTM! V1 ABI correctly defines bump and refund operations.The ABI definition for
offrampCallerV1is properly structured with the necessary functions for fee bumping and order refunds.
80-172: LGTM! V2 ABI properly extends functionality with affiliate support.The V2 ABI includes the enhanced
createOrderfunction withsatSolverFeeMax,satAffiliateFee, andaffiliateFeeRecipientfields. Note thatbumpFeeOfExistingOrderandrefundOrderare duplicated between V1 and V2, which is acceptable for versioning but means both ABIs need updates if these functions change.sdk/test/gateway.test.ts (4)
658-680: LGTM! Test properly validates V2 order creation.The test correctly validates the new affiliate fee fields (
satSolverFeeMax,satAffiliateFee,affiliateFeeRecipient) in the offramp order creation flow.
688-722: LGTM! Test correctly uses the new mapping function.The test properly constructs a mock
OfframpRawOrderwith all new fields and usesmapRawOrderToOfframpOrderto validate the transformation. The previous issue with field naming (evmTxvsrefundedEvmTx) appears to be resolved.
739-745: LGTM! Test reflects the new static registry address approach.The test correctly validates that
getOfframpRegistryAddress()returns the expected testnet registry address without requiring an API call.
964-965: Test skipped pending V2 contract deployment.This test for offramp gas cost estimation is appropriately skipped until the V2 Offramp Registry contracts are deployed to mainnet and testnet. The TODO comment aligns with similar TODOs in
sdk/src/gateway/client.tslines 95 and 100.Please ensure this test is re-enabled once the V2 contracts are deployed and the registry addresses are updated.
sdk/src/gateway/types/offramp.ts (3)
2-2: LGTM! Type definitions properly migrated to V2.The import change to
offrampCallerV2and the update toOfframpQuote(addingaffiliateFeeRecipient, removingregistryAddress) correctly reflect the V2 API changes. The registry address is now handled separately at the call site level.Also applies to: 43-44, 61-61
68-72: LGTM! Well-documented field additions for V2.The new fields in
OfframpCreateOrderParamsandOfframpOrderare properly documented:
satSolverFeeMax(renamed fromsatFeesMax) clarifies the fee's purpose- Affiliate fields enable fee tracking and distribution
offrampRegistryVersionenables ABI version selectionbumpFeeAmountInSatssupports dynamic fee bumping logicAlso applies to: 94-94, 112-120
159-164: LGTM! Raw order type and params updated consistently.
OfframpRawOrdercorrectly includes all new fields as strings (matching the API response format), andBumpFeeParams/UnlockOrderParamsnow acceptofframpRegistryAddressto support operations across multiple registry versions.Also applies to: 175-175, 181-181
sdk/src/gateway/client.ts (5)
405-408: LGTM! Affiliate parameters properly handled in onramp quote.The code correctly checks that both
affiliateFeeRecipientandaffiliateFeeSatsare provided before adding them to the query parameters, ensuring affiliate fee data is complete.
484-492: LGTM! Clean registry address resolution.The method correctly returns the appropriate registry address based on chain ID and throws a clear error for unsupported chains. Note that this method uses the constants with TODO comments (lines 95, 100), so it will need verification once V2 contracts are deployed.
614-622: LGTM! Order creation correctly uses V2 ABI with affiliate support.The
createOfframpOrdermethod properly usesofframpCallerV2and includes all new fields (satSolverFeeMax,satAffiliateFee,affiliateFeeRecipient) in the order arguments.
639-671: LGTM! Bump and unlock correctly support multi-version registries.Both methods properly:
- Accept
offrampRegistryAddressas a parameter- Fetch order details via API to determine the registry version
- Use
getOfframpAbi()to select the appropriate ABI- Validate order state before proceeding
The null check on
bumpFeeAmountInSats(line 653) correctly prevents unnecessary bump fee transactions.Also applies to: 680-716
92-100: V2 registry addresses flagged as TODO: confirm deployment status and whether runtime validation is needed.The review comment is factually accurate—both TODOs exist at lines 95 and 100, and the referenced skipped test exists at line 965 of the test file. Code verification confirms V2 ABI support is imported and used, but no runtime validation prevents address misuse if deployed addresses don't match the hardcoded constants.
Key findings:
- No deployment timeline, contract status, or address validity info found in codebase
- No runtime guards or validation to prevent V2 operations with potentially incorrect addresses
- Current addresses are hardcoded with no environment-based fallback or configuration
To resolve: Confirm the V2 contract deployment status externally and determine whether the code should include validation to prevent operations until correct addresses are in place.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
sdk/src/gateway/client.ts (1)
197-226: Type assertions bypass runtime safety checks.This method uses multiple type assertions (e.g.,
order.status as OfframpOrderStatus,order.offrampRegistryAddress as Address) without validation. As noted in a previous review, using type guards or a validation library like Zod would provide better runtime safety if the API returns unexpected data.
🧹 Nitpick comments (1)
sdk/test/gateway.test.ts (1)
751-765: Consider mocking the contract call for better test isolation.This test calls
canOrderBeUnlocked, which performs areadContractcall to fetch the claim delay. While the previous review noted this is acceptable because the registry address is valid, mocking this call would make the test faster, more deterministic, and able to run offline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sdk/src/gateway/client.ts(17 hunks)sdk/src/gateway/types/offramp.ts(7 hunks)sdk/src/gateway/utils/common.ts(0 hunks)sdk/test/gateway.test.ts(9 hunks)
💤 Files with no reviewable changes (1)
- sdk/src/gateway/utils/common.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-17T11:18:53.195Z
Learnt from: slavastartsev
Repo: bob-collective/bob PR: 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/types/offramp.tssdk/test/gateway.test.ts
🧬 Code graph analysis (3)
sdk/src/gateway/types/offramp.ts (1)
sdk/src/gateway/abi.ts (1)
offrampCallerV2(80-172)
sdk/src/gateway/client.ts (2)
sdk/src/gateway/abi.ts (2)
offrampCallerV1(41-78)offrampCallerV2(80-172)sdk/src/gateway/types/offramp.ts (5)
OfframpRawOrder(124-141)OfframpOrder(86-121)OfframpOrderStatus(13-13)BumpFeeParams(149-152)UnlockOrderParams(154-158)
sdk/test/gateway.test.ts (2)
sdk/src/gateway/client.ts (1)
SIGNET_GATEWAY_BASE_URL(82-82)sdk/src/gateway/types/offramp.ts (1)
OfframpOrderStatus(13-13)
🪛 Gitleaks (8.29.0)
sdk/test/gateway.test.ts
[high] 678-678: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (18)
sdk/test/gateway.test.ts (5)
662-684: LGTM! Test updated to match V2 offramp contract.The test correctly includes the new affiliate fee fields and uses the updated field naming (satSolverFeeMax, satAffiliateFee, affiliateFeeRecipient) that aligns with the V2 offramp ABI.
692-726: Test now uses mapping function for better consistency.The test has been improved to use
mapRawOrderToOfframpOrderfor constructing expected results, which reduces duplication of mapping logic and makes the test more maintainable. The field naming issue from the previous review (evmTx vs refundedEvmTx) has also been resolved.
743-748: LGTM! Test correctly validates static registry address lookup.The test has been updated to reflect the change from dynamic registry address fetching to static, chain-specific address constants.
968-969: Skipped test blocks V2 gas cost feature validation.This test remains skipped pending V2 offramp registry deployment. Once the new registry is deployed and the address is updated in the constants, this test should be re-enabled to validate the gas cost estimation feature.
890-948: LGTM! Test correctly updated to spy on static registry address method.The test spy has been updated from
fetchOfframpRegistryAddresstogetTargetOfframpRegistryAddress, correctly reflecting the change from dynamic fetching to static lookup.sdk/src/gateway/types/offramp.ts (4)
2-44: LGTM! OfframpQuote updated for V2 contract.The import has been updated to use
offrampCallerV2, and theOfframpQuotetype correctly replaces the dynamicregistryAddressfield withaffiliateFeeRecipient, aligning with the V2 contract requirements where registry addresses are now statically determined by chain.
57-83: LGTM! CreateOrderParams aligned with V2 ABI structure.The type correctly specifies
offrampCallerV2ABI and includes the new V2 contract fields:satSolverFeeMax,satAffiliateFee, andaffiliateFeeRecipient. The field renaming fromsatFeesMaxtosatSolverFeeMaxis more descriptive and clearly indicates this fee is for the solver.
85-141: LGTM! Enhanced order types support versioning and affiliate fees.The
OfframpOrderandOfframpRawOrdertypes have been extended with essential fields:
offrampRegistryVersionenables handling both V1 and V2 registries- Affiliate fee fields (
satAffiliateFee,affiliateFeeRecipient) support the new revenue-sharing modelbumpFeeAmountInSatsprovides users with fee bump guidanceuserAddressenables user-specific order filteringAll fields are well-documented and the raw/domain type separation is maintained correctly.
148-157: LGTM! Operation params now include registry address.Both
BumpFeeParamsandUnlockOrderParamscorrectly includeofframpRegistryAddress, which is necessary for supporting multiple registry versions (V1 and V2) and directing operations to the correct contract.sdk/src/gateway/client.ts (9)
90-98: Mainnet V2 registry address needs to be updated before deployment.The
MAINNET_TARGET_OFFRAMP_REGISTRY_ADDRESSconstant has a TODO comment indicating it needs to be replaced with the new V2 address. This must be completed before the V2 offramp feature can be enabled on mainnet.
193-195: LGTM! Version-based ABI selection supports V1/V2 registries.The method cleanly selects the appropriate ABI based on version, defaulting to V2 for future versions.
403-406: LGTM! Affiliate parameters correctly added to onramp quotes.The implementation properly requires both
affiliateFeeRecipientandaffiliateFeeSatstogether, preventing partial affiliate configuration.
482-490: LGTM! Static registry address lookup improves reliability.Replacing dynamic registry address fetching with static, chain-specific constants improves reliability and performance, as stated in the PR objectives.
534-589: LGTM! Offramp quote correctly handles affiliate parameters.The implementation properly:
- Makes affiliate parameters optional
- Requires both affiliate parameters together when provided
- Defaults
affiliateFeeRecipienttozeroAddresswhen not specified- Maintains consistency with the onramp flow
599-628: LGTM! Order creation uses V2 ABI with affiliate fee support.The method correctly uses
offrampCallerV2and populates all V2-specific fields includingsatSolverFeeMax,satAffiliateFee, andaffiliateFeeRecipient.
637-668: LGTM! Fee bumping now supports both V1 and V2 registries.The implementation correctly:
- Accepts explicit
offrampRegistryAddressparameter- Validates that fee bumping is needed via
bumpFeeAmountInSats- Uses version-specific ABI selection
- Uses the API-provided bump fee amount
678-714: LGTM! Order unlocking supports both V1 and V2 registries.Consistent with fee bumping, this implementation correctly handles registry address and version-specific ABI selection for both V1 and V2 registries.
722-783: LGTM! Order fetching now uses API instead of on-chain reads.The changes successfully replace on-chain order fetching with API-based fetching, improving reliability and performance as stated in the PR objectives. Both methods correctly use
mapRawOrderToOfframpOrderfor consistent data transformation.
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 (1)
sdk/src/gateway/client.ts (1)
1-1: Fix code formatting with Prettier.The pipeline reports code style issues detected by Prettier.
Run the following command to fix formatting:
prettier --write sdk/src/gateway/client.ts
♻️ Duplicate comments (1)
sdk/src/gateway/client.ts (1)
187-216: Type assertions bypass runtime safety—consider validation.Similar to the previous review comment, this function uses multiple type assertions (
order.status as OfframpOrderStatus,order.offrampRegistryAddress as Address, etc.) that bypass TypeScript's type safety. If the API returns unexpected data, this will cause runtime errors.Consider using validation guards or a library like Zod to validate the raw API response before casting:
private validateOfframpOrderStatus(status: string): OfframpOrderStatus { const validStatuses: OfframpOrderStatus[] = ['Active', 'Accepted', 'Processed', 'Refunded']; if (!validStatuses.includes(status as OfframpOrderStatus)) { throw new Error(`Invalid order status: ${status}`); } return status as OfframpOrderStatus; } public async mapRawOrderToOfframpOrder(order: OfframpRawOrder): Promise<OfframpOrder> { const status = this.validateOfframpOrderStatus(order.status); if (!isAddress(order.offrampRegistryAddress)) { throw new Error(`Invalid registry address: ${order.offrampRegistryAddress}`); } const offrampRegistryAddress = order.offrampRegistryAddress as Address; // ... rest of mapping }
🧹 Nitpick comments (1)
sdk/src/gateway/client.ts (1)
183-185: Add version parameter validation.The method assumes version will be either 1 or 2, but doesn't validate this. Invalid versions (0, negative, >2) will default to V2, which may not be the desired behavior.
Consider adding validation:
private getOfframpAbi(version: number) { + if (version !== 1 && version !== 2) { + throw new Error(`Unsupported offramp registry version: ${version}`); + } return version === 1 ? offrampCallerV1 : offrampCallerV2; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sdk/src/gateway/client.ts(14 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-17T11:18:53.195Z
Learnt from: slavastartsev
Repo: bob-collective/bob PR: 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/client.ts
🧬 Code graph analysis (1)
sdk/src/gateway/client.ts (2)
sdk/src/gateway/abi.ts (2)
offrampCallerV1(41-78)offrampCallerV2(80-172)sdk/src/gateway/types/offramp.ts (5)
OfframpRawOrder(124-141)OfframpOrder(86-121)OfframpOrderStatus(13-13)BumpFeeParams(149-152)UnlockOrderParams(154-158)
🪛 GitHub Actions: SDK
sdk/src/gateway/client.ts
[warning] 1-1: Code style issues found by Prettier. Run 'prettier --write' to fix.
⏰ 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/client.ts (8)
24-24: LGTM! Versioned ABI imports align with V2 support.The import of both
offrampCallerV1andofframpCallerV2correctly supports the new versioned offramp functionality.
393-396: LGTM! Affiliate fee parameters properly propagated.The conditional addition of affiliate fee query parameters is correctly implemented, ensuring both
affiliateFeeRecipientandaffiliateFeeSatsare provided together.
449-465: LGTM! Affiliate fee propagation is consistent.The affiliate fee parameters are correctly passed through to
fetchOfframpQuote, maintaining consistency with the onramp flow.
529-584: LGTM! Affiliate fee handling with reasonable defaults.The implementation correctly:
- Conditionally appends affiliate parameters to the API query when both are provided
- Normalizes
affiliateFeeRecipienttozeroAddresswhen not provided- Includes the normalized value in the returned quote
This ensures downstream code always receives a valid address.
594-623: LGTM! New orders correctly use V2 ABI with extended parameters.The method properly:
- Uses
offrampCallerV2for new order creation- Includes the new V2 fields:
satSolverFeeMax,satAffiliateFee, andaffiliateFeeRecipient- Maintains backward compatibility by using versioned ABIs for existing orders
632-664: LGTM! Enhanced with version-aware ABI selection and precise fee calculation.The improvements include:
- Registry address parameterization for multi-registry support
- More precise fee bump validation using
bumpFeeAmountInSatsinstead of a boolean flag- Dynamic ABI selection based on the order's registry version
The exact bump amount check (line 646-648) provides better UX compared to a simple boolean.
673-709: LGTM! Consistent version-aware implementation.The changes mirror the pattern in
bumpFeeForOfframpOrder:
- Registry address parameterization
- API-based order fetching
- Dynamic ABI selection based on order version
This ensures compatibility with both V1 and V2 registries.
717-778: LGTM! Architectural shift to API-based order fetching improves reliability.The changes implement the key PR objective:
fetchOfframpOrdernow retrieves data from the API instead of on-chain reads- Both methods use the new
mapRawOrderToOfframpOrderfor consistent data transformation- Registry address parameterization supports multi-registry queries
This approach improves performance and reliability as noted in the PR description.
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 (1)
sdk/test/gateway.test.ts (1)
692-726: Circular testing: mapping function testing itself.Line 714 generates
expectedResultby callingmapRawOrderToOfframpOrderon the mock data, and Line 726 compares the output ofgetOfframpOrders(which internally uses the samemapRawOrderToOfframpOrder) against this result. This creates circular testing where the mapping function effectively tests itself, which doesn't validate correctness.Consider manually constructing
expectedResultwith explicit field values or at minimum asserting individual critical fields:-const expectedResult = await gatewaySDK.mapRawOrderToOfframpOrder(mockResponse[0]); +const expectedResult: OfframpOrder = { + orderId: '0x0', + token: '0x4496ebE7C8666a8103713EE6e0c08cA0cD25b888', + satAmountLocked: BigInt('0x3e8'), + satSolverFeeMax: BigInt('0x181'), + satAffiliateFee: BigInt('0x181'), + affiliateFeeRecipient: zeroAddress, + status: OfframpOrderStatus.Processed, + btcTx: 'e8d52d6ef6ebf079f2d082dc683c9455178b64e0685c10e93882effaedde4474', + refundedEvmTx: null, + orderTimestamp: 1743679342, + shouldFeesBeBumped: false, + bumpFeeAmountInSats: null, + userAddress: zeroAddress, + offrampRegistryAddress: '0x70e5e53b4f48be863a5a076ff6038a91377da0dd', + offrampRegistryVersion: '0x0', + submitOrderEvmTx: '0xc02c65d88719d92a189d36da2f71108df3cc4d2611e711a5e049c896a4e06283', +};
🧹 Nitpick comments (1)
sdk/test/gateway.test.ts (1)
972-973: Test skipped pending V2 deployment.The test is appropriately skipped with a clear TODO indicating it depends on V2 offramp registry contract deployment. This is acceptable technical debt.
Once the V2 contract is deployed, remember to unskip this test and update the registry address. Would you like me to open a tracking issue for this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sdk/test/gateway.test.ts(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-17T11:18:53.195Z
Learnt from: slavastartsev
Repo: bob-collective/bob PR: 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/test/gateway.test.ts
🧬 Code graph analysis (1)
sdk/test/gateway.test.ts (1)
sdk/src/gateway/types/offramp.ts (1)
OfframpRawOrder(124-141)
🪛 Gitleaks (8.29.0)
sdk/test/gateway.test.ts
[high] 695-695: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant and well-structured changes to support Offramp V2, including affiliate fees and moving order detail fetching from on-chain calls to an API. The refactoring improves the SDK's architecture by centralizing order data and simplifying client-side logic. My review includes a few suggestions to improve robustness and maintainability, mainly concerning data validation from API responses and naming consistency.
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 (1)
sdk/test/gateway.test.ts (1)
870-915: Offramp executeQuote test nock is now out of sync with the client’s query parameters
fetchOfframpQuotealways sendsuserAddressin the query:const queryParams = new URLSearchParams({ amountInWrappedToken: amountInToken.toString(), token, userAddress, });But the test stubs:
const searchParams = new URLSearchParams({ amountInWrappedToken: '1000', token: outputTokenAddress, }); nock(SIGNET_GATEWAY_BASE_URL) .get(`/offramp-quote?${searchParams}`) .reply(201, { ... });Because the actual request includes
userAddress, it will not match this nock, causing a real network call or a failing test. Updating the stub to allow arbitrary query params (or to includeuserAddress) will fix this:- nock(`${SIGNET_GATEWAY_BASE_URL}`) - .get(`/offramp-quote?${searchParams}`) - .reply(201, { + nock(`${SIGNET_GATEWAY_BASE_URL}`) + .get('/offramp-quote') + .query(true) + .reply(201, {
🧹 Nitpick comments (5)
sdk/test/gateway.test.ts (2)
755-769: Prefer using the imported registry constant instead of hard‑coding the addressIn
canOrderBeUnlocked, the test passes a hard‑coded registry address string that appears to correspond to the importedTESTNET_TARGET_OFFRAMP_REGISTRY_ADDRESS. Using the constant here would:
- Remove duplication,
- Keep tests in sync if the registry address ever changes.
Example:
- const result = await gatewaySDK.canOrderBeUnlocked( - status, - orderTimestamp, - '0x70e5e53b4f48be863a5a076ff6038a91377da0dd' as Address - ); + const result = await gatewaySDK.canOrderBeUnlocked( + status, + orderTimestamp, + TESTNET_TARGET_OFFRAMP_REGISTRY_ADDRESS as Address + );
972-999:get offramp gas costtest tightly depends on live RPCsThis test asserts
gasFee > 0for BOB and Ethereum quotes, butgetOfframpCreateOrderGasCostcallsestimateFeesPerGas,getGasPrice, andestimateContractGason real RPC endpoints. If those endpoints are unavailable or estimation fails, the method catches the error and returnsgasFee = 0, causing this test to fail.Consider mocking the viem client / public client for gas estimation (or
getOfframpCreateOrderGasCostitself) so the test remains deterministic and doesn’t depend on external infra.Also applies to: 1000-1045
sdk/src/gateway/types/offramp.ts (1)
2-2: Offramp V2 types are consistent with the new ABI and client usageStructurally, the type updates line up with the V2 ABI and client logic:
OfframpQuotecarriesaffiliateFeeRecipient, which is then threaded intoOfframpCreateOrderParams.OfframpCreateOrderParamsnow usesofframpCallerV2andcreateOrderV2, with tuple components matching the ABI (satAmountToLock,satSolverFeeMax,satAffiliateFee,affiliateFeeRecipient, etc.).OfframpOrderandOfframpRawOrderare extended to include registry address, affiliate fee fields, registry version, bump‑fee amount, and user address, which the client maps inmapRawOrderToOfframpOrder.BumpFeeParamsandUnlockOrderParamsnow includeofframpRegistryAddress, aligning with how the client performs on‑chain calls.One thing worth double‑checking (together with
client.ts): thatfeeBreakdown.overallFeeSats,satSolverFeeMax, andsatAffiliateFeeare kept consistent with how the Solidity contract interprets them (i.e., whetheroverallFeeSatsalready includes affiliate fees or not), so that the user’samountReceiveInSatmatches on‑chain behavior.Also applies to: 31-45, 58-83, 85-121, 123-141, 149-152, 154-158
sdk/src/gateway/client.ts (2)
24-24: Offramp order mapping viamapRawOrderToOfframpOrderis coherent; consider caching claim delayThe new
mapRawOrderToOfframpOrder+getOfframpOrdersflow looks solid:
- Raw fields are converted to the richer
OfframpOrdershape withBigIntwhere appropriate.- New fields (
satAffiliateFee,affiliateFeeRecipient,offrampRegistryVersion,bumpFeeAmountInSats,userAddress) are correctly mapped fromOfframpRawOrder.canOrderBeUnlockedis computed per order usingclaimDelayAbiand the given registry address, which makes unlockability logic explicit.If you expect many orders per user, you might consider caching the
CLAIM_DELAYperofframpRegistryAddressinsidecanOrderBeUnlockedto avoid multiple identicalreadContractcalls, but that’s an optimization, not a blocker.Also applies to: 183-212, 708-717, 719-739
445-458: Potentially sendinguserAddress=undefinedto the offramp quote API whenfromUserAddressis absentIn
getOfframpQuoteyou always passparams.fromUserAddress as Address:const quote = await this.fetchOfframpQuote( tokenAddress, BigInt(params.amount || 0), params.fromUserAddress as Address, params.toUserAddress, params.affiliateFeeRecipient, params.affiliateFeeSats );
fetchOfframpQuotethen does:const queryParams = new URLSearchParams({ amountInWrappedToken: amountInToken.toString(), token, userAddress: userAddress, });If
params.fromUserAddressis omitted (e.g., yourget offramp gas cost ... with no from user addresstest), this will result inuserAddress=undefinedin the query string. Depending on how the backend validates this field, that may be brittle or error‑prone.Given you already synthesize a fallback address in
getOfframpCreateOrderGasCostwhenfromUserAddressis missing, consider doing something similar here, for example:const effectiveFromUser = (params.fromUserAddress as Address | undefined) ?? zeroAddress; const quote = await this.fetchOfframpQuote( tokenAddress, BigInt(params.amount || 0), effectiveFromUser, params.toUserAddress, params.affiliateFeeRecipient, params.affiliateFeeSats );This keeps the API request well‑formed even when callers don’t provide
fromUserAddress.Also applies to: 524-537
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sdk/src/gateway/abi.ts(2 hunks)sdk/src/gateway/client.ts(14 hunks)sdk/src/gateway/types/offramp.ts(7 hunks)sdk/test/gateway.test.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/src/gateway/abi.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-17T11:18:53.195Z
Learnt from: slavastartsev
Repo: bob-collective/bob PR: 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/client.tssdk/src/gateway/types/offramp.tssdk/test/gateway.test.ts
🧬 Code graph analysis (3)
sdk/src/gateway/client.ts (2)
sdk/src/gateway/types/offramp.ts (5)
OfframpRawOrder(124-141)OfframpOrder(86-121)OfframpOrderStatus(13-13)BumpFeeParams(149-152)UnlockOrderParams(154-158)sdk/src/gateway/abi.ts (1)
offrampCallerV2(41-133)
sdk/src/gateway/types/offramp.ts (1)
sdk/src/gateway/abi.ts (1)
offrampCallerV2(41-133)
sdk/test/gateway.test.ts (2)
sdk/src/gateway/types/offramp.ts (1)
OfframpRawOrder(124-141)sdk/src/gateway/client.ts (1)
MAINNET_GATEWAY_BASE_URL(76-76)
⏰ 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 (7)
sdk/test/gateway.test.ts (4)
8-12: Centralizing gateway constants and types in tests looks goodImporting
MAINNET_GATEWAY_BASE_URL,SIGNET_GATEWAY_BASE_URLandOfframpRawOrderfrom the SDK keeps tests aligned with the client implementation and avoids string/type drift. No issues here.Also applies to: 19-22
648-663: Offramp V2 args and affiliate fields are wired correctly in the create‑quote testThe test payload and expectations for
createOfframpOrdercorrectly reflect the new V2 ABI:
satSolverFeeMaxderived fromfeeBreakdown.overallFeeSats.satAffiliateFeefromfeeBreakdown.affiliateFeeSats.affiliateFeeRecipientflowing from the quote into the contract args.This gives good coverage of the new order‑creation surface.
Also applies to: 675-684
691-711: Nice reuse ofmapRawOrderToOfframpOrderin the offramp orders testUsing a typed
OfframpRawOrder[]and derivingexpectedResultviagatewaySDK.mapRawOrderToOfframpOrderkeeps this test resilient to future mapping changes and avoids duplicating mapping logic in the test. The mock shape matchesOfframpRawOrder(including the new registry/affiliate fields), so this looks solid.Also applies to: 714-727
743-753: Fetch offramp registry address test correctly targets the new API flowThe test for
fetchOfframpRegistryAddressnow stubs/offramp-registry-addressand asserts the returned value, matching the updated client method. This is a straightforward and accurate check of the new behavior.sdk/src/gateway/client.ts (3)
389-392: Affiliate fee query parameters are threaded correctly through onramp/offramp quotesFor both onramp and offramp:
- When
affiliateFeeRecipientandaffiliateFeeSatsare provided inGetQuoteParams, you appendaffiliateFeeandaffiliateFeeRecipientto the respective quote URLs.fetchOfframpQuotemirrors this behavior and normalizesaffiliateFeeRecipientto a non‑nullable value (affiliateFeeRecipient ?? zeroAddress), which is then returned in theOfframpQuote.This keeps the HTTP surface and the in‑memory
OfframpQuoteconsistent for affiliate flow. Looks good.Also applies to: 524-546, 560-577
627-657: Bump/unlock flows now correctly use registry‑aware API order dataThe changes to
bumpFeeForOfframpOrder,unlockOfframpOrder, andfetchOfframpOrderlook consistent:
- Both bump/unlock now require
offrampRegistryAddressand use/offramp-order(withregistryAddressandorderIdquery params) to obtain a fully mappedOfframpOrder.bumpFeeForOfframpOrderchecks that the order isActiveand thatbumpFeeAmountInSatsis non‑null before callingbumpFeeOfExistingOrderonofframpCallerV2.unlockOfframpOrderdisallowsProcessed/Refundedorders, re‑checks unlockability viacanOrderBeUnlocked, and then callsrefundOrderonofframpCallerV2.Error handling on the API read path (
!response.okbranch) is also reasonable. Overall this is a clean move away from direct on‑chain reads toward centralized API mapping.Also applies to: 666-700, 741-769
560-577: Verify fee breakdown semantics with backend and contract teamThe review concern is well-founded based on available evidence:
- Test data shows
overallFeeSats: 932 = inclusionFeeSats: 930 + protocolFeeSats: 1 + affiliateFeeSats: 1, indicatingoverallFeeSatsis the sum of all fee components.- Current code passes both
satSolverFeeMax = overallFeeSats(932) andsatAffiliateFee = affiliateFeeSats(1) separately to the contract.- If the contract expects
satSolverFeeMaxto represent only the solver-portion (i.e.,overallFeeSats - affiliateFeeSats = 931), this creates double-counting.The JSDoc comment on
satSolverFeeMax("Max sats to be paid as fees") is ambiguous and does not clarify whether it should include or exclude the affiliate portion.Confirm with the offramp contract and backend team whether
satSolverFeeMaxshould be:
overallFeeSats(as currently implemented), oroverallFeeSats - affiliateFeeSats(solver-only portion)Then adjust lines 608–609 and the secondary location (589–618) if needed.
📝 PR Description
Summary by CodeRabbit
New Features
Improvements
Tests