Skip to content

fix: show the amount that leaves on spending confirm - #772

Merged
ovitrif merged 3 commits into
masterfrom
fix/spending-confirm-leaving-amount
Sep 24, 2026
Merged

ovitrif merged 3 commits into
masterfrom
fix/spending-confirm-leaving-amount

Conversation

@ovitrif

@ovitrif ovitrif commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #773

Description

  • Fixes the spending confirm total so it shows the amount of bitcoin that leaves the wallet when send-all funds the transfer, matching bitkit-android#1323.
  • The transfer record uses that same amount on a send-all sweep.

Out of Scope

  • bitkit-android#1323: the Android change
  • app logs: unchanged
  • confirm swipe: a rebuilt plan that differs from the one shown stays as it has been since bitkit-ios#741

Design

Transfer to Spending Balance From Savings (Channel Opening) › Transfer to Spending Confirm. Same frame as the Android screen.

Preview

N/A — screenshot needs a funded send-all confirm, not captured in this pass.

QA Notes

Journeys

N/A

Manual Tests

  • 1. Transfer → Spending Confirm with a normal (non send-all) funding path → total equals order fee + network fee.
  • 2. Transfer → Spending Confirm on a dust/send-all funding path → total equals the spendable balance that leaves (maxSendable + network fee), while the network-fee row stays the miner fee alone.

Automated Checks

  • updated SpendingConfirmTotalTests.swift — overflow saturation on both paths
  • ran SpendingConfirmTotalTests.swift — 6 passed, 0 failed

@ovitrif ovitrif self-assigned this Sep 22, 2026
@ovitrif
ovitrif marked this pull request as ready for review September 22, 2026 19:27
@ovitrif
ovitrif requested review from a team, piotr-iohk and pwltr and removed request for a team September 22, 2026 19:27
@greptile-apps

greptile-apps Bot commented Sep 22, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

The PR appears safe to merge, with the corrected total matching the amount swept from the wallet and focused tests covering both funding paths.

Summary

This PR corrects the spending-confirm total shown for send-all funding.

  • Adds a dedicated calculation that displays the full spendable balance leaving the wallet for send-all transfers.
  • Preserves the existing order-fee-plus-network-fee total for normal transfers.
  • Adds unit coverage for normal, stale-state, send-all, and missing-value cases.
  • Documents the user-facing correction in the changelog.

Reviews (1) · Last reviewed commit: "fix: show the amount that leaves on spen..."

pwltr
pwltr previously approved these changes Sep 23, 2026

@jvsena42 jvsena42 left a comment

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.

No HIGH or MEDIUM findings. The displayed total matches what leaves the wallet on both paths. Two LOWs inline.

Traced:

  • Normal path. The display is feeSat + transactionFee, and orderForDisplayedFee keeps order.feeSat <= uiState.feeSat, otherwise it bumps the fee, throws and re-sizes. The mining fee is recomputed at the displayed rate right before payOrder, which sends order.feeSat with the selected UTXOs. What leaves is at most what is shown.
  • Send-all. maxSendable + sendAllFee reconstructs the spendable balance, guarded when the balance is below the fee, and sendAllToAddress(retainReserve: true) spends exactly that. onConfirm re-sizes immediately before broadcast.
  • Re-sizing. sizeFunding doesn't clear state first, so the previous total stays visible instead of flashing zero. A stale maxSendableAmount after a normal-path re-size is ignored because shouldUseSendAll is false, which testNormalPath_IgnoresStaleMaxSendable pins. On failure everything resets and the swipe is disabled.
  • Hardware. Untouched. The grid is feeSat + miningFeeSats, the PSBT is an exact-amount compose for order.feeSat, and there is no send-all there.
  • Stacking. This sits on top of #741 (merged) and relies on its orderForDisplayedFee and .task(id: feeSat). No conflict.

Pre-existing and out of scope, worth a follow-up issue on both platforms: shouldUseSendAllToAvoidDust decides on the selected UTXO subset, but sendAllToAddress then drains every spendable UTXO and ignores utxosToSpend. With several UTXOs, if the largest one alone covers the order and leaves dust change, the whole wallet is swept to the LSP address and the order is overpaid. This PR makes the screen show that honestly; the swipe behaviour is unchanged. synonymdev/bitkit-android#1323 has the same shape.

Comment thread Bitkit/Utilities/SpendingConfirmTotal.swift
Comment thread Bitkit/Utilities/SpendingConfirmTotal.swift Outdated

