fix: enforce max send fee drains confirmed amount

[coreyphillips]

Fixes #1144

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]

Greptile Summary

The PR prevents an on-chain max send from draining at a fee speed different from the speed used to calculate the confirmed amount.

• Adds a repository API to estimate the maximum sendable amount for a specified address, fee speed, and fee-rate snapshot.
• Rechecks the maximum at the selected speed before enabling send-all behavior.
• Falls back to sending the confirmed amount exactly when the estimates differ or recomputation fails.
• Adds repository and send-flow tests for matching, mismatched, failed, and zero-balance estimates.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking defects identified.

The selected-speed estimate is passed consistently into the drain decision, subtraction safely saturates at zero, and mismatched or unavailable estimates avoid sending more than the amount the user confirmed.

Important Files Changed

Filename	Overview
app/src/main/java/to/bitkit/repositories/LightningRepo.kt	Adds selected-speed maximum estimation using the spendable balance and existing saturating fee subtraction.
app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt	Revalidates cached max amounts against the selected fee speed before enabling on-chain drain mode.
app/src/test/java/to/bitkit/repositories/LightningRepoTest.kt	Covers selected-speed fee subtraction and the zero-spendable-balance shortcut.
app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt	Covers drain eligibility when estimates match and exact-send fallback on mismatch or estimation failure.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[User confirms cached max amount] --> B{Amount equals cached max?}
    B -- No --> E[Send exact amount]
    B -- Yes --> C[Estimate max at selected fee speed]
    C --> D{Estimate succeeds and matches?}
    D -- Yes --> F[Send using drain mode]
    D -- No --> E
    E --> G[LightningRepo sendOnChain]
    F --> G

Loading

