fix(frontend): scale token amounts with string arithmetic, not floats - #443
Merged
zachyo merged 1 commit intoSep 2, 2026
Merged
Conversation
soropad#320 got the plumbing right — supply is typed as a string in the schema, stays a string through the form, and every caller routes through one shared helper. toBaseUnits then threw the guarantee away on its first line: const amount = typeof display === "string" ? Number(display) : display; return BigInt(Math.round(amount * (10 ** decimals))); Number() puts the value back into an IEEE-754 double before the multiply, which is what soropad#253 and soropad#320 were raised to stop, and wrapping the result in BigInt cannot recover precision already gone. The multiply is exact only while supply * 5^decimals < 2^53, so the safe ceiling collapses as decimals rise: about 115 billion at 7 decimals, but 2,361 at 18. Measured against the old helper: - 1,000,000 at 18 decimals minted 999999999999999983222784 rather than 1000000000000000000000000 — 16,777,216 base units short. - 123,456,789,012,345 at 7 decimals was 41,600 base units short. This reached every amount the app sends, not just deploy: the helper has eleven call sites across transfer, burn, mint, mint_batch and vesting. Scale by moving the decimal point through string surgery instead — split on ".", reject a fraction longer than the token's decimals, right-pad, concatenate, and parse once with BigInt. A string amount now never passes through a Number. Numbers are still accepted for the callers that pass literals, and are expanded from exponent notation first, since String(1e21) is "1e+21" and no string surgery survives that. Two edges worth naming: - An absent or unparseable amount still yields 0n, because preview and preflight paths call this against half-filled forms. - A well-formed amount with more fraction digits than the token supports now throws rather than silently rounding. Rounding would send an amount the user did not ask for, which is the same class of bug this function exists to prevent. Every call site already runs inside a try/catch that surfaces the message, so this shows up as a visible error rather than a wrong transfer. The existing tests covered 1,000,000 at 7 decimals and 0.1 at 7 decimals — small round values that survive the float exactly — so every assertion passed against the buggy helper. The added regression tests include both failing cases above; five of the six new assertions fail against the old implementation. Closes soropad#395
|
@DSOTec Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The plumbing from #320 is correct — supply is typed as a string in the schema,
stays a string through the form, and every caller routes through one shared
helper.
toBaseUnitsthen threw the guarantee away on its first line:Number()puts the value back into an IEEE-754 double before the multiply,which is exactly what #253 and then #320 were raised to stop. Wrapping the
result in
BigIntcannot recover precision that is already gone.The multiply is exact only while
supply * 5 ** decimals < 2 ** 53, so thesafe ceiling collapses as decimals rise — about 115 billion at 7 decimals, but
2,361 at 18. Reproduced against the old helper:
1,000,000@ 189999999999999999832227841000000000000000000000000123,456,789,012,345@ 712345678901234499584001234567890123450000000Worth noting the blast radius is wider than deploy: the helper has eleven
call sites across transfer, burn, mint,
mint_batchand vesting, so thisreached every amount the app sends.
Solution
Scale by moving the decimal point through string surgery: split on
., rejecta fraction longer than the token's
decimals, right-pad, concatenate, andparse once with
BigInt. A string amount never passes through aNumber.numberis still accepted for the callers that pass literals, but is expandedout of exponent notation first —
String(1e21)is"1e+21", which no stringsurgery survives. The doc comment now says plainly that a
numberbeyond~15 significant digits has already lost precision before the call, and to pass
a string.
Two edges worth calling out explicitly:
0n. Preview and preflightpaths call this against half-filled forms, so throwing there would break the
deploy wizard's fee estimate.
silently rounding.
toBaseUnits("1.12345678", 7)used to quietly become1.1234568— sending an amount the user did not ask for, which is the sameclass of bug this function exists to prevent. I checked all eleven call
sites: every one already runs inside a
try/catchthat surfaces the messageas a toast or an error panel, so this becomes a visible, actionable error
rather than a crash.
Testing
The reason the bug survived review is the existing test file: it covered
1,000,000 at 7 decimals and 0.1 at 7 decimals — small, round values that
survive the float exactly — so every assertion passed against the buggy
helper, including the ScVal case #319 asked for.
The added regression tests include both failing cases from the issue. I checked
they are real coverage rather than more false confidence — five of the six
new assertions fail against the old implementation:
Also added: exactness across every
decimalsvalue 0–18, amounts pastNumber.MAX_SAFE_INTEGERup to the 38-digit schema limit, fraction padding,sign preservation, exponent-notation input, invalid
decimals, and an exactfromBaseUnitsround-trip at 18 decimals.lib/__tests__/utils.test.ts: 25 passed. Full suite unchanged from masterapart from the additions — same 5 pre-existing failing suites before and after,
180 → 192 passing.
eslintclean on both changed files;tscreports no newerrors.
Scope
Two files, no behaviour change to any caller beyond correctness. I deliberately
did not touch
app/deploy/DeployForm.tsxorapp/hooks/useDeployToken.ts:the schema there already constrains supply to
/^[0-9]+$/, so the fix in thehelper is sufficient and the call sites need no change.
Two pre-existing problems found along the way
Neither is addressed here — flagging them because both are worth their own
issue.
1.
useDeployToken.tsis structurally broken.npm run type-checkfails onmasterwith four syntax errors, three of them in this file. The cause is a badmerge: the entire
const deployToken = useCallback(async (params) => {declaration was dropped, leaving an orphaned body, dependency array and
);.app/claim/ClaimVesting.tsx:37has a matching break —useStatelost itsopening
<.I repaired both, and it revealed worse damage behind them:
initializeContract(live, called at line 241) splices a typed-
TokenClientconstruction into a rawTransactionBuilderchain, and referencesadminScVal,decimalScVal,nameScValand others that are never defined. That is a reimplementation of thedeploy-initialize step, not a repair, and it is a maintainer's call. I reverted
my two fixes rather than leave this PR reporting eight type errors instead of
four while still not being green.
2.
.github/workflows/ci.ymlis still an invalid workflow file. As noted in#437,
components:is a YAML sequence where Actions requires a scalar, so thefile fails validation — every run is 0s,
failure, zero jobs, listed underits file path rather than
name: CI. It is still failing this way onmasteras of
dfe5db4. The fix iscomponents: rustfmt, clippy. Until that lands, theLint / Type Check / Jest steps this PR would be measured against do not execute
at all, which is why the two breakages above went unnoticed.
Closes #395