Skip to content

fix(buy,sell): correct how the amount field handles grouped and rejected input - #918

Draft
joshuakrueger-dfx wants to merge 18 commits into
stagingfrom
fix/ambiguous-amount-feedback
Draft

fix(buy,sell): correct how the amount field handles grouped and rejected input#918
joshuakrueger-dfx wants to merge 18 commits into
stagingfrom
fix/ambiguous-amount-feedback

Conversation

@joshuakrueger-dfx

@joshuakrueger-dfx joshuakrueger-dfx commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

EN:
The amount field now resolves a thousands separator instead of refusing it — typing or pasting 1.000 shows 1000, so the user sees what was understood before committing to a purchase, and 1.000,50 resolves the same way whichever path it arrives by. When the parser rejection still fires, which after this change means genuinely unreadable input such as 1.000,000, it names the accepted form instead of falling through to a generic "contact support" message. Both converter cubits also stop leaving a stale counter value behind after a failed conversion, closing the half of audit finding M5 in #615 that fa3a8266 marked as fixed without ever touching the cubit. 4997 tests pass, every behavioural change is pinned by a mutation probe, and six review passes were needed to get there.

DE:
Das Betragsfeld löst einen Tausenderpunkt jetzt auf, statt ihn abzulehnen — aus 1.000 wird sichtbar 1000, sodass vor dem verbindlichen Kauf klar ist, welcher Betrag verstanden wurde, und 1.000,50 ergibt denselben Wert, egal ob getippt oder eingefügt. Greift die Ablehnung im Parser doch noch, was nach dieser Änderung wirklich unlesbare Eingaben wie 1.000,000 betrifft, benennt sie die akzeptierte Form, statt in eine allgemeine „Support kontaktieren"-Meldung zu fallen. Beide Converter-Cubits lassen nach einer fehlgeschlagenen Umrechnung ausserdem keinen veralteten Gegenwert mehr stehen; damit ist die Hälfte von Audit-Finding M5 aus #615 geschlossen, die fa3a8266 als erledigt markierte, ohne den Cubit je anzufassen. 4997 Tests laufen grün, jede Verhaltensänderung ist per Mutationsprobe abgesichert, und es brauchte sechs Review-Durchläufe bis dahin.

Details

Symptom (verbatim): "Bei einer Änderung dieses Wertes erscheint die Fehlermeldung." The message quoted is the text behind paymentInformationFailed"Beim Abrufen der Zahlungsinformationen ist ein Fehler aufgetreten … wenden Sie sich an unseren Support." Reproduced on staging: typing 1.000 produced exactly that message, plus a share count left over from the previous quote.
Scale: The amount field is the entry point of every purchase that is not the default. Any German-speaking user writing a four-digit amount as 1.000 hits it — the field explicitly accepts . and ,. The stale counter affects every failed conversion, on the buy and the sell screen.
Smaller fix considered: ~20 lines — a normalisation function plus one TextInputFormatter. That is the core of what was built; the rest of the diff is the review fallout documented below. Loosening the parser rejection was rejected: it would restore a silent factor-1000 error. Reworking the cubit's mapping of the other 26 QuoteError codes was rejected as a separate change.

Why resolving is safe here

The app offers only EUR and CHF, both two-decimal currencies, so a separator followed by exactly three digits cannot be a decimal place: 1.000 and 1,000 both mean one thousand. Consecutive groups with the same separator (1.000.000) and thousands-plus-decimal with different separators (1.000,50, 1,000.50) resolve atomically, so pasting and typing agree. Partials are left alone while typing (1, 1., 1.0, 1.00), and the caret keeps its position relative to the edit — the formatter counts only the separators actually dropped to its left.

Worth being precise about the source: audit finding M5 in #615 asked for comma decimals (300,75) to parse, not for grouped input to be refused — the blanket rejection was a side effect of that fix, not the finding. The rejection stays in the parser for input that remains ambiguous. double.tryParse returns 1.0 for 1.000 and 5.0 for 5.000 (measured); that failure mode was silent, this one is on screen.

The sell error branch was dead code

The sell payment-info cubit got the same treatment at first. It was removed again: SellButton.amount is fed from a field carrying FilteringTextInputFormatter.digitsOnly, so a separator can never arrive there and chargedFiatAmount never throws on that path. The branch, its snackbar, its widget test and its golden are gone. What stays on the sell screen is the formatter on the fiat field and the stale-counter clearing — both independent of that branch.

