[coreyphillips]

Fixes #1144
Closes #1765

This PR keeps max on-chain sends in step with the fee speed and recipient they are sent at, so the amount shown on the confirm screen is the amount that goes out.

Description

• Recomputes the max sendable amount for the actual recipient at the selected fee speed whenever the on-chain send is refreshed (continue from amount entry, speed change, coin selection, funding source switch).
• When the entered amount reaches that max, lowers the amount to it and flags the send as a max send, so the confirmed figure matches what the send delivers. Below the max the send stays an exact-amount send.
• Passes the same fee rate snapshot to both the max estimate and the send, so the two can no longer be built from different fee tables.
• Falls back to the cached wallet max only when the estimate is unavailable, and leaves hardware wallet sends untouched since their available amount is tracked separately.
• Adds a repository helper that derives the max from the spendable balance minus the send-all fee for a given address, speed and fee rates.
• Merges latest master, folding the max recheck into the cancellable on-chain refresh job introduced there.

Preview

N/A, the only visible change is the confirm amount updating when the fee speed changes.

QA Notes

Manual Tests

• 1a. Send → Amount → tap Max → Confirm → switch speed to Fast: displayed amount lowers to the max at Fast and the send completes with no on-chain balance left behind.
  • 1b. Same flow → switch speed to Slow: displayed amount stays as confirmed and exactly that amount is delivered.
• 2. Send → Amount → tap Max → Confirm with a Taproot recipient (bcrt1p...) at the default speed: send completes with no on-chain balance left behind.
• 3. regression: Send → Amount below max → Confirm → change speed: amount is unchanged and exactly that amount is delivered.
• 4. regression: Hardware wallet funding source → Amount → tap Max → Confirm → change speed: amount follows the hardware wallet's available balance.
• 5. regression: Send → Amount → Coin Selection → Continue → Confirm: fee and amount still refresh.

Automated Checks

• Unit tests added: cover a speed change lowering a max send to the max at the new speed, exact sends below the max, and the cached-max fallback when the estimate fails in AppViewModelSendFlowTest.kt.
• Unit tests added: cover the send-all fee subtraction and the zero spendable balance shortcut in LightningRepoTest.kt.
• Local verification: just compile, just test, just lint.

[greptile-apps]

Confidence Score: 5/5

[Critical impact] The PR appears safe to merge; no new blocking issue was found.

Summary

Keeps max on-chain sends tied to the recipient and selected fee speed.

• Max on-chain sends match the recipient and fee speed selected for the send.

No new actionable issues were found. No previous review threads were supplied. Tests were inspected but not run.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Amount, speed, or selected coins change] --> B[Estimate maximum for recipient and speed]
  B --> C[Update amount and max flag]
  C --> D[Refresh fees]
  D --> E[Enable confirmation swipe]
  E --> F[Sync wallet before sending]
  F --> G{Confirmed amount still equals maximum?}
  G -->|Yes, and marked max| H[Prepare drain]
  G -->|No| I[Prepare exact-amount send]
  H --> J[Retain receipt and broadcast]
  I --> J

Loading

_{Reviews (2) · Last reviewed commit: "chore: merge master into spar/issue-1144" · Reviewed by Greptile}

[coreyphillips]

Review by codex (round 1): The approach is sound, but the actual drain can still use a different fee-rate snapshot than the new max validation. Existing test results show 1,919 tests passing, including both affected suites. A fresh targeted run was blocked by the sandbox denying the Gradle cache lock.

• [blocking] Use the same fee rates for validation and sending (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:2882)
  At line 2901, shouldDrainOnchain validates the amount using state.feeRates, but the sendOnChain call at line 2882 does not pass those rates. It therefore receives feeRates = null, and LightningRepo.kt:1333 fetches a fresh fee table. If rates changed, the guard can approve draining at one rate and the transaction can drain at another, reproducing the confirmed-amount mismatch. I confirmed this through the production call path to BlocktankService.getFees, which refreshes rates. Snapshot the send state and pass the same speed and feeRates to both operations, then assert this in the send-flow test.

[coreyphillips]

Applied the review's blocking fix: sendOnchain now snapshots the send state once and passes the same speed and feeRates to both estimateMaxSendOnchain (the drain guard) and sendOnChain, so the guard and the transaction can no longer be built from different fee tables. Updated the send-flow test to use a non-null FeeRates and assert both calls receive it. Targeted unit tests and detekt pass; committed as e00b553, not pushed.

• Fixed: Use the same fee rates for validation and sending

[coreyphillips]

Machine state for resuming this PR. Safe to ignore.

{
  "version": 1,
  "round": 1,
  "next_actor": "claude",
  "status": "pending",
  "ledger": {},
  "filed": []
}

[coreyphillips]

Review by claude (round 2): The PR mirrors the iOS approach: recompute the max sendable amount at confirm time with the selected speed/fee rates and fall back to an exact-amount send when the cached max no longer matches. Direction is right, plumbing state.feeRates into both the drain check and sendOnChain is a good touch, and the new unit tests pass (./gradlew testDevDebugUnitTest --tests "*AppViewModelSendFlowTest*" --tests "*LightningRepoTest*" -> BUILD SUCCESSFUL). But the equality check compares two estimates that are not computed against the same address (and sometimes not against a real estimate at all), so it degrades to an exact-amount send in cases where the speed never changed, and the degraded send is arithmetically guaranteed to be unfundable.

