fix(numbers): convert amounts to base units with BigNumber instead of float math - #519
fix(numbers): convert amounts to base units with BigNumber instead of float math#519maymuneth wants to merge 3 commits into
Conversation
👋 Thanks for contributing to Miden, @maymuneth!We really appreciate you taking the time to open this pull request. Miden is building an edge-first, zero-knowledge blockchain, and thoughtful contributions from the community are a big part of how it gets better. A maintainer will review your changes as soon as they can — in the meantime, please make sure the CI checks are green and that your change follows the repository's contribution guidelines. We're genuinely excited to have you here. 🧡 One thing we want to be transparent about up front: contributing to this repository will not make you eligible for any token airdrop, allocation, or other reward — not now, and not at any point in the future. Thanks again for being part of the community — we're glad you chose to contribute. 🚀 |
|
The core change is right and this is a real correctness bug worth fixing — Two things I'd want to see before this merges, though:
Core logic LGTM; I'd just like the regression tests and a note on the input contract. |
What
stringToBigIntconverts a user-entered amount string into base units viaparseFloat(str) * 10 ** decimals, thenMath.round. Doubles carry ~15-16 significant decimal digits, so the result drifts from the true value once the amount plus its decimal scale exceeds that.Observed:
"1.1""99999999.99999999""12345678901.12345678"User impact
This is the only path that turns the amount a user types into the base-unit figure that gets signed and broadcast. It is called from the send review (
ReviewTransaction.tsx,SendManager.tsx), the swap flow (SwapManager.tsx), and the earn deposit review (EarnDepositReview.tsx). The wallet already handles 18-decimal tokens (BRIDGEABLE_EVM_OUTPUT_TOKEN_DECIMALS = 18), which is where the drift is largest.Change
Convert through
bignumber.js— already imported at the top of the file — usingshiftedBy(decimals)andROUND_HALF_UP, which matches the rounding the previousMath.roundapplied. Behaviour on empty input is preserved:stringToBigInt('')still throws, whichSwapManager.tsx:193relies on.Testing
I wasn't able to run the workspace suite locally, so this is verified by evaluating the two expressions directly rather than through the test runner:
That reproduces the three "Current" values in the table above exactly. The "Expected" column is the digit-shift of the same input string, which is what
BigNumber.shiftedByproduces.Rounding mode is preserved: the old code used
Math.round(half away from zero for positives), andROUND_HALF_UPmatches it on this input domain, which is non-negative amounts.Empty input still throws, so the guard
SwapManager.tsxrelies on is unaffected:parseFloat('')gaveNaN→BigInt(NaN)→RangeError, andnew BigNumber('')givesNaN→BigInt('NaN')→SyntaxError. Both throw, though the error type changes. Say the word if you'd rather I add a unit test with the table cases.