SDK: all quotes, Query modifications #1716
Conversation
ChiTimesChi
requested review from
aureliusbtc,
trajan0x and
nautsimon
as code owners
WalkthroughThe updates involve enhancing the SDK router's functionality. New test cases and methods for slippage application and deadline modification have been introduced for Changes
Poem
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? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- packages/sdk-router/src/module/query.test.ts (2 hunks)
- packages/sdk-router/src/module/query.ts (1 hunks)
- packages/sdk-router/src/module/types.ts (1 hunks)
- packages/sdk-router/src/operations/bridge.ts (2 hunks)
- packages/sdk-router/src/sdk.test.ts (1 hunks)
- packages/sdk-router/src/sdk.ts (3 hunks)
Files skipped from review due to trivial changes (1)
- packages/sdk-router/src/sdk.test.ts
Additional comments: 13
packages/sdk-router/src/module/types.ts (1)
- 76-78: The
BridgeRoutetype definition is consistent with the rest of the type definitions and includes all necessary properties for representing a bridge route. The addition ofbridgeModuleNamealigns with the PR's objective to allow filtering by bridge module.packages/sdk-router/src/sdk.ts (4)
- 2-24: The addition of
allRouterSetsas a public property in theSynapseSDKclass is a significant change. It's important to ensure that this property is intended to be public and that its usage is secure and consistent with the SDK's design.Verification successful
The search results indicate that the
allRouterSetsproperty is indeed used within thesdk.tsfile and in thebridge.tsfile under theoperationsdirectory. The property is initialized in theSynapseSDKclass constructor and is used in an asynchronous map operation inbridge.ts. This suggests thatallRouterSetsis being used to iterate over router sets, likely as part of the functionality to handle bridge operations.Based on the information provided, there are no immediate red flags regarding the security or correctness of the
allRouterSetsusage. However, without a broader view of the context in which it's used, especially inbridge.ts, it's not possible to fully assess the security implications or whether it adheres to best practices.* 51-51: The initialization of `allRouterSets` with both `synapseRouterSet` and `synapseCCTPRouterSet` is consistent with the PR's objectives to enhance bridge functionality.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the allRouterSets property is used securely throughout the codebase. rg --type typescript "allRouterSets"Length of output: 71
Script:
#!/bin/bash # Verify that the allRouterSets property is used securely throughout the codebase. rg "allRouterSets" --type tsLength of output: 297
- 57-57: The
allBridgeQuotesmethod is added to theSynapseSDKclass. It's crucial to ensure that this method is implemented correctly and securely, as it's a key part of the PR's objectives.Verification successful
The search for the
allBridgeQuotesmethod in the codebase has produced several results:
- The method is assigned to the
SynapseSDKclass inpackages/sdk-router/src/sdk.ts.- There are tests written for
allBridgeQuotesinpackages/sdk-router/src/sdk.test.ts.- The actual implementation of the
allBridgeQuotesfunction is inpackages/sdk-router/src/operations/bridge.ts.This confirms that the
allBridgeQuotesmethod is present in theSynapseSDKclass, has an associated test suite, and the implementation is located in theoperations/bridge.tsfile. The method appears to be correctly implemented and securely added to the class, as per the PR's objectives.* 79-81: The addition of `applySlippage`, `applySlippageInBips`, and `modifyDeadline` methods to the `SynapseSDK` class is consistent with the PR's objectives to allow for more granular control over query parameters.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of the allBridgeQuotes method. rg --type typescript "allBridgeQuotes"Length of output: 73
Script:
#!/bin/bash # Verify the implementation of the allBridgeQuotes method without specifying file type. rg "allBridgeQuotes"Length of output: 538
packages/sdk-router/src/module/query.ts (3)
113-117: The
modifyDeadlinefunction correctly creates a newQueryobject with an updated deadline, adhering to the principle of immutability by not modifying the original object.131-148: The
applySlippagefunction correctly applies slippage to theminAmountOutof a query and returns a newQueryobject. The use ofinvariantensures that the slippage parameters are valid before applying them.162-166: The
applySlippageInBipsfunction is a specialized version ofapplySlippagethat uses basis points. It reuses theapplySlippagefunction, which is good practice to avoid code duplication.packages/sdk-router/src/operations/bridge.ts (2)
89-108: The
bridgeQuotefunction has been modified to use the newallBridgeQuotesfunction to retrieve and sort quotes, and then select the best quote based on theexcludeCCTPparameter. This change aligns with the PR's objectives and appears to be implemented correctly.122-157: The
allBridgeQuotesfunction fetches quotes from multiple bridge modules, filters them, and sorts them bymaxAmountOut. This implementation is consistent with the PR's objectives to provide comprehensive bridge quotes.packages/sdk-router/src/module/query.test.ts (3)
79-127: The tests for
modifyDeadlinecorrectly check that the deadline is modified and that the original query object remains unchanged. This is good practice to ensure immutability of function inputs.129-285: The tests for
applySlippagecover a range of slippage percentages and ensure that the function correctly applies slippage to theminAmountOutproperty. The tests also verify that the original query object is not mutated, which is a good practice for functional programming.287-339: The tests for
applySlippageInBipscheck for parity withapplySlippageand include validation for negative and excessively high basis points, which could indicate potential misuse of the function. These tests are important for ensuring that the function behaves as expected in various scenarios.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1716 +/- ##
===================================================
+ Coverage 51.38047% 51.42416% +0.04368%
===================================================
Files 360 360
Lines 24593 24611 +18
Branches 290 290
===================================================
+ Hits 12636 12656 +20
Misses 10722 10722
+ Partials 1235 1233 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/sdk-router/src/module/query.test.ts (2 hunks)
Additional comments: 3
packages/sdk-router/src/module/query.test.ts (3)
79-127: The tests for
modifyDeadlinecorrectly check that the deadline is modified and that the original query object remains unchanged. This is good practice for ensuring immutability in functional programming.129-312: The tests for
applySlippagecover a range of slippage percentages and check for immutability. They also include error handling for invalid slippage values. This thorough testing ensures the function behaves as expected under various conditions.315-381: The tests for
applySlippageInBipsensure that it behaves consistently withapplySlippageand that it handles edge cases such as negative basis points and values exceeding 100%. These tests are crucial for validating the correctness of slippage calculations.
Description
This PR aims to expand the bridge-related functionality:
allBridgeQuotes()function that returns all quotes for bridging using all available bridge modules.bridgeModuleNameis included into eachBridgeQuoteobject, allowing the consumer to do filtering on their side, if required.Queryobject, which is required for every bridge or swap interaction:modifyDeadlineto set the query's deadline.applySlippageandapplySlippageInBipsto apply the slippage to the query's "amount out"Additional context
#1695 will be rebased once this is merged into master.
Summary by CodeRabbit
New Features
Enhancements
Refactor
findBestRoutefunction to streamline the SDK's exposed functionality.Bug Fixes