• [blocking] Drain check compares estimates made against different addresses, so ordinary max sends degrade to a send that cannot be funded (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt)
  The cached max is DeriveBalanceStateUseCase.getMaxSendAmount (line 214-228), which calls lightningRepo.estimateSendAllFee(speed = speed) with address = null. LightningRepo.estimateSendAllFee (line 1479) then resolves address ?: cacheStore.data.first().onchainAddress, i.e. the wallet's OWN next receive address. The new shouldDrainOnchain (AppViewModel.kt:2899-2903) recomputes via estimateMaxSendOnchain(address = address, ...) with the DESTINATION address. LightningService.estimateSendAllFee (line 1210-1222) forwards the address to node.onchainPayment().calculateSendAllFee(address, retainReserves, feeRate), whose result depends on the output script length. A P2WPKH output is 31 bytes vs 43 for P2TR, so sending max to a bc1p address from a P2WPKH wallet yields feeDest = feeOwn + ~12 vB * rate even when the selected speed IS the default speed. The equality amount != maxAtSelectedSpeed then holds, drain is disabled, and the code sends amount == cachedMax non-max. That send needs cachedMax + feeDest (plus a change output, ~31 more vB) while only spendable == cachedMax + feeOwn exists, so it is short by construction and LDK must fail with insufficient funds. Same failure, much larger, whenever the cached max fell back to Defaults.fallbackFeePercent (Env.kt:251 = 0.1, i.e. 10% of spendable) because the estimate errored: the recomputed max will never match, so every max send degrades and fails. Net effect: a plain max send that worked on master now errors for common address-type combinations. Confirmed by reading the exact call sites above; the new AppViewModel test max onchain send falls back to exact amount when selected speed changes the max already asserts precisely this degrade behavior (isMaxAmount=false with sats = cachedMax). I did not run against a real LDK node. Suggested direction: recompute BOTH sides against the same destination address (compare max-at-selected-speed vs max-at-the-speed the cached value used), or treat amount >= maxAtSelectedSpeed as drain-eligible and only degrade when the selected rate is genuinely cheaper, rather than exact equality against a differently-parameterized estimate.
