fix: mint/burn large numbers #82
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the handling of numerical amounts across several form components in the application. A new utility function, 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: 4
🧹 Outside diff range and nitpick comments (4)
utils/maths.ts (1)
34-47: Document number handling strategyThe module implements a robust strategy for handling large numbers using BigNumber for calculations and BigInt for storage. However, this approach should be documented to ensure consistent usage across the application.
Consider adding a module-level JSDoc comment explaining:
- When to use
parseNumberToBigIntvstoNumber- The relationship between
maxDigitsand token decimals- Precision considerations and limitations
- Examples of proper usage
Example documentation:
/** * Mathematical utilities for handling token amounts * * Strategy: * - Use BigNumber for calculations to maintain precision * - Use BigInt for storage of token amounts * - Convert to/from number only for display purposes * * @example * // Converting user input to token amount * const baseUnits = parseNumberToBigInt("1000.5", token.decimals); * * // Converting token amount to display value * const displayValue = toNumber(baseUnits.toString(), token.decimals); */components/factory/forms/MintForm.tsx (1)
Line range hint
75-78: Enhance error handling specificityThe current error handling could be more informative. Consider categorizing and handling specific error types.
- } catch (error) { - console.error('Error during minting:', error); + } catch (error: unknown) { + if (error instanceof Error) { + console.error('Minting failed:', { + message: error.message, + amount, + recipient, + denom: denom.base + }); + } else { + console.error('Unknown error during minting:', error); + }components/bank/forms/sendForm.tsx (1)
Line range hint
52-57: Consider using parseNumberToBigInt consistentlyThe validation schema and MAX button calculations still use parseFloat and Math.pow for number conversions. For consistency and to prevent potential precision issues with large numbers, consider using parseNumberToBigInt in these locations as well.
Example refactor for the MAX button calculation:
- const maxAmount = Number(values.selectedToken.amount) / Math.pow(10, exponent); + const maxAmount = Number(shiftDigits(Number(values.selectedToken.amount), -exponent));And for the validation schema:
- const balance = parseFloat(selectedToken.amount) / Math.pow(10, exponent); + const balance = Number(shiftDigits(Number(selectedToken.amount), -exponent));Also applies to: 238-252
components/bank/forms/ibcSendForm.tsx (1)
Line range hint
111-124: Consider potential timestamp overflow in timeout calculationThe timeout calculation
(stamp + 1.2e6) * 1e6could potentially overflow for large timestamps. Consider using BigInt for this calculation as well:- const stamp = Date.now(); - const timeoutInNanos = (stamp + 1.2e6) * 1e6; + const stamp = BigInt(Date.now()); + const timeoutInNanos = (stamp + BigInt(1.2e6)) * BigInt(1e6);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
components/bank/forms/ibcSendForm.tsx(2 hunks)components/bank/forms/sendForm.tsx(2 hunks)components/factory/forms/BurnForm.tsx(3 hunks)components/factory/forms/MintForm.tsx(3 hunks)utils/maths.ts(1 hunks)
🧰 Additional context used
📓 Learnings (2)
components/bank/forms/ibcSendForm.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/bank/forms/ibcSendForm.tsx:149-152
Timestamp: 2024-11-12T13:03:18.927Z
Learning: In `components/bank/forms/ibcSendForm.tsx`, the `handleSend` function relies on the `useTx` hook for error handling, so additional error handling or user notifications within `handleSend` are unnecessary.
components/factory/forms/BurnForm.tsx (1)
Learnt from: chalabi2
PR: liftedinit/manifest-app#9
File: components/factory/forms/BurnForm.tsx:262-265
Timestamp: 2024-11-12T13:03:18.927Z
Learning: When suggesting the removal of state variables managed by Formik, ensure that any functions or components relying on those state variables outside of Formik are refactored to use Formik's values.
🔇 Additional comments (8)
utils/maths.ts (1)
34-42: Consider adding validation for maxDigits parameter
While the implementation handles invalid inputs well, consider adding validation for the maxDigits parameter to ensure it matches the token's decimal places. Different tokens might have varying decimal places (e.g., USDC has 6, while some tokens have 18).
Let's check the token decimals used in the codebase:
Consider adding parameter validation:
export const parseNumberToBigInt = (v: string, maxDigits: number = 6) => {
+ if (maxDigits < 0 || maxDigits > 18) {
+ throw new Error(`Invalid maxDigits: ${maxDigits}. Must be between 0 and 18.`);
+ }
const amount = new BigNumber(v);components/factory/forms/MintForm.tsx (2)
6-6: LGTM: Import statement updated correctly
The addition of parseNumberToBigInt import aligns with the PR's objective to improve handling of large numbers.
46-46: Verify network transaction limits for large numbers
While removing the maximum limit addresses the minting issue, we should ensure that the network can handle arbitrarily large numbers. Consider:
- Network transaction size limits
- Gas cost implications for large numbers
- Economic impacts of unlimited minting
components/factory/forms/BurnForm.tsx (2)
7-7: LGTM: Appropriate utility import for large number handling
The addition of parseNumberToBigInt import aligns with the PR's objective of fixing large number handling in mint/burn operations.
105-105: Verify amount conversion accuracy for edge cases
The switch to parseNumberToBigInt improves precision by avoiding floating-point arithmetic issues. However, let's verify the conversion accuracy for edge cases.
✅ Verification successful
Amount conversion implementation is robust and safe
The parseNumberToBigInt implementation is well-designed with proper safeguards:
- Uses
BigNumberfor precise decimal arithmetic - Validates input with
isFinite()check and returnsBigInt(0)for invalid inputs - Correctly handles decimal places using
pow()andtimes() - Has consistent error logging
- Used consistently across mint, burn, and send operations
- Input validation is present in all forms (
isNaNchecks)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases that verify the amount conversion
rg -A 5 "parseNumberToBigInt.*test" || \
rg -A 5 "test.*parseNumberToBigInt"
# Search for any existing amount conversion tests
rg -A 5 "amount.*conversion.*test" || \
rg -A 5 "test.*amount.*conversion"
Length of output: 149
Script:
#!/bin/bash
# Let's first check the implementation of parseNumberToBigInt
ast-grep --pattern 'function parseNumberToBigInt' -A 10
# Also search for any test files related to number parsing or conversion
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx | xargs rg -l "parseNumberToBigInt|amount.*conversion"
# Check usage patterns of parseNumberToBigInt across the codebase
rg "parseNumberToBigInt" -B 2 -A 2
Length of output: 4211
Script:
#!/bin/bash
# Let's examine the implementation details of parseNumberToBigInt in utils/maths.ts
cat utils/maths.ts
# Also check for any BigNumber usage and validation
rg "new BigNumber" -B 2 -A 2
# Look for any error handling or validation patterns around amount conversion
rg "amount.*isFinite|amount.*isNaN|amount.*validation" -B 2 -A 2
Length of output: 6161
components/bank/forms/sendForm.tsx (1)
6-6: LGTM: Import of parseNumberToBigInt utility
The addition of parseNumberToBigInt is aligned with the PR objective of improving large number handling.
components/bank/forms/ibcSendForm.tsx (2)
5-5: LGTM: Import addition aligns with large number handling requirements
The addition of parseNumberToBigInt import is appropriate for improving large number handling capabilities.
111-111: LGTM: Improved large number handling with parseNumberToBigInt
The change from mathematical operations to parseNumberToBigInt improves precision and prevents potential issues with large numbers. Let's verify the behavior with large amounts.
✅ Verification successful
Let me analyze the implementation of parseNumberToBigInt to verify its behavior with large numbers.
LGTM: parseNumberToBigInt implementation is safe for large numbers
The implementation safely handles large numbers by:
- Using BigNumber.js for precise decimal arithmetic
- Converting to base units with correct decimal precision using
pow - Proper error handling for invalid inputs
- Safe conversion to BigInt for blockchain transactions
The function is used consistently across different forms (send, burn, mint) with proper exponent handling, making it a reliable solution for token amount conversions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify parseNumberToBigInt handles large numbers correctly
# Search for test cases or usage patterns
rg -A 3 "parseNumberToBigInt"
Length of output: 3395
Script:
#!/bin/bash
# Get the full implementation of parseNumberToBigInt and its usage context
cat utils/maths.ts
Length of output: 2085
utils/maths.ts
Outdated
| export const toNumber = (val: string, decimals: number = 6) => { | ||
| return new BigNumber(val).decimalPlaces(decimals).toNumber(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling consistent with parseNumberToBigInt
The function should handle invalid inputs and large numbers consistently with parseNumberToBigInt. Consider adding similar error handling and validation.
Suggested implementation:
export const toNumber = (val: string, decimals: number = 6) => {
- return new BigNumber(val).decimalPlaces(decimals).toNumber();
+ const amount = new BigNumber(val);
+ if (!amount.isFinite()) {
+ console.error(`Invalid input passed to toNumber: ${val}`);
+ return 0;
+ }
+ if (amount.gt(Number.MAX_SAFE_INTEGER)) {
+ console.warn(`Number ${val} exceeds MAX_SAFE_INTEGER, precision may be lost`);
+ }
+ return amount.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 amount = new BigNumber(val); | |
| if (!amount.isFinite()) { | |
| console.error(`Invalid input passed to toNumber: ${val}`); | |
| return 0; | |
| } | |
| if (amount.gt(Number.MAX_SAFE_INTEGER)) { | |
| console.warn(`Number ${val} exceeds MAX_SAFE_INTEGER, precision may be lost`); | |
| } | |
| return amount.decimalPlaces(decimals).toNumber(); | |
| }; |
| @@ -56,8 +53,7 @@ export default function MintForm({ | |||
| } | |||
| setIsSigning(true); | |||
| try { | |||
| const amountInBaseUnits = BigInt(parseFloat(amount) * Math.pow(10, exponent)).toString(); | |||
|
|
|||
| const amountInBaseUnits = parseNumberToBigInt(amount, exponent).toString(); | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the results, I can now generate the final response:
Error handling in parseNumberToBigInt is implemented but could be improved
The function has basic error handling:
- Uses BigNumber for precise decimal handling
- Validates input with
isFinite()check - Returns
BigInt(0)and logs error for invalid inputs
However, silently returning 0 for invalid inputs could mask errors. Consider:
- Throwing an error instead of returning 0
- Adding input validation in the calling components
- Documenting the behavior in JSDoc
🔗 Analysis chain
Verify error handling in parseNumberToBigInt
The switch to parseNumberToBigInt improves precision for large numbers, but we should verify:
- How overflow scenarios are handled
- Error handling for invalid inputs
- Documentation of the function's behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for the implementation and documentation of parseNumberToBigInt
rg -A 10 "parseNumberToBigInt"
# Check for any existing error handling patterns
ast-grep --pattern 'try {
$$$
parseNumberToBigInt($_, $_)
$$$
} catch'
Length of output: 8442
| @@ -166,7 +164,7 @@ export default function BurnForm({ | |||
| authority: admin ?? '', | |||
| burnCoins: burnPairs.map(pair => ({ | |||
| denom: denom.base, | |||
| amount: BigInt(parseFloat(pair.amount) * Math.pow(10, exponent)).toString(), | |||
| amount: parseNumberToBigInt(pair.amount, exponent).toString(), | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding balance validation for multi-burn operations
While the conversion to parseNumberToBigInt improves precision, consider adding validation to ensure each burn amount doesn't exceed the available balance for its target address.
const handleMultiBurn = async () => {
- if (burnPairs.some(pair => !pair.address || !pair.amount || isNaN(Number(pair.amount)))) {
+ if (burnPairs.some(pair => {
+ if (!pair.address || !pair.amount || isNaN(Number(pair.amount))) return true;
+ const balance = await useTokenFactoryBalance(pair.address, denom.base);
+ const balanceNumber = parseFloat(shiftDigits(balance?.amount || '0', -exponent));
+ return Number(pair.amount) > balanceNumber;
+ })) {
setToastMessage({
type: 'alert-error',
- title: 'Missing fields',
- description: 'Please fill in all fields with valid values.',
+ title: 'Invalid input',
+ description: 'Please ensure all fields are filled and amounts do not exceed available balances.',
bgColor: '#e74c3c',
});
return;
}Committable suggestion skipped: line range outside the PR's diff.
components/bank/forms/sendForm.tsx
Outdated
| parseFloat(values.amount) * Math.pow(10, exponent) | ||
| ).toString(); | ||
|
|
||
| const amountInBaseUnits = parseNumberToBigInt(values.amount, exponent).toString(); |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent number handling found in multi-mint and multi-burn modals
The verification revealed inconsistent number handling patterns. While sendForm.tsx correctly uses parseNumberToBigInt, the multi-mint and multi-burn modals are using a less precise approach with BigInt(parseFloat(pair.amount) * Math.pow(10, exponent)).
Locations that need attention:
components/factory/modals/multiMfxBurnModal.tsx:86: ReplaceBigInt(parseFloat(pair.amount) * Math.pow(10, exponent))withparseNumberToBigInt(pair.amount, exponent)components/factory/modals/multiMfxMintModal.tsx:92: ReplaceBigInt(parseFloat(pair.amount) * Math.pow(10, exponent))withparseNumberToBigInt(pair.amount, exponent)
🔗 Analysis chain
Verify the precision handling for large numbers
The change from using Math.floor/parseFloat to parseNumberToBigInt addresses the large number handling issue. However, let's verify that this change maintains precision across the entire range of possible token amounts.
Let me verify the usage of number handling in the related files to ensure consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other instances of Math.floor/parseFloat with Math.pow
# to ensure consistent number handling across the codebase
# Look for potentially inconsistent number handling patterns
rg -n "Math\.(floor|ceil|round)\(parseFloat.*\*\s*Math\.pow"
# Look for other instances where we might need to use parseNumberToBigInt
rg -n "parseFloat.*\*\s*Math\.pow\(10,"
# Look for other token amount conversions that might need updating
ast-grep --pattern 'amount: [
{
denom: $_,
amount: $_
}
]'
Length of output: 459
Script:
#!/bin/bash
# Check the implementation of parseNumberToBigInt and its usage
rg -n "parseNumberToBigInt" -A 3
# Look for the specific files where we found potential inconsistencies
cat components/factory/modals/multiMfxBurnModal.tsx
cat components/factory/modals/multiMfxMintModal.tsx
Length of output: 27898
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #82 +/- ##
==========================================
- Coverage 59.59% 59.59% -0.01%
==========================================
Files 146 146
Lines 14140 14134 -6
==========================================
- Hits 8427 8423 -4
+ Misses 5713 5711 -2 ☔ View full report in Codecov by Sentry. |
Fixes #79
Contains #81
Summary by CodeRabbit
Release Notes
New Features
parseNumberToBigInt, for improved precision in amount processing across various forms (IBCSendForm, SendForm, BurnForm, MintForm).Bug Fixes
Documentation
Chores