fix(rest-api): Adds validSwap validation [SLT-321] #3247
Conversation
WalkthroughThe changes in this pull request enhance the swap route functionality within an Express application by introducing a new validation step for token swaps. A new function, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: |
a674a21
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1232df0c.sanguine-fe.pages.dev |
| Branch Preview URL: | https://rest-api-valid-swap-path.sanguine-fe.pages.dev |
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
packages/rest-api/src/utils/tokenAddressToToken.ts (1)
19-19: Approve the addition of theswappableproperty with a suggestion for type safety.The addition of the
swappableproperty enhances the token information and aligns well with the PR objectives. This change will support the new validation mechanism for swap requests.To improve type safety, consider defining an interface for the token info structure:
interface TokenInfo { symbol: string; decimals: number; swappable: boolean; } // Then use it in the function: const tokenInfo: TokenInfo = chainData[tokenAddress];This will ensure type consistency and make the code more robust against future changes.
packages/rest-api/src/tests/swapRoute.test.ts (1)
81-94: LGTM: New test case for invalid token comboThe new test case effectively validates the handling of invalid swap requests, which aligns with the PR objectives. It provides good coverage for the edge case where ETH to USDC swap is not supported.
Consider adding a comment explaining why this specific combination (ETH to USDC) is invalid. This would enhance the test's readability and provide context for future developers. For example:
// ETH to USDC direct swap is not supported in this implementation it('should return 400 for invalid fromToken + toToken combo', async () => { // ... (rest of the test case) })packages/rest-api/src/routes/swapRoute.ts (1)
Line range hint
1-172: Summary: New validation enhances API robustness for swap requests.The addition of the
validSwapcheck significantly improves the API's ability to handle invalid swap requests. This change directly addresses the PR objective of returning a 400 error with a relevant message instead of a 500 error for unsupported swaps. The implementation is clean, modular, and follows good practices.To fully validate this change, ensure that:
- The
validSwapfunction in '../validations/validSwap' is correctly implemented.- Appropriate unit tests are added or updated to cover this new validation scenario.
- The error handling middleware correctly processes this new validation error.
Consider adding logging for failed validations to help with monitoring and debugging in production.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- packages/rest-api/src/routes/swapRoute.ts (2 hunks)
- packages/rest-api/src/tests/swapRoute.test.ts (2 hunks)
- packages/rest-api/src/utils/tokenAddressToToken.ts (1 hunks)
- packages/rest-api/src/validations/validSwap.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
packages/rest-api/src/validations/validSwap.ts (2)
1-1: LGTM: Import statement is correct and follows best practices.The import of
tokenAddressToTokenfrom a relative path suggests good organization of the project structure.
7-9: LGTM: Validation logic is clear and prevents potential errors.The check for both
fromTokenInfoandtoTokenInfobeing truthy is a good safeguard against potential errors if token information couldn't be retrieved. This simple validation enhances the function's robustness.packages/rest-api/src/tests/swapRoute.test.ts (2)
6-6: LGTM: ETH import addedThe addition of ETH to the import statement is consistent with the PR objectives and necessary for the new test case.
Line range hint
1-124: Overall assessment: Changes improve test coverageThe modifications to this test file are well-aligned with the PR objectives. The addition of the ETH import and the new test case for invalid token combinations enhance the test coverage for the swap route functionality. The changes are minimal, focused, and maintain consistency with the existing code style and structure.
packages/rest-api/src/routes/swapRoute.ts (3)
11-11: LGTM: Import statement forvalidSwapis correctly added.The import statement is properly placed and follows good practices for modularity by importing the
validSwapfunction from a separate file.
166-172: LGTM: New validation check for swap support is well-implemented.The new validation check is correctly placed within the existing array of checks and uses the appropriate methods from express-validator. The error message is clear and concise, and the use of destructuring to extract query parameters is a good practice.
166-172: Verify the implementation of thevalidSwapfunction.The new validation relies on the
validSwapfunction, which is imported from another file. To ensure the correctness of this validation, we should verify the implementation of thevalidSwapfunction.Please run the following script to check the implementation of the
validSwapfunction:
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3247 +/- ##
===================================================
- Coverage 91.00237% 90.43902% -0.56335%
===================================================
Files 60 54 -6
Lines 1267 1025 -242
Branches 154 82 -72
===================================================
- Hits 1153 927 -226
+ Misses 111 95 -16
Partials 3 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
abtestingalpha
changed the title
Description
When a user requests an invalid swap through the api, the response is a 500 instead of a 400 with error message. This adds a validation to the
swapRouteto check tha the from and to token are swappable.Summary by CodeRabbit
New Features
Bug Fixes
Improvements