[DNM] fix(sdk-router): use /rfq endpoint for calculating the RFQ quote [SLT-436] #3373
Conversation
WalkthroughThe changes in this pull request primarily involve the introduction of a new utility function Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
Deploying sanguine-fe with
|
| Latest commit: |
8942a74
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dcfc7749.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-sdk-rfq-endpoint.sanguine-fe.pages.dev |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3373 +/- ##
====================================================
+ Coverage 13.60118% 34.22366% +20.62248%
====================================================
Files 313 525 +212
Lines 30490 33471 +2981
Branches 82 86 +4
====================================================
+ Hits 4147 11455 +7308
+ Misses 25869 21000 -4869
- Partials 474 1016 +542
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Bundle ReportChanges will decrease total bundle size by 3.51MB (-9.84%) ⬇️. This is within the configured threshold ✅ Detailed changes
ℹ️ *Bundle size includes cached data from a previous commit |
ChiTimesChi
force-pushed
the
feat/sdk-rfq-endpoint
branch
from
6205011 to
e027265
Compare
ChiTimesChi
requested review from
bigboydiamonds,
abtestingalpha,
aureliusbtc and
trajan0x
as code owners
ChiTimesChi
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![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
🧹 Outside diff range and nitpick comments (15)
packages/sdk-router/src/utils/addressUtils.ts (1)
1-3: Basic implementation looks good but needs address validation.The function correctly handles case-insensitive comparison and null/undefined checks. However, as this is used in a blockchain context, it should validate the address format.
Consider adding address format validation:
export const isSameAddress = (addr1?: string, addr2?: string): boolean => { + const isValidAddress = (addr?: string) => { + return typeof addr === 'string' && + addr.startsWith('0x') && + addr.length === 42 && + /^0x[0-9a-f]{40}$/i.test(addr) + } + if (!addr1 || !addr2 || !isValidAddress(addr1) || !isValidAddress(addr2)) { + return false + } return addr1.toLowerCase() === addr2.toLowerCase() }For better performance in hot paths, consider caching the lowercase results:
export const isSameAddress = (addr1?: string, addr2?: string): boolean => { + // Cache lowercase addresses in a WeakMap if this function is called frequently + const lowercaseCache = new WeakMap<string, string>() + const toLowerCached = (addr: string) => { + if (!lowercaseCache.has(addr)) { + lowercaseCache.set(addr, addr.toLowerCase()) + } + return lowercaseCache.get(addr)! + } return !!addr1 && !!addr2 && toLowerCached(addr1) === toLowerCached(addr2) }packages/sdk-router/src/rfq/quote.test.ts (1)
Line range hint
10-44: Consider adding validation tests for quote dataThe test data includes critical fields like addresses and amounts, but there are no tests verifying input validation. Consider adding tests for:
- Invalid addresses
- Negative or zero amounts
- Mismatched chain IDs
Example test cases to add:
it('should throw on invalid addresses', () => { const invalidQuoteAPI = { ...quoteAPI, origin_token_addr: '0xinvalid' } expect(() => unmarshallFastBridgeQuote(invalidQuoteAPI)).toThrow() }) it('should throw on negative amounts', () => { const invalidQuoteAPI = { ...quoteAPI, dest_amount: '-1000' } expect(() => unmarshallFastBridgeQuote(invalidQuoteAPI)).toThrow() })packages/sdk-router/src/rfq/api.integration.test.ts (3)
8-10: Consider investigating and addressing test flakinessThe presence of retry logic suggests these tests are unstable. While retrying is a valid temporary solution, it would be beneficial to:
- Investigate the root cause of the flakiness
- Add comments documenting known causes of instability
- Consider implementing more robust test setup/teardown
33-43: Expand test coverage for successful quote scenariosConsider adding test cases for:
- Different token pairs (not just ETH to ETH)
- Various amount ranges
- Validation of quote calculation accuracy
- Verification of all returned quote fields
45-79: Enhance error validation in zero quote testsThe test structure and cleanup are well implemented. However, consider:
- Validating the specific error messages being logged
- Adding assertions for expected error conditions
- Testing additional edge cases (e.g., zero amount)
- expect(console.error).toHaveBeenCalled() + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining('expected error message') + )packages/sdk-router/src/utils/addressUtils.test.ts (3)
1-11: Consider adding documentation for test addressesThe test data setup is well-structured, but could benefit from documentation explaining:
- Why these specific addresses were chosen
- How the checksum variants were generated
- What the last digit difference signifies
Add JSDoc comments above the constant declarations:
+/** Test address for Alice, intentionally differing from Bob's address in last digit */ const lowerCaseAlice = '0x0123456789abcdef0123456789abcdef01234567' +/** EIP-55 checksummed version of Alice's address */ const checkSumdAlice = '0x0123456789abcDEF0123456789abCDef01234567'
5-5: Fix typo in variable name 'checkSumd'The variable name contains a typo:
checkSumdshould becheckSummed-const checkSumdAlice = '0x0123456789abcDEF0123456789abCDef01234567' +const checkSummedAlice = '0x0123456789abcDEF0123456789abCDef01234567' - expect(isSameAddress(checkSumdAlice, checkSumdAlice)).toBe(true) - expect(isSameAddress(checkSumdBob, checkSumdBob)).toBe(true) + expect(isSameAddress(checkSummedAlice, checkSummedAlice)).toBe(true) + expect(isSameAddress(checkSummedBob, checkSummedBob)).toBe(true)Also applies to: 19-19, 20-20
106-107: Consider improving TypeScript type handlingInstead of using type assertions with
as any, consider creating a proper type for the function parameters that allows null.- expect(isSameAddress(null as any, lowerCaseAlice)).toBe(false) - expect(isSameAddress(lowerCaseAlice, null as any)).toBe(false) - expect(isSameAddress(null as any, null as any)).toBe(false) + expect(isSameAddress(null as Address | null, lowerCaseAlice)).toBe(false) + expect(isSameAddress(lowerCaseAlice, null as Address | null)).toBe(false) + expect(isSameAddress(null as Address | null, null as Address | null)).toBe(false)Consider adding a type definition in the main utility file:
type Address = string;Also applies to: 111-111
packages/sdk-router/src/rfq/api.test.ts (1)
119-261: Great test coverage, consider adding edge casesThe test suite is comprehensive and well-structured, covering success scenarios, timeouts, missing fields, and error conditions. Consider adding these edge cases:
- Malformed
dest_amountthat isn't parseable- Negative
dest_amount- Extremely large
dest_amount(near max safe integer)Here's a suggested implementation:
it('when the response dest_amount is malformed', async () => { const responseWithMalformedAmount = { ...quoteFound, dest_amount: 'not-a-number' } fetchMock.mockResponseOnce(JSON.stringify(responseWithMalformedAmount)) const result = await getBestRelayerQuote(ticker, bigAmount, userAddress) expect(result).toEqual(quoteZero) expect(console.error).toHaveBeenCalled() }) it('when the response dest_amount is negative', async () => { const responseWithNegativeAmount = { ...quoteFound, dest_amount: '-1000' } fetchMock.mockResponseOnce(JSON.stringify(responseWithNegativeAmount)) const result = await getBestRelayerQuote(ticker, bigAmount, userAddress) expect(result).toEqual(quoteZero) expect(console.error).toHaveBeenCalled() }) it('when the response dest_amount is extremely large', async () => { const responseWithLargeAmount = { ...quoteFound, dest_amount: '115792089237316195423570985008687907853269984665640564039457584007913129639935' // 2^256 - 1 } fetchMock.mockResponseOnce(JSON.stringify(responseWithLargeAmount)) const result = await getBestRelayerQuote(ticker, bigAmount, userAddress) expect(result).toEqual(quoteZero) expect(console.error).toHaveBeenCalled() })packages/sdk-router/src/rfq/api.ts (3)
26-26: Reminder: Makeintegrator_ida required fieldThere is a TODO comment indicating that
integrator_idshould be made required in thePutRFQRequestAPItype. Addressing this will ensure that integrator identification is consistently provided in API requests.Would you like assistance in updating the type definition and related code to make
integrator_idrequired?
Line range hint
90-90: Follow-up on TODO: Add active quotes once fixedThe TODO comment suggests adding active quotes to the
quote_typesarray when they are fixed. Please ensure this is tracked so that active quotes can be integrated into the RFQ process promptly.Let me know if you'd like me to open a GitHub issue to track this task.
110-117: Enhance response validation and error handlingCurrently, the code checks if
rfqResponse.successisfalsebut does not handle cases wheresuccessmight beundefinedor missing. Strengthening the validation can prevent unexpected errors.Consider updating the condition to explicitly check for truthiness:
- if (!rfqResponse.success) { + if (rfqResponse.success !== true) {Additionally, ensure that all necessary fields are present and valid before proceeding.
packages/sdk-router/src/rfq/fastBridgeRouterSet.ts (3)
120-131: Consider adding tests forgetBestRelayerQuoteintegrationThe code block starting at line 120, where
getBestRelayerQuoteis used withinfilteredTickers.map, is not covered by tests according to static analysis tools. Adding unit tests for this integration would enhance reliability and ensure that the quote retrieval logic behaves as expected.Would you like assistance in writing unit tests for this code segment?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 120-120: packages/sdk-router/src/rfq/fastBridgeRouterSet.ts#L120
Added line #L120 was not covered by tests
271-274: Add tests forgetOriginAmountOutusageThe call to
getOriginAmountOutat line 271 withinfilterTickersWithPossibleSwapis not covered by tests. Including tests for this method call would help verify that the origin queries are correctly generated for the provided tickers.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 271-271: packages/sdk-router/src/rfq/fastBridgeRouterSet.ts#L271
Added line #L271 was not covered by tests
314-314: Address the TODO: "don't filter by age here"There is a TODO comment at line 314 indicating that age filtering should not occur here. Consider moving the age filtering logic to the appropriate place or clarify if age filtering is needed at this step.
Would you like assistance in refactoring this logic?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
packages/sdk-router/src/module/synapseModuleSet.ts(2 hunks)packages/sdk-router/src/rfq/api.integration.test.ts(1 hunks)packages/sdk-router/src/rfq/api.test.ts(3 hunks)packages/sdk-router/src/rfq/api.ts(2 hunks)packages/sdk-router/src/rfq/fastBridgeRouterSet.ts(3 hunks)packages/sdk-router/src/rfq/quote.test.ts(1 hunks)packages/sdk-router/src/rfq/quote.ts(0 hunks)packages/sdk-router/src/sdk.ts(1 hunks)packages/sdk-router/src/utils/addressUtils.test.ts(1 hunks)packages/sdk-router/src/utils/addressUtils.ts(1 hunks)packages/sdk-router/src/utils/handleNativeToken.ts(2 hunks)packages/synapse-interface/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/sdk-router/src/rfq/quote.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/sdk-router/src/rfq/fastBridgeRouterSet.ts
[warning] 120-120: packages/sdk-router/src/rfq/fastBridgeRouterSet.ts#L120
Added line #L120 was not covered by tests
[warning] 133-134: packages/sdk-router/src/rfq/fastBridgeRouterSet.ts#L133-L134
Added lines #L133 - L134 were not covered by tests
[warning] 271-271: packages/sdk-router/src/rfq/fastBridgeRouterSet.ts#L271
Added line #L271 was not covered by tests
[warning] 277-277: packages/sdk-router/src/rfq/fastBridgeRouterSet.ts#L277
Added line #L277 was not covered by tests
[warning] 319-319: packages/sdk-router/src/rfq/fastBridgeRouterSet.ts#L319
Added line #L319 was not covered by tests
[warning] 322-322: packages/sdk-router/src/rfq/fastBridgeRouterSet.ts#L322
Added line #L322 was not covered by tests
[warning] 324-324: packages/sdk-router/src/rfq/fastBridgeRouterSet.ts#L324
Added line #L324 was not covered by tests
🔇 Additional comments (14)
packages/sdk-router/src/utils/handleNativeToken.ts (1)
5-5: LGTM! Clean import of the address utility
The addition of the isSameAddress import follows good practices and maintains clean import organization.
packages/sdk-router/src/rfq/quote.test.ts (2)
Line range hint 46-52: LGTM! The marshalling tests are well-structured.
The bi-directional conversion tests are clear and focused. They properly verify the data transformation between API and internal representations.
Line range hint 1-52: Verify test coverage after removing applyQuote tests
While the marshalling tests are well-implemented, the removal of several test cases (applyQuote related tests) has significantly reduced the test coverage. Please ensure that the quote calculation functionality is now properly tested wherever it has been moved to (likely in the API integration tests).
✅ Verification successful
Let me gather more specific information about the quote calculation tests.
Based on the results from our verification scripts, I can now provide a final response:
The removal of applyQuote tests is justified as the functionality is covered elsewhere
The verification shows that quote-related functionality has been moved to a more appropriate location in the API integration tests (packages/sdk-router/src/rfq/api.integration.test.ts), which now includes:
- Tests for successful quote calculations with non-zero amounts
- Edge case tests for zero-amount quotes
- Integration with actual relayer responses
The original test file's focus on marshalling/unmarshalling is correct as it's now a pure data transformation unit test.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for new RFQ quote calculation tests
rg -l "test|spec|describe.*rfq.*quote" --type ts
# Look for the new location of quote calculation logic
ast-grep --pattern 'function $_(quote: FastBridgeQuote, amount: $_) {
$$$
}'
Length of output: 6512
Script:
#!/bin/bash
# Check for quote calculation tests in the API integration tests and router tests
rg -A 10 "describe.*quote|test.*quote" packages/sdk-router/src/rfq/api.integration.test.ts packages/sdk-router/src/rfq/fastBridgeRouter.test.ts
# Look for quote calculation logic in the router implementation
rg -A 10 "class.*FastBridgeRouter|function.*quote" packages/sdk-router/src/rfq/fastBridgeRouter.ts
Length of output: 2431
Script:
#!/bin/bash
# Check for quote calculation tests in the router set tests
rg -A 10 "describe.*quote|test.*quote" packages/sdk-router/src/rfq/fastBridgeRouterSet.test.ts
# Look for quote calculation logic in the router set
rg -A 10 "getQuote|calculateQuote" packages/sdk-router/src/rfq/fastBridgeRouterSet.ts
Length of output: 184
packages/sdk-router/src/rfq/api.integration.test.ts (1)
20-31: LGTM! Well-structured test setup
The test fixtures are clearly defined and use constants appropriately. The setup provides a good foundation for the test cases.
packages/sdk-router/src/sdk.ts (1)
30-30: Verify implementation status of RFQ endpoint integration
The PR objective mentions moving RFQ quote calculation to use the /rfq endpoint, but the implementation appears incomplete. The current changes only add a test build indicator.
Let's verify the implementation status:
packages/sdk-router/src/utils/addressUtils.test.ts (2)
50-80: LGTM! Comprehensive test coverage for different addresses
The test cases thoroughly cover all format combinations with bidirectional testing, ensuring robust validation of address differences.
1-1: Verify implementation of isSameAddress utility
Let's ensure the implementation exists and matches these test cases.
✅ Verification successful
Implementation of isSameAddress utility is present and matches test usage
The implementation exists in addressUtils.ts and follows a straightforward pattern:
- Takes two optional string parameters (addr1, addr2)
- Returns boolean after case-insensitive comparison
- Handles undefined/null cases via truthiness checks
Implementation and tests for isSameAddress are properly aligned
The implementation correctly handles all test cases including:
- Case-insensitive comparison of valid addresses (lowercase, uppercase, checksum)
- Proper handling of undefined values
- Null value checks
- Empty string validation
The simple implementation using toLowerCase() matches the comprehensive test coverage, ensuring reliable address comparison across all scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the isSameAddress implementation
echo "Searching for isSameAddress implementation..."
ast-grep --pattern 'export function isSameAddress($_: $_, $_: $_) {
$$$
}'
# Verify the file exists
echo "Verifying file existence..."
fd --type f "addressUtils.ts" --exec cat {}
Length of output: 443
Script:
#!/bin/bash
# Let's verify the test cases to ensure complete coverage
cat packages/sdk-router/src/utils/addressUtils.test.ts
Length of output: 4335
packages/sdk-router/src/module/synapseModuleSet.ts (2)
9-9: LGTM! Good utility function extraction.
The addition of the isSameAddress utility import follows good software engineering practices by centralizing address comparison logic.
74-74: LGTM! Verify consistent address comparison usage.
The refactoring to use isSameAddress improves code maintainability. Let's verify that all address comparisons in the codebase follow this pattern.
packages/sdk-router/src/rfq/api.test.ts (2)
4-10: LGTM: Import changes and utility updates align with the new RFQ endpoint integration
The new imports and utility function modifications properly support the transition to using the /rfq endpoint for quote calculations.
Also applies to: 17-20
73-73: LGTM: Existing test adaptations maintain coverage
The modifications correctly adapt the existing tests to the new API response format while maintaining the same test scenarios and coverage.
Also applies to: 110-110
packages/sdk-router/src/rfq/api.ts (2)
119-124: Avoid logging sensitive information
Logging the entire rfqResponse object when an error occurs may expose sensitive data, such as user addresses or quote details.
[security]
Apply this diff to limit the logged information:
console.error(
'Error fetching quote: missing dest_amount or relayer_address in response:',
- rfqResponse
+ {
+ quote_id: rfqResponse.quote_id,
+ reason: rfqResponse.reason,
+ }
)This change ensures that only relevant, non-sensitive information is logged.
131-134: Include null checks for optional fields
While quoteID and relayerAddress are optional in the RelayerQuote type, they are critical for downstream processes.
Please verify that the absence of quoteID or relayerAddress does not lead to issues elsewhere in the codebase. Run the following script to search for usages that assume these fields are present:
✅ Verification successful
Optional fields are handled correctly throughout the codebase
The codebase properly handles the optional nature of quoteID and relayerAddress:
- The
RelayerQuotetype explicitly marks both fields as optional with? - Test cases specifically cover scenarios with undefined
quoteID - No direct property access without null checks was found
- The only usage in integration tests uses
?.optional chaining
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of RelayerQuote where quoteID or relayerAddress are assumed to be non-null.
# Search for destructuring assignments that might assume the fields are present.
rg -A2 'const.*{.*quoteID.*}.*=.*' src/
# Search for direct property accesses without null checks.
rg '\.quoteID\b' src/
Length of output: 226
Script:
#!/bin/bash
# Search for usages of quoteID and relayerAddress
rg --type-add 'ts:*.ts' --type ts -A 3 'quoteID|relayerAddress'
# Search for RelayerQuote type definition
ast-grep --pattern 'type RelayerQuote = {
$$$
}'
Length of output: 2749
packages/sdk-router/src/rfq/fastBridgeRouterSet.ts (1)
315-316: Verify time units of quote.updatedAt when calculating age
In the calculation of age at line 315, ensure that quote.updatedAt is in milliseconds to match Date.now(). If quote.updatedAt is in seconds, you should multiply it by 1000 to convert it to milliseconds before performing the subtraction.
packages/sdk-router/src/sdk.ts
Outdated
| @@ -27,6 +27,7 @@ class SynapseSDK { | |||
| * @param {Provider[]} providers - The Ethereum providers for the respective chains. | |||
| */ | |||
| constructor(chainIds: number[], providers: Provider[]) { | |||
| console.log('GM - this is a test build') | |||
There was a problem hiding this comment.
Remove debug console.log statement
This debug statement should not be committed to production code as it would pollute the logs of applications using the SDK.
Apply this diff to remove the debug statement:
- console.log('GM - this is a test build')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log('GM - this is a test build') |
| @@ -40,7 +40,7 @@ | |||
| "@reduxjs/toolkit": "^1.9.5", | |||
| "@rtk-query/graphql-request-base-query": "^2.2.0", | |||
| "@segment/analytics-next": "^1.53.0", | |||
| "@synapsecns/sdk-router": "^0.11.6", | |||
| "@synapsecns/sdk-router": "file:../sdk-router", | |||
There was a problem hiding this comment.
Deployment blocker: Remove local file reference
Since this is marked as DNM (Do Not Merge), remember to revert this change before deployment:
- "@synapsecns/sdk-router": "file:../sdk-router",
+ "@synapsecns/sdk-router": "^0.11.6",This is critical because:
- Local file references don't work in production environments
- Version pinning is important for deployment stability
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "@synapsecns/sdk-router": "file:../sdk-router", | |
| "@synapsecns/sdk-router": "^0.11.6", |
💡 Codebase verification
Local SDK reference needs to be updated before deployment
The repository structure confirms that both packages exist under packages/ directory, but using a local file reference (file:../sdk-router) instead of a versioned dependency is problematic for production deployment. This needs to be reverted to a proper version (currently other packages use ^0.11.6).
packages/synapse-interface/package.json: Revert to"@synapsecns/sdk-router": "^0.11.6"before merging- Multiple other packages (
widget,rest-api) are using the versioned dependency^0.11.6, which indicates this is the current production version
🔗 Analysis chain
Verify local SDK development setup
The change to use a local SDK version is appropriate for development, but please ensure:
- The local SDK build process is documented
- All developers can successfully build and test the changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if sdk-router package exists and has necessary build files
if [ -d "../sdk-router" ]; then
echo "SDK directory exists"
cat ../sdk-router/package.json | jq '.scripts'
# Check for build instructions
fd -t f "README|CONTRIBUTING" ../sdk-router
else
echo "Error: sdk-router directory not found"
exit 1
fi
Length of output: 120
Script:
#!/bin/bash
# Let's verify the repository structure and package locations
fd -t d "sdk-router|synapse-interface" .
# Check package.json files in the repository
fd -t f "package.json" . --exec cat {} | jq -c '{name: .name, dependencies: .dependencies}'
Length of output: 7395
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/sdk-router/src/rfq/api.ts (3)
25-26: Address the TODO comment regarding integrator_idThe integrator_id should be required for proper request tracking and attribution. This is important for security and analytics purposes.
Would you like me to help implement the required integrator_id change?
114-115: Document the limitation of quote_typesThe comment mentions adding active quotes later, but the current implementation only supports 'passive' quotes. Consider documenting this limitation in the function's JSDoc.
Add this to the function's documentation:
/** * Hits Quoter API /rfq PUT endpoint to get the best quote for a given ticker and origin amount. + * Note: Currently only supports 'passive' quote types. Active quotes will be added in the future. * * @returns A promise that resolves to the best quote. * Will return a zero quote if the request fails or times out. */
133-134: Enhance error logging for better debuggingThe current error messages could be more specific and include relevant context for debugging. Consider adding request details and structuring the error messages.
Improve error logging:
- console.error('Error fetching quote:', response.statusText) + console.error('RFQ API request failed:', { + status: response.status, + statusText: response.statusText, + ticker: `${ticker.originToken.token}-${ticker.destToken.token}`, + chainIds: `${ticker.originToken.chainId}-${ticker.destToken.chainId}`, + }) - console.error( - 'No RFQ quote returned:', - rfqResponse.reason ?? 'Unknown reason' - ) + console.error('RFQ API returned unsuccessful response:', { + reason: rfqResponse.reason ?? 'Unknown reason', + quoteType: rfqResponse.quote_type, + ticker: `${ticker.originToken.token}-${ticker.destToken.token}`, + }) - console.error( - 'Error fetching quote: missing dest_amount or relayer_address in response:', - rfqResponse - ) + console.error('RFQ API response missing required fields:', { + hasDestAmount: !!rfqResponse.dest_amount, + hasRelayerAddress: !!rfqResponse.relayer_address, + quoteType: rfqResponse.quote_type, + }) - console.error('No RFQ quote returned') + console.error('RFQ API returned zero or negative dest_amount:', { + destAmount: rfqResponse.dest_amount, + quoteType: rfqResponse.quote_type, + }) - console.error('Error fetching quote:', error) + console.error('RFQ API request failed with error:', { + error: error instanceof Error ? error.message : String(error), + ticker: `${ticker.originToken.token}-${ticker.destToken.token}`, + })Also applies to: 139-144, 146-151, 154-156, 163-164
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/sdk-router/src/rfq/api.ts(2 hunks)packages/sdk-router/src/utils/logs.ts(2 hunks)
🔇 Additional comments (2)
packages/sdk-router/src/utils/logs.ts (1)
5-5: LGTM! Good modularization of address comparison logic.
The import of isSameAddress aligns with the codebase's effort to standardize address comparison operations.
packages/sdk-router/src/rfq/api.ts (1)
17-21: Consider increasing the EXPIRATION_WINDOW
The current 1000ms window might be too short for relayers to respond, especially during high network latency or congestion. Consider increasing this value to provide a more reliable window for quote responses.
Let's analyze the typical response times:
✅ Verification successful
Let me gather more specific information about RFQ timing configurations:
The EXPIRATION_WINDOW of 1000ms is appropriate
Based on the test cases and implementation:
- The API has a timeout of 2000ms (API_TIMEOUT)
- Tests consider responses up to 1900ms as acceptable (OK_RESPONSE_TIME)
- The 1000ms EXPIRATION_WINDOW is sufficient since:
- It's used as a parameter in RFQ requests to inform relayers about the quote validity period
- The actual API timeout (2000ms) provides enough buffer for the round-trip communication
- Test cases validate that responses within this window work correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any configuration or documentation about response times
rg -i "response.*time|latency|timeout" --type ts
Length of output: 6612
Script:
#!/bin/bash
# Search for RFQ-related timing configurations and tests
rg -A 5 "EXPIRATION_WINDOW|API_TIMEOUT" --type ts
Length of output: 1879
Script:
#!/bin/bash
# Search for RFQ test cases to understand timing expectations
cat packages/sdk-router/src/rfq/api.test.ts
Length of output: 8289
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/sdk-router/src/rfq/api.ts (2)
12-21: Consider environment-specific configuration for API settingsThe hardcoded API URL and timeout values should be configurable based on the environment (development, staging, production).
Consider:
- Moving these values to a configuration file
- Making them environment-dependent
- Allowing override through environment variables
-const API_URL = 'https://rfq-api.omnirpc.io' -const API_TIMEOUT = 2000 -const EXPIRATION_WINDOW = 1000 +const API_URL = process.env.RFQ_API_URL || 'https://rfq-api.omnirpc.io' +const API_TIMEOUT = Number(process.env.RFQ_API_TIMEOUT) || 2000 +const EXPIRATION_WINDOW = Number(process.env.RFQ_EXPIRATION_WINDOW) || 1000
106-166: Add retry logic for transient failuresWhile the error handling is comprehensive, the function could benefit from retry logic for transient failures, which are common in network requests.
Consider implementing exponential backoff:
const getQuoteWithRetry = async ( rfqRequest: PutRFQRequestAPI, maxRetries = 3, baseDelay = 500 ): Promise<Response> => { for (let i = 0; i < maxRetries; i++) { try { const response = await fetchWithTimeout(`${API_URL}/rfq`, API_TIMEOUT, { method: 'PUT', body: JSON.stringify(rfqRequest), headers: { 'Content-Type': 'application/json', }, }); if (response.ok) return response; if (response.status < 500) break; // Don't retry client errors } catch (error) { if (i === maxRetries - 1) throw error; } await new Promise(resolve => setTimeout(resolve, baseDelay * Math.pow(2, i))); } throw new Error('Max retries exceeded'); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/sdk-router/src/rfq/api.ts(2 hunks)packages/synapse-interface/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/synapse-interface/package.json
🔇 Additional comments (1)
packages/sdk-router/src/rfq/api.ts (1)
112-124: Consider validating quote_types array
The quote_types array is hardcoded to ['passive'] with a TODO comment about adding active quotes.
Let's verify the supported quote types:
| export type PutRFQRequestAPI = { | ||
| user_address?: string | ||
| // TODO: make integrator_id required | ||
| integrator_id?: string | ||
| quote_types: string[] | ||
| data: { | ||
| origin_chain_id: number | ||
| dest_chain_id: number | ||
| origin_token_addr: string | ||
| dest_token_addr: string | ||
| origin_amount_exact: string | ||
| expiration_window: number | ||
| } | ||
| } |
There was a problem hiding this comment.
Address TODO comment regarding integrator_id
The TODO comment indicates that integrator_id should be required, which is important for tracking and security purposes.
Make the field required:
export type PutRFQRequestAPI = {
user_address?: string
- // TODO: make integrator_id required
- integrator_id?: string
+ integrator_id: string
quote_types: string[]
data: {
// ... rest of the fields
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type PutRFQRequestAPI = { | |
| user_address?: string | |
| // TODO: make integrator_id required | |
| integrator_id?: string | |
| quote_types: string[] | |
| data: { | |
| origin_chain_id: number | |
| dest_chain_id: number | |
| origin_token_addr: string | |
| dest_token_addr: string | |
| origin_amount_exact: string | |
| expiration_window: number | |
| } | |
| } | |
| export type PutRFQRequestAPI = { | |
| user_address?: string | |
| integrator_id: string | |
| quote_types: string[] | |
| data: { | |
| origin_chain_id: number | |
| dest_chain_id: number | |
| origin_token_addr: string | |
| dest_token_addr: string | |
| origin_amount_exact: string | |
| expiration_window: number | |
| } | |
| } |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/sdk-router/src/rfq/api.ts (1)
19-47: Add JSDoc documentation for type definitionsThe types
PutRFQRequestAPI,PutRFQResponseAPI, andRelayerQuotelack documentation. Add JSDoc comments to describe their purpose and field meanings.Example for
PutRFQRequestAPI:+/** + * Request type for the RFQ API PUT endpoint. + * @property {string} [user_address] - Optional address of the user requesting the quote + * @property {string} [integrator_id] - Identifier for the integrator making the request + * @property {string[]} quote_types - Types of quotes to request (e.g., 'passive', 'active') + * @property {Object} data - Quote request parameters + */ export type PutRFQRequestAPI = {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/sdk-router/src/rfq/api.ts(3 hunks)
🔇 Additional comments (4)
packages/sdk-router/src/rfq/api.ts (4)
53-62: LGTM! Well-implemented timeout handling
The function correctly implements timeout handling using AbortController and properly merges the init options.
110-111: Address the TODO comment regarding active quotes
The comment indicates that active quotes need to be added. This might affect quote availability and pricing.
Let's check the implementation status:
✅ Verification successful
Active quotes feature is currently disabled by design
The codebase shows that active quotes functionality is intentionally disabled with a TODO comment. This is supported by:
- The presence of active quotes expiration window configuration
- Explicit TODO comment indicating active quotes need to be fixed before enabling
- Current implementation explicitly limiting to passive quotes only
The TODO comment should remain until the active quotes feature is ready to be implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for active quote implementations and tests
rg -A 5 "quote_types.*active" --type ts
Length of output: 41
Script:
#!/bin/bash
# Let's search for any mentions of quote types in the codebase
rg "quote_types" --type ts -B 2 -A 2
# Also search for any mentions of active quotes
rg "active.*quote" --type ts -B 2 -A 2
Length of output: 1447
102-106:
Add input parameter validation
The function should validate input parameters before making the API request to fail fast and provide clear error messages.
Add these validations:
export const getBestRelayerQuote = async (
ticker: Ticker,
originAmount: BigNumber,
originUserAddress?: string
): Promise<RelayerQuote> => {
+ // Validate input parameters
+ if (!ticker.originToken?.chainId || !ticker.destToken?.chainId) {
+ console.error('Invalid ticker: missing chain IDs')
+ return ZeroQuote
+ }
+ if (!ticker.originToken?.token || !ticker.destToken?.token) {
+ console.error('Invalid ticker: missing token addresses')
+ return ZeroQuote
+ }
+ if (originAmount.lte(0)) {
+ console.error('Invalid origin amount: must be positive')
+ return ZeroQuote
+ }Likely invalid or redundant comment.
13-17:
Consider increasing the EXPIRATION_WINDOW value
1 second might be too short for relayers to respond with quotes, especially considering network latency and processing time. Consider increasing this value to provide a more reasonable window (e.g., 5-10 seconds).
Let's analyze the typical response times:
Description
WIP, will match the description below once completed
sanguine/packages/sdk-router/src/rfq/quote.ts
Lines 73 to 89 in f2f1139
/rfqendpoint that does the quote calculation on the API side.6d9fd8e: synapse-interface preview link
c679893: synapse-interface preview link
251268b: synapse-interface preview link
c77f2e5: synapse-interface preview link
b20e3e0: synapse-interface preview link
Summary by CodeRabbit
New Features
Bug Fixes
Tests
applyQuotefunction.Refactor
68d91fb: synapse-interface preview link
1560611: synapse-interface preview link