Skip to content

fix: strip sats separators from raw input - #1285

Open
jvsena42 wants to merge 6 commits into
masterfrom
fix/558-strip-sats-separator-raw-input
Open

jvsena42 wants to merge 6 commits into
masterfrom
fix/558-strip-sats-separator-raw-input

Conversation

@jvsena42

@jvsena42 jvsena42 commented Sep 16, 2026

Copy link
Copy Markdown
Member

Fixes #558
Related to #1275

This PR strips the sats grouping spaces from the number pad's raw input, so preset amounts and unit switches no longer block digits or waste delete presses.

Description

  • Fixes the sats number pad ignoring a delete press on each grouping space because the raw input kept the spaces after a preset amount (25%, MAX) filled the field.
  • Fixes the sats number pad stopping at 8 digits after a preset amount or a fiat-to-Bitcoin unit switch because the spaces counted toward the input length cap.
  • Keeps the displayed amount grouped (e.g. 78 777); only the raw input underneath loses the spaces, so the sats value and the max amount checks are unchanged.
  • Adds a changelog fragment for the fix.

Out of Scope

  • Fiat and classic Bitcoin input: unchanged, classic display has no grouping and fiat already strips its separators.
  • journeys/amount-limits: the existing journeys only type from an empty field and still pass unchanged; the new preset + delete journey is committed at journeys/amount-limits/transfer-spending-preset-delete.xml and shown inline in QA Notes.
  • journeys/amount-limits/transfer-spending-over-max.xml: its opening steps look stale ("Spending card → Transfer from Savings" is now "Savings card → Transfer To Spending"); left for a separate change.
  • iOS parity: already fixed in fix: various fixes bitkit-ios#289, which this PR ports site for site.

Design

The number pad looks the same; the design does not specify delete or digit cap behaviour.

  • Send Amount: Send (Paste) (On-chain) › Set amount
  • Spending Amount maps to Transfer to Spending Balance From Savings (Channel Opening) › Transfer to Spending Input in docs/screens-map.md; its node id was not looked up, so it is not linked.

Preview

Before After
Before: 25% preset, 78 777; the 4th delete press did nothing After: 25% preset, 78 777; each delete removes one digit
MAX preset over cap Fiat → Bitcoin, append 9 digits after unit switch
After: MAX, delete, over-cap digit shows toast and clamps After: $10 switched to Bitcoin (15 017), append 1: 150 171 After: Edit Invoice $1,500 to Bitcoin, reaches 225 259 012

QA Notes

Manual Tests

Setup: display unit modern Bitcoin (sats), primary display Bitcoin, positive Savings balance.

  • 1a. Savings → Transfer To Spending → Spending Amount → tap 25% → tap delete until 0: every press removes one digit (78 7777 8777877870).
    • 1b. Tap MAX → delete → tap a digit: digit is accepted; a digit over the cap shows the Spending Balance Maximum toast and clamps.
  • 2. Spending Amount → toggle unit to fiat → type 10 → toggle back to Bitcoin → tap 1 → delete until 0: the 1 is appended and no delete press is wasted.
  • 3. Receive → Edit Invoice → type $1,500 in fiat → toggle to Bitcoin → append digits: accepts up to 9 digits (e.g. 225 259 012), rejects the 10th.
  • 4. regression: Send → Send Amount → type digits from an empty field and delete: grouping still shows and the 9-digit cap still applies.
  • 5. regression: Classic Bitcoin and fiat input: typing, delete and unit switch behave as before.

Steps 1a and 2 are the journey below, which passed on the emulator against this branch; before the fix both wasted a delete press on each space. It is committed at journeys/amount-limits/transfer-spending-preset-delete.xml, with its row added to journeys/README.md (amount-limits 4 → 5) and a cross-platform row noting it is not ported to iOS yet. Its precondition asks for at least 250 000 sats of Savings: the fiat round trip plus the appended digit puts about 100 001 sats in the pad, and the pad rejects anything above maxAllowedToSend, so a smaller balance would false-fail that step (the passing run had about 315 000 sats available).

