SDK: return estimated bridge time along with the quotes #1520
Conversation
ChiTimesChi
requested review from
aureliusbtc,
trajan0x and
nautsimon
as code owners
|
Here is the updated walkthrough: WalkthroughThe changes introduce a new feature to estimate the time for bridge transactions across different blockchain networks. This is achieved by adding new data structures for median times, updating router classes with a new method Changes
TipsChat with CodeRabbit Bot (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1520 +/- ##
===================================================
- Coverage 51.16094% 50.76872% -0.39222%
===================================================
Files 366 358 -8
Lines 25195 23871 -1324
Branches 271 272 +1
===================================================
- Hits 12890 12119 -771
+ Misses 11035 10598 -437
+ Partials 1270 1154 -116
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
![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: 2
Configuration used: CodeRabbit UI
Files selected for processing (9)
- packages/sdk-router/src/constants/index.ts (1 hunks)
- packages/sdk-router/src/constants/medianTime.ts (1 hunks)
- packages/sdk-router/src/router/routerSet.ts (2 hunks)
- packages/sdk-router/src/router/synapseCCTPRouterSet.test.ts (2 hunks)
- packages/sdk-router/src/router/synapseCCTPRouterSet.ts (2 hunks)
- packages/sdk-router/src/router/synapseRouterSet.test.ts (2 hunks)
- packages/sdk-router/src/router/synapseRouterSet.ts (2 hunks)
- packages/sdk-router/src/router/types.ts (1 hunks)
- packages/sdk-router/src/sdk.test.ts (6 hunks)
Files skipped from review due to trivial changes (3)
- packages/sdk-router/src/constants/index.ts
- packages/sdk-router/src/constants/medianTime.ts
- packages/sdk-router/src/router/types.ts
Additional comments: 11
packages/sdk-router/src/router/synapseRouterSet.test.ts (3)
5-11: The new imports
MEDIAN_TIME_BRIDGEandSUPPORTED_CHAIN_IDSare correctly added. Ensure that these constants are correctly defined in the../constantsmodule.58-73: The new
getEstimatedTimefunction is correctly tested. It checks for both supported and unsupported chains. The error message for unsupported chains is clear and informative.75-77: No changes are made to the
getRouterfunction. Ensure that this function still works as expected after the changes made in this pull request.packages/sdk-router/src/router/synapseCCTPRouterSet.test.ts (2)
5-11: The new imports
MEDIAN_TIME_CCTPandCCTP_SUPPORTED_CHAIN_IDSare used in the tests for thegetEstimatedTimefunction. Ensure that these constants are correctly defined in the../constantsmodule.68-92: The tests for the
getEstimatedTimefunction are comprehensive, covering both supported and unsupported chains. The error messages in the tests are clear and informative. However, it would be beneficial to add a test case for a chain that is supported but does not have a provider.it('Throws error for supported chain without a provider', () => { // Use a chain ID for a supported chain that does not have a provider const unsupportedChainId = ...; expect(() => routerSet.getEstimatedTime(unsupportedChainId)).toThrow( `No estimated time for chain ${unsupportedChainId}` ) })packages/sdk-router/src/router/routerSet.ts (2)
63-71: The
getEstimatedTimemethod is a good addition to theRouterSetclass. It provides an abstract method that can be implemented by subclasses to provide estimated times for transactions. This is a good use of the abstraction principle in OOP. However, it would be helpful to clarify what happens when the chain ID is not supported. Does it return a default value or throw an error? This should be clearly documented in the method's comments.197-209: The
finalizeBridgeRoutemethod now includes theestimatedTimein its return value. This is a good addition as it provides more information to the user about the transaction. However, it would be helpful to handle the case wheregetEstimatedTimethrows an error (if it does so when the chain ID is not supported). This could be done with a try-catch block around the call togetEstimatedTime.Here is a suggested change to handle potential errors from
getEstimatedTime:originQuery.minAmountOut, hasComplexBridgeAction(destQuery) ) + try { + const estimatedTime = this.getEstimatedTime(bridgeRoute.originChainId) + } catch (error) { + console.error(`Error getting estimated time for chain ${bridgeRoute.originChainId}: ${error}`) + const estimatedTime = 0 // or some other default value + } return { feeAmount, feeConfig, routerAddress: originRouter.address, maxAmountOut: destQuery.minAmountOut, originQuery, destQuery, estimatedTime, } } }packages/sdk-router/src/sdk.test.ts (4)
25-26: The new global variables
MEDIAN_TIME_BRIDGEandMEDIAN_TIME_CCTPare imported correctly. These variables are used to map supported chain IDs to specific time values.570-572: The test cases are correctly checking the estimated time for bridge quotes. They are comparing the estimated time returned by the
bridgeQuotefunction with the expected median time for the corresponding chain ID.562-574: New test cases are added to check the functionality of the
bridgeQuotefunction for different bridging scenarios. These tests are correctly checking the router address, the presence of the swapAdapter property in the originQuery, and the estimated time for the bridge quote.502-574: The
SynapseSDKclass is instantiated with different chain configurations and thebridgeQuotefunction is called with different parameters to fetch bridge quotes. The test cases are correctly checking the returned bridge quotes.
| public getEstimatedTime(chainId: number): number { | ||
| const medianTime = | ||
| MEDIAN_TIME_BRIDGE[chainId as keyof typeof MEDIAN_TIME_BRIDGE] | ||
| invariant(medianTime, `No estimated time for chain ${chainId}`) | ||
| return medianTime | ||
| } |
There was a problem hiding this comment.
The getEstimatedTime function is well implemented. It retrieves the median time from the MEDIAN_TIME_BRIDGE constant using the chainId as the key. If no estimated time is found for the given chain, it throws an error. This is a good practice as it ensures that the function fails fast when it encounters an unsupported chain ID. However, it would be beneficial to include a more detailed error message that also suggests a solution, such as using a supported chain ID.
- invariant(medianTime, `No estimated time for chain ${chainId}`)
+ invariant(medianTime, `No estimated time for chain ${chainId}. Please use a supported chain ID.`)Commitable suggestion (Beta)
| public getEstimatedTime(chainId: number): number { | |
| const medianTime = | |
| MEDIAN_TIME_BRIDGE[chainId as keyof typeof MEDIAN_TIME_BRIDGE] | |
| invariant(medianTime, `No estimated time for chain ${chainId}`) | |
| return medianTime | |
| } | |
| public getEstimatedTime(chainId: number): number { | |
| const medianTime = | |
| MEDIAN_TIME_BRIDGE[chainId as keyof typeof MEDIAN_TIME_BRIDGE] | |
| invariant(medianTime, `No estimated time for chain ${chainId}. Please use a supported chain ID.`) | |
| return medianTime | |
| } |
| public getEstimatedTime(chainId: number): number { | ||
| const medianTime = | ||
| MEDIAN_TIME_CCTP[chainId as keyof typeof MEDIAN_TIME_CCTP] | ||
| invariant(medianTime, `No estimated time for chain ${chainId}`) | ||
| return medianTime | ||
| } |
There was a problem hiding this comment.
The getEstimatedTime function is well implemented. It correctly retrieves the estimated time from the MEDIAN_TIME_CCTP constant and throws an error if the time is not available for the specified chain. However, it would be better to handle the case when the chainId is not in MEDIAN_TIME_CCTP more gracefully. Instead of throwing an error, you could return a default value or a message indicating that the time is not available. This would prevent the application from crashing if an unsupported chainId is passed to the function.
public getEstimatedTime(chainId: number): number | string {
const medianTime =
MEDIAN_TIME_CCTP[chainId as keyof typeof MEDIAN_TIME_CCTP]
- invariant(medianTime, `No estimated time for chain ${chainId}`)
+ if (!medianTime) {
+ return `No estimated time for chain ${chainId}`
+ }
return medianTime
}Commitable suggestion (Beta)
| public getEstimatedTime(chainId: number): number { | |
| const medianTime = | |
| MEDIAN_TIME_CCTP[chainId as keyof typeof MEDIAN_TIME_CCTP] | |
| invariant(medianTime, `No estimated time for chain ${chainId}`) | |
| return medianTime | |
| } | |
| public getEstimatedTime(chainId: number): number | string { | |
| const medianTime = | |
| MEDIAN_TIME_CCTP[chainId as keyof typeof MEDIAN_TIME_CCTP] | |
| if (!medianTime) { | |
| return `No estimated time for chain ${chainId}` | |
| } | |
| return medianTime | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- packages/sdk-router/src/operations/bridge.ts (1 hunks)
- packages/sdk-router/src/sdk.test.ts (7 hunks)
- packages/sdk-router/src/sdk.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/sdk-router/src/sdk.ts
Additional comments: 3
packages/sdk-router/src/sdk.test.ts (3)
25-26: The new constants
MEDIAN_TIME_BRIDGEandMEDIAN_TIME_CCTPare imported correctly.570-572: The test cases correctly check if the estimated time matches the expected median time for the respective chains and routers.
687-752: The new test cases for the
getEstimatedTimefunction are well-structured and cover all the necessary scenarios, including both supported and unsupported chains and routers.
| return bestSet.finalizeBridgeRoute(bestRoute, deadline) | ||
| } | ||
|
|
||
| /** | ||
| * Returns the estimated time for a bridge operation from a given origin chain using a given router. | ||
| * This will be the estimated time for the bridge operation to be completed regardless of the destination chain, | ||
| * or the bridge token. | ||
| * | ||
| * @param originChainId - The ID of the origin chain. | ||
| * @param routerAddress - The address of the router deployed on the origin chain. | ||
| * @returns - The estimated time for a bridge operation. | ||
| * @throws - Will throw an error if the router address is unknown for the given chain. | ||
| */ | ||
| export function getEstimatedTime( | ||
| this: SynapseSDK, | ||
| originChainId: number, | ||
| routerAddress: string | ||
| ): number { | ||
| if (this.synapseRouterSet.getRouter(originChainId, routerAddress)) { | ||
| return this.synapseRouterSet.getEstimatedTime(originChainId) | ||
| } | ||
| if (this.synapseCCTPRouterSet.getRouter(originChainId, routerAddress)) { | ||
| return this.synapseCCTPRouterSet.getEstimatedTime(originChainId) | ||
| } | ||
| throw new Error('Unknown router address') | ||
| } |
There was a problem hiding this comment.
The getEstimatedTime function is well implemented. It checks both synapseRouterSet and synapseCCTPRouterSet for the router and returns the estimated time accordingly. If the router is not found in either set, it throws an error, which is a good practice for error handling. However, it would be more informative if the error message included the originChainId and routerAddress for better debugging.
- throw new Error('Unknown router address')
+ throw new Error(`Unknown router address for originChainId: ${originChainId}, routerAddress: ${routerAddress}`)Commitable suggestion (Beta)
| return bestSet.finalizeBridgeRoute(bestRoute, deadline) | |
| } | |
| /** | |
| * Returns the estimated time for a bridge operation from a given origin chain using a given router. | |
| * This will be the estimated time for the bridge operation to be completed regardless of the destination chain, | |
| * or the bridge token. | |
| * | |
| * @param originChainId - The ID of the origin chain. | |
| * @param routerAddress - The address of the router deployed on the origin chain. | |
| * @returns - The estimated time for a bridge operation. | |
| * @throws - Will throw an error if the router address is unknown for the given chain. | |
| */ | |
| export function getEstimatedTime( | |
| this: SynapseSDK, | |
| originChainId: number, | |
| routerAddress: string | |
| ): number { | |
| if (this.synapseRouterSet.getRouter(originChainId, routerAddress)) { | |
| return this.synapseRouterSet.getEstimatedTime(originChainId) | |
| } | |
| if (this.synapseCCTPRouterSet.getRouter(originChainId, routerAddress)) { | |
| return this.synapseCCTPRouterSet.getEstimatedTime(originChainId) | |
| } | |
| throw new Error('Unknown router address') | |
| } | |
| throw new Error(`Unknown router address for originChainId: ${originChainId}, routerAddress: ${routerAddress}`) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- packages/sdk-router/src/constants/medianTime.ts (1 hunks)
- packages/sdk-router/src/operations/bridge.ts (1 hunks)
Additional comments: 1
packages/sdk-router/src/constants/medianTime.ts (1)
- 1-39: The new data structures
MEDIAN_TIME_BRIDGEandMEDIAN_TIME_CCTPare well defined and follow the correct syntax. The use of theSupportedChainIdenum as keys is a good practice as it ensures consistency and avoids potential errors due to typos. However, ensure that the median times are accurate and up-to-date. If these values change frequently, consider fetching them from a reliable source at runtime instead of hardcoding them.
| return bestSet.finalizeBridgeRoute(bestRoute, deadline) | ||
| } | ||
|
|
||
| /** | ||
| * Returns the estimated time for a bridge operation from a given origin chain using a given router. | ||
| * This will be the estimated time for the bridge operation to be completed regardless of the destination chain, | ||
| * or the bridge token. | ||
| * | ||
| * @param originChainId - The ID of the origin chain. | ||
| * @param routerAddress - The address of the router deployed on the origin chain. | ||
| * @returns - The estimated time for a bridge operation, in seconds. | ||
| * @throws - Will throw an error if the router address is unknown for the given chain. | ||
| */ | ||
| export function getEstimatedTime( | ||
| this: SynapseSDK, | ||
| originChainId: number, | ||
| routerAddress: string | ||
| ): number { | ||
| if (this.synapseRouterSet.getRouter(originChainId, routerAddress)) { | ||
| return this.synapseRouterSet.getEstimatedTime(originChainId) | ||
| } | ||
| if (this.synapseCCTPRouterSet.getRouter(originChainId, routerAddress)) { | ||
| return this.synapseCCTPRouterSet.getEstimatedTime(originChainId) | ||
| } | ||
| throw new Error('Unknown router address') | ||
| } |
There was a problem hiding this comment.
The getEstimatedTime function is well implemented. It checks both synapseRouterSet and synapseCCTPRouterSet for the router and returns the estimated time accordingly. If the router is not found in either set, it throws an error, which is a good practice for error handling. However, it would be more informative if the error message included the originChainId and routerAddress for easier debugging.
- throw new Error('Unknown router address')
+ throw new Error(`Unknown router address for originChainId: ${originChainId}, routerAddress: ${routerAddress}`)Commitable suggestion (Beta)
| return bestSet.finalizeBridgeRoute(bestRoute, deadline) | |
| } | |
| /** | |
| * Returns the estimated time for a bridge operation from a given origin chain using a given router. | |
| * This will be the estimated time for the bridge operation to be completed regardless of the destination chain, | |
| * or the bridge token. | |
| * | |
| * @param originChainId - The ID of the origin chain. | |
| * @param routerAddress - The address of the router deployed on the origin chain. | |
| * @returns - The estimated time for a bridge operation, in seconds. | |
| * @throws - Will throw an error if the router address is unknown for the given chain. | |
| */ | |
| export function getEstimatedTime( | |
| this: SynapseSDK, | |
| originChainId: number, | |
| routerAddress: string | |
| ): number { | |
| if (this.synapseRouterSet.getRouter(originChainId, routerAddress)) { | |
| return this.synapseRouterSet.getEstimatedTime(originChainId) | |
| } | |
| if (this.synapseCCTPRouterSet.getRouter(originChainId, routerAddress)) { | |
| return this.synapseCCTPRouterSet.getEstimatedTime(originChainId) | |
| } | |
| throw new Error('Unknown router address') | |
| } | |
| throw new Error(`Unknown router address for originChainId: ${originChainId}, routerAddress: ${routerAddress}`) |
Description
bridgeQuote()alongside other quote dataSummary by CodeRabbit
New Features:
getEstimatedTimefunction toRouterSet,SynapseCCTPRouterSet, andSynapseRouterSetclasses, providing estimated transaction completion times for various blockchain chains.estimatedTimeproperty to theBridgeQuoteinterface, enhancing transaction information with estimated completion times.Tests:
getEstimatedTimefunction across supported and unsupported chains.estimatedTimeproperty.Chores:
getEstimatedTimefunction andestimatedTimeproperty.