Skip to content

fix(react-sdk): correct sign handling in parseAssetAmount / formatAssetAmount - #275

Open
batuhankocyigit wants to merge 5 commits into
0xMiden:mainfrom
batuhankocyigit:fix/negative-amount-sign
Open

batuhankocyigit wants to merge 5 commits into
0xMiden:mainfrom
batuhankocyigit:fix/negative-amount-sign

Conversation

@batuhankocyigit

Copy link
Copy Markdown

fix(react-sdk): correct sign handling in parseAssetAmount / formatAssetAmount for negative amounts

The bug

formatAssetAmount / parseAssetAmount (exported from @miden-sdk/react, src/utils/amounts.ts) mishandle negative values.

parseAssetAmount splits the input on . and combines the whole and
fraction parts as BigInt(whole) * factor + BigInt(fraction). When the
input is negative, the minus sign only affects the whole part, not the
combined magnitude:

parseAssetAmount("-5.25", 2) // => -475n, expected -525n
parseAssetAmount("-0.5", 2)  // => 50n,   expected -50n (sign lost entirely,
                              //           since BigInt("-0") === 0n)

formatAssetAmount has the mirror problem: dividing/modding a negative
BigInt by a positive factor yields a negative remainder, which gets
concatenated after the decimal point, producing an invalid string:

formatAssetAmount(-525n, 2) // => "-5.-25", not a valid number at all

Both functions are part of the package's public API surface
(export { formatAssetAmount, parseAssetAmount } from "./utils/amounts" in
src/index.ts) and formatAssetAmount is also used internally in
utils/notes.ts to render note amounts, so any consumer surfacing a
negative amount (e.g. a balance delta, a signed transfer amount, a refund)
gets silently wrong output today — no throw, no warning.

The fix

Extract the sign once up front, operate on the absolute magnitude, and
re-apply the sign to the final result in both functions. Minimal, symmetric
change, no public API/signature change.

Testing

  • Added regression tests for both functions covering negative whole
    numbers, negative fractional amounts, and the "-0.x" edge case, plus a
    round-trip test (parseAssetAmount → formatAssetAmount → same string)
    for negative inputs.
  • Ran typecheck, lint, and the full unit test suite locally — all green
    (853/853 passing, 845 existing + 8 new).

Notes for reviewers

  • No behavior change for non-negative amounts (existing test suite passes
    unmodified).
  • I did not touch any other files; this is scoped to the one utility module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant