Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
jvsena42
left a comment
There was a problem hiding this comment.
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.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
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.
-
✅ 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).
-
✅ 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.
-
✅ passed: Edit Invoice $1,500.00 converted to 2 252 590 sats, appending reached 225 259 099, and the tenth digit was rejected.
-
✅ passed: Send Amount typed 1 to 9 grouped to 123 456 789, the tenth digit was rejected, and nine deletes removed one digit each.
-
✅ passed: Classic 1.57000000 took one character per press and per delete, and fiat typing, delete and the unit switch preserved the amount.
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)
|
@jvsena42 conflicts |
|
Known — master moved under the batch twice this afternoon. #1281 merged at 13:53 and added the 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
left a comment
There was a problem hiding this comment.
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.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jvsena42
left a comment
There was a problem hiding this comment.
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.
|
@jvsena42 ci red and conflicts |
|
Both points are covered centrally in #1275 (comment): the conflict is the shared 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
# Conflicts: # journeys/README.md
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 1 file.
Findings:
N/A
Audit:
Already done in comment.
QA: carried
- ⏭️ skipped: tested on 4a68dcb
- ⏭️ skipped: tested on 70388b6
- ⏭️ skipped: tested on 70388b6
- ⏭️ skipped: tested on 70388b6
- ⏭️ 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)


















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
78 777); only the raw input underneath loses the spaces, so the sats value and the max amount checks are unchanged.Out of Scope
journeys/amount-limits: the existing journeys only type from an empty field and still pass unchanged; the new preset + delete journey is committed atjourneys/amount-limits/transfer-spending-preset-delete.xmland 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.Design
The number pad looks the same; the design does not specify delete or digit cap behaviour.
Transfer to Spending Balance From Savings (Channel Opening) › Transfer to Spending Inputindocs/screens-map.md; its node id was not looked up, so it is not linked.Preview
QA Notes
Manual Tests
Setup: display unit modern Bitcoin (sats), primary display Bitcoin, positive Savings balance.
78 777→7 877→787→78→7→0).225 259 012), rejects the 10th.regression:Send → Send Amount → type digits from an empty field and delete: grouping still shows and the 9-digit cap still applies.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 tojourneys/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 abovemaxAllowedToSend, so a smaller balance would false-fail that step (the passing run had about 315 000 sats available).transfer-spending-preset-delete.xml
Automated Checks
AmountInputViewModelTest.kt: modern BitcoinsetSatsthen append reaches 9 digits;setSatsthen delete removes a digit on every press; fiat → modern Bitcoin switch accepts an appended digit. All three fail with the fix reverted.just compile,just test(2597 tests, 0 failures),just lint(no findings in changed files).