fix(rest-api): Adds chain validation for swap #3271
Conversation
WalkthroughThe changes involve updates to several files within the REST API package. A new constant 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
|
![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/validations/validSwapChain.ts (1)
3-5: Consider adding a JSDoc comment for better documentation.To improve code readability and provide better documentation, consider adding a JSDoc comment above the function. This will help other developers understand the function's purpose, parameters, and return value at a glance.
Here's a suggested JSDoc comment:
/** * Checks if the given chain ID is supported for swaps. * @param {number | string} chain - The chain ID to validate. * @returns {boolean} True if the chain is supported, false otherwise. */ export const validSwapChain = (chain: number | string) => { // ... existing implementation }packages/rest-api/src/validations/validSwapTokens.ts (2)
Line range hint
3-16: Consider adding chain validationThe function doesn't validate the
chainparameter. Consider adding a check to ensure the provided chain is supported before proceeding with token validation.Here's a suggested improvement:
+ import { SUPPORTED_SWAP_CHAIN_IDS } from '../constants' export const validSwapTokens = ( chain: number | string, fromToken: string, toToken: string ) => { + if (!SUPPORTED_SWAP_CHAIN_IDS.includes(Number(chain))) { + return false + } const fromTokenInfo = tokenAddressToToken(chain.toString(), fromToken) const toTokenInfo = tokenAddressToToken(chain.toString(), toToken) if (!fromTokenInfo || !toTokenInfo) { return false } return fromTokenInfo.swappable.includes(toToken) }
Line range hint
3-16: Enhance error handlingThe current implementation returns a boolean value, which doesn't provide detailed information about why a swap might be invalid. Consider enhancing the error handling to provide more specific error messages.
Here's a suggested improvement:
export const validSwapTokens = ( chain: number | string, fromToken: string, toToken: string -) => { +): { valid: boolean; reason?: string } => { + if (!SUPPORTED_SWAP_CHAIN_IDS.includes(Number(chain))) { + return { valid: false, reason: 'Unsupported chain' } + } const fromTokenInfo = tokenAddressToToken(chain.toString(), fromToken) const toTokenInfo = tokenAddressToToken(chain.toString(), toToken) if (!fromTokenInfo || !toTokenInfo) { - return false + return { valid: false, reason: 'Invalid token information' } } - return fromTokenInfo.swappable.includes(toToken) + return { + valid: fromTokenInfo.swappable.includes(toToken), + reason: fromTokenInfo.swappable.includes(toToken) ? undefined : 'Tokens not swappable' + } }packages/rest-api/src/constants/index.ts (1)
12-29: Approved: New constant added correctly, but consider adding a comment.The new
SUPPORTED_SWAP_CHAIN_IDSconstant is well-structured and uses theCHAINSobject consistently. However, to improve maintainability and clarity, consider adding a brief comment explaining the purpose of this constant and any criteria for inclusion in this list.Consider adding a comment above the constant, like this:
/** * List of chain IDs that support swap functionality. * This constant is used for validating swap requests across supported networks. */ export const SUPPORTED_SWAP_CHAIN_IDS = [ // ... (existing chain IDs) ]Also, it might be helpful to document the process for updating this list if new chains need to be added or removed in the future.
packages/rest-api/src/tests/swapRoute.test.ts (1)
81-94: LGTM: New test case for unsupported chain swapThe new test case effectively validates the behavior for swap attempts on unsupported chains, which aligns with the PR objectives. The test structure and assertions are appropriate.
Suggestion for improvement:
Consider using a constant for the unsupported chain ID (59144) to enhance maintainability. This could be defined at the top of the file or imported from a constants file if it's used elsewhere.Example:
const UNSUPPORTED_CHAIN_ID = '59144'; // In the test case chain: UNSUPPORTED_CHAIN_ID,This change would make it easier to update the unsupported chain ID if needed in the future.
packages/rest-api/src/routes/swapRoute.ts (1)
167-173: LGTM: New validation for supported swap chains.The addition of
validSwapChaincheck improves the API's robustness by ensuring only supported chains are used for swaps. This is a valuable safeguard against potential errors.Consider extracting the error message to a constant for consistency and easier maintenance:
+const UNSUPPORTED_SWAP_CHAIN_ERROR = 'Swap not supported for given chain'; check() .custom((_value, { req }) => { const { chain } = req.query return validSwapChain(chain) }) - .withMessage('Swap not supported for given chain'), + .withMessage(UNSUPPORTED_SWAP_CHAIN_ERROR),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- packages/rest-api/src/constants/index.ts (2 hunks)
- packages/rest-api/src/routes/swapRoute.ts (2 hunks)
- packages/rest-api/src/tests/swapRoute.test.ts (2 hunks)
- packages/rest-api/src/validations/validSwapChain.ts (1 hunks)
- packages/rest-api/src/validations/validSwapTokens.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (9)
packages/rest-api/src/validations/validSwapChain.ts (1)
1-5: LGTM! The implementation looks correct and concise.The
validSwapChainfunction effectively validates if a given chain ID is supported for swaps. It handles both number and string inputs, which provides good flexibility.packages/rest-api/src/validations/validSwapTokens.ts (2)
3-3: Approve function name changeThe renaming of the function from
validSwaptovalidSwapTokensis a good improvement. The new name is more descriptive and accurately reflects the function's purpose of validating swap tokens.
3-3: Verify impact of function name changeThe function name has been changed from
validSwaptovalidSwapTokens. Ensure that all imports and usages of this function have been updated accordingly throughout the codebase.Run the following script to check for any remaining usages of the old function name:
✅ Verification successful
Impact of Function Name Change Verified
All imports and usages of the old function name
validSwaphave been successfully updated tovalidSwapTokensandvalidSwapChain. No remaining usages ofvalidSwapfound.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining usages of the old function name 'validSwap' # Test: Search for 'validSwap' in all TypeScript and JavaScript files rg --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' --type ts --type js 'validSwap'Length of output: 677
packages/rest-api/src/constants/index.ts (2)
1-1: LGTM: Import statement is correct and necessary.The new import statement for
CHAINSis correctly added and is required for the new constantSUPPORTED_SWAP_CHAIN_IDS. This change follows good practices by importing only the necessary module.
Line range hint
1-29: Summary: Changes align with PR objective, minor improvement suggested.The addition of the
SUPPORTED_SWAP_CHAIN_IDSconstant aligns well with the PR objective of adding chain validation for swap functionality. This constant will likely be used in the swap route validation logic mentioned in the PR summary. The implementation is correct and follows good practices.To further improve the code:
- Consider adding a comment explaining the purpose and usage of the
SUPPORTED_SWAP_CHAIN_IDSconstant.- Document the process for maintaining this list of supported chains.
These improvements will enhance the maintainability and clarity of the codebase for future developers working on swap functionality across different chains.
packages/rest-api/src/tests/swapRoute.test.ts (1)
6-6: LGTM: Import statement updated correctlyThe addition of USDT to the import statement is consistent with the new test case and aligns with the PR objectives.
packages/rest-api/src/routes/swapRoute.ts (3)
11-12: LGTM: New imports for improved validation.The addition of
validSwapTokensandvalidSwapChainimports indicates a good separation of concerns for validation logic. This change aligns with best practices for modular code organization.
178-178: LGTM: Updated token validation function.The change from
validSwaptovalidSwapTokensimproves code clarity by using a more specific function name. This modification aligns well with the principle of having clear and focused function names.
Line range hint
1-190: Summary: Improved swap validation enhances API robustness.The changes in this file significantly improve the swap validation process by:
- Introducing separate validations for swap chains and tokens.
- Using more specific function names for better code clarity.
- Adding an additional layer of validation to ensure only supported chains are used for swaps.
These enhancements align well with the PR objectives and contribute to a more robust and maintainable API.
| export const validSwapChain = (chain: number | string) => { | ||
| return SUPPORTED_SWAP_CHAIN_IDS.includes(Number(chain)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for invalid inputs.
While the current implementation works for valid numeric inputs, it doesn't explicitly handle non-numeric string inputs. Consider adding error handling to improve robustness.
Here's a suggested implementation with error handling:
export const validSwapChain = (chain: number | string): boolean => {
const chainId = Number(chain);
if (isNaN(chainId)) {
throw new Error('Invalid chain ID: must be a number or numeric string');
}
return SUPPORTED_SWAP_CHAIN_IDS.includes(chainId);
}This change will throw an error for non-numeric inputs, making it easier to debug issues related to invalid chain IDs.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3271 +/- ##
=============================================
Coverage 31.76937% 31.76937%
=============================================
Files 427 427
Lines 28496 28496
Branches 82 82
=============================================
Hits 9053 9053
Misses 18597 18597
Partials 846 846
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: |
7ce97d8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5a57fae9.sanguine-fe.pages.dev |
| Branch Preview URL: | https://rest-api-validates-swap-chai.sanguine-fe.pages.dev |
Summary by CodeRabbit
New Features
Bug Fixes
Tests