transfer-spending-preset-delete.xml
diff --git a/journeys/amount-limits/transfer-spending-preset-delete.xml b/journeys/amount-limits/transfer-spending-preset-delete.xml
new file mode 100644
index 000000000..cda616526
--- /dev/null
+++ b/journeys/amount-limits/transfer-spending-preset-delete.xml
@@ -0,0 +1,47 @@
+<journey name="transfer to spending preset amount deletes one digit per press">
+  <description>
+    Verifies that after the "Transfer to Spending" amount is filled by a preset (25% or MAX) or by
+    switching the number pad unit from fiat, every delete press removes exactly one digit and
+    appended digits are accepted. The displayed amount keeps its sats grouping spaces, but the
+    number pad input underneath does not, so no press is spent on a space (synonymdev/bitkit-android#558).
+
+    Precondition: onboarded dev wallet with the display unit set to modern Bitcoin (sats), primary
+    display Bitcoin, a POSITIVE on-chain Savings balance of at least 250 000 sats, LSP limits above
+    that, and a running node connected to the LSP. Start on the wallet home screen. No funds move:
+    the journey never taps Continue.
+
+    Why 250 000: the number pad rejects any sats input above its max
+    (AmountInputViewModel.handleNumberPadInput, the guard comparing the new amount to maxAmount),
+    and that max is `maxAllowedToSend` — the Savings balance less the quoted LSP order fee, also
+    capped by the LSP's own max client balance (TransferViewModel, SpendingAmountScreen's
+    `setMaxAmount`). It is the value shown by "SpendingAmountUnit". Entering 10 fiat units and
+    switching back to sats gives about 10 000 sats at a BTC price near 100 000 fiat units, and the
+    append in the step below makes that about 100 001, so a smaller balance has the digit dropped
+    and the max toast shown instead, which false-fails the step. The guard in the fiat-entry step
+    below keeps that true if the price moves far enough for 10 fiat units to be worth more than a
+    tenth of the max.
+
+    iOS counterpart: the same fix shipped in synonymdev/bitkit-ios#289, but the journey is not
+    ported to the iOS corpus yet.
+  </description>
+  <actions>
+    <action>Tap the Savings balance card (testTag "ActivitySavings") on the home screen</action>
+    <action>Tap "Transfer To Spending" (testTag "TransferToSpending")</action>
+    <action>If the spending intro screen appears, tap "Get Started" (testTag "SpendingIntro-button")</action>
+    <action>Verify the spending amount screen (testTag "SpendingAmount") is visible</action>
+    <action>Wait until the available amount (testTag "SpendingAmountUnit") finishes loading and shows a positive value</action>
+    <action>Tap the 25% button (testTag "SpendingAmountQuarter")</action>
+    <action>Verify the amount in the input field (testTag "SpendingAmountNumberField") shows a grouped value with a space, e.g. "78 777"</action>
+    <action>Tap the delete key (testTag "NRemove") repeatedly until the amount is 0, reading the amount after each press</action>
+    <action>Verify every delete press changed the amount, e.g. "78 777" → "7 877" → "787" → "78" → "7" → "0", with no press leaving the amount unchanged</action>
+    <action>Tap the number pad unit toggle (testTag "SpendingNumberPadUnit") so it shows the fiat currency</action>
+    <action>Tap "1" (testTag "N1") and "0" (testTag "N0") to enter 10 fiat units; if 10 fiat units are worth more than a tenth of the amount shown in "SpendingAmountUnit", tap only "1" (testTag "N1") instead, so the digit appended below still fits under that amount</action>
+    <action>Tap the number pad unit toggle (testTag "SpendingNumberPadUnit") so it shows "BITCOIN"</action>
+    <action>Verify the amount in the input field is a grouped sats value of at least 4 digits</action>
+    <action>Tap "1" (testTag "N1")</action>
+    <action>Verify the amount gained exactly one trailing digit "1", is not over the amount shown in "SpendingAmountUnit", and that no "Spending Balance Maximum" warning toast appeared</action>
+    <action>Tap the delete key (testTag "NRemove") repeatedly until the amount is 0, reading the amount after each press</action>
+    <action>Verify every delete press changed the amount</action>
+    <action>Tap the back button (testTag "NavigationBack") to leave without transferring</action>
+  </actions>
+</journey>