Verification

Full suite (flutter test --exclude-tags golden) 4997 passed, 0 failed
flutter analyze 1 pre-existing warning in real_unit_transfer_service.dart:109, untouched here
Visual baselines rendered by golden-regenerate.yaml on the self-hosted runner; no existing baseline changed
Review passes 6, two independent lanes per pass

Every behavioural change is proved by mutation, not by deletion:

Mutation Result
normalizeFiatInput regex \d{3}\d{4} 4 fail, 17 pass
invalidAmountFormatunknown in buy_payment_info_cubit.dart 2 fail, 73 pass
Broad on FormatException reinstated around the service call 1 fail, 22 pass
Remove sharesText clearing in buy_converter_cubit.dart (2 sites, hit count asserted) 2 fail, 15 pass
Empty-amount guard removed from the listener 1 fail, 16 pass
Caret math back to the global length delta 3 fail
Caret loop counting past the cursor 2 fail
strippedFiatGroupingSeparator returning the decimal separator 6 fail

The last three each pin a caret regression that an earlier pass introduced — the calculation went through three revisions, and each revision was caught by the pass after it.

Reproduction

test/screens/buy/buy_amount_change_repro_test.dart drives the real BuyPage with the real cubits and only the two services mocked. On the unfixed code, typing 1.000 produced:

amountField="1.000" converter(fiat=1.000, shares=209, loading=false, currency=CHF)
paymentInfo=Failure(PaymentInfoError.unknown) supportText=true spinner=false

Both defects in one snapshot. Further tests walk the intermediate states one at a time asserting field text and controller.selection, and drive a purchase through after changing the amount — the quote id encodes amount and currency, so a leftover 300 fails the assertion instead of passing silently.

Final pass (9e8d8fb):
Coherent: Every file serves one behaviour — what the amount field does with a grouped or unreadable value, and what the screen shows afterwards. The parser gains a normaliser, one formatter carries it to the buy and sell fields, the buy payment-info cubit classifies the leftover rejection, both converter cubits stop showing a figure that no longer belongs to the input, and the notice names the accepted form. Each part has a test that fails under mutation; the golden that was missing now exists and renders the state it claims.
Nothing extra: The parser rejection was not loosened, no retry button was added for input that would fail again, the sell shares field stays digitsOnly, the sell error branch was removed once it proved unreachable, and the other 26 QuoteError codes still map to unknown — a real weakness, but a separate risk surface. Two findings from the review passes were deliberately left out as their own money-path changes: the window in which a landed quote stays confirmable while a new one is in flight, and the rounding in chargedFiatAmount (300,75 is charged as 301).
Sources closed: Audit finding M5 in #615 — the stale-preview half, marked "Fixed here" in fa3a8266 without the cubit ever being touched, is closed here. The support report is traced: the amount error is reproduced and fixed. Six review passes, thirteen findings raised, all resolved or explicitly carried out of scope above; the final pass reports none. No issue is linked; no review comments exist on any of the three channels. Commit messages check out against the diff.

…g generically

An amount typed with a thousands separator (1.000) is deliberately rejected
by chargedFiatAmount, which throws a FormatException. Both payment-info
cubits caught it in their generic catch and surfaced PaymentInfoError.unknown,
so the user was told to contact support instead of being told to drop the
separator.

Give the rejection its own error value and its own message. The parsing rule
itself is unchanged: a grouping-ambiguous amount is still refused rather than
guessed.
When the brokerbot conversion threw, both converter cubits only cleared the
loading flag and left the previously computed counter value in place. The buy
screen therefore showed the rejected input 1.000 next to 209 shares, a number
belonging to the 300 quote before it.

Clear the result field in every catch so no figure outlives the input it was
computed from. The _seq race guard is untouched.
…tead of refusing it

The app only offers EUR and CHF, both two-decimal currencies, so a separator
followed by exactly three digits cannot be a decimal place: 1.000 and 1,000
both mean one thousand. The field now resolves that pattern and shows the
result, so the user sees 1000 before committing to a purchase rather than
being told to retype it.

