Fix API decimals handling #3378
Conversation
|
Warning Rate limit exceeded@Defi-Moses has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request enhance the validation logic for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3378 +/- ##
===================================================
+ Coverage 30.49751% 31.65700% +1.15949%
===================================================
Files 417 397 -20
Lines 27658 26367 -1291
Branches 82 82
===================================================
- Hits 8435 8347 -88
+ Misses 18437 17237 -1200
+ Partials 786 783 -3
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: |
61bc34e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c667d993.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-api-decimals.sanguine-fe.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/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 (2)
packages/rest-api/src/utils/formatAmounts.ts (1)
4-25: Consider adding comprehensive error handling and documentationAs this is a critical utility function for amount processing:
- Add JSDoc documentation with examples and edge cases
- Consider creating custom error types for different validation failures
- Add unit tests covering various scenarios:
- Different decimal places
- Edge cases (max values, zero, very small numbers)
- Invalid inputs
- Overflow scenarios
Would you like me to help create comprehensive unit tests for this utility function?
packages/rest-api/src/controllers/bridgeController.ts (1)
27-30: LGTM! Consider adding specific error handling for amount validationThe replacement of
parseUnitswithformatAndValidateAmountproperly addresses the decimal truncation issue. The implementation correctly handles the conversion by:
- Converting amount to string
- Using the token's decimal places for proper formatting
Consider adding specific error handling for amount validation failures:
- const amountInWei = formatAndValidateAmount( - amount.toString(), - fromTokenInfo.decimals - ) + let amountInWei; + try { + amountInWei = formatAndValidateAmount( + amount.toString(), + fromTokenInfo.decimals + ); + } catch (error) { + logger.warn(`Invalid amount format`, { + amount, + decimals: fromTokenInfo.decimals, + error: error.message + }); + return res.status(400).json({ + error: 'Invalid amount format. Please check the decimal places.' + }); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
packages/rest-api/src/controllers/bridgeController.ts(2 hunks)packages/rest-api/src/controllers/bridgeTxInfoController.ts(2 hunks)packages/rest-api/src/utils/formatAmounts.ts(1 hunks)
🔇 Additional comments (3)
packages/rest-api/src/controllers/bridgeTxInfoController.ts (2)
5-5: LGTM: Clean import addition
The new utility import is properly organized with other utility imports.
27-30: Verify decimal truncation behavior
While the change addresses the decimal truncation issue, we should verify the exact behavior of formatAndValidateAmount for edge cases.
Let's check the implementation of the formatting utility:
Consider adding explicit error handling
The formatting function might throw errors for invalid inputs (e.g., scientific notation, extremely large numbers). Consider wrapping this in a try-catch block for better error reporting.
- const amountInWei = formatAndValidateAmount(
- amount.toString(),
- fromTokenInfo.decimals
- )
+ let amountInWei;
+ try {
+ amountInWei = formatAndValidateAmount(
+ amount.toString(),
+ fromTokenInfo.decimals
+ );
+ } catch (error) {
+ logger.error('Amount formatting failed', {
+ amount,
+ decimals: fromTokenInfo.decimals,
+ error: error.message
+ });
+ return res.status(400).json({
+ error: 'Invalid amount format. Please provide a valid decimal number.'
+ });
+ }✅ Verification successful
Implementation handles decimal truncation safely
The formatAndValidateAmount implementation properly handles decimal truncation and edge cases:
- Sanitizes input by removing non-numeric characters except decimal point
- Handles empty/invalid inputs by returning 0
- Correctly truncates decimals to token precision
- Uses
parseUnitsfor final conversion to wei
No explicit error handling is needed as the implementation already:
- Safely handles invalid inputs
- Performs proper decimal truncation
- Returns valid BigNumber in all cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the formatAndValidateAmount implementation
# Expected: Should find the implementation showing decimal handling logic
ast-grep --pattern 'export function formatAndValidateAmount($_) {
$$$
}'
# Also check for any test cases
fd -g "*test*" -x rg -l "formatAndValidateAmount"
Length of output: 126
Script:
#!/bin/bash
# Let's try different approaches to find the implementation
# Search for the function definition with ripgrep
rg -l "formatAndValidateAmount"
# Search for the function with more context
rg "formatAndValidateAmount" -A 5 -B 5
# Look for files containing "amount" and "validate" in their names
fd -g "*amount*" -g "*validate*"
# Search for any utility files that might contain this function
fd -g "*util*" -x rg -l "formatAndValidateAmount"
Length of output: 4925
Script:
#!/bin/bash
# Let's examine the full implementation of formatAndValidateAmount
cat packages/rest-api/src/utils/formatAmounts.ts
# Also check for any test files specifically for formatAmounts
fd -g "*formatAmounts*test*"
Length of output: 803
packages/rest-api/src/controllers/bridgeController.ts (1)
4-4: Verify the implementation of formatAndValidateAmount utility
The new import aligns with the PR's objective to fix decimal truncation issues. Let's verify the implementation to ensure it handles decimals correctly.
✅ Verification successful
Implementation of formatAndValidateAmount is appropriate for decimal handling
The utility function is well-implemented for handling decimal truncation:
- Properly imports BigNumber from @ethersproject/bignumber
- Cleans input by removing non-numeric characters except decimal points
- Takes decimals parameter to handle token-specific decimal places
- Returns BigNumber type which is safe for precise numeric operations
- Used consistently in both bridge controllers for amount processing
The implementation aligns with the PR's objective to fix decimal truncation issues and is being used correctly in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of formatAndValidateAmount
# Test: Look for the implementation and any tests
echo "Checking implementation:"
rg -A 10 "export.*formatAndValidateAmount" "packages/rest-api/src/utils/"
echo "\nChecking tests:"
rg -A 5 "formatAndValidateAmount" "packages/rest-api/src/**/*.test.ts"
Length of output: 1114
Script:
#!/bin/bash
# Let's verify how formatAndValidateAmount is used in the codebase
echo "Checking usage of formatAndValidateAmount:"
rg -A 5 "formatAndValidateAmount" "packages/rest-api/src/"
# Let's also check for any test files with .spec.ts extension
echo "\nChecking spec tests:"
rg -A 5 "formatAndValidateAmount" "packages/rest-api/src/**/*.spec.ts"
# Let's examine the BigNumber import and usage
echo "\nChecking BigNumber import:"
rg "import.*BigNumber" "packages/rest-api/src/utils/formatAmounts.ts"
Length of output: 3212
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
packages/rest-api/src/validations/validateDecimals.ts (2)
1-4: Add JSDoc documentationAdd documentation to describe the function's purpose, parameters, return value, and examples of usage.
+/** + * Validates if a decimal number string has at most the expected number of decimal places + * @param amount - The decimal number as a string (e.g., "123.456") + * @param expectedDecimals - The maximum number of decimal places allowed + * @returns true if the number has valid decimals, false otherwise + * @example + * validateDecimals("123.456", 2) // returns false + * validateDecimals("123.45", 2) // returns true + * validateDecimals("123", 2) // returns true + */ export const validateDecimals = ( amount: string, expectedDecimals: number ): boolean => {
1-11: Consider adding unit testsThe function handles critical validation logic and should be thoroughly tested with various edge cases.
Would you like me to generate comprehensive unit tests covering:
- Valid inputs with different decimal places
- Invalid inputs (non-numeric, scientific notation)
- Edge cases (empty strings, multiple decimal points)
- Boundary conditions for expectedDecimals
packages/rest-api/src/routes/bridgeRoute.ts (1)
231-248: Consider caching the token info to avoid duplicate calls.The validation logic is correct, but
tokenAddressToTokenis called twice - once in the validation and once in the error message. Consider storing the result to improve performance:check('amount') .exists() .withMessage('amount is required') .isNumeric() .custom((value, { req }) => { - const fromTokenInfo = tokenAddressToToken( - req.query.fromChain, - req.query.fromToken - ) + req.tokenInfo = tokenAddressToToken( + req.query.fromChain, + req.query.fromToken + ) - return validateDecimals(value, fromTokenInfo.decimals) + return validateDecimals(value, req.tokenInfo.decimals) }) .withMessage((_value, { req }) => { - const fromTokenInfo = tokenAddressToToken( - req.query.fromChain, - req.query.fromToken - ) - return `Amount has too many decimals, maximum allowed for this token is ${fromTokenInfo.decimals}` + return `Amount has too many decimals, maximum allowed for this token is ${req.tokenInfo.decimals}` }),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
packages/rest-api/src/routes/bridgeRoute.ts(2 hunks)packages/rest-api/src/routes/bridgeTxInfoRoute.ts(2 hunks)packages/rest-api/src/routes/swapRoute.ts(2 hunks)packages/rest-api/src/routes/swapTxInfoRoute.ts(2 hunks)packages/rest-api/src/validations/validateDecimals.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/rest-api/src/routes/swapTxInfoRoute.ts (4)
Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/controllers/swapTxInfoController.ts:14-14
Timestamp: 2024-10-08T15:41:49.333Z
Learning: In this project, request parameter validation is handled in the route files (e.g., `swapTxInfoRoute.ts`) rather than in the controller files.
Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/controllers/swapTxInfoController.ts:14-14
Timestamp: 2024-10-08T15:41:48.274Z
Learning: In this project, request parameter validation is handled in the route files (e.g., `swapTxInfoRoute.ts`) rather than in the controller files.
Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/controllers/swapTxInfoController.ts:14-14
Timestamp: 2024-09-17T23:09:44.463Z
Learning: In this project, request parameter validation is handled in the route files (e.g., `swapTxInfoRoute.ts`) rather than in the controller files.
Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/controllers/swapTxInfoController.ts:14-14
Timestamp: 2024-10-08T15:41:49.333Z
Learning: In this project, request parameter validation is handled in the route files (e.g., `swapTxInfoRoute.ts`) rather than in the controller files.
🔇 Additional comments (7)
packages/rest-api/src/validations/validateDecimals.ts (1)
1-11: Verify integration with token decimals
The function is used to validate amounts against token decimals. Let's verify that the token decimals are correctly passed to this function.
packages/rest-api/src/routes/swapTxInfoRoute.ts (2)
12-13: LGTM! New imports are well-organized.
The new utility imports are properly organized and align with the project's structure.
146-163: Implementation aligns with PR objectives.
The decimal validation logic successfully addresses the API decimals truncation issue by:
- Validating the amount against token-specific decimal places
- Providing clear error messages with the maximum allowed decimals
Let's verify the token decimal configuration:
✅ Verification successful
Let me verify the token decimal validation implementation by checking the validateDecimals function and tokenAddressToToken implementation.
Decimal validation implementation is correctly configured
The implementation properly validates token decimals by:
- Using a robust
validateDecimalsfunction that checks if the input amount's decimal places are within the token's specified limit - Retrieving token-specific decimal information through
tokenAddressToTokenwhich sources from well-maintained token configuration maps - Providing clear error messages that include the token's correct decimal precision
The token decimal configurations are consistently defined across the codebase with proper handling for:
- Standard ERC20 tokens (18 decimals)
- Stablecoins (6 decimals)
- Special tokens (8 decimals)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify token decimal configuration consistency
# Expected: Find token decimal configurations to ensure they match with the validation
# Search for token decimal configurations
echo "Searching for token decimal configurations..."
rg -l "decimals.*=.*\d+" --type ts
# Search for any hardcoded decimal values
echo "Searching for hardcoded decimal values..."
rg "decimals:.*\d+" --type ts
Length of output: 85704
Script:
#!/bin/bash
# Check validateDecimals implementation
echo "Checking validateDecimals implementation..."
rg -A 10 "validateDecimals.*=.*" --type ts
# Check tokenAddressToToken implementation
echo "Checking tokenAddressToToken implementation..."
rg -A 10 "tokenAddressToToken.*=.*" --type ts
Length of output: 3858
packages/rest-api/src/routes/swapRoute.ts (1)
13-14: LGTM: New imports for decimal validation.
The new utility imports are well-organized and clearly named, supporting the PR's objective of fixing decimal truncation.
packages/rest-api/src/routes/bridgeTxInfoRoute.ts (2)
13-14: LGTM: New imports for decimal validation.
The added imports align with the PR's objective of implementing proper decimal validation.
167-184: Consider optimizing token info lookup and error handling.
The validation logic correctly implements decimal checking, but there are a few points to consider:
- The token info lookup is performed twice - once for validation and once for the error message. Consider storing the result to avoid duplicate calls.
- Error handling for
tokenAddressToTokenshould be verified.
Consider refactoring to this pattern:
check('amount')
.exists()
.withMessage('amount is required')
.isNumeric()
.custom((value, { req }) => {
- const fromTokenInfo = tokenAddressToToken(
+ const fromTokenInfo = req.fromTokenInfo = tokenAddressToToken(
req.query.fromChain,
req.query.fromToken
)
return validateDecimals(value, fromTokenInfo.decimals)
})
.withMessage((_value, { req }) => {
- const fromTokenInfo = tokenAddressToToken(
- req.query.fromChain,
- req.query.fromToken
- )
+ const fromTokenInfo = req.fromTokenInfo
return `Amount has too many decimals, maximum allowed for this token is ${fromTokenInfo.decimals}`
})Let's verify the error handling in tokenAddressToToken:
packages/rest-api/src/routes/bridgeRoute.ts (1)
13-14: LGTM: New imports for decimal validation.
The new utility imports are well-organized and follow the codebase conventions.
Fixing this request:
https://api.synapseprotocol.com/bridge?amount=4.99696492210126891&fromChain=10&toChain=8453&fromToken=0x0b2C639c533813f4Aa9D7837CAf62653d097Ff85&toToken=0xd9aAEc86B65D86f6A7B5B1b0c42FFA531710b6CA&originUserAddress=0xa61dcdf0b74a72f676742ee87e31facf4026d5bf
Issue is that decimals arent truncated, so instead mimicked the same logic in the frontend, we return an error like
{"error":{"value":"4.99696492210126891","message":"Amount has too many decimals, maximum allowed is 6","field":"amount","location":"query"}}
Summary by CodeRabbit
New Features
amountparameter across multiple routes, ensuring it adheres to token-specific decimal requirements.Bug Fixes
amountinputs, dynamically reflecting token constraints.Documentation
validateDecimalsandtokenAddressToTokento support enhanced validation logic.