Skip to content

fix: cap number pad entry at available balance across amount screens#585

Open
CypherPoet wants to merge 8 commits into
synonymdev:masterfrom
CypherPoet:fix/amount-screens-cap
Open

fix: cap number pad entry at available balance across amount screens#585
CypherPoet wants to merge 8 commits into
synonymdev:masterfrom
CypherPoet:fix/amount-screens-cap

Conversation

@CypherPoet

@CypherPoet CypherPoet commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Description

Caps number-pad entry at the contextual maximum across the amount-entry screens, so you can no longer type an amount you cannot send. Originally split across three PRs, this work was consolidated here per maintainer request to keep the E2E updates pointed at a single PR (supersedes #584, folds in #586).

  • Send amount (fix: cap send amount pad at available balance #584's commits): introduces maxAmountOverride on AmountInputViewModel and caps the send screen at the available balance.
  • Remaining screens with a contextual maximum get the same cap: LNURL pay, LNURL withdraw, transfer-to-spending, advanced spending, and manual external channel funding.
  • Over-cap deletion: deletions always apply, even when the current amount sits above the cap (for example a prefilled invoice over the available balance), so the amount can be reduced instead of trapping the user.
  • Manual external funding gap: Continue stayed enabled for amounts above the fundable balance; that screen is now gated on the available balance like the others.
  • Refactor (folded in from refactor: AmountInputViewModel to @Observable #586): migrates AmountInputViewModel from ObservableObject to @Observable.

The receive and CJIT amount screens are intentionally left uncapped: a receive amount has no balance ceiling.

Linked Issues/Tasks

Related: #346
Supersedes: #584, #586

Screenshot / Video

pr2-spending-amount-cap.mp4

QA Notes

Manual Tests

  • 1. Send → Amount: type past the available balance: amount freezes at the max.
  • 2. With an amount above the cap (e.g. prefilled from an invoice over the balance): delete still reduces the amount; typing more digits stays blocked.
  • 3. Transfer → Spending Amount: type past Available: amount freezes at the max.
  • 4. Spending Advanced: type past the max LSP balance: capped.
  • 5a. Fund Manual (external node) → Amount: type past Available: capped.
    • 5b. Fund Manual → enter an amount above the fundable balance: Continue is disabled (previously enabled).
  • 6. LNURL Pay → Amount: type past max sendable: capped.
  • 7. LNURL Withdraw → Amount: type past max withdrawable: capped.
  • 8. regression: Receive / CJIT Amount: no upper cap; any amount can still be entered.

Automated Checks

  • BitkitTests/NumberPadTests.swift covers the cap mechanism, including the maxAmountOverride cap and deleting an over-cap amount (testDeleteAllowedWhenAmountAboveCap).
  • NumberPadTests passes locally on the iOS Simulator (25 tests, 0 failures, run against this branch merged with current master).
  • Verified on-device (geoblock-off regtest build): Transfer-to-Spending number pad capped at the max transfer amount (see recording); Receive/CJIT confirmed uncapped (minimum only).
  • CI: standard build/test checks run by the PR bot.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5f439703d6

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread changelog.d/next/585.fixed.md Outdated
The send number pad now rejects keystrokes that would push the amount above the available sendable balance, reusing the existing over-max block (haptic + error flash) via a dynamic maxAmountOverride. Continue-button validation is unchanged as a backstop.

Closes synonymdev#346
@CypherPoet CypherPoet force-pushed the fix/amount-screens-cap branch from b96b812 to 6af6d68 Compare June 6, 2026 09:13
The cap rejected every keystroke whose result still exceeded it, including deletions. When an amount lands above the cap (a prefilled invoice over the available balance, or a cap that dropped after input), the user could not backspace to reduce it, since each intermediate value was still over the cap. Deletions now always apply; the cap only blocks growing the amount.
Applies the maxAmountOverride cap (from the send-amount fix) to the LNURL pay/withdraw, spending, advanced-spending, and manual channel-funding screens, so the number pad refuses entry above each screen's contextual maximum. FundManualAmountView also gains the previously-missing upper-bound button validation, which let users proceed above the fundable balance.

Receive and CJIT amount screens are intentionally left uncapped (a receive amount has no balance ceiling).
@jvsena42

jvsena42 commented Jun 9, 2026

Copy link
Copy Markdown
Member

Please, if possible, consolidate both PRs into a single one like synonymdev/bitkit-android#908 to facilitate the E2E updates

cc @piotr-iohk

@ovitrif ovitrif requested review from jvsena42 and piotr-iohk June 9, 2026 15:24
@ovitrif ovitrif added this to the 2.4.0 milestone Jun 9, 2026
@ovitrif

ovitrif commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Updated PR description by replacing -> with , posted this for transparency.

ovitrif and others added 2 commits June 9, 2026 17:25
Now that the send-amount cap (synonymdev#584) and the remaining-screens cap live in a single PR, merge the two Fixed fragments into one entry.
@CypherPoet

CypherPoet commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Please, if possible, consolidate both PRs into a single one like synonymdev/bitkit-android#908 to facilitate the E2E updates

Done. I've consolidated the changes for #584 into this PR and merged the two changelog fragments into one. I also closed #584 in favor of this one.

@jvsena42 One question: I still have the @Observable refactor of AmountInputViewModel as
separate follow-up in #586. Do you want that folded in here, too? The main reason I originally separated stuff was to err on the side of caution regarding too many changes at once... but I'm happy to do either 🙂

@CypherPoet CypherPoet changed the title fix: cap number pad on remaining amount screens fix: cap number pad entry at available balance across amount screens Jun 9, 2026
@ovitrif

ovitrif commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

One question: I still have the @Observable refactor of AmountInputViewModel as separate follow-up in #586. Do you want that folded in here, too? The main reason I originally separated stuff was to err on the side of caution regarding too many changes at once...

That would still keep PR under +500 -500 edits, it should be fine from our team's perspective. We're currently still working with PRs of ±2500 LoC's changes ;).

Replaces the legacy ObservableObject/@StateObject/@ObservedObject pattern with @observable + @State, aligning with the project's stated SwiftUI direction. No behavior change: there are no two-way bindings to the view model, so no @bindable is needed. Touches the view model, NumberPadTextField, and the 8 amount-entry call sites.
@CypherPoet

CypherPoet commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

That would still keep PR under +500 -500 edits, it should be fine from our team's perspective. We're currently still working with PRs of ±2500 LoC's changes ;).

@ovitrif Ha!... fair enough. Folded the @Observable refactor into this PR (last commit). Final
size is +151/-16 across 12 files. Also closed #586

@jvsena42

Copy link
Copy Markdown
Member

Please, if possible, consolidate both PRs into a single one like synonymdev/bitkit-android#908 to facilitate the E2E updates

Done. I've consolidated the changes for #584 into this PR and merged the two changelog fragments into one. I also closed #584 in favor of this one.

@jvsena42 One question: I still have the @Observable refactor of AmountInputViewModel as separate follow-up in #586. Do you want that folded in here, too? The main reason I originally separated stuff was to err on the side of caution regarding too many changes at once... but I'm happy to do either 🙂

Thanks! I asked for this case just to facilitate the E2E tests updates, because the Android changes were implemented in a single PR

@piotr-iohk

Copy link
Copy Markdown
Collaborator

The android counter part PR (synonymdev/bitkit-android#908) implements an error toast (identified by test id: SendAmountExceededToast) in case when user tries to tap amount exceeding balance. For consistency between apps and e2e tests we need to have the same behavior here on bitkit-ios side as well - using the same accessibilityIdentifier = SendAmountExceededToast for the toast, so e2e tests can utilize it.

See (ios left / android right):

Screen.Recording.2026-06-10.at.12.41.37.mov

@piotr-iohk piotr-iohk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

See: #585 (comment) 🙏

Comment on lines +81 to +84
.onAppear {
updateInputCap()
}
.onChange(of: wallet.channelFundableBalanceSats) { updateInputCap() }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

replace with

.onChange(of: wallet.channelFundableBalanceSats, initial: true) { _, _ in
    updateInputCap()
}

Comment on lines +84 to +87
.onAppear {
updateInputCap()
}
.onChange(of: maxAmount) { updateInputCap() }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

4 participants