Partial input is left alone while typing (1, 1., 1.0, 1.00) and the caret
stays at the end, so entering 1.000.000 keeps working character by character.
The parser-level rejection stays in place as a safety net for values that do
not come through the field, such as a deeplink or a paste.
Every other failure state of the buy screen has a golden. This one did not,
so nothing guarded how the new panel renders.

The state is driven with 1.300,75 rather than 1.000: the field formatter
rewrites a lone thousands group before the parser sees it, so only a mixed
grouping-and-decimal value still reaches chargedFiatAmount and throws.
@joshuakrueger-dfx joshuakrueger-dfx changed the title fix(buy,sell): tell the user an ambiguous amount is ambiguous fix(buy,sell): correct how the amount field handles grouped and rejected input Aug 19, 2026
Rendered by the golden-regenerate workflow on the self-hosted runner, so it
matches the toolchain that validates it on PRs. Committed here signed rather
than pushed by the workflow bot, which commits unsigned.
@joshuakrueger-dfx
joshuakrueger-dfx force-pushed the fix/ambiguous-amount-feedback branch from 2443abb to 2fb4b9a Compare August 19, 2026 15:41
The customer reported he could only buy at the pre-filled 300. Production
shows valid quotes at 100 and 200 that he never completed, which does not
distinguish would-not from could-not.

Drive the purchase through BuyPage after replacing the amount, and after a
currency switch, asserting the confirmed quote is the current one. The quote
id encodes amount and currency, so a leftover 300 fails the assertion instead
of passing silently.
The field now resolves a lone thousands group, so 1.000 no longer reaches the
parser from the keyboard. What still reaches it are values with more than one
separator, such as 1.300,75 — for those, telling the user to drop the thousands
separator describes the wrong problem.

Name the condition that is left: at most one separator.
…ding

Rendered by golden-regenerate on the self-hosted runner, committed here signed
rather than pushed by the workflow bot.
@joshuakrueger-dfx
joshuakrueger-dfx force-pushed the fix/ambiguous-amount-feedback branch from 1280714 to 43a4993 Compare August 20, 2026 08:40
Six findings from the review lanes:

An emptied amount no longer triggers a quote. Clearing the counter value
made the listener sync the field to empty and fetch a quote for it, which
chargedFiatAmount turns into 0 — and since no loading state is emitted while
a success is present, the previous quote stayed confirmable during that
request. The empty case now drops the landed quote instead.

FormatException is caught around chargedFiatAmount only. It used to wrap the
whole service call, where jsonDecode and DateTime.parse throw it too, so a
malformed backend response was reported to the user as an unreadable amount.

Pasting 1.000.000 now resolves like typing it. Consecutive groups with the
same separator are unambiguous, and the two input paths disagreed.

The caret keeps its position relative to the edit instead of jumping to the
end, the sell error state gets the widget test and golden its buy counterpart
already had, and the ARB files end in a newline again.
Rendered by golden-regenerate on the self-hosted runner, committed here signed
rather than pushed by the workflow bot.
@joshuakrueger-dfx
joshuakrueger-dfx force-pushed the fix/ambiguous-amount-feedback branch from d6a8a95 to f0a3bda Compare August 20, 2026 10:41
The generated sell_invalid_amount_format.png came out bit-identical to
sell_unknown_error.png: on the sell screen the error is a SnackBar from the
listener, and a state-stubbed golden does not render it, so the image shows
an unchanged screen.

A golden that cannot fail hides the gap it was meant to close. The SnackBar
is covered by the widget test instead, and a comment marks why no golden
exists here.
The sell golden was removed on a premise the repo disproves: whenListen drives
a BlocConsumer through the transition so a listener-triggered SnackBar does
render, which is how the twelve existing _snackbar baselines work. It is back,
built that way.

Pasting 1.000,50 disagreed with typing it: the paste never matched the
thousands rule, so the parser saw 1.000.50 and rejected it, while typing had
already normalised the intermediate 1.000 to 1000. Thousands grouping followed
by one or two decimals of the other separator now resolves atomically.