_{Reviews (1): Last reviewed commit: "fix: verify max onchain send at selected..." | Re-trigger 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.

[jvsena42]

Address-type mismatch makes this comparison apples-to-oranges — max-send is broken for P2TR/P2SH/P2PKH recipients.

This recomputes the max using the recipient address, but the cached value it gets compared against on L2897 (walletRepo.balanceState.value.maxSendOnchainSats) comes from DeriveBalanceStateUseCase.getMaxSendAmount, which passes address = null and therefore falls back to cacheStore.onchainAddress — our own receive address. calculateSendAllFee depends on the output script size, so the two values differ for any recipient whose script type differs from selectedAddressType, with no speed change at all.

Reproduced on regtest (balance 1,000,000 sats, default speed):

• P2TR recipient → Sending exact amount '999890' instead of draining, max at speed 'Medium' is '999878' → isMaxAmount = false → AppError='The available funds are insufficient to cover the transaction' and an "Error Sending" toast.
• P2WPKH recipient, same wallet and speed → isMaxAmount = true, drain succeeds (txid 498ced33…).

So MAX now fails for P2TR/P2SH/P2PKH recipients, and symmetrically for P2WPKH recipients once the user switches selectedAddressType to Taproot. iOS avoids this because its MAX amount is itself derived from calculateMaxSendableAmount(address: recipient, rate: selected).

Suggest comparing like with like: either recompute with the same address the cached max used, or derive the MAX button amount from the recipient + selected rate.

[coreyphillips]

Good catch, fixed. Dropped the equality check entirely. The max is now recomputed for the recipient address at the selected speed in refreshMaxSendOnchain, so the cached own-address value never gates the drain. It only remains as a fallback when the estimate is unavailable.

[jvsena42]

The intended "fall back to an exact-amount send" path can never succeed.

Whenever maxAtSelectedSpeed < amount, the headroom left over is exactly the 1-output send-all fee at the default rate, while an exact-amount send needs a 2-output tx (recipient + change) at the selected — higher — rate. That is always more, so returning false here doesn't degrade to an exact-amount send, it degrades to an insufficient-funds error. This is what the P2TR repro above ends in.

Consider clamping amount to maxAtSelectedSpeed (and updating the displayed amount/fee when the speed changes, as iOS does), or surfacing an actionable "reduce amount" error instead of the generic LDK failure.

[coreyphillips]

Agreed, that path was unfundable by construction. There is no exact-amount fallback now: when the amount reaches the max at the selected speed we lower the amount to that max and drain, so the confirmed figure matches what is delivered.

[jvsena42]

🟠 Do not convert exact-amount sends into drains when amount >= max

amount >= max reclassifies any near-max exact-amount send as a drain and rewrites amount. Pre-PR the flag was amount == balanceState.maxSendOnchainSats, so this send failed in LDK with insufficient funds and no funds moved; now it becomes sendAllToAddress(retainReserve = true) for a smaller figure, with no drain indicator anywhere in SendConfirmScreen (isMaxAmount is never read by the UI). Two reachable paths: onAmountContinue (:2321), where a typed amount is silently reduced between the amount screen and Confirm; and the Paykit-request path (:2801), where the lowered amount then fails hasMismatchedIncomingPaymentRequest's strict acceptsPaymentAmount at :3344 and a legitimate request becomes unpayable with a "payment request mismatch" toast. The window is widest when the selected speed differs from defaultTransactionSpeed (the cached max is derived at the default speed, DeriveBalanceStateUseCase.kt:219) or when balanceState is stale-high against the live getBalancesAsync() read inside estimateMaxSendOnchain. iOS is the reference here: SendConfirmationView.shouldUseMaxOnchainSend requires an explicit wallet.isMaxAmountSend intent flag AND amountSats == currentMaxSendable, and never rewrites the amount. Do the same: carry an explicit Max-intent flag from onClickMax, drain only on equality with the max the user was shown, and for any other amount above max set isAmountInputValid = false and surface insufficient savings rather than rewriting the amount.

@Test
fun `exact onchain amount above the max at the selected speed is not lowered into a drain`() = test {
    val address = "bcrt1qexactamount"
    val requested = 99_800uL
    val feeRates = FeeRates(fast = 20u, mid = 10u, slow = 5u)
    balanceState.value = BalanceState(maxSendOnchainSats = 100_000uL)
    whenever {
        lightningRepo.estimateMaxSendOnchain(address = address, speed = TransactionSpeed.Fast, feeRates = feeRates)
    }.thenReturn(Result.success(99_500uL))
    whenever { lightningRepo.getFeeRateForSpeed(any(), anyOrNull()) }.thenReturn(Result.success(20uL))
    setSendState(
        SendUiState(
            address = address,
            amount = requested,
            isAmountInputValid = true,
            payMethod = SendMethod.ONCHAIN,
            speed = TransactionSpeed.Medium,
            feeRates = feeRates,
        ),
    )

    sut.setTransactionSpeed(TransactionSpeed.Fast)
    advanceUntilIdle()

    assertEquals(requested, sut.sendUiState.value.amount)
    assertFalse(sut.sendUiState.value.isMaxAmount)
}

Regression test:

@Test
fun `exact onchain amount above the max at the selected speed is not lowered into a drain`() = test {
    val address = "bcrt1qexactamount"
    val requested = 99_800uL // e.g. BIP21 amount=, below the cached max but above the max at Fast
    val feeRates = FeeRates(fast = 20u, mid = 10u, slow = 5u)
    balanceState.value = BalanceState(maxSendOnchainSats = 100_000uL)
    whenever {
        lightningRepo.estimateMaxSendOnchain(address = address, speed = TransactionSpeed.Fast, feeRates = feeRates)
    }.thenReturn(Result.success(99_500uL))
    whenever { lightningRepo.getFeeRateForSpeed(any(), anyOrNull()) }.thenReturn(Result.success(20uL))
    setSendState(
        SendUiState(
            address = address,
            amount = requested,
            isAmountInputValid = true,
            payMethod = SendMethod.ONCHAIN,
            speed = TransactionSpeed.Medium,
            feeRates = feeRates,
        ),
    )

    sut.setTransactionSpeed(TransactionSpeed.Fast)
    advanceUntilIdle()

    // the requested amount must survive; the send may become invalid, but never a smaller drain
    assertEquals(requested, sut.sendUiState.value.amount)
    assertFalse(sut.sendUiState.value.isMaxAmount)
}

[jvsena42]

TransactionSpeed has no toString, so this logs Failed to recompute max send amount for speed 'to.bitkit.models.TransactionSpeed$Medium@987bb0b' on device. Use the existing state.speed.serialized() to keep the reference traceable.

[coreyphillips]

Fixed, the remaining log uses state.speed.serialized().

[jvsena42]

Same here — '${state.speed}' renders as to.bitkit.models.TransactionSpeed$Medium@987bb0b. Use state.speed.serialized().

[coreyphillips]

Fixed, that log is gone and the one left uses serialized().

[jvsena42]

This duplicates DeriveBalanceStateUseCase.getMaxSendAmount with different inputs, and the drain decision is exact equality between the two.

getMaxSendAmount (~L214) computes the same quantity but fetches fee rates fresh via blocktank.getFees() and applies the 1%-of-balance fallback; this one takes the send-sheet snapshot state.feeRates (captured in resetSendState) and has no fallback. Since shouldDrainOnchain requires amount == maxAtSelectedSpeed exactly, any blocktank rate refresh between the last balance derivation and confirm silently disables drain — same failure mode as the AppViewModel comments.

Having the use case delegate to this new repo method (same address, same rates) would make the two agree by construction rather than by coincidence.

[coreyphillips]

The drain decision no longer compares the two, so they cannot disagree. estimateMaxSendOnchain is used on its own for the send-flow max; getMaxSendAmount keeps its fallback for the balance display.

[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.

[ovitrif]

The drain decision now lives in refreshMaxSendOnchain, which this job runs in the background. sendOnchain reads state.isMaxAmount without waiting for that job, and the confirm swipe stays enabled while onchainFeeUi.isLoading is true for software wallets. After Max → Confirm → Fast, a swipe that lands before the refresh finishes still calls sendAllToAddress at the new rate while the header still shows the previous amount, which is the mismatch this change is meant to close. The same write can also restore amount after onAmountChange has already cleared isMaxAmount.

Could we join onchainSendRefreshJob before sending, or disable swipe while the on-chain fee refresh is in flight, so the confirmed amount and drain flag are the ones the user actually paid?

[coreyphillips]

Fixed in bdf6963. proceedWithPayment now joins onchainSendRefreshJob before reading the amount, so the swipe pays the amount and drain flag the refresh settled on. Also guarded the refresh write itself: it no longer applies if amount, address, speed or funding source changed while the estimate was in flight, so it cannot restore an amount onAmountChange already cleared.

Covered by max onchain send waits for the in-flight max refresh before paying and max refresh does not restore the amount after it was edited in AppViewModelSendFlowTest.kt; both fail without the fixes.

[jvsena42]

🟡 Gate the swipe on the refresh instead of re-deciding the amount after consent

The join() fixes the ordering but not the consent problem, and it lands too late for the Paykit check. validateIncomingPaymentRequest -> hasMismatchedIncomingPaymentRequest runs at :3344 against the pre-refresh amount using strict acceptsPaymentAmount equality; the join at :3362 then lowers amount and sets isMaxAmount, so a refresh still in flight at swipe time gets the mismatch guard to pass on the requested amount and then underpays it - and completeOnchainPaymentProofInBackground posts a proof for the underpaying txid. The swipe is reachable while the refresh runs because SendConfirmScreen:400 only consults onchainFeeUi.isLoading via isHardwareFeeLoading, which is gated on hardwareWalletId != null. Disable SwipeToConfirm while onchainSendRefreshJob is active (or drop the hardwareWalletId != null qualifier on isHardwareFeeLoading), keep join() only as a backstop, and never change amount after the swipe.

[ovitrif]

refreshMaxSendOnchain returns immediately when hardwareWalletId is set, which is what stops a hardware max send from being rewritten using the software wallet's spendable balance. None of the new send-flow tests set a hardware funding source, so removing that guard would not fail the suite.

Could we add a case that a hardware max send keeps hardwareAvailableSats and never calls estimateMaxSendOnchain?

[coreyphillips]

Added hardware max send keeps its available amount and skips the onchain max estimate in AppViewModelSendFlowTest.kt. It switches speed on a hardware max send and asserts the amount tracks hardwareAvailableSats with no estimateMaxSendOnchain call. Verified it fails if the guard is removed.

[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.

[jvsena42]

🟡 A drain sends the balance at send time, not the confirmed max

Setting amount = max makes the confirm screen promise a figure the drain does not honour. isMaxAmount routes to LightningService.send -> node.onchainPayment().sendAllToAddress(...), which ignores sats and sweeps whatever is spendable when it runs - and LightningRepo.sendOnChain calls ensureSyncedBeforeSend() immediately before it, so a deposit that confirms between the estimate and the swipe is swept to the recipient too. Pre-existing for an explicit Max send, but this PR widens the trigger from amount == cachedMax to any amount at or above the live speed-specific max, so ordinary sends now take the sweep path. Re-check the max inside sendOnchain after ensureSyncedBeforeSend and abort (or re-confirm) when it moved above the confirmed amount, rather than letting sendAllToAddress define the amount.

[jvsena42]

🔵 Cover fee >= spendable and the coin-selection path

fee >= spendable is untested: spendableSats.safe() - fee.safe() saturates to 0, takeIf { it > 0uL } makes it look like "no estimate", and refreshMaxSendOnchain then falls back to amount == maxSendOnchainSats, which still flags a drain LDK will reject. Fail-closed but unverified - add a LightningRepoTest case for fee > spendable returning 0, and a send-flow case asserting the fallback does not set isMaxAmount when the estimate is unavailable.

[jvsena42]

⚪ Drop KDoc/inline comments on private functions

AGENTS.md: 'NEVER add code comments to private functions'. New KDoc on private refreshMaxSendOnchain plus the inline comments at 3361 and 3855. Move the rationale into the function/log names or the PR description.