fix(wallet): full 8-decimal Dash amounts, and accept & as a URI query separator - #914
Merged
romchornyi merged 2 commits intoAug 3, 2026
Merged
Conversation
`NumberFormatter.dashFormatter` capped `maximumFractionDigits` at 5, so a scanned `dash:…?amount=0.23243214` rendered as "0.23243" — 214 duffs dropped on screen. It backs the amount-entry screen (`BaseAmountModel`, `AmountObject`) and `BalanceView`, so both entry and display were affected. Five was an oversight rather than a deliberate display cap: - the paste path already clamps Dash input to 8 (`BaseAmountModel.updateFromPasteboard`); - `formattedDashAmountWithoutCurrencySymbol` already goes through `_dashDecimalFormatter`, which uses 8; - `DashAmountFormatterTests.testGeneralDashFormatterSupportsEightFractionDigits\ WithoutTrailingZeros` already asserts `dashFormatter.maximumFractionDigits == 8` and has been failing against this; - `Numbers+Dash.swift`'s own doc comment advertises `12345678 -> "1.2345678"`. `cryptoFormatter` leaves `minimumFractionDigits` at 0, so whole amounts still render as "1" rather than "1.00000000" — only amounts that actually carry sub-5-decimal precision get longer. Also decouples `dashFormat.maximum` from the display precision. It was `kMaxDashSupplyDuffs / pow(10, maximumFractionDigits)`, which at 5 evaluated to 21,000,000,000 instead of the 21,000,000 DASH max supply; it now divides by `kOneDash` the way `BaseAmountModel` already does. The two expressions only agree once the precision reaches 8, so leaving the coupling would have hidden the next change. Note for follow-up (not changed here): `cryptoFormatter` caches by currency code alone and returns the cached instance without honouring a different `exponent`, so the first caller for a code wins. Harmless today — the only other DASH-capable callers pass the Coinbase account exponent, which is also 8. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Scanning `dash:<address>&amount=0.23243214` was rejected outright. BIP21 introduces the query with `?`, and the parser split only on that, so for an `&`-only URI the entire tail stayed in `addressPart` — the address then failed validation and the whole request was refused, rather than merely losing the amount as the bug was originally reported. Split on whichever of `?` or `&` comes first. `&` is already the pair separator for the rest of the query, so a leading one yields the same key/value list and every following pair keeps parsing. Verified against generated QRs (four variants in `~/Desktop/bug-25-qr/`): the `?` form with 2 decimals pre-filled correctly before this change, the `&` form was rejected, and a bare address was accepted with no amount. Tests cover the `&` form, that following pairs still parse, and that all 8 decimals survive the parse. The remaining half of BUG-25 — an 8-decimal amount misbehaving on the send screen where a 2-decimal one works — is the display side, fixed by the 5→8 formatter change in this branch's other commit: the internal value already kept full precision (`AmountObject.dashInputString` uses the 8-digit `dashDecimalFormatter`), while `mainFormatted` was rendered through the 5-digit `dashFormatter`. Needs a device retest to confirm. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
romchornyi
changed the title
& as a URI query separator& as a URI query separator
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
2 participants
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.
Issue being fixed or feature implemented
Two carry-over bugs from 2026-08-03 testing, both about amount precision on the send path.
BUG-26 —
NumberFormatter.dashFormattercappedmaximumFractionDigitsat 5, so0.23243214DASH rendered as0.23243: 214 duffs dropped on screen.BUG-25 — a scanned payment request misbehaved. Retested with four generated QRs:
dash:<addr>?amount=0.23243214dash:<addr>&amount=0.23243214dash:<addr>?amount=0.25<addr>(bare)3 and 4 passing means the prefill path was never broken — the original "amount isn't
auto-populated" framing was wrong. That leaves two separate defects, one per commit here.
What was done?
fix(amount)— 5 → 8 decimal places. Five was an oversight, not a display cap:BaseAmountModel.updateFromPasteboard);formattedDashAmountWithoutCurrencySymbolalready goes through the 8-digit_dashDecimalFormatter, so the same amount rendered differently with and without the symbol;DashAmountFormatterTestsalready assertsdashFormatter.maximumFractionDigits == 8and hasbeen failing against this;
Numbers+Dash.swift's own doc comment advertises12345678 -> "1.2345678".cryptoFormatterleavesminimumFractionDigitsat 0, so whole amounts still render as "1", not"1.00000000" — only amounts that actually carry sub-5-decimal precision get longer.
This also decouples
dashFormat.maximumfrom the display precision. It waskMaxDashSupplyDuffs / pow(10, maximumFractionDigits), which at 5 evaluated to 21,000,000,000instead of the 21,000,000 DASH max supply; it now divides by
kOneDash, the wayBaseAmountModelalready does.fix(payments)— accept&as the query separator. BIP21 introduces the query with?andthe parser split only on that, so for QR 2 the entire tail stayed in
addressPart; the addressthen failed validation and the whole request was refused. It now splits on whichever of
?or&comes first —
&is already the pair separator for the rest of the query, so following pairs keepparsing unchanged.
That accounts for QR 2. QR 1 is the display side: the internal value already kept full precision
(
AmountObject.dashInputStringuses the 8-digitdashDecimalFormatter) whilemainFormattedwasrendered through the 5-digit
dashFormatter, falling back to the literal"Invalid Input"onfailure — which the first commit fixes.
How Has This Been Tested?
xcodebuild … -scheme dashpay … ARCHS=arm64 build→ BUILD SUCCEEDED (verified with #913applied on top, since that hotfix is what currently unblocks the branch's build).
Added to
PaymentProtocolTests: the&form parses address and amount, following pairs stillparse, and all 8 decimals survive the parse. The unit-test target remains unrunnable per
CLAUDE.md, so these are compile-ready rather than executed.Still owed — a device retest of QR 1 to confirm the 8-decimal send screen is clean. If it is,
BUG-25 closes entirely.
Breaking Changes
None. Amounts with more than 5 decimals now display in full; nothing that was previously shown
changes.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code