_expectHealthyQuote also asserts the healthy state is present, not just that
no error is, and the buy golden comment matches the widened rule.
Rendered by golden-regenerate on the self-hosted runner, committed here signed
rather than pushed by the workflow bot.
@joshuakrueger-dfx
joshuakrueger-dfx force-pushed the fix/ambiguous-amount-feedback branch from f1a36fe to ef150a5 Compare August 20, 2026 12:16
The notice named the condition that was rejected, and every widening of the
normalisation made that wording wrong again — most recently 1.000,50, which
carries two separators and is now accepted. Naming the accepted form instead
keeps the text true independently of what the parser tightens or relaxes.
Rendered by golden-regenerate on the self-hosted runner, committed here signed
rather than pushed by the workflow bot.
@joshuakrueger-dfx
joshuakrueger-dfx force-pushed the fix/ambiguous-amount-feedback branch from 0e59dfc to 727396c Compare August 20, 2026 12:24
SellButton.amount comes from a digitsOnly field, so chargedFiatAmount can
never throw on the sell path — the invalidAmountFormat branch there was dead
code, reachable only by injecting the state in a test. Branch, snackbar, its
widget test and its golden are gone; the formatter on the sell fiat field and
the stale-counter clearing stay, they are independent.

The mixed-separator normalisation moved the caret one place too early: the
formatter counted every separator before it while only the thousands one is
removed. It now uses the actual length difference, and the tests assert the
selection, not just the text.

The buy golden fixture used 1.300,75, which the widened rule now normalises
away; it uses a value that still reaches the parser.
The previous caret fix used the global length delta, which also subtracts
separators sitting behind the caret. Typing a digit at the start of .000,50
moved the caret to 0 instead of 1 — the next keystroke would land inside the
number.

The formatter now asks which separator gets stripped and counts only its
occurrences left of the caret.
Every mixed-separator test placed the caret where both separators appear
equally often before it, so returning the decimal separator instead of the
thousands one passed the whole suite. Two cases now type into 1.00,50 and
1,00.50 where the counts differ.

The same/mixed decision also lived twice, once in each public function; it is
derived once now. The repro stub floor and its comment agreed on different
numbers and now name the same one.
@joshuakrueger-dfx

Copy link
Copy Markdown
Collaborator Author

EN:
Ready after 6 review passes.
The amount field resolves a thousands separator instead of refusing it, the leftover rejection names the accepted form instead of pointing at support, and both converter cubits stop leaving a stale counter value behind.

DE:
Bereit nach 6 Review-Durchläufen.
Das Betragsfeld löst einen Tausenderpunkt auf, statt ihn abzulehnen, die verbleibende Ablehnung benennt die akzeptierte Form statt auf den Support zu verweisen, und beide Converter-Cubits lassen keinen veralteten Gegenwert mehr stehen.

Details

Findings per pass — 13 raised across two independent lanes per pass, all resolved or explicitly carried out of scope:

  • Pass 1 — 6: generic error instead of a named one for grouped input; stale counter value on both screens; the same defect in the sell payment-info cubit; missing golden; no newline at the end of the ARB files; twin site in the sell converter.
  • Pass 2 — 4: an emptied field triggered a quote for 0 while the previous quote stayed confirmable; on FormatException wrapped the whole service call, so a malformed backend response read as an unreadable amount; paste and typing disagreed on 1.000.000; the caret jumped to the end.
  • Pass 3 — 3: the sell golden had been removed on a premise the repo disproves (whenListen renders listener-driven snackbars — twelve such baselines exist); a stale comment; a fixture pairing a value the widened rule now normalises with the error state it no longer triggers.
  • Pass 4 — 1: the sell error branch was dead code, unreachable because SellButton.amount comes from a digitsOnly field. Removed.
  • Pass 5 — 3: the caret math used the global length delta and was wrong when a dropped separator sat behind the cursor; a duplicated case distinction; a test stub whose comment and threshold named different numbers.
  • Pass 6 — none.

Two findings were deliberately left for their own PRs, both money-path changes of their own: the window in which a landed quote stays confirmable while a replacement is in flight, and the rounding in chargedFiatAmount (300,75 is charged as 301).

Gates confirmed on 9e8d8fb6: all four checks green (Analyze & Test, Visual Regression, Coverage Floor Gate, BitBox quirks audit), including the three required by the branch ruleset. mergeable: MERGEABLE. No open comments on any of the three channels (issue comments, reviews, inline comments) and no unresolved review threads.

Not exercised in the running app. It builds, installs and launches with this code — the new strings are present in the installed kernel_blob.bin — but the buy screen needs a registered KYC-30 account, so the field behaviour is covered by widget tests driving the real formatter chain rather than by a manual run.

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