Fix: Remove floating-point precision loss in currency conversions usi…#90
Fix: Remove floating-point precision loss in currency conversions usi…#90Rav1Chauhan wants to merge 1 commit intoDjedAlliance:mainfrom
Conversation
📝 WalkthroughWalkthroughAll currency conversion helper functions in the SDK have been refactored to use BigInt arithmetic instead of floating-point operations. Function parameters were standardized from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@djed-sdk/src/helpers.js`:
- Around line 120-154: Both calculateBcUsdEquivalent and
calculateRcUsdEquivalent can throw on BigInt division by zero when
coinsDetails.scaledScExchangeRate is "0"; after converting scaledScExchangeRate
to BigInt (adaPerUsd) add an explicit guard if (adaPerUsd === 0n) return
"0.000000" (or the same zero-formatted string used elsewhere) before performing
any division; similarly, ensure any other BigInt denominators (e.g., adaPerRc if
used as a divisor) are checked for zero to avoid RangeError.
- Around line 120-171: The conversion functions use BigInt() directly on
decimal-formatted strings which throws; update calculateBcUsdEquivalent,
calculateRcUsdEquivalent, and getScAdaEquivalent to call decimalUnscaling(...)
on scaledScExchangeRate, scaledSellPriceRc, and scaledPriceSc respectively
(after removing commas if needed) and pass the integer string returned by
decimalUnscaling to BigInt; keep the existing null/undefined guards and then use
the resulting BigInt values (adaPerUsd, adaPerRc, adaPerSc) in the same
arithmetic as before.
…ng BigInt
Addressed Issues:
Fixes #71
Issue
Conversion functions in helpers.js used parseFloat and 1e6 multipliers, leading to floating-point precision loss for large currency amounts (> 2^53 - 1).
This is unacceptable for financial/blockchain software.
Solution
Replaced all float-based arithmetic with BigInt
Maintained full integer precision internally
Converted to formatted string only at final display stage
Added test coverage for large values
Result
Ensures mathematically correct USD equivalents for large blockchain-denominated values.
Screenshots/Recordings:
Not applicable.
This change modifies internal arithmetic logic in djed-sdk/src/helpers.js and does not affect UI components directly.
To validate behavior, large-value test cases were manually verified to ensure no precision loss for values exceeding 2^53 - 1.
Additional Notes:
🔍 Problem Summary
The following functions used floating-point arithmetic:
calculateBcUsdEquivalent
calculateRcUsdEquivalent
getScAdaEquivalent
They relied on:
parseFloat
1e6
.toFixed()
JavaScript Number is limited to 53-bit precision. For blockchain-scale values (10^18 and above), this causes rounding errors and loss of precision — which is unacceptable for financial software.
Checklist
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit
Bug Fixes
Refactor