feat(api): max bridge amounts [SLT-165] #3131
Conversation
bigboydiamonds
requested review from
abtestingalpha,
aureliusbtc,
trajan0x and
Defi-Moses
as code owners
WalkthroughThe changes involve enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
|
github-actions
bot
added
M-synapse-interface
Sol
Typescript
M-deps
size/m
needs-yarn-install
labels
Bundle ReportChanges will increase total bundle size by 409.84kB (1.14%) ⬆️. This is within the configured threshold ✅ Detailed changes
ℹ️ *Bundle size includes cached data from a previous commit |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3131 +/- ##
====================================================
+ Coverage 38.23421% 90.96267% +52.72846%
====================================================
Files 423 54 -369
Lines 24363 1018 -23345
Branches 119 82 -37
====================================================
- Hits 9315 926 -8389
+ Misses 14309 91 -14218
+ Partials 739 1 -738
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Deploying sanguine-fe with
|
| Latest commit: |
d006117
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2417be54.sanguine-fe.pages.dev |
| Branch Preview URL: | https://api-bridge-max-amounts.sanguine-fe.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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 (4)
- packages/rest-api/package.json (1 hunks)
- packages/rest-api/src/controllers/getBridgeLimitsController.ts (1 hunks)
- packages/rest-api/src/routes/getBridgeLimitsRoute.ts (1 hunks)
- packages/rest-api/src/routes/index.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/rest-api/package.json
Additional comments not posted (4)
packages/rest-api/src/routes/index.ts (2)
8-8: LGTM!The import statement for
getBridgeLimitsRoutefollows the same pattern as other route imports and is consistent with the file's structure.
21-21: Looks good!The new route
/getBridgeLimitsis added correctly using the importedgetBridgeLimitsRoute. This addition enhances the API's capabilities related to bridge transactions without modifying existing functionality.packages/rest-api/src/routes/getBridgeLimitsRoute.ts (1)
11-31: LGTM!The route definition is well-structured and follows best practices:
- The route path is appropriate for retrieving bridge limits.
- The validation checks ensure that the required parameters are provided and are valid.
- The
showFirstValidationErrormiddleware is used to handle validation errors.- The
getBridgeLimitsControlleris invoked after successful validation.packages/rest-api/src/controllers/getBridgeLimitsController.ts (1)
110-116: Error handling for unexpected errors looks good!The function includes a
catchblock that handles any unexpected errors that may occur during its execution. It returns a 500 status code and an error message to the client, which is an appropriate way to handle unexpected errors in an API endpoint.
| const toTokenInfo = res.locals.tokenInfo.toToken | ||
|
|
||
| const rfqResponse = await axios.get('https://rfq-api.omnirpc.io/quotes', { | ||
| params: { | ||
| originChainId: fromChain, | ||
| originTokenAddress: fromTokenInfo.address, | ||
| destChainId: toChain, | ||
| destTokenAddress: toTokenInfo.address, | ||
| }, | ||
| }) | ||
|
|
||
| const rfqQuotes = rfqResponse.data | ||
|
|
||
| let bestRfqQuote = null | ||
|
|
||
| if (Array.isArray(rfqQuotes) && rfqQuotes.length > 0) { | ||
| const filteredQuotes = rfqQuotes | ||
| .slice(0, 20) | ||
| .filter( | ||
| (quote) => | ||
| Number(quote.origin_chain_id) === Number(fromChain) && | ||
| Number(quote.dest_chain_id) === Number(toChain) && | ||
| getAddress(quote.origin_token_addr) === | ||
| getAddress(fromTokenInfo.address) && | ||
| getAddress(quote.dest_token_addr) === | ||
| getAddress(toTokenInfo.address) | ||
| ) | ||
|
|
||
| bestRfqQuote = filteredQuotes.reduce((maxQuote, currentQuote) => { | ||
| const currentAmount = Number(currentQuote.max_origin_amount) | ||
| const maxAmount = maxQuote ? Number(maxQuote.max_origin_amount) : 0 | ||
| return currentAmount > maxAmount ? currentQuote : maxQuote | ||
| }, null) | ||
| } | ||
|
|
||
| const upperLimitAmount = parseUnits('1000000', fromTokenInfo.decimals) | ||
| const upperLimitBridgeQuotes = await Synapse.allBridgeQuotes( | ||
| Number(fromChain), | ||
| Number(toChain), | ||
| fromTokenInfo.address, | ||
| toTokenInfo.address, | ||
| upperLimitAmount | ||
| ) | ||
|
|
||
| const lowerLimitAmount = parseUnits('100', fromTokenInfo.decimals) | ||
| const lowerLimitBridgeQuotes = await Synapse.allBridgeQuotes( | ||
| Number(fromChain), | ||
| Number(toChain), | ||
| fromTokenInfo.address, | ||
| toTokenInfo.address, | ||
| lowerLimitAmount | ||
| ) | ||
|
|
||
| const bestUpperLimitSDKQuote = upperLimitBridgeQuotes[0] | ||
|
|
||
| let maxOriginQuote | ||
|
|
||
| const minBridgeFeeQuote = lowerLimitBridgeQuotes.reduce( | ||
| (minQuote, currentQuote) => { | ||
| const currentFeeAmount = currentQuote.feeAmount | ||
| const minFeeAmount = minQuote ? minQuote.feeAmount : null | ||
|
|
||
| return !minFeeAmount || currentFeeAmount.lt(minFeeAmount) | ||
| ? currentQuote | ||
| : minQuote | ||
| }, | ||
| null | ||
| ) | ||
|
|
||
| if (bestRfqQuote) { | ||
| const bestRfqQuoteMaxAmountBN = BigNumber.from( | ||
| bestRfqQuote.max_origin_amount | ||
| ) | ||
| maxOriginQuote = bestRfqQuoteMaxAmountBN.gt( | ||
| bestUpperLimitSDKQuote.maxAmountOut | ||
| ) | ||
| ? { source: 'RFQ', amount: bestRfqQuoteMaxAmountBN } | ||
| : { | ||
| source: bestUpperLimitSDKQuote.bridgeModuleName, | ||
| amount: bestUpperLimitSDKQuote.maxAmountOut, | ||
| } | ||
| } else { | ||
| maxOriginQuote = { | ||
| source: bestUpperLimitSDKQuote.bridgeModuleName, | ||
| amount: bestUpperLimitSDKQuote.maxAmountOut, | ||
| } | ||
| } | ||
|
|
||
| return res.json({ | ||
| maxOriginAmount: maxOriginQuote.amount, | ||
| minOriginAmount: minBridgeFeeQuote.feeAmount, | ||
| }) | ||
| } catch (err) { | ||
| res.status(500).json({ | ||
| error: | ||
| 'An unexpected error occurred in /getBridgeLimits. Please try again later.', | ||
| details: err.message, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider breaking down the function into smaller, more focused functions and adding more detailed comments.
The getBridgeLimitsController function is quite large and performs several distinct tasks, such as:
- Validating request parameters
- Retrieving quotes from external services
- Filtering and comparing quotes
- Calculating transfer limits
To improve readability and maintainability, consider breaking down the function into smaller, more focused functions that each perform a single task. Additionally, consider adding more detailed comments explaining the purpose and logic of each section of the function.
| const rfqResponse = await axios.get('https://rfq-api.omnirpc.io/quotes', { | ||
| params: { | ||
| originChainId: fromChain, | ||
| originTokenAddress: fromTokenInfo.address, | ||
| destChainId: toChain, | ||
| destTokenAddress: toTokenInfo.address, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Consider adding error handling for the API call to the RFQ service.
The function makes an API call to the RFQ service to retrieve quotes for the specified token transfer. However, it does not handle potential errors that may occur during this API call, such as network errors or invalid responses.
To improve the robustness of the function, consider adding error handling for this API call, such as catching any errors and returning an appropriate error response to the client.
| const upperLimitAmount = parseUnits('1000000', fromTokenInfo.decimals) | ||
| const upperLimitBridgeQuotes = await Synapse.allBridgeQuotes( | ||
| Number(fromChain), | ||
| Number(toChain), | ||
| fromTokenInfo.address, | ||
| toTokenInfo.address, | ||
| upperLimitAmount | ||
| ) |
There was a problem hiding this comment.
Consider adding error handling for the API calls to the Synapse service.
The function makes two API calls to the Synapse service to retrieve bridge quotes for the upper and lower limit amounts. However, it does not handle potential errors that may occur during these API calls, such as network errors or invalid responses.
To improve the robustness of the function, consider adding error handling for these API calls, such as catching any errors and returning an appropriate error response to the client.
Also applies to: 62-69
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (1)
packages/rest-api/src/utils/tokenAddressToToken.ts (1)
3-17: Add unit tests fortokenAddressToTokento ensure correctness and reliabilityIncluding unit tests for the
tokenAddressToTokenfunction would help validate its behavior across various scenarios and enhance the reliability of the code.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- packages/rest-api/src/controllers/getBridgeLimitsController.ts (1 hunks)
- packages/rest-api/src/routes/getBridgeLimitsRoute.ts (1 hunks)
- packages/rest-api/src/tests/getBridgeLimits.test.ts (1 hunks)
- packages/rest-api/src/utils/tokenAddressToToken.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/rest-api/src/controllers/getBridgeLimitsController.ts
- packages/rest-api/src/routes/getBridgeLimitsRoute.ts
Additional context used
Gitleaks
packages/rest-api/src/tests/getBridgeLimits.test.ts
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
15-15: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
27-27: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
28-28: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
41-41: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
42-42: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
52-52: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
62-62: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Additional comments not posted (2)
packages/rest-api/src/tests/getBridgeLimits.test.ts (2)
10-21: Test case for valid input is correctly implementedThe test accurately checks that the endpoint returns the expected data when provided with valid input parameters.
Tools
Gitleaks
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
15-15: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
23-35: Test case for unsupported 'fromChain' is appropriateThe test correctly ensures that the API returns a 400 status and an appropriate error message when an unsupported 'fromChain' is provided.
Tools
Gitleaks
27-27: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
28-28: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| export const tokenAddressToToken = (chain: string, tokenAddress: string) => { | ||
| const chainData = BRIDGE_MAP[chain] | ||
| if (!chainData) { | ||
| return null | ||
| } | ||
| const tokenInfo = chainData[tokenAddress] | ||
| if (!tokenInfo) { | ||
| return null | ||
| } | ||
| return { | ||
| address: tokenAddress, | ||
| symbol: tokenInfo.symbol, | ||
| decimals: tokenInfo.decimals, | ||
| } | ||
| } |
There was a problem hiding this comment.
Specify the return type of tokenAddressToToken for improved type safety
To enhance type safety and readability, consider specifying the return type of the tokenAddressToToken function. Defining an interface for the returned token information would clarify the expected output.
For example:
interface TokenInfo {
address: string;
symbol: string;
decimals: number;
}
export const tokenAddressToToken = (chain: string, tokenAddress: string): TokenInfo | null => {
// function body...
}| const tokenInfo = chainData[tokenAddress] | ||
| if (!tokenInfo) { | ||
| return null | ||
| } |
There was a problem hiding this comment.
Normalize tokenAddress to ensure consistent token lookups
Token addresses may differ in case, potentially leading to lookup failures since object keys in JavaScript are case sensitive. Consider normalizing tokenAddress, such as converting it to lowercase, and ensure that the keys in BRIDGE_MAP are in the same format.
Apply this diff to normalize tokenAddress:
const tokenInfo = chainData[tokenAddress]
+const normalizedTokenAddress = tokenAddress.toLowerCase()
+const tokenInfo = chainData[normalizedTokenAddress]
if (!tokenInfo) {
return null
}Also, ensure that the keys in BRIDGE_MAP are normalized for consistency.
Committable suggestion was skipped due to low confidence.
bigboydiamonds
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
packages/rest-api/src/tests/getBridgeLimits.test.ts (1)
30-35: Simplify error message assertions for improved readabilityYou can consolidate the error message assertions into single-line statements to enhance readability and maintain consistency across your test cases.
Apply this diff to streamline the assertions:
- expect(response.body.error).toHaveProperty( - 'message', - 'Unsupported fromChain' - ) + expect(response.body.error).toHaveProperty('message', 'Unsupported fromChain') ... - expect(response.body.error).toHaveProperty('message', 'Unsupported toChain') + expect(response.body.error).toHaveProperty('message', 'Unsupported toChain')Also applies to: 44-46
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- packages/rest-api/src/tests/getBridgeLimits.test.ts (1 hunks)
Additional context used
Gitleaks
packages/rest-api/src/tests/getBridgeLimits.test.ts
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
15-15: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
27-27: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
28-28: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
41-41: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
42-42: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
52-52: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
62-62: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Additional comments not posted (2)
packages/rest-api/src/tests/getBridgeLimits.test.ts (2)
9-21: Test case for valid input is comprehensive and well-implementedThe test correctly validates successful responses for valid inputs, ensuring that both
maxOriginAmountandminOriginAmountare present in the response body.Tools
Gitleaks
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
15-15: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
58-66: Consistency in test descriptions and assertionsGood job maintaining consistency in the test description and expected status code for the missing
'toToken'case.Tools
Gitleaks
62-62: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- packages/rest-api/src/tests/getBridgeLimits.test.ts (1 hunks)
Additional context used
Gitleaks
packages/rest-api/src/tests/getBridgeLimits.test.ts
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
15-15: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
27-27: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
28-28: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
41-41: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
42-42: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
55-55: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
56-56: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
66-66: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
76-76: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- packages/rest-api/src/constants/bridgeable.ts (1 hunks)
- packages/rest-api/src/constants/index.ts (1 hunks)
- packages/rest-api/src/controllers/getBridgeLimitsController.ts (1 hunks)
- packages/rest-api/src/utils/tokenAddressToToken.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/rest-api/src/constants/bridgeable.ts
Files skipped from review as they are similar to previous changes (2)
- packages/rest-api/src/controllers/getBridgeLimitsController.ts
- packages/rest-api/src/utils/tokenAddressToToken.ts
| @@ -3,3 +3,5 @@ export const VALID_BRIDGE_MODULES = [ | |||
| 'SynapseCCTP', | |||
| 'SynapseRFQ', | |||
| ] | |||
|
|
|||
| export const ZeroAddress = '0x0000000000000000000000000000000000000000' | |||
There was a problem hiding this comment.
Use consistent naming conventions for constants
To maintain consistency with existing constants like VALID_BRIDGE_MODULES, consider renaming ZeroAddress to ZERO_ADDRESS.
Apply this diff to rename the constant:
-export const ZeroAddress = '0x0000000000000000000000000000000000000000'
+export const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'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 const ZeroAddress = '0x0000000000000000000000000000000000000000' | |
| export const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000' |
|
Logic looks great to me, no structural changes that I see. The two things I think that need to be updated:
(the response isnt formatted for decimals properly and thus returns the incorrect maxAmountOut. My hunch is that you are parsing using fromToken decimals for the output, not toTokenDecimals. Other than that seems good to me |
Thanks for catching this! Updated in bb9aa82 , was using the |
bigboydiamonds
force-pushed
the
api/bridge-max-amounts
branch
2 times, most recently
from
3c6ca0a
to
55c8d85
Compare
bigboydiamonds
force-pushed
the
api/bridge-max-amounts
branch
from
aa521c3
to
d006117
Compare
Description
Adds
/getBridgeLimitsendpoint to provide min/max origin input values for bridge routes based on query params ofSummary by CodeRabbit
Summary by CodeRabbit
New Features
/get-quote-maxfor fetching maximum bridge quotes for token transfers across blockchain networks.Bug Fixes
714ad0d: synapse-interface preview link
eaa421f: synapse-interface preview link