feat: add execute quote method #616
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
""" WalkthroughThis update refactors the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GatewayApiClient
participant WalletClient
participant PublicClient
participant BTCSigner
User->>GatewayApiClient: executeQuote(order, btcSigner, walletClient, publicClient)
alt Onramp
GatewayApiClient->>GatewayApiClient: startOrder()
GatewayApiClient->>BTCSigner: signAllInputs(psbtBase64)
BTCSigner-->>GatewayApiClient: signedTx
GatewayApiClient->>GatewayApiClient: finalizeOrder(signedTx)
GatewayApiClient-->>User: bitcoinTxid
else Offramp
GatewayApiClient->>GatewayApiClient: createOfframpOrder()
GatewayApiClient->>GatewayApiClient: fetchOfframpRegistryAddress()
GatewayApiClient->>WalletClient: approve(erc20, MaxUint256) (if needed)
GatewayApiClient->>PublicClient: simulateContractCall()
GatewayApiClient->>WalletClient: writeContract()
GatewayApiClient->>PublicClient: waitForTransactionReceipt()
GatewayApiClient-->>User: txHash
else Stake
GatewayApiClient->>GatewayApiClient: buildStakeParams()
GatewayApiClient->>PublicClient: simulateContractCall(with state override)
GatewayApiClient->>WalletClient: writeContract()
GatewayApiClient->>PublicClient: waitForTransactionReceipt()
GatewayApiClient-->>User: txHash
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (8)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
sdk/src/gateway/strategy.ts (2)
264-264: Non-null assertion added for totalSupplyThe non-null assertion (
!) assumes thattotalSupplywill never be null or undefined. While this satisfies TypeScript's strict null checks, it would be safer to add runtime validation.Consider adding validation instead of using non-null assertion:
-totalUnderlying: totalSupply!, +totalUnderlying: totalSupply ?? 0n,
322-323: Non-null assertions in return valuesSimilar to previous comments, the use of non-null assertions here assumes these values will always be present.
Add validation:
-address: underlyingAddress!, -totalUnderlying: totalSupply!, +address: underlyingAddress ?? zeroAddress, +totalUnderlying: totalSupply ?? 0n,sdk/src/gateway/types.ts (1)
396-400: Consider validating hex length foroutputScriptConfining
outputScriptto0x${string}guarantees a hex prefix but still allows odd-length or empty strings at compile time.
If you want to avoid malformed scripts slipping through, add a branded type or a runtime assertion:type HexString<T extends string = string> = `0x${T}` & { readonly __hexBrand: unique symbol }; function assertEvenLengthHex(value: string): asserts value is HexString { if (!/^0x([0-9a-fA-F]{2})+$/.test(value)) { throw new Error('outputScript must be 0x-prefixed and byte-aligned'); } }sdk/src/gateway/client.ts (1)
680-699: Race-condition & gas optimisation opportunity in allowance flowCalling
approveevery timeamount > allowancecan front-run the create-order tx and wastes gas if another approval is inflight.
Optionally checkpendingallowance viapublicClient.getTransactionCountor ask users to provide a max-approval upfront to skip the second RPC round-trip.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (12)
sdk/src/gateway/client.ts(13 hunks)sdk/src/gateway/strategy.ts(4 hunks)sdk/src/gateway/tokens.ts(2 hunks)sdk/src/gateway/types.ts(3 hunks)sdk/src/inscription.ts(1 hunks)sdk/src/ordinal-api/index.ts(1 hunks)sdk/src/ordinals/commit.ts(1 hunks)sdk/src/ordinals/index.ts(1 hunks)sdk/src/wallet/utxo.ts(3 hunks)sdk/test/gateway.test.ts(5 hunks)sdk/test/ordinal-api.test.ts(3 hunks)sdk/tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
sdk/test/ordinal-api.test.ts (1)
sdk/src/ordinal-api/index.ts (1)
OutputJson(116-177)
sdk/src/gateway/types.ts (1)
sdk/src/gateway/abi.ts (1)
strategyCaller(3-39)
🔇 Additional comments (19)
sdk/src/ordinal-api/index.ts (1)
143-146: LGTM! Well-documented property addition.The
confirmationsproperty has been properly added to theOutputJsoninterface with appropriate JSDoc documentation. This enriches the output data structure with valuable confirmation count information.sdk/test/ordinal-api.test.ts (3)
2-2: LGTM! Appropriate import update.Import statement correctly updated to include
expectfrom vitest, which is used later in the test file.
73-73: LGTM! Effective type adaptation.Good use of TypeScript's
Omitutility type to exclude the newly addedconfirmationsproperty from the expected test object, avoiding the need to update the test fixture.
85-85: LGTM! Improved assertion approach.Excellent change from
assert.deepStrictEqualtoexpect(...).toMatchObject(...), which allows for partial matching rather than requiring exact equality. This handles the addition of theconfirmationsproperty in the actual response without having to specify it in the test expectations.sdk/src/inscription.ts (1)
44-44: Improved null safety for the body propertyExcellent change! Adding
Buffer.alloc(0)as a fallback ensures thatthis.bodyis always initialized with a valid Buffer instance, even when no body argument is provided. This is particularly important for strict null checks.sdk/tsconfig.json (1)
15-15: Great addition of strictNullChecksThis compiler option enhances type safety by detecting potential null/undefined errors at compile time. This is a positive step toward more robust code and aligns perfectly with the other null-safety improvements in this PR.
sdk/src/ordinals/index.ts (1)
30-30: Safer handling of tapLeafScript in PSBT constructionThis improvement ensures that
tapLeafScriptis only wrapped in an array when it exists, preventing the passing of arrays with undefined values. This change is essential for proper PSBT construction and aligns well with the new strict null checks.sdk/src/ordinals/commit.ts (2)
43-43: Modern and safer array accessUsing
.at(-1)is a more modern and readable way to access the last element of an array compared to manual index calculation. This also adds optional chaining for better null safety.
48-48: Enhanced null safety for controlBlockAdding
Buffer.alloc(0)as a fallback ensures thecontrolBlockis always a valid Buffer, even whencblockis null or undefined. This is important for consistent behavior and aligns with the strictNullChecks compiler option.sdk/src/gateway/tokens.ts (1)
372-386: Well-implemented token address resolution functionThis new function centralizes token address resolution logic, which improves code maintainability. It properly handles both addresses and symbols, and has clear error handling.
Two minor suggestions for improvement:
- Add JSDoc comments to document the parameters and return value
- Consider normalizing the input token string to lowercase for more consistent lookups when it's a symbol
/** * Resolves a token symbol or address to its canonical address for a given chain * @param chain The chain ID or name * @param toToken The token symbol or address * @returns The resolved token address * @throws Error if the token cannot be resolved */ export function getTokenAddress(chain: string | number, toToken: string): Address { const isMainnet = chain === ChainId.BOB || (typeof chain === 'string' && chain.toLowerCase() === Chain.BOB); const isTestnet = chain === ChainId.BOB_SEPOLIA || (typeof chain === 'string' && chain.toLowerCase() === Chain.BOB_SEPOLIA); if (isAddress(toToken)) { return toToken; - } else if (isMainnet && SYMBOL_LOOKUP[chain][toToken]) { + } else if (isMainnet && SYMBOL_LOOKUP[chain][toToken.toLowerCase()]) { - return SYMBOL_LOOKUP[chain][toToken].address; + return SYMBOL_LOOKUP[chain][toToken.toLowerCase()].address; - } else if (isTestnet && SYMBOL_LOOKUP[chain][toToken]) { + } else if (isTestnet && SYMBOL_LOOKUP[chain][toToken.toLowerCase()]) { - return SYMBOL_LOOKUP[chain][toToken].address; + return SYMBOL_LOOKUP[chain][toToken.toLowerCase()].address; } else { throw new Error('Unknown output token'); } }sdk/test/gateway.test.ts (4)
709-764: Good test implementation for new executeQuote methodThe test properly verifies the behavior of the new
executeQuotemethod for onramp quotes. It mocks the necessary responses, spies on the relevant methods, and asserts the expected result.The test covers:
- Proper setup of mocks and test data
- Calling the executeQuote method with correct parameters
- Verifying that startOrder and finalizeOrder are called
- Asserting that a Bitcoin transaction ID is returned
This ensures that the new functionality is properly tested.
561-561: Parameter naming updated from bitcoinUserAddress to toUserAddressThis change aligns with the overall API improvement in the PR to make parameter naming more consistent.
650-650: Parameter naming updated from bitcoinUserAddress to toUserAddressThis change aligns with the overall API improvement in the PR to make parameter naming more consistent.
660-660: Updated mock response format for offramp registry addressThe mock response is now a raw string instead of a JSON string, matching the updated API response format.
sdk/src/wallet/utxo.ts (2)
108-108: Made publicKey parameter optional with sensible default for isSignetThis change improves the API by making the
publicKeyparameter optional and providing a default value forisSignet. This increases flexibility while maintaining backward compatibility.
370-370: Updated return type to allow undefinedThe return type now correctly includes
undefinedas a possible return value, which improves type safety.sdk/src/gateway/types.ts (2)
50-53: TypedstrategyFunctionNameis a welcome safety improvementReferencing the name directly from
strategyCaller((typeof strategyCaller)[number]['name']) eliminates stringly-typed errors at compile-time.
444-449: Nullable tx hashes enhance clarityMarking
submitOrderEvmTxandbtcTxasstring | nullexplicitly communicates that these fields may be absent—nice improvement for consumers.sdk/src/gateway/client.ts (1)
189-195: Potential undefined look-ups inADDRESS_LOOKUP
ADDRESS_LOOKUP[this.chainId][quote.baseTokenAddress](and the output-token line below) will throw if either lookup is missing.
Consider defensive fallback logic or a clear error so the SDK fails gracefully when token metadata is incomplete.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
sdk/test/gateway.test.ts (1)
651-654:⚠️ Potential issueAdd error handling for undefined psbtBase64
The code uses a non-null assertion (
psbtBase64!) without verifying that the value exists. This could lead to runtime errors ifstartOrderdoesn't return a PSBT.-const bitcoinTxHex = await btcSigner.signAllInputs(psbtBase64!); +if (!psbtBase64) { + throw new Error('No PSBT was created. Check if startOrder was successful.'); +} +const bitcoinTxHex = await btcSigner.signAllInputs(psbtBase64);
♻️ Duplicate comments (3)
sdk/src/gateway/client.ts (3)
193-196:⚠️ Potential issueMissing error handling in fetchOfframpRegistryAddress
The method doesn't check if the HTTP response was successful before parsing the result, which can lead to unexpected behavior with error responses.
async fetchOfframpRegistryAddress(): Promise<Address> { const response = await this.fetchGet(`${this.baseUrl}/offramp-registry-address`); - return response.text() as Promise<Address>; + if (!response.ok) { + const message = await response.text().catch(() => 'Failed to fetch offramp registry address'); + throw new Error(`Offramp registry API error: ${message}`); + } + return (await response.text()) as Address; }
651-652:⚠️ Potential issueMissing null check before using psbtBase64
The code uses the non-null assertion operator (
!) onpsbtBase64without first checking if it exists. This could cause runtime errors ifstartOrderdoesn't return a valid PSBT.-const bitcoinTxHex = await btcSigner.signAllInputs(psbtBase64!); +if (!psbtBase64) { + throw new Error('startOrder did not return a PSBT. Ensure fromChain is "bitcoin" and wallet connector is configured.'); +} +const bitcoinTxHex = await btcSigner.signAllInputs(psbtBase64);
749-759:⚠️ Potential issueImprove getFinal function to handle undefined values
The
getFinalfunction uses type assertions to force the type checker to treat potentially undefined values as non-nullable, which could lead to runtime errors.-function getFinal<L, R>(base?: L, output?: R): NonNullable<L | R> { - return order.status - ? order.strategyAddress - ? output - ? output - : (base as NonNullable<typeof base>) - : (base as NonNullable<typeof base>) - : order.strategyAddress - ? (output as NonNullable<typeof output>) - : (base as NonNullable<typeof base>); +function getFinal<L, R>(base: L | undefined, output: R | undefined): NonNullable<L | R> { + const result = + order.status + ? order.strategyAddress + ? output ?? base + : base + : order.strategyAddress + ? output + : base; + + if (result === undefined) { + throw new Error('Missing token address/amount in order payload'); + } + return result as NonNullable<L | R>; }
🧹 Nitpick comments (3)
sdk/test/gateway.test.ts (1)
767-780: Consider typing the mock objects more preciselyThe mock objects for
walletClientandpublicClientare cast usingas unknown aswhich bypasses type checking. Consider creating properly typed mocks.-const walletClient = { +// Create a properly typed mock +const walletClient = { writeContract: () => Promise.resolve('0x35f5bca7f984f4ed97888944293b979f3abb198a5716d04e10c6bdc023080075'), -} as unknown as Parameters<typeof gatewaySDK.executeQuote>[2]; +} satisfies Partial<WalletClient<Transport, ViemChain, Account>> as Parameters<typeof gatewaySDK.executeQuote>[2];sdk/src/gateway/client.ts (2)
590-601: Better error handling for PSBT creationThe code now throws a specific error message when the PSBT creation fails, which will make debugging easier. However, the function could be improved by clarifying the return type in the JSDoc.
/** * Start an order via the Gateway API to reserve liquidity. This is step 1 of 2, see the {@link finalizeOrder} method. * * @param gatewayQuote The quote given by the {@link getQuote} method. * @param params The parameters for the quote, same as before. + * @returns {Promise<GatewayStartOrder & { psbtBase64?: string }>} Success object with PSBT if fromChain is Bitcoin. */
777-778: Consider adding null check for outputTokenThe property access for
order.outputTokenAddress!uses a non-null assertion without a check, which could lead to runtime errors.-outputToken: ADDRESS_LOOKUP[chainId][order.outputTokenAddress!], +outputToken: order.outputTokenAddress ? ADDRESS_LOOKUP[chainId][order.outputTokenAddress] : undefined,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
sdk/src/gateway/client.ts(15 hunks)sdk/src/gateway/tokens.ts(3 hunks)sdk/test/gateway.test.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/src/gateway/tokens.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
sdk/test/gateway.test.ts (3)
sdk/src/gateway/client.ts (1)
MAINNET_GATEWAY_BASE_URL(60-60)sdk/src/gateway/types.ts (2)
GatewayQuote(220-237)GatewayTokensInfo(336-341)sdk/src/gateway/tokens.ts (1)
getTokenAddress(384-398)
sdk/src/gateway/client.ts (3)
sdk/src/gateway/tokens.ts (1)
getTokenAddress(384-398)sdk/src/gateway/types.ts (4)
OfframpCreateOrderParams(385-405)GatewayQuote(220-237)GatewayTokensInfo(336-341)GatewayQuoteParams(91-128)sdk/src/wallet/utxo.ts (1)
getAddressInfo(22-24)
🪛 Gitleaks (8.26.0)
sdk/test/gateway.test.ts
558-558: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
647-647: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (11)
sdk/test/gateway.test.ts (4)
1-7: Good addition of necessary importsThe imports added are appropriate for the new functionality being tested, including bitcoin-js, viem utilities, and Ethereum's ZeroAddress.
709-721: Good test setup for the new executeQuote onramp featureThis test properly sets up the testing environment with a mock BTC signer and spies on the internal methods that should be called during execution.
817-820: Good test validation for offramp flowThe test correctly verifies the transaction hash is returned and that the expected methods are called exactly once.
558-558: False positive from static analysis for API keysThe static analysis tool flagged these lines as containing API keys, but these are just token addresses used in tests.
Also applies to: 647-647
🧰 Tools
🪛 Gitleaks (8.26.0)
558-558: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
sdk/src/gateway/client.ts (7)
32-48: Good import organizationThe imports have been properly organized, with the addition of necessary viem utilities and types for contract interactions.
157-158: Improved token address handling with centralized functionThe code now uses the centralized
getTokenAddresshelper function instead of duplicating the logic in multiple places, which is a good refactoring.Also applies to: 302-303
263-264: Improved error message in fetchOfframpQuoteThe error message now includes the query parameters, which will make debugging easier.
328-329: Improved type safety for outputScriptThe explicit type definition for
outputScriptas a hex-prefixed string improves type safety.
615-700: Well-implemented executeQuote methodThe new
executeQuotemethod provides a unified API for both onramp and offramp flows. It handles token approvals, contract interactions, and transaction monitoring, which is a great addition.
1034-1042: Simplified fetchGet implementationRemoving the unnecessary
awaitinfetchGetis a good cleanup. The function now correctly returns the Promise directly.
1102-1105: Good helper function for creating viem clientsThe
viemClientfunction is a good abstraction that centralizes the creation of viem clients, which promotes code reuse.
slavastartsev
force-pushed
the
feat/execute-quote
branch
from
4e18be0 to
d84c23b
Compare
…eat/execute-quote
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
sdk/src/gateway/client.ts (2)
197-200: Missingresponse.okguard repeats an earlier issueThe method still returns
response.text()without verifyingresponse.ok. A 4xx/5xx will silently propagate garbage data and break downstream logic – exactly the problem raised in the previous review.
818-828:getFinalstill risks returningundefinedThe earlier feedback about replacing type-assertions with explicit checks hasn’t been addressed; the helper can still emit
undefinedat runtime. Please revisit the suggested refactor or add a runtime guard.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
sdk/src/gateway/client.ts(19 hunks)sdk/src/gateway/types.ts(4 hunks)sdk/src/ordinal-api/index.ts(1 hunks)sdk/test/ordinal-api.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- sdk/test/ordinal-api.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- sdk/src/ordinal-api/index.ts
- sdk/src/gateway/types.ts
🔇 Additional comments (1)
sdk/src/gateway/client.ts (1)
665-668: 🛠️ Refactor suggestionUnnecessary non-null assertion & unclear error message
startOrderalready guaranteespsbtBase64is defined or throws, so!is redundant and hides future regressions.
Additionally, the follow-up error'no psbt'is vague.-const bitcoinTxHex = await btcSigner.signAllInputs(psbtBase64!); - -if (!bitcoinTxHex) throw new Error('no psbt'); +const bitcoinTxHex = await btcSigner.signAllInputs(psbtBase64); + +if (!bitcoinTxHex) + throw new Error('Failed to sign PSBT – wallet connector returned empty hex.');Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
sdk/src/gateway/client.ts (3)
197-200:fetchOfframpRegistryAddressstill ignores HTTP errorsThis method doesn't check
response.okbefore callingtext(), unlike other API methods in this file. This could result in returning error responses as valid addresses.-const response = await this.fetchGet(`${this.baseUrl}/offramp-registry-address`); -return response.text() as Promise<Address>; +const response = await this.fetchGet(`${this.baseUrl}/offramp-registry-address`); +if (!response.ok) { + const errorMessage = await response.text() || 'Failed to fetch registry address'; + throw new Error(`Offramp registry API error: ${errorMessage}`); +} +return (await response.text()) as Address;
818-828:getFinalstill has type safety issuesThis function still uses type assertions without proper runtime checks, which could lead to
undefinedvalues at runtime.-function getFinal<L, R>(base?: L, output?: R): NonNullable<L | R> { - return order.status - ? order.strategyAddress - ? output - ? output // success - : (base as NonNullable<typeof base>) // failed - : (base as NonNullable<typeof base>) // success - : order.strategyAddress // pending - ? (output as NonNullable<typeof output>) - : (base as NonNullable<typeof base>); +function getFinal<L, R>(base: L | undefined, output: R | undefined): NonNullable<L | R> { + const result = order.status + ? order.strategyAddress + ? output ?? base + : base + : order.strategyAddress + ? output + : base; + + if (result === undefined) { + throw new Error('Missing token address/amount in order payload'); + } + return result as NonNullable<L | R>; +}
607-769:⚠️ Potential issueNew unified
executeQuotemethod implementationThis method provides a high-level interface that handles the full flow of executing quotes, including onramp, offramp, and staking operations.
However, there are a few issues that need to be addressed:
- On line 667, you're using a non-null assertion (
psbtBase64!) without first checking if it exists.- The
stateOverrideparameter (lines 750-761) has an incorrect structure for viem's API.-const bitcoinTxHex = await btcSigner.signAllInputs(psbtBase64!); +if (!psbtBase64) { + throw new Error('PSBT not created. Ensure fromChain is Bitcoin and parameters are correct.'); +} +const bitcoinTxHex = await btcSigner.signAllInputs(psbtBase64);-stateOverride: [ - // overriding token allowance - { - address: order.params.token, - stateDiff: [ - { - slot: allowanceSlot, - value: maxAllowance, - }, - ], - }, -], +stateOverride: { + [order.params.token]: { + storageDiff: { + [allowanceSlot]: maxAllowance, + }, + }, +},
🧹 Nitpick comments (1)
sdk/src/gateway/client.ts (1)
669-669: Redundant null check after awaitThis check for
bitcoinTxHexis executed after awaiting the promise. IfsignAllInputsfails, an exception would be thrown before reaching this line, making this check redundant.-if (!bitcoinTxHex) throw new Error('no psbt');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
sdk/src/gateway/client.ts(24 hunks)sdk/src/gateway/index.ts(1 hunks)sdk/src/gateway/types.ts(9 hunks)sdk/src/ordinal-api/index.ts(1 hunks)sdk/test/gateway.test.ts(15 hunks)
✅ Files skipped from review due to trivial changes (1)
- sdk/src/gateway/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- sdk/src/ordinal-api/index.ts
- sdk/test/gateway.test.ts
- sdk/src/gateway/types.ts
🔇 Additional comments (10)
sdk/src/gateway/client.ts (10)
32-32: Improved token address resolution with centralized helperThe
getTokenAddressfunction fromtokens.tshas been properly imported and used in place of manual token address resolution logic, making the code more maintainable.
161-162: Using centralized token resolution functionGood refactoring to use the dedicated
getTokenAddressfunction instead of duplicating token address resolution logic.
267-267: Improved error reportingGood improvement to include query parameters in the error message, making debugging easier.
306-307: Using centralized token address resolutionGood refactoring to use the dedicated
getTokenAddressfunction for consistent token resolution.
332-333: Added explicit type casting for output scriptThe type cast to
0x${string}ensures the output script is properly typed as a hex string, improving type safety.
429-430: Standardized timestamp handlingChanged
orderTimestampfrombiginttonumberfor consistent timestamp handling across the codebase.Also applies to: 440-440
594-604: Improved error handling for failed PSBT creationBetter structured return statement and clearer error message for when Bitcoin PSBT creation fails.
1104-1113: Added aggregated orders methodGreat addition! This method provides a convenient way to fetch both onramp and offramp orders in a single call.
1115-1123: Extracted common fetch patternGood refactoring to extract the common fetch GET pattern to reduce duplication.
1183-1186: Added helper for creating viem clientsGood abstraction to centralize the client creation logic based on the chain.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sdk/test/gateway.test.ts (1)
785-798: Consider adding more comprehensive error testing.While the happy path for offramp execution is tested, consider adding test cases for error scenarios such as:
- Insufficient allowance
- Contract execution failures
- Network errors during API calls
This would improve the robustness of the test suite and ensure proper error handling in the implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
sdk/src/gateway/tokens.ts(2 hunks)sdk/test/gateway.test.ts(15 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/src/gateway/tokens.ts
🧰 Additional context used
🪛 GitHub Actions: SDK
sdk/test/gateway.test.ts
[error] 61-61: Test failure: 'Gateway Tests > should return evm txid for offramp' failed with error 'Cannot cast mainnet to non-mainnet'. Stack trace includes castTestnetTo at bitcoin-address-validation/src/index.ts:61:11.
🔇 Additional comments (12)
sdk/test/gateway.test.ts (12)
1-7: Organized imports with proper type modules.The imports have been reorganized and expanded to include additional libraries and types such as bitcoinjs-lib, ethers' ZeroAddress, nock, and various utilities from viem, which aligns with the new requirements for the executeQuote functionality.
10-10: New token utility imports added.The addition of
getTokenAddressandSYMBOL_LOOKUPimports is appropriate for the new token address resolution functionality needed by the executeQuote method.
12-17: Updated type imports for enhanced type safety.The type imports have been updated to include new types and refinements:
BuildStakeParamshas been replaced withStakeParams- Added
GatewayQuote,GatewayTokensInfo, andOfframpOrderThese changes reflect the type refinements made in the gateway types file and improve type safety.
345-345: Method name changed to be more specific.The method call was changed from
getOrderstogetOnrampOrders, aligning with the new specialized order retrieval methods added in the client implementation.
558-559: Parameter names corrected to match updated method signatures.The parameter names were appropriately corrected from
toTokentofromTokenand frombitcoinUserAddresstotoUserAddressto match the updated method signatures in the client implementation.Also applies to: 561-562
588-588: Type changes for orderTimestamp from BigInt to number.The type assertions for
orderTimestampwere changed fromBigInttonumberto reflect the type changes inOfframpOrderand related types in the types file.Also applies to: 600-600
616-616: New method for retrieving offramp orders.The implementation now correctly uses the dedicated
getOfframpOrdersmethod, which provides better separation of concerns for different order types.
660-660: API response format updated.The API response format has been updated to return a plain string rather than a JSON string that needs to be parsed.
669-670: Timestamp handling updated to use seconds.The timestamp handling now correctly uses seconds since epoch (standard Unix timestamp) with
Math.floor(Date.now() / 1000), which aligns with the requirements for order timestamp processing.
709-764: New test case for executing an onramp quote.This test verifies that executing an onramp quote returns a Bitcoin transaction ID by properly mocking:
- Order creation
- Bitcoin signer functionality
- Order finalization HTTP calls
The test is well-structured and covers the core functionality of the onramp flow.
822-858: New test case for executing a stake quote.This test verifies that executing a stake quote returns an EVM transaction ID by mocking contract interactions and staking parameter building. The test case provides good coverage for the stake flow in the executeQuote method.
860-875: New test for the aggregated getOrders method.This test verifies that the new
getOrdersmethod correctly calls bothgetOnrampOrdersandgetOfframpOrdersand returns combined results. The test uses appropriate spies to verify that both underlying methods are called.
slavastartsev
force-pushed
the
feat/execute-quote
branch
from
6f71e0d to
e798302
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
sdk/src/gateway/client.ts (4)
825-835: 🛠️ Refactor suggestionImprove safety of
getFinalfunctionThe
getFinalfunction uses type assertions without runtime checks, which could lead to undefined values at runtime.-function getFinal<L, R>(base?: L, output?: R): NonNullable<L | R> { - return order.status - ? order.strategyAddress - ? output - ? output // success - : (base as NonNullable<typeof base>) // failed - : (base as NonNullable<typeof base>) // success - : order.strategyAddress // pending - ? (output as NonNullable<typeof output>) - : (base as NonNullable<typeof base>); +function getFinal<L, R>(base: L | undefined, output: R | undefined): NonNullable<L | R> { + const result = + order.status + ? order.strategyAddress + ? output ?? base + : base + : order.strategyAddress + ? output + : base; + + if (result === undefined) { + throw new Error('Missing token address/amount in order payload'); + } + return result as NonNullable<L | R>; +}
214-217:⚠️ Potential issueMissing error response handling in
fetchOfframpRegistryAddressThe method calls
response.text()unconditionally without checking if the response was successful. If the API returns a 4xx or 5xx error, it will attempt to parse the error response as an address.async fetchOfframpRegistryAddress(): Promise<Address> { const response = await this.fetchGet(`${this.baseUrl}/offramp-registry-address`); + if (!response.ok) { + const errorMessage = await response.text().catch(() => 'Failed to fetch registry address'); + throw new Error(`Offramp registry API error: ${errorMessage}`); + } return response.text() as Promise<Address>; }
690-692:⚠️ Potential issueMissing null check before using non-null assertion
The code uses
psbtBase64!without verifying that it's defined, which could cause a runtime error.-const bitcoinTxHex = await btcSigner.signAllInputs(psbtBase64!); - -if (!bitcoinTxHex) throw new Error('no psbt'); +if (!psbtBase64) { + throw new Error('Failed to obtain PSBT from startOrder. Ensure fromChain is "bitcoin" and wallet is configured correctly.'); +} +const bitcoinTxHex = await btcSigner.signAllInputs(psbtBase64); + +if (!bitcoinTxHex) throw new Error('Failed to sign PSBT. The signer returned an empty transaction.');
745-745:⚠️ Potential issuePotential undefined reference to wallet account address
The code accesses
walletClient.account.addresswithout checking ifaccountoraddressis defined.-args: [walletClient.account.address as Address, result.strategyAddress], +if (!walletClient.account?.address) { + throw new Error('Wallet client account address is not defined'); +} +args: [walletClient.account.address as Address, result.strategyAddress],
🧹 Nitpick comments (2)
sdk/src/gateway/client.ts (2)
621-628: Improved error handling for PSBT creationThe enhanced error handling for PSBT creation is good, but the code path could be simplified further.
-if (!psbtBase64) { - throw new Error( - 'Failed to create bitcoin psbt. Please check `fromChain` and `fromUserAddress` parameters.' - ); -} - -throw new Error('Failed to create bitcoin psbt due to an unexpected error.'); +throw new Error( + psbtBase64 + ? 'Failed to create bitcoin psbt due to an unexpected error.' + : 'Failed to create bitcoin psbt. Please check `fromChain` and `fromUserAddress` parameters.' +);
711-723: Duplicate token approval logicThe token allowance check and approval logic is duplicated for both offramp and stake paths.
Consider extracting this into a helper method:
private async ensureTokenAllowance( tokenAddress: Address, spenderAddress: Address, amount: bigint, walletClient: WalletClient<Transport, ViemChain, Account>, publicClient: PublicClient<Transport> ): Promise<void> { const allowance = await publicClient.readContract({ address: tokenAddress, abi: erc20Abi, functionName: 'allowance', args: [walletClient.account.address as Address, spenderAddress], }); if (amount > allowance) { const { request } = await publicClient.simulateContract({ account: walletClient.account, address: tokenAddress, abi: erc20Abi, functionName: 'approve', args: [spenderAddress, MaxUint256], }); const approveTxHash = await walletClient.writeContract(request); await publicClient.waitForTransactionReceipt({ hash: approveTxHash }); } }Then call it in both places:
await this.ensureTokenAllowance( tokenAddress, offrampRegistryAddress, BigInt(order.params.amount), walletClient, publicClient ); // And later for staking: await this.ensureTokenAllowance( order.params.token, result.strategyAddress, BigInt(order.params.amount), walletClient, publicClient );Also applies to: 748-760
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sdk/src/gateway/client.ts(25 hunks)
🔇 Additional comments (5)
sdk/src/gateway/client.ts (5)
323-326: Improved token address resolutionGood job using the new
getTokenAddressutility function to consistently resolve token addresses.
137-206: Well-structured discriminated union for quote handlingThe refactored
getQuotemethod with discriminated union types provides a clean API for handling both onramp and offramp quotes. The separation of concerns between onramp and offramp logic improves readability.
1110-1127: Well-implemented order aggregation methodThe new
getOrdersmethod effectively combines both onramp and offramp orders with proper type discrimination, making it easier for clients to handle different order types uniformly.
1082-1084: Improved error message with contextAdding the serialized strategies list to the error message is helpful for debugging when a strategy is not found.
644-686: Well-designed method signature forexecuteQuoteThe
executeQuotemethod has a comprehensive type definition that clearly separates the different quote types and their specific parameters. The discriminated union pattern makes the API both type-safe and intuitive.
* fix: overload getQuote method * chore: merge types for get quote --------- Co-authored-by: Slava <slava@interlay.io>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
sdk/test/gateway.test.ts (2)
794-809: Fix Bitcoin address in Signet offramp testThe test is using a mainnet Bitcoin address (bc1qar0srrr7xfkvy5l643lydnw9re59gtzzwf5mdq) with the Signet test network, which will cause validation to fail. This was previously flagged in pipeline failures.
850-889:⚠️ Potential issueAccount property needed in mockWalletClient for stake execution
The
mockWalletClientin the stake execution test needs anaccountproperty with anaddressfield that theexecuteQuotemethod will access.Apply this diff to add the missing account property:
const mockWalletClient = { + account: { + address: zeroAddress, + }, writeContract: () => Promise.resolve('0x35f5bca7f984f4ed97888944293b979f3abb198a5716d04e10c6bdc023080075'), } as unknown as Parameters<typeof gatewaySDK.executeQuote>[2];sdk/src/gateway/client.ts (3)
218-221:⚠️ Potential issueAdd response status check in fetchOfframpRegistryAddress
The method doesn't check if the response is successful before parsing the text, which could lead to unexpected behavior if the request fails.
async fetchOfframpRegistryAddress(): Promise<Address> { const response = await this.fetchGet(`${this.baseUrl}/offramp-registry-address`); + if (!response.ok) { + const message = await response.text().catch(() => 'Failed to fetch registry address'); + throw new Error(`Offramp registry API error: ${message}`); + } return response.text() as Promise<Address>; }
694-694:⚠️ Potential issueAdd null check before using psbtBase64 in executeQuote
The code uses a non-null assertion (
!) withpsbtBase64without verifying its presence, which can cause runtime errors if undefined.-const bitcoinTxHex = await btcSigner.signAllInputs(psbtBase64!); +if (!psbtBase64) { + throw new Error('startOrder did not return a PSBT. Ensure fromChain is "bitcoin" and wallet connector is configured.'); +} +const bitcoinTxHex = await btcSigner.signAllInputs(psbtBase64);
748-750:⚠️ Potential issueVerify account.address is defined before accessing
The code accesses
walletClient.account.addresswithout checking if it exists, which could lead to runtime errors.+if (!walletClient.account?.address) { + throw new Error('Wallet client account address is not defined'); +} const allowance = await publicClient.readContract({ address: order.params.token, abi: erc20Abi, functionName: 'allowance', args: [walletClient.account.address as Address, result.strategyAddress], });
🧹 Nitpick comments (2)
sdk/test/gateway.test.ts (1)
908-957: Consider removing console.log statements from testsThe test includes console.log statements that print object keys, which may pollute test output.
- console.log('Type keys offramp:', Object.keys(result_onramp)); - console.log('Type keys offramp:', Object.keys(result_offramp));sdk/src/gateway/client.ts (1)
288-288: Remove query parameters from error message to prevent potential data leakageThe error message includes the query parameters, which might leak sensitive information in logs.
-throw new Error(`Offramp API Error: ${errorMessage} ${queryParams}`); +throw new Error(`Offramp API Error: ${errorMessage}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sdk/src/gateway/client.ts(29 hunks)sdk/test/gateway.test.ts(23 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
sdk/test/gateway.test.ts (3)
sdk/src/gateway/types.ts (4)
StakeParams(26-39)OfframpOrder(430-451)GatewayQuote(218-235)GatewayTokensInfo(334-339)sdk/src/gateway/client.ts (2)
MAINNET_GATEWAY_BASE_URL(60-60)SIGNET_GATEWAY_BASE_URL(72-72)sdk/src/gateway/tokens.ts (1)
getTokenAddress(372-386)
🔇 Additional comments (7)
sdk/test/gateway.test.ts (3)
737-792: Good implementation of onramp quote execution testThe test properly validates the full flow of onramp quote execution, mocking the necessary components and verifying that both
startOrderandfinalizeOrderare called once.
794-848: Good implementation of offramp quote execution testThe test properly validates the full flow of offramp quote execution, including API calls, contract interactions, and transaction handling.
891-906: Good implementation of combined orders retrieval testThe test properly verifies that the new
getOrdersmethod calls bothgetOnrampOrdersandgetOfframpOrdersand combines their results.sdk/src/gateway/client.ts (4)
130-168: Well-structured overloaded getQuote method with discriminated union patternThe refactored
getQuotemethod using a discriminated union pattern with atypefield provides better type safety and makes the API more explicit about what operation is being performed.
353-354: Good type safety improvement for outputScriptUsing the more specific
0x${string}type foroutputScriptenhances type safety and makes the API more robust.
451-461: Good conversion of timestamps from bigint to numberConverting
orderTimestampfrombiginttonumberis more appropriate for Unix timestamps and aligns with JavaScript's Date APIs.
1114-1131: Good implementation of unified orders retrievalThe new
getOrdersmethod efficiently combines results from bothgetOnrampOrdersandgetOfframpOrdersinto a single array with type discrimination.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores