[wip] feat(api): bridge limit script #3131
Conversation
bigboydiamonds
requested review from
abtestingalpha,
aureliusbtc,
trajan0x and
Defi-Moses
as code owners
WalkthroughThe changes enhance the 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
|
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 41.54443% 37.96419% -3.58024%
===================================================
Files 460 418 -42
Lines 25770 24236 -1534
Branches 357 82 -275
===================================================
- Hits 10706 9201 -1505
+ Misses 14326 14297 -29
Partials 738 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: |
60138d4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9f0288e1.sanguine-fe.pages.dev |
| Branch Preview URL: | https://api-bridge-limits-v1.sanguine-fe.pages.dev |
| import { formatBNToString } from '../utils/formatBNToString' | ||
| import { BRIDGE_LIMIT_MAPPING } from '../utils/bridgeLimitMapping' | ||
|
|
||
| export const getBridgeLimitsController = async (req, res) => { |
There was a problem hiding this comment.
rename to bridgeLimitsController
| @@ -0,0 +1,81 @@ | |||
| import request from 'supertest' | |||
| import express from 'express' | |||
|
|
|||
There was a problem hiding this comment.
rename this test file to bridgeLimitsRoute.test.ts to match the rest of the tests. we might write controller/middleware tests later, so this naming convention keeps them unique.
| import { getBridgeLimitsController } from '../controllers/getBridgeLimitsController' | ||
|
|
||
| const router = express.Router() | ||
|
|
There was a problem hiding this comment.
add swagger docs (see other routes for examples)
| '/', | ||
| [ | ||
| check('fromChain') | ||
| .isNumeric() |
There was a problem hiding this comment.
existence checks should come first:
does it exist ->
if it exists, is it numeric ->
if it exists and is numeric, is it supported
| it('should return min/max origin amounts for valid input', async () => { | ||
| const response = await request(app).get('/getBridgeLimits').query({ | ||
| fromChain: 1, | ||
| fromToken: '0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48', |
There was a problem hiding this comment.
import USDC from bridgeable vs. hardcoding address where we already have it in app
| expect(response.body).toHaveProperty('maxOriginAmount') | ||
| expect(response.body).toHaveProperty('minOriginAmount') | ||
| }, 10_000) | ||
|
|
There was a problem hiding this comment.
Add tests not just for stables but also ETH.
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Outside diff range and nitpick comments (4)
packages/rest-api/src/tests/bridgeLimitsRoute.test.ts (4)
9-21: LGTM: Valid input test case is well-structured.The test case for valid input is comprehensive, checking both the status code and the presence of expected properties in the response. The 10-second timeout is appropriate for an API call that might involve external services.
Consider adding assertions to check if
maxOriginAmountandminOriginAmountare numbers and ifmaxOriginAmountis greater thanminOriginAmount. This would provide more robust validation of the response structure and values.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)
62-70: LGTM: Missing fromToken test case is correct.The test case for missing fromToken is well-structured and consistent with the previous error case tests. It correctly checks both the status code and the specific error field.
Consider adding an assertion to check the error message as well, similar to the unsupported chain tests. This would ensure that the API provides a clear error message for missing parameters.
Tools
Gitleaks
66-66: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
72-81: LGTM: Missing toToken test case is correct and consistent.The test case for missing toToken is well-structured and consistent with the previous missing parameter test. It correctly checks both the status code and the specific error field.
As with the missing fromToken test, consider adding an assertion to check the error message. This would ensure that the API provides a clear error message for all missing parameters.
Tools
Gitleaks
76-76: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1-81: Overall, the test suite is well-structured and comprehensive.The test cases cover both valid and error scenarios, providing good coverage for the bridgeLimitsRoute. Here are the main points for improvement:
- Remove the duplicate test case for unsupported fromChain (lines 37-49).
- Add assertions to check if
maxOriginAmountandminOriginAmountare numbers and ifmaxOriginAmountis greater thanminOriginAmountin the valid input test.- Add assertions to check the error messages in the missing fromToken and toToken tests.
- Consider adding test cases for edge cases, such as minimum and maximum allowed values for chain IDs and token addresses with different formats (checksummed vs. non-checksummed).
Implementing these improvements will enhance the robustness and completeness of the test suite.
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)
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)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- packages/rest-api/src/controllers/bridgeLimitsController.ts (1 hunks)
- packages/rest-api/src/routes/bridgeLimitsRoute.ts (1 hunks)
- packages/rest-api/src/tests/bridgeLimitsRoute.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/rest-api/src/routes/bridgeLimitsRoute.ts
Additional context used
Gitleaks
packages/rest-api/src/tests/bridgeLimitsRoute.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)
Additional comments not posted (3)
packages/rest-api/src/tests/bridgeLimitsRoute.test.ts (3)
1-7: LGTM: Imports and app setup are correct.The necessary dependencies are imported, and the Express app is set up correctly with the bridgeLimitsRoute. This follows best practices for setting up a test environment for an Express route.
51-60: LGTM: Unsupported toChain test case is correct and consistent.The test case for unsupported toChain is well-structured and consistent with the previous error case test. It correctly checks both the status code and the specific error message.
Tools
Gitleaks
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)
13-13: False positives in static analysis: Ethereum addresses mistaken for API keys.The static analysis tool has flagged several lines containing Ethereum addresses as potential Generic API Key exposures. These are false positives, as Ethereum addresses can sometimes be mistaken for API keys due to their similar format (long hexadecimal strings).
To prevent future confusion, consider adding a comment at the top of the file explaining that the hexadecimal strings used in the tests are Ethereum addresses and not API keys. This will help other developers and static analysis tools understand the nature of these values.
Also applies to: 15-15, 27-28, 41-42, 55-56, 66-66, 76-76
Tools
Gitleaks
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| it('should return 400 for unsupported fromChain', async () => { | ||
| const response = await request(app).get('/bridgeLimits').query({ | ||
| fromChain: '999', | ||
| toChain: '137', | ||
| fromToken: '0x176211869cA2b568f2A7D4EE941E073a821EE1ff', | ||
| toToken: '0x3c499c542cEF5E3811e1192ce70d8cC03d5c3359', | ||
| }) | ||
| expect(response.status).toBe(400) | ||
| expect(response.body.error).toHaveProperty( | ||
| 'message', | ||
| 'Unsupported fromChain' | ||
| ) | ||
| }, 10_000) |
There was a problem hiding this comment.
LGTM: Unsupported fromChain test case is correct, but there's a duplication.
The test case for unsupported fromChain is well-structured, checking both the status code and the error message. However, there's a duplicate test case (lines 37-49) with identical content.
Remove the duplicate test case (lines 37-49) to maintain a clean and efficient test suite.
Also applies to: 37-49
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)
| const errors = validationResult(req) | ||
| if (!errors.isEmpty()) { | ||
| return res.status(400).json({ errors: errors.array() }) | ||
| } |
There was a problem hiding this comment.
Ensure validation middleware is correctly set up for request parameters
While validationResult(req) checks for validation errors, there is no indication that validation rules for fromChain, fromToken, toChain, and toToken have been established. To prevent invalid or malicious inputs, please ensure that appropriate validation middleware is applied to these parameters. This should include checks such as isNumeric for chain IDs and isTokenAddress for token addresses to enhance error messaging and debugging.
| return res.json({ | ||
| BRIDGE_LIMIT_MAPPING, | ||
| maxOriginAmount, | ||
| minOriginAmount, | ||
| }) |
There was a problem hiding this comment.
Evaluate the necessity of including BRIDGE_LIMIT_MAPPING in the response
Including BRIDGE_LIMIT_MAPPING in the API response may expose internal configuration details that are not necessary for the client. Unless there is a specific need for the client to receive this data, consider removing it from the response to keep the API output clean and focused.
Apply this diff to remove BRIDGE_LIMIT_MAPPING from the response:
return res.json({
- BRIDGE_LIMIT_MAPPING,
maxOriginAmount,
minOriginAmount,
})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.
| return res.json({ | |
| BRIDGE_LIMIT_MAPPING, | |
| maxOriginAmount, | |
| minOriginAmount, | |
| }) | |
| return res.json({ | |
| maxOriginAmount, | |
| minOriginAmount, | |
| }) |
| const fromTokenInfo = tokenAddressToToken(fromChain, fromToken) | ||
| const toTokenInfo = tokenAddressToToken(toChain, toToken) | ||
|
|
There was a problem hiding this comment.
Add error handling for invalid token information
Currently, if tokenAddressToToken returns undefined or null for fromTokenInfo or toTokenInfo, accessing properties like .decimals will cause a runtime error. To prevent this, please add error handling to check if fromTokenInfo and toTokenInfo are valid before proceeding.
Apply this diff to implement error handling:
const fromTokenInfo = tokenAddressToToken(fromChain, fromToken)
const toTokenInfo = tokenAddressToToken(toChain, toToken)
+ if (!fromTokenInfo) {
+ return res.status(400).json({ error: 'Invalid fromToken address' })
+ }
+ if (!toTokenInfo) {
+ return res.status(400).json({ error: 'Invalid toToken address' })
+ }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.
| const fromTokenInfo = tokenAddressToToken(fromChain, fromToken) | |
| const toTokenInfo = tokenAddressToToken(toChain, toToken) | |
| const fromTokenInfo = tokenAddressToToken(fromChain, fromToken) | |
| const toTokenInfo = tokenAddressToToken(toChain, toToken) | |
| if (!fromTokenInfo) { | |
| return res.status(400).json({ error: 'Invalid fromToken address' }) | |
| } | |
| if (!toTokenInfo) { | |
| return res.status(400).json({ error: 'Invalid toToken address' }) | |
| } |
| 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.
Avoid exposing sensitive error details in the response
Returning err.message in the API response can inadvertently expose sensitive server information or implementation details. For security best practices, consider logging the error internally and returning a generic error message to the client.
Apply this diff to modify the error handling:
+ console.error('Error in /getBridgeLimitsController:', err)
res.status(500).json({
error:
'An unexpected error occurred in /getBridgeLimits. Please try again later.',
- details: err.message,
})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.
| res.status(500).json({ | |
| error: | |
| 'An unexpected error occurred in /getBridgeLimits. Please try again later.', | |
| details: err.message, | |
| }) | |
| console.error('Error in /getBridgeLimitsController:', err) | |
| res.status(500).json({ | |
| error: | |
| 'An unexpected error occurred in /getBridgeLimits. Please try again later.', | |
| }) |
| const maxBridgeAmountQuote = upperLimitBridgeQuotes.reduce( | ||
| (maxQuote, currentQuote) => { | ||
| const currentMaxAmount = currentQuote.maxAmountOut | ||
| const maxAmount = maxQuote ? maxQuote.maxAmountOut : BigNumber.from(0) | ||
|
|
||
| return currentMaxAmount.gt(maxAmount) ? currentQuote : maxQuote | ||
| }, | ||
| null | ||
| ) | ||
|
|
There was a problem hiding this comment.
Handle empty upperLimitBridgeQuotes to prevent runtime errors
The reduce method on upperLimitBridgeQuotes assumes that the array has at least one element. If upperLimitBridgeQuotes is empty or null, this will cause a runtime error. Please add a check to ensure upperLimitBridgeQuotes is not empty before calling reduce.
Apply this diff to add the necessary check:
+ let maxBridgeAmountQuote = null
+ if (upperLimitBridgeQuotes && upperLimitBridgeQuotes.length > 0) {
maxBridgeAmountQuote = upperLimitBridgeQuotes.reduce(
(maxQuote, currentQuote) => {
const currentMaxAmount = currentQuote.maxAmountOut
const maxAmount = maxQuote ? maxQuote.maxAmountOut : BigNumber.from(0)
return currentMaxAmount.gt(maxAmount) ? currentQuote : maxQuote
},
null
)
+ }Similarly, ensure that upperLimitBridgeQuotes is not null or empty before proceeding.
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.
| const maxBridgeAmountQuote = upperLimitBridgeQuotes.reduce( | |
| (maxQuote, currentQuote) => { | |
| const currentMaxAmount = currentQuote.maxAmountOut | |
| const maxAmount = maxQuote ? maxQuote.maxAmountOut : BigNumber.from(0) | |
| return currentMaxAmount.gt(maxAmount) ? currentQuote : maxQuote | |
| }, | |
| null | |
| ) | |
| let maxBridgeAmountQuote = null | |
| if (upperLimitBridgeQuotes && upperLimitBridgeQuotes.length > 0) { | |
| maxBridgeAmountQuote = upperLimitBridgeQuotes.reduce( | |
| (maxQuote, currentQuote) => { | |
| const currentMaxAmount = currentQuote.maxAmountOut | |
| const maxAmount = maxQuote ? maxQuote.maxAmountOut : BigNumber.from(0) | |
| return currentMaxAmount.gt(maxAmount) ? currentQuote : maxQuote | |
| }, | |
| null | |
| ) | |
| } |
| const minBridgeAmountQuote = lowerLimitBridgeQuotes.reduce( | ||
| (minQuote, currentQuote) => { | ||
| const currentFeeAmount = currentQuote.feeAmount | ||
| const minFeeAmount = minQuote ? minQuote.feeAmount : null | ||
|
|
||
| return !minFeeAmount || currentFeeAmount.lt(minFeeAmount) | ||
| ? currentQuote | ||
| : minQuote | ||
| }, | ||
| null | ||
| ) | ||
|
|
There was a problem hiding this comment.
Handle empty lowerLimitBridgeQuotes to prevent runtime errors
Just like with upperLimitBridgeQuotes, calling reduce on lowerLimitBridgeQuotes without ensuring it's not empty can lead to a runtime error. Please add a check to ensure lowerLimitBridgeQuotes has elements before using reduce.
Apply this diff to add the necessary check:
+ let minBridgeAmountQuote = null
+ if (lowerLimitBridgeQuotes && lowerLimitBridgeQuotes.length > 0) {
minBridgeAmountQuote = lowerLimitBridgeQuotes.reduce(
(minQuote, currentQuote) => {
const currentFeeAmount = currentQuote.feeAmount
const minFeeAmount = minQuote ? minQuote.feeAmount : null
return !minFeeAmount || currentFeeAmount.lt(minFeeAmount)
? currentQuote
: minQuote
},
null
)
+ }Ensure that you handle cases where lowerLimitBridgeQuotes may be null or empty.
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.
| const minBridgeAmountQuote = lowerLimitBridgeQuotes.reduce( | |
| (minQuote, currentQuote) => { | |
| const currentFeeAmount = currentQuote.feeAmount | |
| const minFeeAmount = minQuote ? minQuote.feeAmount : null | |
| return !minFeeAmount || currentFeeAmount.lt(minFeeAmount) | |
| ? currentQuote | |
| : minQuote | |
| }, | |
| null | |
| ) | |
| let minBridgeAmountQuote = null | |
| if (lowerLimitBridgeQuotes && lowerLimitBridgeQuotes.length > 0) { | |
| minBridgeAmountQuote = lowerLimitBridgeQuotes.reduce( | |
| (minQuote, currentQuote) => { | |
| const currentFeeAmount = currentQuote.feeAmount | |
| const minFeeAmount = minQuote ? minQuote.feeAmount : null | |
| return !minFeeAmount || currentFeeAmount.lt(minFeeAmount) | |
| ? currentQuote | |
| : minQuote | |
| }, | |
| null | |
| ) | |
| } |
| const maxOriginAmount = formatBNToString( | ||
| maxBridgeAmountQuote.maxAmountOut, | ||
| maxAmountOriginQueryTokenOutInfo.decimals | ||
| ) | ||
|
|
||
| const minOriginAmount = formatBNToString( | ||
| minBridgeAmountQuote.feeAmount, | ||
| minAmountOriginQueryTokenOutInfo.decimals | ||
| ) |
There was a problem hiding this comment.
Verify correct usage of token decimals in amount formatting
When formatting maxOriginAmount and minOriginAmount, the decimals used are from maxAmountOriginQueryTokenOutInfo.decimals and minAmountOriginQueryTokenOutInfo.decimals, which are derived from destQuery.tokenOut and originQuery.tokenOut. Please verify that these are the appropriate token decimals to use.
For maxOriginAmount, if maxBridgeAmountQuote.maxAmountOut corresponds to the destination token, you should use toTokenInfo.decimals to ensure correct formatting. Similarly, confirm the correctness for minOriginAmount.
Consider updating the formatting to use the correct token decimals:
const maxOriginAmount = formatBNToString(
maxBridgeAmountQuote.maxAmountOut,
- maxAmountOriginQueryTokenOutInfo.decimals
+ toTokenInfo.decimals
)
const minOriginAmount = formatBNToString(
minBridgeAmountQuote.feeAmount,
- minAmountOriginQueryTokenOutInfo.decimals
+ fromTokenInfo.decimals
)Committable suggestion was skipped due to low confidence.
* script for fetching min values * max/min origin values queried and stored in ts file * add script * add retries for fetching bridge quotes
github-actions
bot
added
javascript
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (3)
packages/synapse-interface/scripts/utils/fetchBridgeQuote.js (3)
1-2: LGTM! Consider environment-based configuration.The
BRIDGE_URLconstant is correctly defined. For improved maintainability and flexibility across different environments (e.g., development, staging, production), consider using an environment variable for the API URL.You could modify the constant definition as follows:
const BRIDGE_URL = process.env.BRIDGE_API_URL || 'https://api.synapseprotocol.com/bridge'This allows for easy configuration changes without modifying the code.
5-11: LGTM! Consider adding parameter validation.The function signature is well-defined and uses async appropriately. To improve robustness, consider adding parameter validation at the beginning of the function.
You could add validation as follows:
const fetchBridgeQuote = async ( fromChain, toChain, fromToken, toToken, amount ) => { if (!fromChain || !toChain || !fromToken || !toToken || !amount) { throw new Error('Missing required parameters'); } if (typeof amount !== 'string' && isNaN(amount)) { throw new Error('Amount must be a number or a numeric string'); } // Rest of the function... }This ensures all required parameters are provided and the amount is valid before making the API call.
13-20: LGTM! Consider using object destructuring for conciseness.The URL construction and parameter appending are implemented correctly. For slightly improved readability and conciseness, you could use object destructuring and
Object.entries().Here's an alternative implementation:
const url = new URL(BRIDGE_URL) const params = { fromChain, toChain, fromToken, toToken, amount } Object.entries(params).forEach(([key, value]) => url.searchParams.append(key, value))This approach reduces repetition and makes it easier to add or remove parameters in the future.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- packages/rest-api/package.json (1 hunks)
- packages/rest-api/src/controllers/bridgeLimitsController.ts (1 hunks)
- packages/synapse-interface/package.json (1 hunks)
- packages/synapse-interface/scripts/generateLimits.js (1 hunks)
- packages/synapse-interface/scripts/utils/fetchBridgeQuote.js (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/rest-api/package.json
- packages/rest-api/src/controllers/bridgeLimitsController.ts
Additional comments not posted (2)
packages/synapse-interface/scripts/utils/fetchBridgeQuote.js (1)
35-35: LGTM! Export syntax is correct.The module export is implemented correctly. Exporting as an object property allows for easy addition of more exports in the future if needed.
packages/synapse-interface/scripts/generateLimits.js (1)
68-70: Ensure correct decimal handling for origin valuesGiven previous issues with decimal formatting, verify that
minOriginValueandmaxOriginValueare correctly calculated using the appropriate token decimals, especially considering the decimals of the destination token.Run the following script to check whether the correct decimals are used in the calculations:
This script searches for patterns where
minOriginValueandmaxOriginValueare assigned values that should involve correct decimal handling.Also applies to: 109-112
| const response = await fetch(url) | ||
| if (!response.ok) { | ||
| throw new Error(`HTTP error! status: ${response.status}`) | ||
| } | ||
|
|
||
| const quotes = await response.json() | ||
| return quotes | ||
| } catch (error) { | ||
| console.error('Failed to fetch bridge quote:', error) | ||
| return [] | ||
| } |
There was a problem hiding this comment.
Enhance error handling and consider return values.
While the basic error handling is in place, there are opportunities for improvement:
- The error message could include more details about the request.
- Returning an empty array on error might mask issues. Consider throwing an error or returning a structured error object.
- The catch block could differentiate between network errors and API errors.
Here's a suggested improvement:
try {
const response = await fetch(url)
if (!response.ok) {
throw new Error(`HTTP error! status: ${response.status}, statusText: ${response.statusText}`)
}
return await response.json()
} catch (error) {
console.error('Failed to fetch bridge quote:', error)
if (error instanceof TypeError) {
throw new Error('Network error: Unable to reach the API')
} else {
throw new Error(`API error: ${error.message}`)
}
}This approach provides more detailed error information and allows the caller to handle errors more effectively.
| @@ -22,6 +22,7 @@ | |||
| "build:slither": " ", | |||
| "graphql:generate": "graphql-codegen --config codegen.ts", | |||
| "maps:generate": "node scripts/generateMaps.js", | |||
| "limits:generate": "node scripts/generateLimits.js", | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Issue Detected: Missing generateLimits.js Script
The limits:generate script references scripts/generateLimits.js, which does not exist in the repository. Please ensure that generateLimits.js is added or remove the script if it's unnecessary.
Analysis chain
LGTM! Consider documenting the new script.
The addition of the "limits:generate" script aligns well with the PR objectives of introducing bridge limits functionality. This automation will likely improve the development workflow.
To ensure the script is properly implemented, please run the following command:
Additionally, consider adding a brief description of this script's purpose in the project's README or documentation to help other developers understand its function.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and basic structure of the generateLimits.js script
# Check if the script file exists
if [ -f "scripts/generateLimits.js" ]; then
echo "generateLimits.js exists."
# Display the first few lines of the script to verify its basic structure
head -n 10 scripts/generateLimits.js
else
echo "Error: scripts/generateLimits.js not found."
fi
Length of output: 135
| return !bestMaxAmountOut || | ||
| parseFloat(currentMaxAmountOut) < | ||
| parseFloat(bestMaxAmountOut) | ||
| ? currentQuote | ||
| : maxQuote | ||
| }, |
There was a problem hiding this comment.
Incorrect comparison when determining maxAmountOutQuote
The comparison in the reduce function is selecting the quote with the smallest maxAmountOutStr instead of the largest. When calculating the maximum origin value, you should compare and select the quote with the largest maxAmountOutStr.
Apply this diff to correct the comparison:
- parseFloat(currentMaxAmountOut) <
+ parseFloat(currentMaxAmountOut) >So the corrected comparison is:
return !bestMaxAmountOut ||
parseFloat(currentMaxAmountOut) > parseFloat(bestMaxAmountOut)
? currentQuote
: maxQuoteThis ensures that maxAmountOutQuote will have the maximum maxAmountOutStr.
| for (const limitValue of lowerLimitValues) { | ||
| try { | ||
| const bridgeQuotes = await retryFetchBridgeQuote( | ||
| originChainId, | ||
| destinationChainId, | ||
| originTokenAddress, | ||
| destinationTokenAddress, | ||
| limitValue | ||
| ) | ||
|
|
||
| if (bridgeQuotes && bridgeQuotes.length > 0) { | ||
| const minBridgeAmountQuote = bridgeQuotes.reduce( | ||
| (minQuote, currentQuote) => { | ||
| const currentFee = currentQuote.bridgeFeeFormatted | ||
| const minFee = minQuote ? minQuote.bridgeFeeFormatted : null | ||
|
|
||
| return !minFee || | ||
| parseFloat(currentFee) < parseFloat(minFee) | ||
| ? currentQuote | ||
| : minQuote | ||
| }, | ||
| null | ||
| ) | ||
|
|
||
| minOriginValue = minBridgeAmountQuote.bridgeFeeFormatted | ||
|
|
||
| break | ||
| } | ||
| } catch (error) { | ||
| console.error( | ||
| `Failed to fetch bridge quote for ${originChainId} ${originTokenAddress} to ${destinationChainId} ${destinationTokenAddress}:`, | ||
| error | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider parallelizing bridge quote fetches to improve performance
Currently, bridge quotes are fetched sequentially within loops, which can be time-consuming. Consider fetching bridge quotes in parallel to improve performance, especially when dealing with numerous tokens and chains.
Example refactor using Promise.all:
// Fetch bridge quotes for lower limit values in parallel
const lowerLimitPromises = lowerLimitValues.map(limitValue =>
retryFetchBridgeQuote(
originChainId,
destinationChainId,
originTokenAddress,
destinationTokenAddress,
limitValue
).then(bridgeQuotes => ({ limitValue, bridgeQuotes }))
)
try {
const lowerLimitResults = await Promise.all(lowerLimitPromises)
// Process lowerLimitResults to find minOriginValue
} catch (error) {
console.error('Failed to fetch bridge quotes:', error)
}
// Similarly, fetch upper limit bridge quotes in parallel
const upperLimitPromises = upperLimitValues.map(limitValue =>
retryFetchBridgeQuote(
originChainId,
destinationChainId,
originTokenAddress,
destinationTokenAddress,
limitValue
).then(bridgeQuotes => ({ limitValue, bridgeQuotes }))
)
try {
const upperLimitResults = await Promise.all(upperLimitPromises)
// Process upperLimitResults to find maxOriginValue
} catch (error) {
console.error('Failed to fetch bridge quotes:', error)
}This approach can significantly reduce the script's total execution time.
Also applies to: 82-119
| minOriginValue = minBridgeAmountQuote.bridgeFeeFormatted | ||
|
|
||
| break |
There was a problem hiding this comment.
Possible incorrect assignment to minOriginValue
Currently, minOriginValue is being assigned the value of bridgeFeeFormatted, which represents the bridge fee. However, minOriginValue should represent the minimum origin amount needed to perform the bridge transaction. Consider assigning the appropriate amount from minBridgeAmountQuote, such as minAmountInStr.
Apply this correction:
- minOriginValue = minBridgeAmountQuote.bridgeFeeFormatted
+ minOriginValue = minBridgeAmountQuote.minAmountInStrEnsure that minOriginValue reflects the minimum input amount required.
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.
| minOriginValue = minBridgeAmountQuote.bridgeFeeFormatted | |
| break | |
| minOriginValue = minBridgeAmountQuote.minAmountInStr | |
| break |
| maxOriginValue = maxAmountOutQuote.maxAmountOutStr | ||
|
|
||
| break | ||
| } |
There was a problem hiding this comment.
Possible misassignment of maxOriginValue
maxOriginValue is assigned maxAmountOutQuote.maxAmountOutStr, which represents the maximum amount out. However, maxOriginValue should represent the maximum origin input value allowed for the bridge transaction. Consider assigning maxAmountOutQuote.maxAmountInStr instead.
Apply this correction:
- maxOriginValue = maxAmountOutQuote.maxAmountOutStr
+ maxOriginValue = maxAmountOutQuote.maxAmountInStrPlease verify that this reflects the intended logic.
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.
| maxOriginValue = maxAmountOutQuote.maxAmountOutStr | |
| break | |
| } | |
| maxOriginValue = maxAmountOutQuote.maxAmountInStr | |
| break | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
packages/rest-api/src/controllers/bridgeLimitsController.ts (2)
1-9: Consider cleaning up unused importsThere are several commented-out import statements in the file. If these are no longer needed, it would be best to remove them entirely to keep the code clean and avoid confusion. If they might be needed in the future, consider adding a TODO comment explaining why they're kept.
Apply this diff to clean up the imports:
import { validationResult } from 'express-validator' -// import { BigNumber } from 'ethers' -// import { parseUnits } from '@ethersproject/units' -// import { Synapse } from '../services/synapseService' -// import { tokenAddressToToken } from '../utils/tokenAddressToToken' -// import { formatBNToString } from '../utils/formatBNToString' import { BRIDGE_LIMITS_MAP } from '../constants/bridgeLimitsMap' -// import { BRIDGE_LIMIT_MAPPING } from '../utils/bridgeLimitMapping'If some of these imports might be needed in the future, add a TODO comment explaining why:
// TODO: These imports might be needed for future implementation of dynamic bridge limit calculation // import { BigNumber } from 'ethers' // import { parseUnits } from '@ethersproject/units' // ...
111-114: Add null checks before processing amountsThe current implementation calls
processAmountwithout checking ifmaxOriginAmountorminOriginAmountare null. While theprocessAmountfunction handles null values, it's generally a good practice to perform these checks before calling the function.Consider adding null checks before processing the amounts:
return res.json({ - maxOriginAmount: processAmount(maxOriginAmount), - minOriginAmount: processAmount(minOriginAmount), + maxOriginAmount: maxOriginAmount !== null ? processAmount(maxOriginAmount) : null, + minOriginAmount: minOriginAmount !== null ? processAmount(minOriginAmount) : null, })This change ensures that
processAmountis only called when the input is not null, improving code clarity and potentially avoiding unnecessary function calls.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- packages/rest-api/src/controllers/bridgeLimitsController.ts (1 hunks)
- packages/rest-api/src/routes/bridgeLimitsRoute.ts (1 hunks)
- packages/rest-api/src/utils/bridgeLimitMapping.ts (1 hunks)
- packages/synapse-interface/scripts/generateLimits.js (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- packages/rest-api/src/routes/bridgeLimitsRoute.ts
- packages/rest-api/src/utils/bridgeLimitMapping.ts
- packages/synapse-interface/scripts/generateLimits.js
Additional context used
Biome
packages/rest-api/src/controllers/bridgeLimitsController.ts
[error] 127-127: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
| export const bridgeLimitsController = async (req, res) => { | ||
| const errors = validationResult(req) | ||
| if (!errors.isEmpty()) { | ||
| return res.status(400).json({ errors: errors.array() }) | ||
| } | ||
| try { | ||
| const { fromChain, fromToken, toChain, toToken } = req.query | ||
|
|
||
| // const fromTokenInfo = tokenAddressToToken(fromChain, fromToken) | ||
| // const toTokenInfo = tokenAddressToToken(toChain, toToken) | ||
|
|
There was a problem hiding this comment.
Improve error handling to avoid exposing sensitive information
The current error handling in the catch block might expose sensitive server information by including err.message in the response. It's generally a good practice to log the error details server-side but return a generic error message to the client.
Apply this diff to improve the error handling:
} catch (err) {
+ console.error('Error in bridgeLimitsController:', err);
res.status(500).json({
error:
'An unexpected error occurred in /bridgeLimits. Please try again later.',
- details: err.message,
})
}This change logs the full error details server-side for debugging purposes while providing a generic error message to the client, enhancing security.
Also applies to: 115-121
| let maxOriginAmount = null | ||
| let minOriginAmount = null | ||
|
|
||
| if (BRIDGE_LIMITS_MAP[fromChain][fromToken].routes[toChain][toToken]) { | ||
| minOriginAmount = | ||
| BRIDGE_LIMITS_MAP[fromChain][fromToken].routes[toChain][toToken] | ||
| .minOriginValue | ||
| maxOriginAmount = | ||
| BRIDGE_LIMITS_MAP[fromChain][fromToken].routes[toChain][toToken] | ||
| .maxOriginValue | ||
| } |
There was a problem hiding this comment.
Add validation for route existence and consider fallback logic
The current implementation assumes that the requested route exists in the BRIDGE_LIMITS_MAP. If it doesn't, this could lead to a runtime error when trying to access undefined properties.
Consider adding a check for the route's existence and implement fallback logic. Here's a suggested improvement:
let maxOriginAmount = null
let minOriginAmount = null
- if (BRIDGE_LIMITS_MAP[fromChain][fromToken].routes[toChain][toToken]) {
+ if (BRIDGE_LIMITS_MAP[fromChain]?.[fromToken]?.routes?.[toChain]?.[toToken]) {
minOriginAmount =
BRIDGE_LIMITS_MAP[fromChain][fromToken].routes[toChain][toToken]
.minOriginValue
maxOriginAmount =
BRIDGE_LIMITS_MAP[fromChain][fromToken].routes[toChain][toToken]
.maxOriginValue
+ } else {
+ // Consider implementing fallback logic or returning an appropriate error
+ return res.status(400).json({ error: 'Unsupported bridge route' })
}This change adds optional chaining to safely check for the existence of the route and provides an error response if the route is not supported.
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.
| let maxOriginAmount = null | |
| let minOriginAmount = null | |
| if (BRIDGE_LIMITS_MAP[fromChain][fromToken].routes[toChain][toToken]) { | |
| minOriginAmount = | |
| BRIDGE_LIMITS_MAP[fromChain][fromToken].routes[toChain][toToken] | |
| .minOriginValue | |
| maxOriginAmount = | |
| BRIDGE_LIMITS_MAP[fromChain][fromToken].routes[toChain][toToken] | |
| .maxOriginValue | |
| } | |
| let maxOriginAmount = null | |
| let minOriginAmount = null | |
| if (BRIDGE_LIMITS_MAP[fromChain]?.[fromToken]?.routes?.[toChain]?.[toToken]) { | |
| minOriginAmount = | |
| BRIDGE_LIMITS_MAP[fromChain][fromToken].routes[toChain][toToken] | |
| .minOriginValue | |
| maxOriginAmount = | |
| BRIDGE_LIMITS_MAP[fromChain][fromToken].routes[toChain][toToken] | |
| .maxOriginValue | |
| } else { | |
| // Consider implementing fallback logic or returning an appropriate error | |
| return res.status(400).json({ error: 'Unsupported bridge route' }) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- packages/rest-api/src/controllers/bridgeLimitsController.ts (1 hunks)
Additional context used
Biome
packages/rest-api/src/controllers/bridgeLimitsController.ts
[error] 41-41: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
Additional comments not posted (1)
packages/rest-api/src/controllers/bridgeLimitsController.ts (1)
1-5: Imports and function signature look good.The imports and the
bridgeLimitsControllerfunction signature are appropriate for an Express.js controller handling bridge limits.
| const errors = validationResult(req) | ||
| if (!errors.isEmpty()) { | ||
| return res.status(400).json({ errors: errors.array() }) | ||
| } | ||
| try { | ||
| const { fromChain, fromToken, toChain, toToken } = req.query |
There was a problem hiding this comment.
Add validation rules for query parameters
While the code correctly checks for validation errors, there are no visible validation rules for the query parameters (fromChain, fromToken, toChain, toToken). Consider adding specific validation rules, such as:
[
query('fromChain').isNumeric(),
query('toChain').isNumeric(),
query('fromToken').isEthereumAddress(),
query('toToken').isEthereumAddress(),
]This will ensure that the input parameters are of the correct type and format before processing.
| let maxOriginAmount = null | ||
| let minOriginAmount = null | ||
|
|
||
| if (BRIDGE_LIMITS_MAP[fromChain][fromToken]) { | ||
| minOriginAmount = | ||
| BRIDGE_LIMITS_MAP[fromChain][fromToken].routes[toChain][toToken] | ||
| .minOriginValue | ||
| maxOriginAmount = | ||
| BRIDGE_LIMITS_MAP[fromChain][fromToken].routes[toChain][toToken] | ||
| .maxOriginValue | ||
| } |
There was a problem hiding this comment.
Improve error handling for bridge limits retrieval
The current implementation assumes the existence of all nested properties in BRIDGE_LIMITS_MAP, which could lead to runtime errors. Consider adding null checks and handling for non-existent routes:
if (BRIDGE_LIMITS_MAP[fromChain]?.[fromToken]?.routes?.[toChain]?.[toToken]) {
minOriginAmount = BRIDGE_LIMITS_MAP[fromChain][fromToken].routes[toChain][toToken].minOriginValue
maxOriginAmount = BRIDGE_LIMITS_MAP[fromChain][fromToken].routes[toChain][toToken].maxOriginValue
} else {
return res.status(400).json({ error: 'Unsupported bridge route' })
}This change adds optional chaining to safely check for the existence of the route and provides an error response if the route is not supported.
| const processAmount = (amount: string): number | null => { | ||
| const value = parseFloat(amount) | ||
|
|
||
| if (isNaN(value)) { | ||
| return null | ||
| } | ||
|
|
||
| if (value < 0.01) { | ||
| return value // If the value is less than 0.01, keep the same value | ||
| } else if (value >= 0.01 && value < 0.1) { | ||
| return 0.1 // If the value is between 0.01 and 0.1, use 0.1 | ||
| } else if (value >= 0.1 && value < 0.5) { | ||
| return 0.5 // If the value is between 0.1 and 0.5, use 0.5 | ||
| } else if (value >= 0.5 && value <= 1900000) { | ||
| return value // If the value is between 0.5 and 1900000, use that value | ||
| } else { | ||
| return null // If the value is above 1900000, set value to null | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve processAmount function for better safety and readability
- Replace
isNaNwithNumber.isNaNfor safer type checking:
- if (isNaN(value)) {
+ if (Number.isNaN(value)) {- Consider simplifying the conditional logic using early returns:
const processAmount = (amount: string): number | null => {
const value = parseFloat(amount);
if (Number.isNaN(value) || value < 0.01 || value > 1900000) {
return null;
}
if (value < 0.1) return 0.1;
if (value < 0.5) return 0.5;
return value;
};This refactored version is more concise and easier to read while maintaining the same logic.
Tools
Biome
[error] 41-41: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
| return res.json({ | ||
| maxOriginAmount: processAmount(maxOriginAmount), | ||
| minOriginAmount: processAmount(minOriginAmount), | ||
| }) | ||
| } catch (err) { | ||
| res.status(500).json({ | ||
| error: | ||
| 'An unexpected error occurred in /bridgeLimits. Please try again later.', | ||
| details: err.message, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error handling to avoid exposing sensitive information
The current error handling in the catch block might expose sensitive server information by including err.message in the response. It's generally a good practice to log the error details server-side but return a generic error message to the client.
Apply this diff to improve the error handling:
} catch (err) {
+ console.error('Error in bridgeLimitsController:', err);
res.status(500).json({
error:
'An unexpected error occurred in /bridgeLimits. Please try again later.',
- details: err.message,
})
}This change logs the full error details server-side for debugging purposes while providing a generic error message to the client, enhancing security.
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.
| return res.json({ | |
| maxOriginAmount: processAmount(maxOriginAmount), | |
| minOriginAmount: processAmount(minOriginAmount), | |
| }) | |
| } catch (err) { | |
| res.status(500).json({ | |
| error: | |
| 'An unexpected error occurred in /bridgeLimits. Please try again later.', | |
| details: err.message, | |
| }) | |
| } | |
| } | |
| return res.json({ | |
| maxOriginAmount: processAmount(maxOriginAmount), | |
| minOriginAmount: processAmount(minOriginAmount), | |
| }) | |
| } catch (err) { | |
| console.error('Error in bridgeLimitsController:', err); | |
| res.status(500).json({ | |
| error: | |
| 'An unexpected error occurred in /bridgeLimits. Please try again later.', | |
| }) | |
| } | |
| } |
bigboydiamonds
changed the title
|
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. |
Description
PR introduces a new
/bridgeLimitsendpoint that provides the minimum and maximum origin input values for specific bridge routes. The endpoint allows users to query these limits by passing the following parameters:Adds new
/bridgeLimitsEndpoint:• The endpoint determines the min/max input values for a given bridge route.
• The input parameters (fromChain, toChain, fromToken, toToken) are used to match and return the appropriate route limits.
• returns { minOriginValue, maxOriginValue }
Adds Bridge Route Mapping:
• Constructs an internal mapping of all available bridge routes using following data structure:
Adds generating bridge limits script
generateLimits.ts:• script generates the min/max origin values for all supported bridge routes.
• script ingests JSON object at scripts/data/bridgeLimitMap.json to programatically fetch bridge quote min/max limits
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
714ad0d: synapse-interface preview link
eaa421f: synapse-interface preview link
81d7c47: synapse-interface preview link
a2095b1: synapse-interface preview link
39bf444: synapse-interface preview link
1a8147a: synapse-interface preview link
7fe5029: synapse-interface preview link
fda5d62: synapse-interface preview link
1be1b45: synapse-interface preview link