fix: (ibc)send big numbers #81
Conversation
WalkthroughThe changes in this pull request primarily focus on the Changes
Assessment against linked issues
Possibly related PRs
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
components/bank/forms/sendForm.tsx (1)
Line range hint
99-114: Consider enhancing error handling for better user feedbackWhile the fix addresses the core issue, the error handling could be more user-friendly. Currently, errors are only logged to console (line 113).
Consider adding user-friendly error messages for common failure cases:
} catch (error) { console.error('Error during sending:', error); + // Extract error message + const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred'; + // Set error state for UI display + setError(errorMessage); } finally { setIsSending(false); }utils/maths.ts (2)
41-42: Consider potential precision loss with.toNumber()Using
.toNumber()can introduce precision loss for very large numbers, which might be critical depending on the application's requirements. To maintain precision, consider returning aBigNumberor converting to a string representation instead.You might modify the function as follows:
-export const toNumber = (val: string, decimals: number = 6) => { - return new BigNumber(val).decimalPlaces(decimals).toNumber(); +export const toNumberString = (val: string, decimals: number = 6) => { + return new BigNumber(val).decimalPlaces(decimals).toString(); };
34-38: Add unit tests forparseNumberToBigIntGiven the importance of accurately parsing numbers to
BigInt, it's crucial to include unit tests that cover a range of inputs, including large numbers, decimals, and invalid strings, to ensure the function behaves as expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
components/bank/forms/sendForm.tsx(2 hunks)utils/maths.ts(1 hunks)
🔇 Additional comments (2)
components/bank/forms/sendForm.tsx (2)
6-6: LGTM: Import changes align with large number handling requirements
The addition of parseNumberToBigInt import is appropriate for handling large token amounts.
99-99: Approve core fix with verification needed
The switch to parseNumberToBigInt is a solid fix for handling large token amounts, eliminating precision loss and floating-point errors that were likely causing the silent failures.
Let's verify the fix handles extreme cases properly:
✅ Verification successful
Implementation verified with proper safeguards in place
The parseNumberToBigInt implementation in utils/maths.ts is well-designed and safely handles token amounts:
- Uses
BigNumberfor initial parsing to prevent floating-point errors - Properly scales the amount by the token's decimal places
- Safely converts to BigInt for blockchain compatibility
The code shows consistent usage across the codebase:
- Used in send forms for token transfers
- Applied in mint/burn operations
- Handles token decimals via
exponentparameter
The implementation includes proper validation through:
- Form validation before the conversion (from the Formik context)
- Adjustments for maximum amounts in send forms
- Safe conversion through BigNumber before BigInt
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any maximum amount limitations or validations in the codebase
# that might affect large token transfers
# Look for amount validations or limits
rg -i "amount.*max|max.*amount" --type ts --type tsx
# Look for parseNumberToBigInt implementation and tests
ast-grep --pattern 'function parseNumberToBigInt($_) {
$$$
}'
# Look for related test cases
rg -i "test.*large.*amount|test.*big.*number" --type test
Length of output: 237
Script:
#!/bin/bash
# Let's try with correct file extensions and more targeted searches
# Look for amount validations or limits
rg -i "amount.*max|max.*amount" -t ts -t tsx -t js -t jsx
# Look for parseNumberToBigInt implementation
rg "parseNumberToBigInt" -A 5 -t ts -t tsx -t js -t jsx
# Look for test files containing amount or number tests
rg -i "test.*amount|test.*number" -t ts -t tsx -t js -t jsx --glob "*test*" --glob "*spec*"
# Look for BigInt related validations
rg -i "bigint.*valid|valid.*amount" -t ts -t tsx -t js -t jsx
Length of output: 385
Script:
#!/bin/bash
# Let's try without explicit file type flags and use simpler patterns
# Look for parseNumberToBigInt implementation
rg "parseNumberToBigInt" -A 5
# Look for amount validations
rg "validateAmount|maxAmount|MAX_AMOUNT"
# Look for test cases in files with test/spec naming
fd -g "*test*" -g "*spec*" -x rg "amount" "{}"
# Look for BigInt usage context
rg "BigInt" -A 2 -B 2
Length of output: 16564
utils/maths.ts
Outdated
| export const parseNumberToBigInt = (v: string, maxDigits: number = 6) => { | ||
| const amount = BigNumber(v); | ||
| const precision = BigNumber(`1e${maxDigits}`); | ||
| const b = amount.times(precision).toFixed(); | ||
| return BigInt(b); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine precision calculation in parseNumberToBigInt
Using BigNumber(\1e${maxDigits}`)may lead to inaccuracies with large values ofmaxDigits. It's recommended to use new BigNumber(10).pow(maxDigits)` for precise and reliable calculations.
Apply the following diff to improve precision calculation:
export const parseNumberToBigInt = (v: string, maxDigits: number = 6) => {
- const amount = BigNumber(v);
- const precision = BigNumber(`1e${maxDigits}`);
+ const amount = new BigNumber(v);
+ const precision = new BigNumber(10).pow(maxDigits);
const b = amount.times(precision).toFixed();
return BigInt(b);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const parseNumberToBigInt = (v: string, maxDigits: number = 6) => { | |
| const amount = BigNumber(v); | |
| const precision = BigNumber(`1e${maxDigits}`); | |
| const b = amount.times(precision).toFixed(); | |
| return BigInt(b); | |
| export const parseNumberToBigInt = (v: string, maxDigits: number = 6) => { | |
| const amount = new BigNumber(v); | |
| const precision = new BigNumber(10).pow(maxDigits); | |
| const b = amount.times(precision).toFixed(); | |
| return BigInt(b); |
Add input validation to handle invalid inputs in parseNumberToBigInt
The function currently lacks validation for the input v. If v is not a valid number, new BigNumber(v) may result in NaN, leading to unexpected behavior when converting to BigInt. Adding input validation ensures robustness and prevents potential errors.
Consider enhancing the function with input validation:
export const parseNumberToBigInt = (v: string, maxDigits: number = 6) => {
+ const amount = new BigNumber(v);
+ if (!amount.isFinite()) {
+ console.error(`Invalid input passed to parseNumberToBigInt: ${v}`);
+ return BigInt(0);
+ }
- const amount = BigNumber(v);
const precision = new BigNumber(10).pow(maxDigits);
const b = amount.times(precision).toFixed();
return BigInt(b);
};Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/bank/forms/ibcSendForm.tsx(2 hunks)
🔇 Additional comments (2)
components/bank/forms/ibcSendForm.tsx (2)
5-5: LGTM: Import of new BigInt utility
The addition of parseNumberToBigInt import aligns with the PR objective of fixing large token amount handling.
Line range hint 82-98: Verify error message visibility for large amounts
While the validation schema includes balance checks, we should ensure that users receive clear error messages when attempting to send large amounts, as the original issue #80 mentioned a lack of UI feedback.
Let's check the error handling implementation:
✅ Verification successful
Based on the results, I can now generate the final response:
Error messages are properly implemented for large amounts
The validation schema's error messages are correctly displayed in the UI:
- The form shows validation errors in red text (
text-red-500) whenerrors.amountexists - Error messages are displayed conditionally based on form state (
touched.amount && errors.amount) - There's also a fee warning system that shows yellow warnings when applicable
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error message handling in the form components
# Check for error message rendering logic
rg -A 2 "errors\.amount"
# Look for error display components
rg -A 2 "text-red-500"
# Check for any console.error usage that might hide errors from UI
rg "console\.error"
Length of output: 12125
| const amountInBaseUnits = Math.floor( | ||
| parseFloat(values.amount) * Math.pow(10, exponent) | ||
| ).toString(); | ||
| const amountInBaseUnits = parseNumberToBigInt(values.amount, exponent).toString(); |
There was a problem hiding this comment.
💡 Codebase verification
Potential precision issues found in factory and groups components
Several components still use floating-point calculations for token amounts which could lead to precision issues:
components/factory/forms/BurnForm.tsx: UsesparseFloat(amount) * Math.pow(10, exponent)for amount calculationscomponents/factory/forms/MintForm.tsx: Similar floating-point math for amount conversioncomponents/factory/modals/multiMfxMintModal.tsxandmultiMfxBurnModal.tsx: Both use floating-point calculationscomponents/groups/forms/proposals/ProposalMessages.tsx: UsesNumber()and floating-point division
These components should be updated to use parseNumberToBigInt from utils/maths.ts which properly handles large numbers using BigNumber.js library, similar to the fix implemented in ibcSendForm.tsx.
🔗 Analysis chain
LGTM: Improved precision for large token amounts
The switch to parseNumberToBigInt eliminates floating-point precision issues that were preventing large token transfers. This directly addresses the issue described in #80.
Let's verify the behavior with large numbers:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if parseNumberToBigInt is used consistently across the codebase
# and verify there are no remaining floating-point calculations for token amounts
# Search for any remaining floating-point calculations of token amounts
rg -l "Math\.floor.*amount|parseFloat.*amount|Number\(.*amount\)"
# Check for consistent usage of parseNumberToBigInt
rg "parseNumberToBigInt"
Length of output: 941
Script:
#!/bin/bash
# Let's examine the actual implementations to understand if there are any risky float calculations
# Check the implementation in files that still use float operations
rg -A 5 "Math\.floor.*amount|parseFloat.*amount|Number\(.*amount\)" components/groups/forms/proposals/ProposalMessages.tsx components/factory/modals/multiMfxMintModal.tsx components/factory/modals/multiMfxBurnModal.tsx components/factory/forms/MintForm.tsx components/factory/forms/BurnForm.tsx
# Also check the parseNumberToBigInt implementation
cat utils/maths.ts
Length of output: 6739
There was a problem hiding this comment.
@fmorency, understood. Thanks for the update. Would you like me to open a new GitHub issue to track the removal of ProposalMessages.tsx?
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
utils/maths.ts(1 hunks)
🔇 Additional comments (1)
utils/maths.ts (1)
34-42: LGTM! Implementation looks robust for handling large numbers.
The function properly handles precision and includes appropriate error handling. This implementation should effectively address the issue of sending large token amounts.
Let's verify the function handles extreme cases correctly:
✅ Verification successful
Implementation verified and used correctly in token transfer scenarios
The verification shows that:
- The function is used correctly in token transfer forms (
sendForm.tsxandibcSendForm.tsx) with proper exponent handling - The implementation properly converts user input amounts to base units for blockchain transactions
- Error handling is in place for invalid inputs
- The function works alongside other numerical utilities (
shiftDigits,toNumber) to provide a robust number handling system
The search results confirm that the implementation is solid and properly used in the critical token transfer paths, making it suitable for handling large token amounts as intended in the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage patterns to ensure the function is used correctly with large numbers
# Look for test cases or usage patterns
rg -A 3 "parseNumberToBigInt.*" --type typescript
# Look for potential error handling of the function's results
rg "BigInt.*" --type typescript
Length of output: 155
Script:
#!/bin/bash
# Search for usage patterns and test cases of parseNumberToBigInt
rg -A 3 "parseNumberToBigInt"
# Look for test files related to maths utilities
fd "maths.*test" --type f
# Look for BigInt usage patterns
rg "BigInt"
# Check the content of the maths utility file
cat utils/maths.ts
Length of output: 8680
| export const toNumber = (val: string, decimals: number = 6) => { | ||
| return new BigNumber(val).decimalPlaces(decimals).toNumber(); |
There was a problem hiding this comment.
Warning: Potential precision loss when handling large numbers
The toNumber function converts values to JavaScript's Number type, which has limitations for large values (maximum safe integer is 2^53 - 1). This could lead to precision loss or incorrect results when handling large token amounts.
Consider this safer implementation:
-export const toNumber = (val: string, decimals: number = 6) => {
- return new BigNumber(val).decimalPlaces(decimals).toNumber();
+export const toNumber = (val: string, decimals: number = 6) => {
+ const num = new BigNumber(val);
+ if (!num.isFinite()) {
+ console.error(`Invalid input passed to toNumber: ${val}`);
+ return 0;
+ }
+ if (num.gt(Number.MAX_SAFE_INTEGER)) {
+ console.warn(`Number ${val} exceeds safe integer limits, precision may be lost`);
+ }
+ return num.decimalPlaces(decimals).toNumber();
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const toNumber = (val: string, decimals: number = 6) => { | |
| return new BigNumber(val).decimalPlaces(decimals).toNumber(); | |
| export const toNumber = (val: string, decimals: number = 6) => { | |
| const num = new BigNumber(val); | |
| if (!num.isFinite()) { | |
| console.error(`Invalid input passed to toNumber: ${val}`); | |
| return 0; | |
| } | |
| if (num.gt(Number.MAX_SAFE_INTEGER)) { | |
| console.warn(`Number ${val} exceeds safe integer limits, precision may be lost`); | |
| } | |
| return num.decimalPlaces(decimals).toNumber(); | |
| }; |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #81 +/- ##
==========================================
+ Coverage 59.42% 59.59% +0.17%
==========================================
Files 146 146
Lines 14187 14140 -47
==========================================
- Hits 8430 8427 -3
+ Misses 5757 5713 -44 ☔ View full report in Codecov by Sentry. |
Fixes #80
Summary by CodeRabbit
New Features
SendFormandIbcSendFormcomponents for improved amount validation and fee management.Bug Fixes
Refactor