[DNM] Bridge endpoint consolidation [SLT-453] #3371
Conversation
WalkthroughThe pull request introduces enhancements to the Synapse Protocol REST API, specifically in 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
|
Deploying sanguine-fe with
|
| Latest commit: |
2782388
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e8d7b183.sanguine-fe.pages.dev |
| Branch Preview URL: | https://api-bridge-change.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3371 +/- ##
============================================
Coverage ? 90.44834%
============================================
Files ? 54
Lines ? 1026
Branches ? 82
============================================
Hits ? 928
Misses ? 95
Partials ? 3
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.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (15)
packages/rest-api/src/controllers/swapController.ts (1)
46-46: Document response schema changesThe addition of
callDatato the response payload is a breaking change that needs proper documentation and potentially a version bump.Please ensure you:
- Update OpenAPI/Swagger documentation to reflect the new response format
- Consider versioning the API endpoint (e.g.,
/v2/swap) to maintain backward compatibility- Add examples in the API documentation showing both cases (with and without address)
packages/rest-api/src/controllers/bridgeController.ts (2)
49-82: Consider performance optimization for response processingThe current implementation processes quotes sequentially within Promise.all. For large response sets, this could impact performance.
Consider parallel processing of independent operations:
const processQuote = async (quote) => { const [originQueryTokenOutInfo, callData] = await Promise.all([ Promise.resolve(tokenAddressToToken(fromChain.toString(), quote.originQuery.tokenOut)), destAddress ? Synapse.bridge(...) : Promise.resolve(null) ]); return { ...quote, maxAmountOutStr: formatBNToString(quote.maxAmountOut, toTokenInfo.decimals), bridgeFeeFormatted: formatBNToString(quote.feeAmount, originQueryTokenOutInfo.decimals), callData, }; };
49-82: Consider implementing pagination for large response setsThe response processing loads all quotes into memory simultaneously. For large datasets, this could cause memory issues.
Consider implementing pagination:
- Add
limitandoffsetquery parameters- Process only the requested subset of quotes
- Return metadata about total available quotes
packages/rest-api/src/tests/swapRoute.test.ts (4)
137-154: Consider parameterizing the test wallet address.The test provides good coverage of the callData structure, but using a hardcoded wallet address might make the test brittle. Consider moving the address to a test constants file or using a deterministic test wallet.
+// At the top of the file with other imports +import { TEST_WALLET_ADDRESS } from '../constants/test' - address: '0x742d35Cc6634C0532925a3b844Bc454e4438f44e', + address: TEST_WALLET_ADDRESS,
156-169: Add descriptive comment explaining the null callData behavior.The test correctly verifies the default behavior, but it would be helpful to document why callData is null when no address is provided.
+ // When no address is provided, callData should be null as transaction details + // cannot be constructed without a destination address it('should return swap quote without callData when address is not provided', async () => {
171-182: Enhance invalid address test coverage.Consider adding more test cases for different types of invalid addresses:
- Zero address (0x0000...)
- Almost valid address (wrong checksum)
- Address with invalid length
Example addition:
it.each([ ['0x0000000000000000000000000000000000000000', 'Zero address not allowed'], ['0x742d35Cc6634C0532925a3b844Bc454e4438f44', 'Invalid address length'], ['0x742d35cc6634c0532925a3b844bc454e4438f44e', 'Invalid checksum'] ])('should return 400 for invalid address: %s', async (address, scenario) => { const response = await request(app).get('/swap').query({ chain: '1', fromToken: USDC.addresses[1], toToken: DAI.addresses[1], amount: '1000', address, }) expect(response.status).toBe(400) expect(response.body.error).toHaveProperty('message', 'Invalid address') })
136-182: Consider grouping related test cases.The new address-related tests would be better organized using a describe block to group them together.
+ describe('address parameter handling', () => { it('should return swap quote with callData when address is provided', async () => { // ... existing test }) it('should return swap quote without callData when address is not provided', async () => { // ... existing test }) it('should return 400 for invalid address', async () => { // ... existing test }) + })packages/rest-api/src/routes/swapRoute.ts (1)
196-199: Consider normalizing the address parameter.The validation for the optional
addressparameter is well implemented. However, for consistency with other address parameters in the route (fromTokenandtoToken), consider adding theaddressparameter to thechecksumAddressesmiddleware.router.get( '/', normalizeNativeTokenAddress(['fromToken', 'toToken']), - checksumAddresses(['fromToken', 'toToken']), + checksumAddresses(['fromToken', 'toToken', 'address']), [packages/rest-api/src/tests/bridgeRoute.test.ts (3)
177-194: Consider enhancing test coverage with additional assertions.While the test effectively verifies the presence of callData and its structure, consider adding:
- Type checks for callData properties
- Assertions for
maxAmountOutStrandbridgeFeeFormattedto maintain consistency with other test casesexpect(response.body[0].callData).toHaveProperty('to') expect(response.body[0].callData).toHaveProperty('data') expect(response.body[0].callData).toHaveProperty('value') +expect(typeof response.body[0].callData.to).toBe('string') +expect(typeof response.body[0].callData.data).toBe('string') +expect(typeof response.body[0].callData.value).toBe('string') +expect(response.body[0]).toHaveProperty('maxAmountOutStr') +expect(response.body[0]).toHaveProperty('bridgeFeeFormatted')
196-209: Maintain consistency with other test cases.Consider adding assertions for
maxAmountOutStrandbridgeFeeFormattedto maintain consistency with other test cases in the file.expect(response.body.length).toBeGreaterThan(0) expect(response.body[0].callData).toBeNull() +expect(response.body[0]).toHaveProperty('maxAmountOutStr') +expect(response.body[0]).toHaveProperty('bridgeFeeFormatted')
211-223: Ensure error response structure consistency.To maintain consistency with other error cases in the file (e.g.,
invalid originUserAddress), consider adding an assertion for the error object structure.expect(response.status).toBe(400) +expect(response.body).toHaveProperty('error') expect(response.body.error).toHaveProperty('message', 'Invalid destAddress')packages/rest-api/src/routes/bridgeRoute.ts (3)
59-64: Enhance OpenAPI documentation for destAddress parameter.While the documentation is technically correct, consider adding more details:
- Specify that it expects an EVM-compatible address
- Add an example value
- Clarify if this is different from
originUserAddressand when to use each* - in: query * name: destAddress * required: false * schema: * type: string -* description: The destination address for the bridge transaction +* description: The EVM-compatible destination address where bridged tokens will be sent. If not provided, tokens will be sent to the origin user's address. +* example: "0x742d35Cc6634C0532925a3b844Bc454e4438f44e"
110-119: Add descriptions and examples for callData response field.The new
callDataobject in the response schema needs more context:
- Add descriptions for each field
- Include example values
- Explain when this object will be null
* callData: * type: object * nullable: true * properties: * to: * type: string +* description: The target contract address for the bridge transaction +* example: "0x742d35Cc6634C0532925a3b844Bc454e4438f44e" * data: * type: string +* description: The encoded function call data for the bridge transaction +* example: "0x095ea7b3000000000000000000000000..." * value: * type: string +* description: The amount of native tokens to send with the transaction (in wei) +* example: "0x0" +* description: Transaction details returned when destAddress is provided, null otherwise
258-261: Consider additional validation rules for destAddress.While the basic address validation is good, consider adding these safety checks:
- Prevent setting
destAddresssame asoriginUserAddressto avoid confusion- Add chain-specific address validation if different chains use different address formats
check('destAddress') .optional() .custom((value) => isAddress(value)) - .withMessage('Invalid destAddress'), + .withMessage('Invalid destAddress') + .custom((value, { req }) => { + if (value && req.query.originUserAddress && + value.toLowerCase() === req.query.originUserAddress.toLowerCase()) { + throw new Error('destAddress cannot be the same as originUserAddress'); + } + return true; + }),packages/rest-api/swagger.json (1)
200-208: Consider enhancing parameter documentation.While the parameter descriptions are clear, consider adding:
- Example values in the parameter definitions
- More detailed descriptions explaining the expected format (e.g., if it should be a checksummed Ethereum address)
- Common use cases or scenarios
{ "in": "query", "name": "destAddress", "required": true, "schema": { "type": "string" }, - "description": "The destination address of the user on the destination chain" + "description": "The destination address of the user on the destination chain. Must be a valid, checksummed Ethereum address (e.g., 0x1234...abcd) where the bridged tokens will be received.", + "example": "0x742d35Cc6634C0532925a3b844Bc454e4438f44e" }Also applies to: 1218-1226
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
packages/rest-api/src/controllers/bridgeController.ts(2 hunks)packages/rest-api/src/controllers/swapController.ts(2 hunks)packages/rest-api/src/routes/bridgeRoute.ts(3 hunks)packages/rest-api/src/routes/swapRoute.ts(4 hunks)packages/rest-api/src/tests/bridgeRoute.test.ts(1 hunks)packages/rest-api/src/tests/swapRoute.test.ts(1 hunks)packages/rest-api/swagger.json(2 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/rest-api/src/controllers/swapController.ts (2)
Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/routes/swapRoute.ts:21-0
Timestamp: 2024-09-17T23:09:10.071Z
Learning: The `/swap` route does not accept an `address` parameter.
Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/routes/swapRoute.ts:21-0
Timestamp: 2024-10-08T15:41:48.274Z
Learning: The `/swap` route does not accept an `address` parameter.
packages/rest-api/src/routes/swapRoute.ts (4)
Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/routes/swapRoute.ts:21-0
Timestamp: 2024-09-17T23:09:10.071Z
Learning: The `/swap` route does not accept an `address` parameter.
Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/routes/swapRoute.ts:21-0
Timestamp: 2024-10-08T15:41:48.274Z
Learning: The `/swap` route does not accept an `address` parameter.
Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/routes/swapRoute.ts:21-0
Timestamp: 2024-10-08T15:41:49.333Z
Learning: The `/swap` route does not accept an `address` parameter.
Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/routes/swapRoute.ts:21-0
Timestamp: 2024-10-08T15:41:49.333Z
Learning: The `/swap` route does not accept an `address` parameter.
🔇 Additional comments (8)
packages/rest-api/src/controllers/swapController.ts (1)
Line range hint 15-46: Verify PR scope and implementation
The PR title mentions "Bridge endpoint consolidation" but the changes are in the swap controller. This seems inconsistent with the stated objectives.
Let's check for related bridge endpoint changes:
packages/rest-api/src/controllers/bridgeController.ts (2)
22-31:
Add validation for destAddress parameter
While the type definition is correct, there's no visible validation for the destAddress parameter to ensure it's a valid Ethereum address format. This could lead to runtime errors if an invalid address is provided.
Let's verify if validation exists in the route file:
#!/bin/bash
# Search for destAddress validation in route files
rg -l "destAddress.*validate" --type tsIf validation is missing, consider adding validation using express-validator:
check('destAddress')
.optional()
.isEthereumAddress()
.withMessage('Invalid destination address format')59-59:
Add type safety for routerAddress
The quote.routerAddress is used without type validation, which could cause runtime errors if the quote structure changes.
Let's verify the quote type definition:
packages/rest-api/src/routes/swapRoute.ts (1)
3-3: LGTM!
The isAddress import is correctly added and necessary for validating the new optional address parameter.
packages/rest-api/src/tests/bridgeRoute.test.ts (1)
177-223: Verify API documentation and error handling consistency.
Let's ensure the API documentation and error handling are consistent with these test changes.
✅ Verification successful
API documentation and error handling are consistent with test changes
The verification confirms that:
- The OpenAPI documentation correctly specifies
destAddressas an optional parameter in the/bridgeendpoint - The response schema includes the
callDataobject that matches the test expectations - Error handling for invalid
destAddressis properly implemented in the route validation - The controller correctly handles the optional
destAddressparameter and generatescallDataaccordingly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify API documentation and error handling consistency
# Test 1: Check if destAddress is documented in swagger
echo "Checking swagger documentation for destAddress..."
rg -A 5 "destAddress" "**/swagger.json"
# Test 2: Check error handling consistency across routes
echo "Checking error handling consistency..."
rg -A 2 "response.status).toBe\(400\)" "**/*.test.ts"
Length of output: 442
Script:
#!/bin/bash
# Let's try a different approach to verify API documentation and error handling
# Check for OpenAPI/Swagger documentation files with different extensions
echo "Checking for API documentation files..."
fd -e yaml -e yml -e json . | grep -i 'swagger\|openapi'
# Check the route handler implementation
echo "Checking bridge route implementation..."
rg -l "destAddress" "packages/rest-api/src"
# Check error handling patterns
echo "Checking error handling patterns..."
rg "status\(400\)" "packages/rest-api/src"
# Look for validation logic
echo "Checking validation logic..."
rg "destAddress.*valid" "packages/rest-api/src"
Length of output: 2725
Script:
#!/bin/bash
# Let's check the API documentation and validation implementation
# Check the bridge route API documentation
echo "Checking bridge route API documentation..."
cat "packages/rest-api/swagger.json"
# Check the validation implementation in bridge route
echo "Checking bridge route validation..."
cat "packages/rest-api/src/routes/bridgeRoute.ts"
# Check the controller implementation
echo "Checking bridge controller implementation..."
cat "packages/rest-api/src/controllers/bridgeController.ts"
Length of output: 109099
packages/rest-api/src/routes/bridgeRoute.ts (1)
Line range hint 1-261: Verify PR readiness given [DNM] tag.
This PR is marked as "Do Not Merge". Please clarify:
- Is this ready for review or still work in progress?
- Are there additional changes planned for the bridge endpoint consolidation?
- What are the remaining tasks for SLT-453?
✅ Verification successful
PR is ready for review - [DNM] tag can be removed
The code appears to be production-ready based on the following evidence:
- Comprehensive test coverage for the bridge endpoints including positive and negative test cases
- No TODOs, FIXMEs or pending work found in the codebase
- All core functionality is implemented and tested:
- Input validation
- Error handling
- Bridge quotes retrieval
- Transaction status checks
- Support for optional parameters
- Tests are passing with realistic test data and proper assertions
The [DNM] tag appears to be a precautionary measure that can now be removed as the implementation is complete and well-tested.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for related changes in other files
echo "Checking for related bridge endpoint changes..."
rg -l "bridge.*endpoint" --type ts
echo "Checking for other files importing bridgeRoute..."
rg -l "from ['\"].*bridgeRoute" --type ts
Length of output: 555
Script:
#!/bin/bash
# Check PR title and description for DNM context
gh pr view --json title,body
# Check commit messages for additional context
git log --pretty=format:"%s" -n 5
# Check for recent changes in bridge-related files
git diff HEAD~1 packages/rest-api/src/routes/bridgeRoute.ts || true
Length of output: 109099
Script:
#!/bin/bash
# Check PR title and description for DNM context
gh pr view --json title,body
# Check for any pending changes or TODOs in the codebase
rg -i "todo|fixme|xxx|hack" --type ts packages/rest-api/
# Check for any related test files and their status
find packages/rest-api/src/tests -name "*bridge*.test.ts" -type f -exec cat {} \;
Length of output: 19953
packages/rest-api/swagger.json (2)
200-208: LGTM! Well-documented parameter additions.
The new required parameters destAddress and address are properly defined with clear descriptions and appropriate schema types.
Also applies to: 1218-1226
200-208: Excellent parameter consistency across endpoints!
The new parameters maintain consistency with their corresponding transaction info endpoints (/bridgeTxInfo and /swapTxInfo), ensuring a coherent API design.
Also applies to: 1218-1226
| @@ -12,7 +12,7 @@ export const swapController = async (req, res) => { | |||
| return res.status(400).json({ errors: errors.array() }) | |||
| } | |||
| try { | |||
| const { chain, amount, fromToken, toToken } = req.query | |||
| const { chain, amount, fromToken, toToken, address } = req.query | |||
There was a problem hiding this comment.
Add address parameter validation
The new address parameter needs proper validation to prevent potential security issues. Consider adding validation using express-validator to ensure it's a valid Ethereum address.
Add this validation to your route configuration:
check('address')
.optional()
.isString()
.custom((value) => isAddress(value))
.withMessage('Invalid Ethereum address format'),| const callData = address | ||
| ? await Synapse.swap( | ||
| Number(chain), | ||
| address, | ||
| fromToken, | ||
| amountInWei, | ||
| quote.query | ||
| ) | ||
| : null |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for swap operation
The conditional swap operation needs specific error handling to distinguish between different failure modes (e.g., invalid address, insufficient funds, network issues).
Consider wrapping the swap call in a try-catch block with specific error types:
let callData = null;
if (address) {
try {
callData = await Synapse.swap(
Number(chain),
address,
fromToken,
amountInWei,
quote.query
);
} catch (error) {
if (error instanceof InvalidAddressError) {
throw new Error('Invalid address provided');
}
// Handle other specific error cases
throw error;
}
}| const callData = destAddress | ||
| ? await Synapse.bridge( | ||
| destAddress, | ||
| quote.routerAddress, | ||
| Number(fromChain), | ||
| Number(toChain), | ||
| fromToken, | ||
| amountInWei, | ||
| quote.originQuery, | ||
| quote.destQuery | ||
| ) | ||
| : null | ||
|
|
There was a problem hiding this comment.
Add error handling for Synapse.bridge call
The Synapse.bridge call could fail independently, but there's no specific error handling for this operation. This could lead to unclear error messages or unhandled promise rejections.
Consider wrapping the bridge call in a try-catch:
- const callData = destAddress
- ? await Synapse.bridge(
+ const callData = destAddress ? await (async () => {
+ try {
+ return await Synapse.bridge(
destAddress,
quote.routerAddress,
Number(fromChain),
Number(toChain),
fromToken,
amountInWei,
quote.originQuery,
quote.destQuery
- )
+ )
+ } catch (error) {
+ logger.error('Failed to generate bridge call data', {
+ destAddress,
+ error: error.message,
+ });
+ return null;
+ }
+ })()
: null📝 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 callData = destAddress | |
| ? await Synapse.bridge( | |
| destAddress, | |
| quote.routerAddress, | |
| Number(fromChain), | |
| Number(toChain), | |
| fromToken, | |
| amountInWei, | |
| quote.originQuery, | |
| quote.destQuery | |
| ) | |
| : null | |
| const callData = destAddress ? await (async () => { | |
| try { | |
| return await Synapse.bridge( | |
| destAddress, | |
| quote.routerAddress, | |
| Number(fromChain), | |
| Number(toChain), | |
| fromToken, | |
| amountInWei, | |
| quote.originQuery, | |
| quote.destQuery | |
| ) | |
| } catch (error) { | |
| logger.error('Failed to generate bridge call data', { | |
| destAddress, | |
| error: error.message, | |
| }); | |
| return null; | |
| } | |
| })() | |
| : null |
| }, | ||
| { | ||
| "in": "query", | ||
| "name": "destAddress", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string" | ||
| }, | ||
| "description": "The destination address of the user on the destination chain" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add address validation patterns for security.
Consider adding regex patterns to validate Ethereum addresses, preventing invalid inputs and potential security issues.
{
"in": "query",
"name": "destAddress",
"required": true,
"schema": {
- "type": "string"
+ "type": "string",
+ "pattern": "^0x[a-fA-F0-9]{40}$",
+ "minLength": 42,
+ "maxLength": 42
},
"description": "The destination address of the user on the destination chain"
}Also applies to: 1218-1226
Summary by CodeRabbit
Release Notes
New Features
destAddressparameter for the/bridgeendpoint, enhancing token bridging capabilities.addressparameter for the/swapendpoint, allowing users to specify the address performing the swap.Documentation
Tests
/bridgeand/swapendpoints to ensure robust handling of new parameters and error scenarios.