Automated Checks

  • Unit tests added in AmountInputViewModelTest.kt: modern Bitcoin setSats then append reaches 9 digits; setSats then delete removes a digit on every press; fiat → modern Bitcoin switch accepts an appended digit. All three fail with the fix reverted.
  • Ran locally: just compile, just test (2597 tests, 0 failures), just lint (no findings in changed files).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jvsena42 jvsena42 self-assigned this Sep 16, 2026
@greptile-apps

greptile-apps Bot commented Sep 16, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

The PR appears safe to merge; the normalization is correctly limited to display-only sats grouping and preserves classic Bitcoin and fiat behavior.

Summary

This PR normalizes modern-Bitcoin keypad state by removing display-only sats grouping spaces from raw input while preserving grouped display text.

  • Applies normalization after preset amounts and unit switches.
  • Reuses the normalization helper during sats conversion.
  • Adds focused regression tests for appending and deleting after presets and unit switches.
  • Adds a changelog fragment describing the fix.

Reviews (1) · Last reviewed commit: "fix: strip sats separators from raw inpu..."

@github-actions

github-actions Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Regtest APK

Built from 70fe7a1 (run).

Download bitkit-dev-debug universal APK (expires in 30 days).

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved — no HIGH/MEDIUM findings at head 4a68dcbf3. (Posted as a comment because GitHub does not allow approving your own PR.)

Checked: Traced AmountInputViewModel at the PR head end to end: setSats (L137-152), switchUnit both branches (L157-201), handleNumberPadInput integer and decimal paths (L68-135), handleIntegerInput/handleDecimalInput, and convertBitcoinToSats/convertFiatToSats. stripSatsGrouping removes exactly SATS_GROUPING_SEPARATOR (' ', models/Currency.kt L17), which is the only separator formatToModernDisplay can emit because it overrides groupingSeparator on DecimalFormatSymbols(locale) and the #,### pattern fixes the group size, so the strip is locale-independent and cannot remove a digit or a decimal point; classic display disables grouping and fiat text is still stripped with replace(",", ""), so those paths are unchanged. Displayed text is always re-derived from the parsed sats (formatDisplayTextFromAmount) in modern mode, and sats is recomputed from the stripped raw text on every key, so the parsed amount cannot diverge from the shown amount after a preset (25%/MAX/min/default across Send, Spending, SpendingHw, External, Receive, EditInvoice, SpendingAdvanced, CreatePaymentRequest callers) or after a fiat->bitcoin toggle; the amountSats > 0 toggle branch keeps sats and only rewrites text/raw, and the sats == 0 branch can only produce 0 from 0, 0., 0.00. Max-amount enforcement is intact: newAmount <= maxAmount still runs on the stripped digits, delete is always allowed, and the 10-char raw cap now permits a 10-digit string only to have it rejected by MAX_AMOUNT (999,999,999), which matches the from-empty typing path on master. The three new tests exercise the exact pre-fix failures (raw containing spaces after setSats/switch) and would fail on master. Non-ASCII locale digits (fa/ar-EG/bn) are parsed by toLongOrNull/BigDecimal via Character.digit, pre-existing and unaffected. iOS twin is synonymdev/bitkit-ios#289 as stated in the body.

@jvsena42
jvsena42 requested a review from ovitrif September 18, 2026 12:26

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM

@jvsena42
jvsena42 enabled auto-merge September 18, 2026 12:28
ovi-reviewer[bot]

This comment was marked as resolved.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ✅ Approve


Review: diff 3 files.

Findings:
N/A

Audit:
Already done in comment.

QA: emu-1 Pixel 10 Pro emulator on Android 16 (API 36), tested on 70388b6

