adding originUserAddress for refunds [SLT-281] #3206
Conversation
WalkthroughThe pull request introduces an optional query parameter, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 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
|
![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/src/controllers/bridgeTxInfoController.ts (3)
14-22: LGTM! Consider adding type annotations for improved type safety.The addition of
originUserAddressto the destructured query parameters aligns well with the PR objective of incorporating the origin user address for refunds. This change enhances the functionality for processing refunds when necessary.To improve type safety and code readability, consider adding type annotations to the destructured parameters. For example:
const { fromChain, toChain, amount, destAddress, fromToken, toToken, originUserAddress, }: { fromChain: string, toChain: string, amount: string, destAddress: string, fromToken: string, toToken: string, originUserAddress?: string } = req.query;This will help catch potential type-related issues early and improve code maintainability.
33-36: LGTM! Consider adding input validation fororiginUserAddress.The implementation of
originUserAddressin theSynapse.allBridgeQuotescall is correct and handles both the presence and absence of the parameter well. The use oftoString()ensures type consistency.To enhance security and data integrity, consider adding input validation for
originUserAddress. Here's a suggested implementation:import { isAddress } from '@ethersproject/address'; // ... (earlier in the function) const originUserAddressObj = originUserAddress && isAddress(originUserAddress.toString()) ? { originUserAddress: originUserAddress.toString() } : {}; const quotes = await Synapse.allBridgeQuotes( Number(fromChain), Number(toChain), fromToken, toToken, amountInWei, originUserAddressObj );This ensures that
originUserAddressis only included if it's a valid Ethereum address, preventing potential issues with invalid input.
Line range hint
1-62: Overall, the changes successfully implement the PR objective.The addition of
originUserAddressto thebridgeTxInfoControllerfunction effectively implements the PR objective of incorporating the origin user address for refunds. The changes are well-integrated into the existing code structure and handle both the presence and absence of the new parameter appropriately.To further improve the implementation, consider the following architectural advice:
Implement proper error handling for cases where
originUserAddressis provided but invalid. This could involve adding a specific error message in the catch block or creating a custom error type.If
originUserAddressbecomes a common parameter across multiple endpoints, consider creating a middleware function to validate and process it before it reaches the controller. This would promote code reuse and maintain a consistent approach to handling this parameter across the application.Document the new
originUserAddressparameter in the API documentation or OpenAPI/Swagger specifications if they exist. This will help maintain up-to-date documentation for API consumers.These suggestions will enhance the robustness, maintainability, and user-friendliness of the implementation.
packages/rest-api/src/controllers/bridgeController.ts (3)
14-21: LGTM! Consider adding type annotations for improved type safety.The addition of
originUserAddressto the destructured parameters aligns well with the PR objective. This change enables the incorporation of the origin user address for refund processing.Consider adding type annotations to the destructured parameters for improved type safety. For example:
const { fromChain, toChain, amount, fromToken, toToken, originUserAddress, }: { fromChain: string, toChain: string, amount: string, fromToken: string, toToken: string, originUserAddress?: string } = req.query;This will help catch potential type-related issues early in development.
33-36: LGTM! Consider using object spread for improved readability.The conditional passing of
originUserAddresstoSynapse.allBridgeQuotesis implemented correctly. It ensures that the origin user address is included only when present, which aligns with the PR objective.For improved readability, consider using object spread syntax:
Synapse.allBridgeQuotes( Number(fromChain), Number(toChain), fromToken, toToken, amountInWei, originUserAddress ? { originUserAddress: originUserAddress.toString() } : {} )This change would make the code more concise while maintaining the same functionality.
Line range hint
14-36: Consider adding validation for originUserAddress.While the implementation of
originUserAddressis correct, it might be beneficial to add some validation to ensure the address is in the expected format before using it.Consider adding a validation check for
originUserAddressbefore using it:import { isAddress } from '@ethersproject/address'; // ... existing code ... if (originUserAddress && !isAddress(originUserAddress)) { return res.status(400).json({ error: 'Invalid origin user address' }); } // ... rest of the function ...This additional check will help prevent potential issues with invalid addresses being passed to the bridging function.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- packages/rest-api/src/controllers/bridgeController.ts (2 hunks)
- packages/rest-api/src/controllers/bridgeTxInfoController.ts (2 hunks)
- packages/rest-api/src/routes/bridgeRoute.ts (1 hunks)
- packages/rest-api/src/routes/bridgeTxInfoRoute.ts (1 hunks)
🔇 Additional comments (2)
packages/rest-api/src/routes/bridgeRoute.ts (2)
Line range hint
1-57: Overall changes look good, but a few points need attention.The addition of the
originUserAddressparameter aligns with the PR objectives of enhancing aggregator integrations for refund processing. However, please address the following points:
- Add validation for the new
originUserAddressparameter as suggested in the previous comment.- Ensure that the
bridgeControlleris updated to handle this new parameter. If not, update it to process theoriginUserAddressfor refund scenarios.- Verify that the addition of this optional parameter doesn't break existing functionality for requests that don't include it.
To check if the
bridgeControllerhas been updated, run:#!/bin/bash # Search for recent changes in the bridgeController related to originUserAddress git log -p -S "originUserAddress" -- src/controllers/bridgeController.tsThis will show us if and how the
originUserAddressis being used in the controller.Consider documenting the purpose and usage of the
originUserAddressparameter in the project's API documentation or README file to ensure other developers understand its importance for refund processing.
52-57: LGTM! Consider adding validation for the new parameter.The addition of the
originUserAddressparameter to the OpenAPI documentation is correct and aligns with the PR objectives. However, consider adding validation for this new parameter to ensure it's a valid Ethereum address when provided.Add the following validation to the existing validation checks:
check('originUserAddress') .optional() .isEthereumAddress() .withMessage('Invalid Ethereum address for originUserAddress'),To ensure the new parameter is being used in the controller, let's check the
bridgeController:
Deploying sanguine-fe with
|
| Latest commit: |
877b09b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://55f4d9a9.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-api-custom-refund-addre.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3206 +/- ##
===================================================
- Coverage 90.67524% 90.56974% -0.10551%
===================================================
Files 60 54 -6
Lines 1244 1018 -226
Branches 148 82 -66
===================================================
- Hits 1128 922 -206
+ Misses 112 93 -19
+ Partials 4 3 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Defi-Moses
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/rest-api/src/tests/bridgeTxInfoRoute.test.ts (2)
32-51: LGTM! Consider adding an assertion fororiginUserAddress.The new test case for valid input with a valid
originUserAddressis well-structured and consistent with the existing tests. It correctly tests the API response and structure.Consider adding an assertion to verify that the
originUserAddressis correctly handled in the response, if applicable. For example:expect(response.body[0]).toHaveProperty('originUserAddress', '0x742d35Cc6634C0532925a3b844Bc454e4438f44e')This would ensure that the
originUserAddressis correctly processed and returned by the API.
53-69: LGTM! Consider using a constant for the error message.The new test case for an invalid
originUserAddressis well-structured and correctly tests the error handling of the API.For consistency and maintainability, consider defining the error message as a constant at the top of the file or in a separate constants file. This approach is often used for error messages that might be reused or changed in the future. For example:
const INVALID_ORIGIN_USER_ADDRESS_ERROR = 'Invalid originUserAddress address' // In the test case expect(response.body.error).toHaveProperty('message', INVALID_ORIGIN_USER_ADDRESS_ERROR)This makes it easier to update the error message across all occurrences if needed.
packages/rest-api/src/tests/bridgeRoute.test.ts (2)
28-43: LGTM! Consider adding an assertion for originUserAddress.The new test case for a valid originUserAddress is well-structured and consistent with the existing tests. It correctly checks for the expected response status and body structure.
Consider adding an assertion to verify that the originUserAddress is correctly handled in the response. For example:
expect(response.body[0]).toHaveProperty('originUserAddress', '0x742d35Cc6634C0532925a3b844Bc454e4438f44e')This would ensure that the API is correctly processing and returning the originUserAddress.
76-91: LGTM! Consider testing with different invalid address formats.The new test case for an invalid originUserAddress is well-structured and correctly checks for the expected error response.
To improve the robustness of the test, consider adding more test cases with different types of invalid addresses. For example:
- An address with an invalid checksum
- An address that's too short
- An address that's too long
This would ensure that the API correctly handles various types of invalid addresses. You could implement this as separate test cases or use a parameterized test if your testing framework supports it.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- packages/rest-api/src/routes/bridgeRoute.ts (3 hunks)
- packages/rest-api/src/routes/bridgeTxInfoRoute.ts (2 hunks)
- packages/rest-api/src/tests/bridgeRoute.test.ts (2 hunks)
- packages/rest-api/src/tests/bridgeTxInfoRoute.test.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rest-api/src/routes/bridgeRoute.ts
- packages/rest-api/src/routes/bridgeTxInfoRoute.ts
🔇 Additional comments (2)
packages/rest-api/src/tests/bridgeTxInfoRoute.test.ts (1)
32-69: Great addition of test cases fororiginUserAddress!The two new test cases effectively cover the handling of the
originUserAddressparameter in the Bridge TX Info Route. They test both the successful case with a valid address and the error case with an invalid address. These additions improve the overall test coverage and help ensure the correct implementation of the new functionality.To further enhance these tests, consider:
- Adding an assertion for
originUserAddressin the successful case, if applicable.- Using a constant for the error message in the invalid case for better maintainability.
These changes will make the tests more robust and easier to maintain in the future.
packages/rest-api/src/tests/bridgeRoute.test.ts (1)
Line range hint
28-91: Overall, the new test cases are well-implemented and valuable additions.The new test cases for originUserAddress effectively cover both valid and invalid scenarios, enhancing the test coverage for the Bridge Route API. They are consistent with the existing test structure and provide good validation for the new functionality.
To further improve the test suite, consider:
- Implementing the suggested assertions and additional test cases for invalid addresses.
- Ensuring that these tests are part of your CI/CD pipeline to catch any regressions related to the originUserAddress functionality.
- Updating the API documentation to reflect the new originUserAddress parameter, if not already done.
Great job on improving the test coverage!
Bundle ReportChanges will decrease total bundle size by 1.76kB (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
ℹ️ *Bundle size includes cached data from a previous commit |
For aggregator integrations using the UI, the aggregator should supply the origin user address so that SynapseRFQ can refund in the case of a refund.
Simply adds the logic from the SDK
Summary by CodeRabbit
New Features
originUserAddressparameter to the bridging API routes, enhancing request capabilities.originUserAddressparameter for improved transaction handling.originUserAddressto ensure it is a valid address format.Tests
originUserAddressinputs in the bridge routes.