Making address (origin and dest) mandated in rest API #3459
Conversation
WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
packages/rest-api/swagger.json (3)
Line range hint
204-209: Update parameter description to reflect requirementThe parameter description should be updated to indicate that this is a required field for generating call data.
Apply this diff to clarify the requirement:
- "description": "The destination address of the user on the destination chain" + "description": "The destination address of the user on the destination chain. Required when call data needs to be generated."
Line range hint
1-1200: Consider adding request/response examples for error casesThe API documentation would benefit from examples showing error responses when required addresses are missing or invalid.
Add example error responses like:
{ "error": { "value": "", "message": "destAddress is required for generating call data", "field": "destAddress", "location": "query" } }
Line range hint
1-1200: Document migration path from deprecated endpointsWhile
/bridgeTxInfois marked as deprecated in favor of/bridge, the documentation should provide a clear migration guide for users.Add a migration guide section in the description:
- "description": "[Deprecated] Originally used to get Bridge transaction information", + "description": "[Deprecated] Originally used to get Bridge transaction information. To migrate, use the /bridge endpoint with destAddress and originUserAddress parameters to receive call data in the response.",packages/rest-api/src/controllers/bridgeController.ts (1)
56-68: Consider improving the conditional logic readabilityWhile the logic is correct, the nested ternary operator could be made more readable using an if statement, especially since it involves multiple conditions and a complex function call.
Consider this alternative implementation:
- const callData = - destAddress && originUserAddress - ? await Synapse.bridge( - destAddress, - quote.routerAddress, - Number(fromChain), - Number(toChain), - fromToken, - amountInWei, - quote.originQuery, - quote.destQuery - ) - : null + let callData = null; + if (destAddress && originUserAddress) { + callData = await Synapse.bridge( + destAddress, + quote.routerAddress, + Number(fromChain), + Number(toChain), + fromToken, + amountInWei, + quote.originQuery, + quote.destQuery + ); + }packages/rest-api/src/tests/bridgeRoute.test.ts (2)
177-185: Use different addresses for origin and destinationUsing the same address for both
originUserAddressanddestAddressmight mask potential issues. Consider using different valid addresses to ensure the system handles distinct addresses correctly.- destAddress: '0x742d35Cc6634C0532925a3b844Bc454e4438f44e', - originUserAddress: '0x742d35Cc6634C0532925a3b844Bc454e4438f44e', + destAddress: '0x742d35Cc6634C0532925a3b844Bc454e4438f44e', + originUserAddress: '0x8626f6940E2eb28930eFb4CeF49B2d1F2C9C1199',
Line range hint
177-211: Add test cases for edge scenariosThe test suite should include additional edge cases:
- When
destAddressis provided butoriginUserAddressis missing- When addresses are provided but in an incorrect format (e.g., wrong checksum)
Here's a suggested test case:
it('should return bridge quotes without callData when only destAddress is provided', async () => { const response = await request(app).get('/bridge').query({ fromChain: '1', toChain: '10', fromToken: USDC.addresses[1], toToken: USDC.addresses[10], amount: '1000', destAddress: '0x742d35Cc6634C0532925a3b844Bc454e4438f44e' }); expect(response.status).toBe(200); expect(response.body[0].callData).toBeNull(); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/rest-api/src/controllers/bridgeController.ts(1 hunks)packages/rest-api/src/routes/bridgeRoute.ts(1 hunks)packages/rest-api/src/tests/bridgeRoute.test.ts(2 hunks)packages/rest-api/swagger.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/rest-api/src/routes/bridgeRoute.ts
🔇 Additional comments (1)
packages/rest-api/src/controllers/bridgeController.ts (1)
56-68: Verify impact on API consumers
The change requires both destAddress and originUserAddress for callData generation, which might affect existing API consumers that only provide destAddress.
Let's check for potential API consumers:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docs/bridge/docs/02-Bridge/02-REST-API.md (1)
Line range hint
1-100: Update the endpoint documentation sectionsThe commented-out endpoint documentation sections need to be updated to reflect:
- The deprecation of /bridgeTxInfo endpoint
- The new mandatory parameters for /bridge endpoint
Please update the /bridge endpoint documentation to:
- Mark
originUserAddressanddestAddressas required parameters- Add a note about /bridgeTxInfo being consolidated into this endpoint
- Update the example URL to include both addresses
### `/bridge` Get a token bridge quote. #### Parameters * `fromChain` (int): `chainId` of origin chain * `toChain` (int): `chainId` of destination chain * `fromToken` (string): Token address to bridge * `toToken` (string): Token address to bridge to * `amount` (int): Amount to bridge -* **Optional**`originUserAddress` (string): Address of the user bridging the token +* `originUserAddress` (string): Address of the user bridging the token (required) +* `destAddress` (string): Address to receive the bridged tokens (required) + +> Note: As of December 12, 2024, both `originUserAddress` and `destAddress` are required parameters to generate call data. #### Example -[`/bridge?fromChain=1&toChain=10&fromToken=0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48&toToken=0x0b2C639c533813f4Aa9D7837CAf62653d097Ff85&amount=1000000`] +[`/bridge?fromChain=1&toChain=10&fromToken=0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48&toToken=0x0b2C639c533813f4Aa9D7837CAf62653d097Ff85&amount=1000000&originUserAddress=0x95222290DD7278Aa3Ddd389Cc1E1d165CC4BAfe5&destAddress=0x95222290DD7278Aa3Ddd389Cc1E1d165CC4BAfe5`]Also, remove or update the /bridgeTxInfo section to indicate its deprecation:
-### `/bridgeTxInfo` -Used to get transaction data for executing a bridge. +### `/bridgeTxInfo` (Deprecated) + +> Note: This endpoint has been deprecated as of November 19, 2024. Please use the `/bridge` endpoint instead, which now returns call data when both `originUserAddress` and `destAddress` are provided.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/bridge/docs/02-Bridge/02-REST-API.md(1 hunks)
🔇 Additional comments (1)
docs/bridge/docs/02-Bridge/02-REST-API.md (1)
29-30: LGTM on the version history updates!
The changes clearly document:
- The consolidation of /bridgeTxInfo into /bridge endpoint
- The new requirement for both addresses in /bridge endpoint
Deploying sanguine-fe with
|
| Latest commit: |
5fe94c1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e5a7a80a.sanguine-fe.pages.dev |
| Branch Preview URL: | https://api-fix-tests.sanguine-fe.pages.dev |
Bundle ReportChanges will decrease total bundle size by 3.05MB (-8.56%) ⬇️. This is within the configured threshold ✅ Detailed changes
ℹ️ *Bundle size includes cached data from a previous commit |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/rest-api/src/tests/bridgeRoute.test.ts (2)
177-185: Consider using different addresses for origin and destinationWhile the test correctly validates the presence of both addresses, using the same address for both
originUserAddressanddestAddressmight not catch potential edge cases where the handling differs between the two parameters.- destAddress: '0x742d35Cc6634C0532925a3b844Bc454e4438f44e', - originUserAddress: '0x742d35Cc6634C0532925a3b844Bc454e4438f44e', + destAddress: '0x742d35Cc6634C0532925a3b844Bc454e4438f44e', + originUserAddress: '0x1234567890123456789012345678901234567890',
Line range hint
177-242: Consider adding tests for additional edge casesTo improve test coverage, consider adding test cases for:
- Empty string addresses
- Addresses with incorrect checksum
- Zero address (0x0000...0000)
These edge cases would help ensure robust input validation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rest-api/src/tests/bridgeRoute.test.ts(2 hunks)
🔇 Additional comments (3)
packages/rest-api/src/tests/bridgeRoute.test.ts (3)
204-211: Add complementary test case as requested
While this test case verifies behavior when only originUserAddress is provided, we should also add the complementary test case where only destAddress is supplied, as suggested in the previous review.
213-220: LGTM!
The test case properly validates that callData is null when originUserAddress is not provided.
Line range hint 229-242: LGTM!
The test case properly validates error handling for invalid destAddress input.
Change was made to mandate origin and dest addresses in /bridge requests (if call data is to be returned). This is in advance of some of the SIN changes (complex destination chain actions)
Summary by CodeRabbit
New Features
/destinationTokens,/destinationTx, and/synapseTxId.Improvements
/bridgeendpoint to clarify parameter requirements.Bug Fixes
originUserAddressanddestAddressfor bridge operations.Documentation
/bridgeTxInfoand/swapTxInfoendpoints as deprecated./bridgeendpoint.c46ada8: docs preview link
770d6d9: docs preview link