6/6 Manual Tests passed.

  1. passed: 25% filled 124 221 and every delete press removed one digit down to 0; MAX then delete then 9 clamped back to 496 884 and raised the Spending Balance Maximum toast at t=0-2 s (1a and 1b).

    Test 1:
    **1a.** Savings → Transfer To Spending → Spending Amount → tap 25% → tap delete until 0: every press removes…
    1.mp4
  2. passed: $10.00 converted to 15 017 sats, tapping 1 appended to 150 171, and six deletes reached 0 with no press wasted on a space.

    Test 2:
    Spending Amount → toggle unit to fiat → type 10 → toggle back to Bitcoin → tap 1 → delete until 0: the 1 is…
    2.mp4
  3. passed: Edit Invoice $1,500.00 converted to 2 252 590 sats, appending reached 225 259 099, and the tenth digit was rejected.

    Test 3:
    Receive → Edit Invoice → type $1,500 in fiat → toggle to Bitcoin → append digits: accepts up to 9 digits…
    3.mp4
  4. passed: Send Amount typed 1 to 9 grouped to 123 456 789, the tenth digit was rejected, and nine deletes removed one digit each.

    Test 4:
    `regression:` Send → Send Amount → type digits from an empty field and delete: grouping still shows and the…
    4.mp4
  5. passed: Classic 1.57000000 took one character per press and per delete, and fiat typing, delete and the unit switch preserved the amount.

    Test 5:
    `regression:` Classic Bitcoin and fiat input: typing, delete and unit switch behave as before.
    5.mp4

Tip

Test 1 worth a journey:

  • Fund the regtest wallet so Savings holds at least 40 000 sats
  • Open the wallet home screen and tap the Savings balance card
  • Tap Transfer To Spending, then Get Started on the intro screen
  • Wait until the available amount under the field shows a positive value
  • Tap 25% and read the grouped amount in the input field
  • Tap delete repeatedly until the amount is 0, reading the amount after each press
  • Verify every press removed exactly one digit
  • Tap MAX, tap delete once, then tap the digit that restores the maximum
  • Verify that digit is accepted and the amount returns to the maximum
  • Tap delete once more and tap a digit that pushes the amount over the maximum
  • Verify the Spending Balance Maximum toast appears and the amount clamps to the maximum
  • Tap back to leave without transferring

Test 2 worth a journey:

  • Open Transfer To Spending and reach the Spending Amount screen
  • Tap delete until the amount reads 0
  • Tap the number pad unit toggle so it shows the fiat currency
  • Tap 1 then 0 to enter ten units of fiat
  • Tap the unit toggle again so it shows Bitcoin
  • Read the converted sats amount in the field
  • Tap 1 and verify exactly one trailing digit was appended
  • Tap delete until the amount is 0 and verify every press removed one digit

Test 3 worth a journey:

  • Tap Receive on the wallet home screen
  • Tap Edit to open the Edit Invoice screen
  • Tap the amount field to open the number pad
  • Tap the unit toggle so it shows the fiat currency
  • Type 1500 and verify the field shows fifteen hundred dollars
  • Tap the unit toggle so it shows Bitcoin and read the sats amount
  • Append digits one at a time until the field stops changing
  • Verify the field accepts nine digits and rejects the tenth

Test 4 worth a journey:

  • Open the Send flow with an on-chain address so the amount screen appears
  • Confirm the amount field is empty and the unit shows Bitcoin
  • Type the digits 1 to 9 one at a time, reading the field after each press
  • Verify grouping spaces appear from the fourth digit onwards
  • Type one more digit and verify the field does not change
  • Tap delete until the amount is 0 and verify every press removed one digit

Test 5 worth a journey:

  • Open Settings, General, Default Unit and select the Classic denomination
  • Open the Send flow with an on-chain address so the amount screen appears
  • Type a digit, the decimal key and two more digits, reading the field after each press
  • Verify each accepted press adds exactly one character
  • Tap delete repeatedly and verify each press removes exactly one character
  • Tap the unit toggle to the fiat currency and type two digits
  • Tap delete once and verify one digit was removed
  • Toggle back to Bitcoin and verify the amount is preserved
  • Restore the Modern denomination in Settings