@jvsena42 jvsena42 left a comment

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.

Follow-up from the android twin synonymdev/bitkit-android#1323, same mechanism here and pre-existing, not introduced by this PR.

onConfirm re-runs calculateTransactionFee against the order address and order.feeSat and pays immediately, while orderForDisplayedFee stops only when the order costs more than the estimate. When it costs the same or less, or a UTXO arrives in between, the recomputed plan can flip to send-all through shouldUseSendAllToAvoidDust, which decides on the selected subset while the sweep is wallet-wide. The new numbers are published to the view, but only for the instant before payment, so there is no second swipe.

Pinning confirmedFeeRate (from #741) removes the fee-rate trigger, which android still has. Worth the same guard: if the recomputed leaving amount differs from the displayed one, update and require another swipe.

@piotr-iohk

Copy link
Copy Markdown
Collaborator

QA reviewed on 4f67926.

Spending confirm now shows the amount that leaves. On a send-all transfer that is the spendable balance. On the normal path it is still the order fee plus the miner fee.

Ran on iPhone 17. MAX showed network fee 143, service fee 596, to spending 40 046, total 40 785. A 39 900 transfer showed network fee 143, service fee 597, to spending 39 900, total 40 640. Both totals match the three rows added together, so this wallet did not hit send-all. Did not swipe.

Twin bitkit-android#1323 showed the send-all case: total 20 000 while the rows added up to 19 895.

Findings

No findings.

Checked and clean

  • The confirm total uses the spendable balance when send-all is on, and the order fee plus the miner fee otherwise.
  • A normal-path total ignores a leftover send-all amount. That is covered by SpendingConfirmTotalTests.
  • The activity row still records the order fee plus the miner fee on a send-all sweep. Already noted on this PR.
  • The plan rebuilt at swipe can still differ from the screen. Already noted, and it is the same issue as on Android.

QA LGTM

piotr-iohk
piotr-iohk previously approved these changes Sep 23, 2026
@ovitrif
ovitrif dismissed stale reviews from piotr-iohk and pwltr via b7c5ba8 September 23, 2026 19:03
@ovitrif

ovitrif commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator Author

Pushed b7c5ba8. The send-all transfer record now uses the same leaving amount as the confirm screen, and that helper saturates instead of trapping. SpendingConfirmTotalTests — 6 passed, 0 failed.

Worth the same guard: if the recomputed leaving amount differs from the displayed one, update and require another swipe.

@jvsena42 That rebuild is unchanged from #741. This push only makes the recorded total match the amount on screen. A second swipe when the rebuilt plan differs is tracked in #783.

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

b7c5ba8

Spending confirm shows the amount that leaves. On send-all that is the savings balance. On the normal path it is the order fee plus the miner fee. The transfer record uses that same amount.

Ran on iPhone 17 / C0525820-E7A8-44DD-8912-9ABF7A825153, regtest, 200 000 in savings.

  • 10 000: network fees 422, service fees 2 260, to spending 10 000, total 12 682.
  • MAX: network fees 329, service fees 2 048, to spending 197 623, total 200 000. The network-fee row stayed the miner fee.
  • Swiped MAX. The total stayed 200 000 through the spinner. Savings went to 0, spending to 197 623, and activity showed a 200 000 transfer from savings.

Twin bitkit-android#1323 showed the same totals.

The swipe that can pay a different amount than the screen is still bitkit-ios#783. This MAX swipe did not hit it.

Findings

No findings.

QA LGTM

@jvsena42 jvsena42 left a comment

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.

Delta since 4f679264d (b7c5ba870): no findings. Both LOWs are fixed.

  • payOrder now records txTotalSats through SpendingConfirmTotal.leavingAmount with isMaxAmount and maxSendableAmount, both of which are its own parameters, so a send-all sweep is no longer under-stated in the transfer record that BalanceManager subtracts. The send-all fallback when maxSendableAmount is nil keeps the old formula, and payOrder already rejects that case earlier when the max is below the order fee.
  • The helper saturates in both branches, with an UInt64.max test for each.

@ovitrif
ovitrif merged commit 521ce0f into master Sep 24, 2026
14 checks passed
@ovitrif
ovitrif deleted the fix/spending-confirm-leaving-amount branch September 24, 2026 20:12
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.

bug: spending confirm shows less than what leaves

4 participants