• [non-blocking] Degrade path has no user-facing handling when the amount exceeds the max at the selected speed (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt)
  When the selected speed really is more expensive than the default (the exact scenario in issue Send-max computed at the default fee speed, but the drain happens at the selected speed #1144), shouldDrainOnchain correctly refuses to drain, but nothing clamps the amount or tells the user. The send then fails deep in LDK and surfaces as a generic send error with no explanation that the faster fee no longer fits the confirmed amount. Recomputing the max on setTransactionSpeed (AppViewModel.kt:1640-1668) and updating/invalidating the amount on the confirm screen, or showing a targeted error, would make the fix visible rather than just non-destructive.
• [non-blocking] [out of scope] estimateMaxSendOnchain ignores selectedUtxos, so the check is meaningless under coin control (app/src/main/java/to/bitkit/repositories/LightningRepo.kt)
  estimateMaxSendOnchain derives from getBalancesAsync().spendableOnchainBalanceSats, i.e. the full spendable balance, while the send may be restricted to state.selectedUtxos. With manual coin selection the recomputed max describes a different UTXO set than the send, so the equality result is effectively arbitrary (it will almost always mismatch and degrade). The pre-existing cached max has the same limitation, so this is not a regression, but the new confirm-time check inherits it.
• [non-blocking] Extra LDK round trip on every max-send confirm (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt)
  shouldDrainOnchain runs estimateMaxSendOnchain (balances + calculateSendAllFee on the LDK service queue) synchronously inside the confirm path, after the user has already swiped to pay. It is only paid on the max-amount path, so the cost is bounded, but it adds latency at the most sensitive moment; caching the fee rate that produced the displayed max (the issue's alternative suggestion) would avoid the extra call entirely.
• [nit] Log wording (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt)
  Logger.info("Sending exact amount '$amount' instead of draining, ...") is clear, but the Logger.warn above it logs the failure that estimateSendAllFee/getFeeRateForSpeed already logged internally (LightningRepo.kt:1507-1511), which the repo guidance discourages as duplicate error logging.

[coreyphillips]

Review by claude (round 2): The PR stops the wallet from draining at a fee speed the confirmed amount didn't account for by recomputing the max at the selected speed/rates and only setting isMaxAmount when it still matches exactly, and it now threads state.feeRates into sendOnChain so the check and the send use the same rates. The repo helper is clean and the new unit tests pass (verified: ./gradlew :app:testDevDebugUnitTest --tests '*AppViewModelSendFlowTest' --tests '*LightningRepoTest' -> BUILD SUCCESSFUL). The problem is the fallback: when the recomputed max is lower than the confirmed amount, the code sends the stale exact amount, which cannot fund itself, so the send fails instead of sending the wrong amount. That converts the issue's exact scenario (max send, then pick Fast) from 'sends less than displayed' into 'send always errors', and the address asymmetry makes it fire even without a speed change. The guard direction is right; the fallback needs rethinking (recompute/refresh the displayed amount, or drain when the recomputed max is <= the confirmed amount).

• [blocking] Exact-amount fallback is arithmetically guaranteed to fail when the selected speed costs more (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt)
  shouldDrainOnchain returns false when maxAtSelectedSpeed != amount, and sendOnChain then takes the non-max path: LightningService.send -> node.onchainPayment().sendToAddress(amountSats = sats, ...) (LightningService.kt:940), where the mining fee is paid on top of amountSats. The confirmed amount is the cached max, i.e. spendable - feeAtDefaultSpeed (DeriveBalanceStateUseCase.getMaxSendAmount, lines 214-228). Funding that send at the selected speed requires amount + feeAtSelectedSpeed <= spendable, i.e. feeAtSelectedSpeed <= feeAtDefaultSpeed. Whenever the user picks a faster-than-default speed (the exact case in Send-max computed at the default fee speed, but the drain happens at the selected speed #1144), that is false and ldk-node returns InsufficientFunds. So the flow the issue describes now reliably errors at swipe-to-pay, and nothing in the UI lowers the amount for the user (setTransactionSpeed at AppViewModel.kt:1641 leaves state.amount untouched; validateAmount at :1794 still checks against the cached max). Confirmed by reading the code paths and the arithmetic; I did not run against a live node, so the only assumption is that sendToAddress errors rather than shaving the output, which is why sendAllToAddress exists as a separate API. Suggested direction: if maxAtSelectedSpeed < amount, either drain at the selected speed after updating the confirmed amount (and re-confirming), or recompute state.amount when the speed changes so the confirmed figure always matches the speed it will be sent at.
• [blocking] Drain check compares against a max computed for a different address, so it mismatches even at the default speed (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt)
  The cached maxSendOnchainSats comes from lightningRepo.estimateSendAllFee(speed = defaultSpeed) with address = null (DeriveBalanceStateUseCase.kt:221), which LightningRepo.estimateSendAllFee resolves to cacheStore.data.first().onchainAddress, the wallet's own receive address (LightningRepo.kt:1479). shouldDrainOnchain instead passes the destination address (AppViewModel.kt:2900). node.onchainPayment().calculateSendAllFee(address = ...) builds the tx against that address, so the send-all fee differs with the output script type (e.g. P2WPKH 31 vB vs P2TR 43 vB, ~12 vB, times the rate). Result: for a max send to an address whose type differs from the wallet's own, the strict equality check fails even when the user never touched the speed, drain is disabled, and the send then hits the same insufficient-funds path as the finding above. Same class of false mismatch arises from the rate source asymmetry (cached max resolves rates via coreService.blocktank.getFees(); the check uses the state.feeRates snapshot taken in resetSendState) and from any balance change between balance derivation and confirm. Exact equality across two differently-parameterised estimates is too brittle to gate a drain on.
• [non-blocking] Extra LDK round-trip added to the swipe-to-pay path (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt)
  shouldDrainOnchain runs getBalancesAsync plus calculateSendAllFee synchronously between the user's confirmation and the send, on every max on-chain send. It is bounded work, but it lands on the most latency-sensitive step of the flow, after the swipe. If the guard survives in some form, consider computing it during refreshOnchainSendIfNeeded (where the fee estimates are already refreshed on speed change) and caching the result in SendUiState, so confirm-time work stays a comparison.
• [non-blocking] [out of scope] Confirm screen never refreshes the max amount when the speed changes (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt)
  setTransactionSpeed (AppViewModel.kt:1641) refreshes fee estimates and UTXO selection but leaves state.amount at the value computed from the default-speed max, and validateAmount (:1794) only ever compares against walletRepo.balanceState.value.maxSendOnchainSats, which is derived at the default speed. This is the underlying cause of Send-max computed at the default fee speed, but the drain happens at the selected speed #1144 and predates the PR; fixing it at this layer (recompute the max for the selected speed and destination when the speed changes) would make the confirm-time guard largely unnecessary. Filing as follow-up rather than a change request on this diff.
• [nit] estimateMaxSendOnchain drops the percentage fallback used elsewhere (app/src/main/java/to/bitkit/repositories/LightningRepo.kt)
  DeriveBalanceStateUseCase.getMaxSendAmount falls back to FALLBACK_FEE_PERCENT of the balance when the send-all fee estimate fails; the new estimateMaxSendOnchain propagates the failure instead. That is a defensible choice for a drain guard (the caller treats failure as 'do not drain'), but the two now disagree about what the max is under estimator failure, which is worth a comment or a shared helper if the method gets reused.

[jvsena42]

Reviewed the drain/max-send change and tested it on device (dev build, regtest, funded wallet). One of the findings is a reproduced regression — details inline.

[ovitrif]

@coreyphillips can you look into this (addressing comments), looks quite important on 1st sight, would like to merge and ship it in upcoming 2.5.0 🙏🏻

[coreyphillips]

@jvsena42 thanks for the device repro, both blockers were real. Pushed 96d17bd.

Rather than gating the drain on equality with the cached max, the send flow now recomputes the max for the actual recipient at the selected speed (refreshMaxSendOnchain, run from refreshOnchainSendIfNeeded). When the amount reaches that max we lower the amount to it and flag the send as a drain, so:

• no more apples-to-oranges comparison, the own-address cached max is only a fallback when the estimate fails
• no unfundable exact-amount fallback, the confirmed amount always matches what the drain delivers
• the confirm screen amount updates on speed change, like iOS
• the extra LDK call moved off the swipe-to-pay path into the existing refresh

Logs now use speed.serialized() and the duplicate warn is gone.

@ovitrif should be good for 2.5.0 once re-checked.

QA notes: max send to a P2TR recipient at default speed should drain, and switching speed on confirm should update the displayed amount. Covered in AppViewModelSendFlowTest.kt.

[ovitrif]

@coreyphillips can you please add PR description details matching our usual format? Ensuring sensitive framing, ofc 🙏🏻

While at it please also update with latest master to resolve conflicts flagged by GitHub.

[coreyphillips]

@ovitrif merged latest master, description updated, ready for re-review.

[ovitrif]

Pay is not sequenced with refreshMaxSendOnchain, so switching a max send to Fast can still drain at the new fee while the confirm amount is the old one.

The hardware early-return in that helper also has no send-flow test that would fail if the guard disappeared.

[coreyphillips]

@ovitrif both addressed in bdf6963.

• pay now joins onchainSendRefreshJob before reading the amount, so a swipe landing mid-refresh pays the settled amount and drain flag
• the refresh write is dropped if amount, address, speed or funding source changed while the estimate was in flight
• added the hardware max send case: amount stays on hardwareAvailableSats and estimateMaxSendOnchain is never called

Three new tests in AppViewModelSendFlowTest.kt, each verified to fail without its fix. compileDevDebugKotlin, testDevDebugUnitTest and detekt pass.

[jvsena42]

Reviewed the max-send math end to end. The arithmetic itself is right: estimateMaxSendOnchain works off spendableOnchainBalanceSats (anchor reserve already excluded) minus calculateSendAllFee(retainReserves = true) at the selected speed, wrapped in USat, and the drain uses the same FeeRates snapshot. No reserve stranding, no dust change, no underflow.

The problem is the classification rule, not the math. amount >= max reclassifies any near-max exact-amount send as a drain and rewrites the amount. Verified by running the regression test below against bdf6963: expected:<99800> but was:<99500>, with the other 195 tests in AppViewModelSendFlowTest still passing.

Two of these are replies on the existing threads where the same ground was already covered — they are there because the shipped fix leaves something specific still open, not to re-litigate.

[ovitrif]

Moved to 2.6.0 milestone

[ovitrif]

making this draft until it receives updates (descoped from 2.5.0, cc @coreyphillips)

[piotr-iohk]

Potential issue on e2e run: multi_address_1 max send pays 399580 with isMaxAmount=false; have 400000 sats, need 400580

multi_address_-_Multi_address_multi_address_1_-_Receive_to_each_address_type_and_send_max_combined-2026-09-04T01-26-18-344Z.mp4

Logs: attempt-3.zip

[coreyphillips]

Not signed off: the last round changed nothing, so the branch is the one that was already reviewed.

Raised and refuted:

• Branch conflicts with master in all four core files (app/src/main/java/to/bitkit/repositories/LightningRepo.kt:1373-1393). The conflicts are valid, but clearing them requires a history update that the no-merge instruction and read-only repository metadata prevent. Prepared source resolutions pass static analysis; compilation and tests are blocked by socket restrictions.

Noted, not blocking:

• Any amount at or above the speed max is silently lowered and drained (app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:3860-3870)
• Two prose commit messages will land in master history

[github-actions]

Regtest APK

Built from c606021 (run).

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

[talosmachina]

Recomputes the on-chain max for the actual recipient at the selected speed on every on-chain refresh, carries an explicit isMaxAmount flag into the send, rechecks it in LightningRepo.sendOnChain after the pre-send sync, and disables the swipe while an on-chain refresh is loading. Reviewed at f2259185, full tier.

No new findings.

Still open from earlier rounds, not re-posted: @jvsena42's thread on amount >= max in refreshMaxSendOnchain. It is unchanged at this head: any amount at or above the speed max is still lowered to the max and flagged as a drain, with no indication on the confirm screen.

Merge confidence: 3/5. That open thread is the main reason, plus the e2e shards (including multi_address_1, the case @piotr-iohk reported) were still pending when I reviewed.

What I checked

• Build skipped: static review. Bitkit Android PR branches are not built on this reviewer's machine, so nothing was compiled or run. Detekt and lint are green in CI.
• Drain decision: sendOnchain now reads SendUiState.isMaxAmount, not amount == maxSendOnchainSats. That flag is cleared by onAmountChange and by the amount reset. It is set only by refreshMaxSendOnchain, and only when there is no payment request and no hardware wallet. The repo recheck after ensureSyncedBeforeSend uses the same satsPerVByte the send uses. It only downgrades to an exact-amount send and never upgrades one, so a deposit landing after confirm is no longer swept.
• Ordering: the earlier join() on the refresh job was removed in 16e658c7. The swipe is now gated on onchainFeeUi.isLoading for on-chain sends, and the repo recheck stands behind it. This covers the swipe-during-refresh case from the earlier threads.
• Stale writes: divergedFrom checks amount, method, address, speed and wallet, and the generation check sits in both update paths.
• Ruled out: the fee-greater-than-spendable case. It saturates to 0, takeIf { it > 0uL } maps that to no estimate, and no estimate now gives isMaxAmount = false, not the cached max.
• Not settled: when the repo recheck disagrees after the sync, the send goes out as an exact send of spendable - sendAllFee. Whether that can pay the fee of a multi-input exact send is what multi_address_1 exercises. Read that shard's result before merging.

Reviewer model: claude-opus-5-5
Reasoning effort: Not exposed (not present in the run metadata available to this reviewer)

[coreyphillips]

Changed

• @jvsena42 on app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt: The cached own-address max no longer decides anything: a Max send now takes the recipient's max at the selected speed, raised or lowered, so a P2WPKH recipient sent from a Taproot wallet drains too instead of paying the cached figure.
• @jvsena42 on app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt: Tapping Max now sends an explicit AmountMax intent that editing the amount clears. Only Max sends are moved to the max for the selected speed and drained; any other amount above it stays as entered, is marked invalid and shows the insufficient savings toast with the shortfall. Your regression test is added.

Answered

• @piotr-iohk: This is still reachable at f225918. Max fills in the cached max, which is priced for our own receive address at the default speed. refreshMaxSendOnchain only drains when amount >= the recipient's max at the selected speed. When the recipient's max is higher (a smaller output script, or a cheaper speed), the send is exact and cannot pay a two-output fee. Before this PR it would have drained. The fix is the Max intent change from c2, and QA case 1b's Slow expectation changes with it.

Not changing

• @jvsena42 on app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:7124: This is already in place at f225918. For on-chain sends the swipe is now disabled while onchainFeeUi.isLoading, not only for hardware wallets (SendConfirmScreen.kt:539-542). The post-swipe join that could rewrite the amount was removed. refreshMaxSendOnchain also returns early while a payment request is active (AppViewModel.kt:7209-7210), so a Paykit amount is never lowered. The drain flag's backstop is now the post-sync recheck in LightningRepo.sendOnChain.
• @jvsena42 on app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:7237: This is already handled at f225918. After ensureSyncedBeforeSend, LightningRepo.sendOnChain recomputes spendable minus the send-all fee at the send's own rate (LightningRepo.kt:1530-1552). It drains only while that still equals the confirmed sats. Otherwise it sends the confirmed amount as an exact send, so a deposit that lands after confirm is not swept. sendOnChain should not drain funds found by its own sync in LightningRepoTest.kt covers it.

Filed separately

• @jvsena42 on app/src/main/java/to/bitkit/repositories/LightningRepo.kt:2086: Filed as estimateMaxSendOnchain has no test for a send-all fee above the spendable balance #1765.

Needs your decision

• @jvsena42 on app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:7202: the two reviewers did not agree, so nothing was changed. CLAUDE.md forbids comments on private functions. At f225918 the PR adds the KDoc on refreshMaxSendOnchain (AppViewModel.kt:7201-7204), its inline comments at :7209 and :7219, and the comment in the private ContentRunning (SendConfirmScreen.kt:523). The comment in the public LightningRepo.sendOnChain is not covered by the rule.
• @talosmachina: the two reviewers did not agree, so nothing was changed. No new findings. The open items it names are handled under c2 and c15.

[talosmachina]

The open item from my review at f2259185 (@jvsena42's amount >= max thread) is closed at df3cffc0, from my side.

• refreshMaxSendOnchain now moves the amount only when isMaxAmount is set, and only the new SendEvent.AmountMax from the Max button sets it (AppViewModel.kt:7237-7248).
• An entered amount above the speed max stays as entered. It is marked invalid, so onSwipeToPay returns early, and the insufficient savings toast shows the shortfall (AppViewModel.kt:7251-7263).
• The LaunchedEffect echo of AmountChange(maxSats) after the Max tap keeps the flag, because onAmountChange now clears it only when the amount differs (AppViewModel.kt:4170). The new test max tap marks an onchain send as max until the amount is edited covers that.

I have nothing to add on the KDoc question. This is a static read of the diff since my review. I did not build it, and it is not a new review.

[jvsena42]

Code scan of the delta since bdf69630 (16e658c7, 8961672f, 55648ef3, df3cffc0). One MEDIUM: the drain recheck from my earlier thread narrows the deposit-sweep window but does not close it. Two LOWs. Device gate not run, since a MEDIUM is open.

Checked and clean:

• Only the Max button sets isMaxAmount, so BIP21, LNURL and Paykit amounts can't become a drain.
• The estimate and the send resolve the same fee rate from the same state.
• The fallback exact-amount send never delivers more than the confirmed amount.
• divergedFrom rejects late refresh writes.

LOW, outside the diff (AppViewModel.kt:4504, switchToLightning): switchToLightning replaces amount with fixedContactLightningAmount without going through onAmountChange, so the flag stays set. Switching back to Savings runs refreshMaxSendOnchain, which overwrites the invoice amount with the on-chain max and flags a drain. The user did tap Max earlier, and the new figure shows before the swipe is enabled, so no unconfirmed amount is sent. But the send no longer follows the invoice. Suggested fix: clear isMaxAmount here when the amount is replaced.

[coreyphillips]

Changed

• @jvsena42 on app/src/main/java/to/bitkit/repositories/LightningRepo.kt:1601: Non-transfer drains now check(prepared.receipt.amountSats == sats) before the receipt is retained, so a swept late deposit fails through the existing release path without broadcasting. The new check shares a when with the input check (unchanged) to stay under detekt's complexity limit, and there is a new test for an oversized drain.
• @jvsena42 on app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:7246: Writing the max now also sets isAmountInputValid = max > dustLimit. I used the dust rule rather than validateAmount(max) because that caps at the cached default-speed max and would reject a max raised at a cheaper speed. A dust-limit test covers it.
• @jvsena42: switchToLightning now clears isMaxAmount when the fixed invoice amount replaces the entered amount (same rule as onAmountChange), so switching back to Savings no longer turns the invoice amount into a drain. Covered by a new test.

[jvsena42]

Approving at dcb69993. All three findings from my last review are fixed:

• Non-transfer drains now check(prepared.receipt.amountSats == sats).
• switchToLightning clears isMaxAmount when the amount changes.
• The max write recomputes isAmountInputValid.

Device gate on a dev build (regtest, 3 UTXOs, 100,000 sats):

• Max at Normal: confirm showed 99,754 with fee 246. The broadcast had one output of 99,754, and the on-chain balance was 0 afterwards.
• Max follows speed: at Fast it was 99,263 (fee 737); switching to Slow rewrote it to 99,754 (log: Updating max amount '99263' to '99754' at speed 'slow').
• Max with manual coin selection: 69,326 at Fast became 69,681 at Normal. It sent exactly 69,681 and left a balance of 0.
• Normal 10,000 send: sent exactly 10,000 with 39,859 change. Changing speed kept the amount.
• The new drain check never failed during these runs.
• Not reachable in the UI: the non-max "above the max is marked invalid" path. The number pad refuses digits past the max.

Out of scope (pre-existing, not from this PR):

• From a Max at Normal, the speed picker greys out Fast and caps Custom at 1 sat/vB, because SendFeeViewModel.getFeeLimit() caps the fee at balance minus amount.
• For a Max, the confirm-screen fee is estimated as a regular send with change. So calculateTotalFee can fall back to 1,000 sats and log Coin selection failed or Insufficient funds before the drain, even though the broadcast is exact.

CI green apart from one pending e2e suite.

[jvsena42]

Re-approving at 2a87d21d. The new commit only adds a LightningRepoTest case for a max on-chain send whose fee exceeds the balance.

[ovi-reviewer]

Verdict: 💬 Comment — one MEDIUM funding-selector regression remains.

Findings:

• MEDIUM — Clear Max intent when the funding selector restores an invoice amount (AppViewModel.kt:7238). For a unified invoice with a fixed Lightning amount, choose Savings → Max → Confirm, then use the funding selector to choose Spending and return to Savings. onPaymentMethodSwitch restores the invoice amount but preserves isMaxAmount, so this branch replaces it with the whole Savings max again. A focused regression through PaymentMethodSelect reproduces 10,000 → 99,000 sats. The new reset in switchToLightning covers a different entry point. Clear the flag when the funding conversion changes the amount and cover the selector path. The replacement is displayed before payment; this does not claim an unconfirmed amount is broadcast.

QA for 2a87d21:

Max sends at the faster fee speed ✅ Confirm and broadcast both used 99,635 sats, leaving Savings at zero.

PR case 1a, regtest build from this head. The cached Max was 99,890; Confirm recomputed it to 99,635 at Fast. Decoding the transaction confirmed one 99,635-sat Taproot output and a 365-sat fee.

Fast was selected with a smaller amount before returning to Amount and tapping Max, because the existing fee-picker cap prevents selecting Fast from a Normal Max. The exact Normal-Max → Fast picker sequence remains constrained by that pre-existing behavior, also noted in the earlier device report.

software-max-fast-2x.2x.mp4

Fast Max: amount entry, confirmation, send and zero Savings balance.

Max follows the slower fee speed ✅ Switching Fast to Slow raised Max from 99,635 to 99,878 sats, and exactly 99,878 was sent.

PR case 1b. Decoding the transaction confirmed one 99,878-sat output and a 122-sat fee; Savings was zero afterwards. This follows the corrected Max-intent behavior: Slow refreshes the amount, rather than retaining the Fast amount as the description still says.

software-max-slow-2x.2x.mp4

Fast Max changes to Slow, then sends the refreshed amount.

Max sends to a Taproot recipient ✅ Default-speed Confirm and broadcast both used 199,878 sats, leaving Savings at zero.

PR case 2. A 200,000-sat wallet initially offered the cached 199,890-sat Max. Confirm adjusted it for the Taproot recipient. Independent transaction decoding confirmed a single 199,878-sat Taproot output and a 122-sat fee.

taproot-max-replay-2x.2x.mp4

Taproot Max at Normal, exact sent amount and zero Savings balance.

Exact amount survives fee speed changes ✅ The existing device report verified an unchanged 10,000-sat send and exact delivery.

PR case 3: relied on the device report at dcb69993. Runtime code is unchanged between that head and 2a87d21; the only subsequent change adds a repository test. This case was not repeated.

Hardware Max follows the hardware balance ✅ Switching Fast to Slow changed hardware Max from 798,259 to 799,047 sats.

PR case 4, deterministic Trezor on regtest. The hardware balance was 799,441 sats, distinct from the software wallet balance. Fast deducted 1,182 sats; Slow deducted 394. This checks confirmation and fee refresh; no hardware transaction was signed or sent.

hardware-max-fast-to-slow-2x.2x.mp4

Hardware funding source remains selected as Max follows the new fee speed.

Coin selection refreshes Max and fees ✅ The existing device report verified refreshed selected-coin amounts and exact delivery.

PR case 5: relied on the device report at dcb69993: manual selection changed Max from 69,326 at Fast to 69,681 at Normal, then sent exactly 69,681. Runtime code is unchanged through 2a87d21, so this case was not repeated.

Fixed invoice amount clears Max through the funding selector ❌ A focused unit regression reproduced 10,000 sats becoming 99,000 on returning to Savings.

Additional regression case 6, using the production PaymentMethodSelect handlers with mocked balances and invoice data. The assertion fails with expected:<10000> but was:<99000>. All original cases in AppViewModelSendFlowTest and LightningRepoTest passed. This selector sequence was not driven on a device.

Built with assembleDevDebug and ran the two affected unit-test classes. Full review covered 44bf50e..2a87d21, including all changed production and test files, callers, refresh cancellation, fee snapshots and prepared-receipt checks. Controlled deposit races, estimator failures and dust boundaries were covered by unit tests rather than device fault injection. Existing resolved threads were checked and left closed.

No dedicated iOS twin is linked. The matching iOS safeguard comes from synonymdev/bitkit-ios#540 [codex] Prevent dust change from triggering send-all: explicit Max intent and the fresh selected-rate equality check match; Android additionally refreshes the displayed Max here.

---

Reviewed by gpt-6.1-sol-high

[ovi-reviewer]

MEDIUM: Clear Max intent when the funding selector restores an invoice amount

With a fixed-amount unified Lightning/on-chain invoice, select Savings and tap Max, then use the confirmation funding selector to switch to Spending and back to Savings. onPaymentMethodSwitch replaces amount with the fixed Lightning invoice amount but leaves isMaxAmount set, so this branch replaces that exact amount with the entire savings max again. A focused PaymentMethodSelect regression at this head reproduces 10000 sats becoming 99000 sats on the return to Savings. The new reset in switchToLightning does not cover this selector path. Clear Max intent whenever the funding conversion replaces the amount, and cover PaymentMethodSelect in the test. The rewritten amount is visible before payment; this is not a claim that an unconfirmed amount is broadcast.

[coreyphillips]

Confirmed against the code. onPaymentMethodSwitch (AppViewModel.kt:4304-4316) replaces amount with the converted or fixed invoice amount but never touches isMaxAmount. Going Savings Max → Spending sets amount to the fixed invoice amount from paymentAmountForSource, and returning to Savings calls selectFundingSource → refreshOnchainSendIfNeeded → refreshMaxSendOnchain. That sees isMaxAmount still true (line 7238) and rewrites the amount to the full max. This is a regression from the PR's Max refresh, and the fix is the same isMaxAmount = it.isMaxAmount && newAmount == it.amount guard already used in onAmountChange and switchToLightning, plus a test. Not changed here: @ovi-reviewer cannot write to this repository, and checkin_trust is "write".

[ovi-reviewer]

Rechecked at 76f7b30: this remains unchanged. A focused regression through the production PaymentMethodSelect handlers restores the fixed 10,000-sat invoice amount on Spending, then gets 99,000 sats on returning to Savings (expected:<10000> but was:<99000>). onPaymentMethodSwitch still replaces amount without clearing isMaxAmount. This confirms the existing finding; the newer c606021 head is not covered by this review.

[coreyphillips]

Confirmed at HEAD: onPaymentMethodSwitch (AppViewModel.kt:4494-4506) replaces amount with the converted or fixed amount but keeps isMaxAmount, so returning to Savings sends refreshMaxSendOnchain (line 7928) down the Max branch and swaps the fixed amount for the full max. The fix is the same isMaxAmount = it.isMaxAmount && amount == it.amount guard already used in onAmountChange and switchToLightning, plus a regression test. Not changed here: @ovi-reviewer cannot write to this repository, and checkin_trust is "write".

[coreyphillips]

Answered

• @ovi-reviewer on app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:7238: Confirmed against the code. onPaymentMethodSwitch (AppViewModel.kt:4304-4316) replaces amount with the converted or fixed invoice amount but never touches isMaxAmount. Going Savings Max → Spending sets amount to the fixed invoice amount from paymentAmountForSource, and returning to Savings calls selectFundingSource → refreshOnchainSendIfNeeded → refreshMaxSendOnchain. That sees isMaxAmount still true (line 7238) and rewrites the amount to the full max. This is a regression from the PR's Max refresh, and the fix is the same isMaxAmount = it.isMaxAmount && newAmount == it.amount guard already used in onAmountChange and switchToLightning, plus a test.

[jvsena42]

@coreyphillips this conflicts with master; please rebase. I'll review it again once it merges cleanly.

[coreyphillips]

Answered

• @jvsena42: The conflict is real: GitHub reports the PR as CONFLICTING, and a trial merge against 7f49f21 conflicts only in AppViewModel.kt, in one hunk where refreshMaxSendOnchain sits on the KDoc master added to refreshOnchainFeeUi, so both sides stay. A literal rebase replays 11 commits and already stops on 3 files at the first one, so it needs a backup branch per CLAUDE.md, a force push, and the full compile, test and lint gate.

[ovi-reviewer]

Verdict: 💬 Comment — two funding-selector defects and a test-helper regression remain at 76f7b30.

The PR moved to c606021 after this run; these results are for 76f7b30.

Findings:

• MEDIUM — Preserve fresh validation after selecting Savings (AppViewModel.kt:7938). The selector awaits refreshMaxSendOnchain, then overwrites its result with validateAmount, which checks the cached default-speed maximum. A valid Max above that cache becomes invalid and disables payment. Conversely, an exact amount above the recipient's selected-speed maximum becomes valid again. Focused PaymentMethodSelect regressions reproduce both: cached 100,000 / live Max 100,100 ends invalid; exact 99,800 / live Max 99,500 ends valid. Keep the fresh recipient/speed validation when the selector completes.
• MEDIUM — Fixed invoice amounts still retain Max intent (existing thread, AppViewModel.kt:7928). The fixed-invoice selector regression still reproduces 10,000 → 99,000 sats on returning from Spending to Savings. onPaymentMethodSwitch replaces the amount without clearing isMaxAmount; the refresh then replaces the invoice amount with Max. This remains the existing finding, with a reply in its thread.
• LOW — Match the new mining-fee argument in send verification (AppViewModelSendFlowTest.kt:18986). verifyOnchainSend omits a matcher for approvedMiningFeeSats. Its callers fail with InvalidUseOfMatchersException: 18 matchers expected, 17 recorded. Adding approvedMiningFeeSats = anyOrNull() locally made those cases pass. The correction was removed after verification.

QA for 76f7b30:

Tested on Android 15, regtest, with a build from this head. All manual cases were repeated because the rebase changed native dependencies and surrounding funding paths.

Max sends at the faster fee speed ✅ Confirm and broadcast both used 99,635 sats, leaving Savings at zero.

Case 1a. A 100,000-sat wallet offered cached Max 99,890; Confirm refreshed it to 99,635 at Fast. Independent transaction decoding confirmed one 99,635-sat Taproot output and a 365-sat fee.

Fast was selected with a smaller amount before returning to Amount and tapping Max. The exact Normal-Max → Fast picker sequence remains constrained by the pre-existing fee cap documented in the earlier device report.

max-fast-2x.2x.mp4

Fast Max, confirmation, payment and zero Savings balance.

Max follows the slower fee speed ✅ Fast → Slow raised Max from 99,635 to 99,878 sats, and exactly 99,878 was sent.

Case 1b. Decoding confirmed one 99,878-sat output and a 122-sat fee; Savings became zero. This follows the corrected Max-intent expectation: Slow refreshes the amount, rather than retaining the Fast amount as the description still says.

max-slow-2x.2x.mp4

Fast Max changes to Slow and sends the refreshed amount.

Max sends to a Taproot recipient ✅ Normal-speed Confirm and broadcast both used 99,878 sats, leaving Savings at zero.

Case 2. Cached Max 99,890 was adjusted for the Taproot recipient. Transaction decoding confirmed a single 99,878-sat Taproot output and a 122-sat fee.

taproot-max-2x.2x.mp4

Taproot Max at Normal, exact delivery and zero Savings balance.

Exact amount survives fee speed changes ✅ Normal → Fast kept 10,000 sats, and the transaction paid exactly 10,000.

Case 3. Transaction decoding confirmed the 10,000-sat recipient output, 89,542-sat change and a 458-sat fee. The app log confirmed isMaxAmount=false. An incoming test deposit briefly covered the success screen; dismissing it revealed the correct payment result.

exact-amount-2x.2x.mp4

Exact amount through the speed change and successful payment.

Hardware Max follows the hardware balance ✅ Fast → Slow changed hardware Max from 499,430 to 499,810 sats.

Case 4. The selected Trezor held 500,000 sats, distinct from the software wallet. Fast deducted 570 sats; Slow deducted 190. The hardware source stayed selected. This case checked confirmation and fee refresh; no hardware transaction was signed or sent.

hardware-max-2x.2x.mp4

Hardware funding source and Max follow the fee-speed change.

Coin selection refreshes amount and fees ✅ Selecting only the 20,000-sat output refreshed the fee and paid exactly 10,000 sats.

Case 5. Deselected the separate 89,542-sat output before Continue. Confirm retained 10,000 sats and used a 153-sat fee. Transaction decoding and the app's selected-input log confirmed that only the chosen 20,000-sat output was spent, with 9,847 sats change.

coin-selection-2x.2x.mp4

Manual input selection, refreshed confirmation and exact payment.

Check: fixed invoice amount clears Max through the funding selector ❌ The focused regression still gets 99,000 sats instead of the fixed 10,000.

Additional case 6. Executed the production PaymentMethodSelect handlers with mocked balances and a fixed unified invoice. Savings Max → Spending restored 10,000; returning to Savings replaced it with 99,000. The final assertion failed with expected:<10000> but was:<99000>. This selector sequence was not driven on a device in this round.

Check: funding selection preserves recipient and speed validation ❌ The focused regressions reproduce both an invalid affordable Max and a valid unaffordable exact amount.

Additional case 7. Executed PaymentMethodSelect(Savings) with controlled balances and fee estimates. With cached Max 100,000, a fresh Max of 100,100 was retained but marked invalid. An exact 99,800 against fresh Max 99,500 was incorrectly marked valid. Both assertions failed. These boundary states were checked through unit regressions, without device fault injection.

Check: existing send regression tests ❌ The affected suites expose the missing matcher in the new verification helper.

Built with assembleDevDebug and ran AppViewModelSendFlowTest and LightningRepoTest. The repository cases passed; send-flow callers of verifyOnchainSend failed on the matcher error above. A temporary matcher correction made the affected cases pass. The reviewed APK used unchanged production code, and all temporary test edits were removed. Full repository tests and lint were not run.

The earlier reviewed head is no longer an ancestor after the rebase. Coverage therefore includes the full current patch f7ad821..76f7b30, its reached callers, and comparison with the previous review. Deposit races, estimator failure and dust boundaries were checked by unit tests rather than device fault injection. The PR moved to c606021 during the hardware case; that newer head is not covered here.

No dedicated iOS twin is linked. The related safeguard in synonymdev/bitkit-ios#540 [codex] Prevent dust change from triggering send-all matches explicit Max intent and fresh-maximum checks; this Android PR also refreshes the displayed Max.

---

Reviewed by gpt-6.1-sol-high

[ovi-reviewer]

MEDIUM: Preserve fresh validation after selecting Savings

Selecting Savings awaits this refresh, but onPaymentMethodSwitch then overwrites isAmountInputValid with validateAmount, which uses the cached default-speed maximum. An affordable selected-speed Max above that cache is immediately invalidated, disabling Swipe To Pay. Conversely, an exact amount above the live maximum but below the cache is re-enabled after this helper rejects it.

Focused PaymentMethodSelect regressions at 76f7b30 reproduce both: cached 100,000 / live Max 100,100 ends invalid; exact 99,800 / live Max 99,500 ends valid. Preserve the fresh recipient/speed validation when the selector completes.

[coreyphillips]

Confirmed at HEAD: selectFundingSource(Savings) joins the refresh (line 4644), then onPaymentMethodSwitch (lines 4509-4519) resets isAmountInputValid with validateAmount, which checks the cached default-speed max (line 5106). A Slow Max above the cache ends invalid and an exact amount over the live max ends valid. Running the cached validation before the Savings refresh lets the fresh check win, and keeps the cached result when the estimate fails. Not changed here: @ovi-reviewer cannot write to this repository, and checkin_trust is "write".

[ovi-reviewer]

LOW: Match the new mining-fee argument in send verification

The rebased sendOnChain signature includes approvedMiningFeeSats, but this new verification helper supplies matchers only for the preceding arguments. Mockito receives the default null as a raw argument, so its callers fail with InvalidUseOfMatchersException: 18 matchers expected, 17 recorded.

Adding approvedMiningFeeSats = anyOrNull() locally made the affected cases pass. Please add that matcher here; the temporary correction used for verification was removed.

[coreyphillips]

Answered

• @ovi-reviewer on app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:7928: Confirmed at HEAD: onPaymentMethodSwitch (AppViewModel.kt:4494-4506) replaces amount with the converted or fixed amount but keeps isMaxAmount, so returning to Savings sends refreshMaxSendOnchain (line 7928) down the Max branch and swaps the fixed amount for the full max. The fix is the same isMaxAmount = it.isMaxAmount && amount == it.amount guard already used in onAmountChange and switchToLightning, plus a regression test.
• @ovi-reviewer on app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:7938: Confirmed at HEAD: selectFundingSource(Savings) joins the refresh (line 4644), then onPaymentMethodSwitch (lines 4509-4519) resets isAmountInputValid with validateAmount, which checks the cached default-speed max (line 5106). A Slow Max above the cache ends invalid and an exact amount over the live max ends valid. Running the cached validation before the Savings refresh lets the fresh check win, and keeps the cached result when the estimate fails.