Coverage:
QA: 6 of 6 manual tests passed


Reviewed by claude-opus-5-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)

@ovitrif

ovitrif commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

@jvsena42 conflicts

@jvsena42

Copy link
Copy Markdown
Member Author

Known — master moved under the batch twice this afternoon. #1281 merged at 13:53 and added the home journey suite plus a cross-platform row to journeys/README.md, which is the same file every PR in this set touches, so the same one-line table collision reappears on each of them.

A merge of current master is running for the remaining six (#1123, #1285, #1288, #1289, #1291, #1295); #1289 is already back to mergeable. Resolution keeps all of master rows and the branch own row, and the merges are validated with compile, unit tests and detekt before pushing. Nothing is rebased or force-pushed.

…separator-raw-input

# Conflicts:
#	journeys/README.md

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved — no blocking HIGH/MEDIUM findings at head 5c9a7f1ad. (Posted as a comment because GitHub does not allow approving your own PR.)

Inline: LOW — 40 000 sats precondition is too small for the append step after 10 fiat units

Checked: Follow-up: Follow-up on 70388b6 (new journey transfer-spending-preset-delete.xml). Every testTag it names exists at head: ActivitySavings (HomeScreen.kt:738), TransferToSpending (SavingsWalletScreen.kt:168), SpendingIntro-button, SpendingAmount, SpendingAmountUnit, SpendingAmountQuarter, SpendingAmountNumberField, SpendingNumberPadUnit (SpendingAmountScreen.kt), NRemove and N (NumberPad.kt:175, :240), NavigationBack. The 25% step goes through setSats, so with the stripSatsGrouping change reverted rawInputText keeps the space and the delete run stalls on the press that removes the digit before it (e.g. "78 " -> "78" leaves the display at 78), which the per-press check catches. The fiat round trip goes through switchUnit's amountSats > 0 branch, which also keeps the space on revert and stalls the second delete run the same way. One LOW on the precondition.

Comment thread journeys/amount-limits/transfer-spending-preset-delete.xml Outdated
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved — no blocking HIGH/MEDIUM findings at head 08eafe9d9. (Posted as a comment because GitHub does not allow approving your own PR.)

Checked: Follow-up on 08eafe9: the balance-precondition LOW is fixed. The journey now asks for 250 000 sats of Savings with LSP limits above it, explains the maxAllowedToSend guard it has to stay under, falls back to entering 1 fiat unit when 10 would exceed a tenth of the max, and fails on the "Spending Balance Maximum" toast instead of silently accepting a dropped digit.

@ovitrif

ovitrif commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

@jvsena42 ci red and conflicts

ovitrif

This comment was marked as resolved.

@jvsena42

Copy link
Copy Markdown
Member Author

Both points are covered centrally in #1275 (comment): the conflict is the shared journeys/README.md row that every PR in this batch touches, and a merge of current master is running across all of them; the red e2e shards failed in setup (snapshot load, adb exit 1, app never reaching Home), not in an assertion.

I will confirm the result on this PR once the merge lands and CI re-runs on the new head.

…separator-raw-input

# Conflicts:
#	journeys/README.md
ovi-reviewer[bot]

This comment was marked as resolved.

ovi-reviewer[bot]

This comment was marked as resolved.

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: ✅ Approve


Reaudit: diff 1 file.

Findings:
N/A

Audit:
Already done in comment.

QA: carried

  1. ⏭️ skipped: tested on 4a68dcb
  2. ⏭️ skipped: tested on 70388b6
  3. ⏭️ skipped: tested on 70388b6
  4. ⏭️ skipped: tested on 70388b6
  5. ⏭️ skipped: tested on 70388b6

Coverage:
QA: 5 of 5 manual tests passed


Reviewed by gpt-5.6-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)

@jvsena42
jvsena42 requested a review from ovitrif September 18, 2026 20:19
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.

number pad fix

2 participants