Skip to content

fix: create order only on swipe - #741

Merged
jvsena42 merged 1 commit into
masterfrom
fix/738-order-on-confirm
Sep 22, 2026
Merged

jvsena42 merged 1 commit into
masterfrom
fix/738-order-on-confirm

Conversation

@ovitrif

@ovitrif ovitrif commented Sep 10, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #738

Description

This PR creates a Blocktank channel order only when the user confirms a transfer from savings to spending.

  • Spending Amount and Advanced request fee estimates when the user continues; they no longer create an order.
  • Software-wallet swipe confirmation and hardware-wallet Open Trezor Connect create the order for the selected amount and receiving capacity.
  • If the created order costs more than the estimate, the confirmation screen shows the new fee, resets the swipe, and requires another confirmation before funding.
  • A failed funding retry from the same confirmation reuses its unpaid order while it remains valid. A pending signed hardware broadcast always reuses its order, while starting a new quote clears the retained order.
  • Final UTXO selection uses the mining-fee rate shown on the confirmation screen.
  • Concurrent hardware order requests use per-request identity so an older response cannot replace or clear a newer confirmation.
  • Back navigation and transfer changes remain disabled during order creation or funding so the confirmed selection cannot change in flight.

Counterpart: synonymdev/bitkit-android#1247

Out of Scope

  • Blocktank fee calculation: estimation and order creation keep using the existing backend pricing paths; this PR does not add a separate fee-reconciliation model.

Design

N/A — no design available.

Preview

ios-transfer-preview.mp4

QA Notes

Journeys

  • updated: transfer-to-spending.xml creates the order from Open Trezor Connect, handles a higher final fee with a second confirmation, and retains the signed, processing, and single-activity checks.
  • regression: @transfer_1 in transfer.e2e.ts covers the existing software-wallet transfer with default and custom receiving capacity.

Manual Tests

  • 1. Hardware Wallet → Transfer To Spending → 25% → Continue → Open Trezor Connect: order is created only at confirmation; a higher final fee resets confirmation; signing and processing create one transfer activity.
  • 2. regression: run @transfer_1 with default and custom receiving capacity → the software-wallet transfer remains user-facing equivalent to the existing flow.
  • 3. Spending Amount → Continue → return and request the same quote again → the old unpaid order is discarded; the next confirmation creates a fresh order.
  • 4. Hardware funding broadcast loses connectivity → Retry: the signed transaction is rebroadcast without creating or signing another order, including after the order expiry time.

Automated Checks

  • updated: TransferViewModelTests.swift covers estimate-only navigation, higher-fee reconfirmation, same-confirmation reuse, new-quote replacement, fractional-second expiry parsing, and funding-address sizing.
  • updated: TransferViewModelHwTests.swift covers pending signed-order retry after expiry, cancellation during hardware order creation, and overlapping request isolation.
  • ran: 74 focused transfer tests passed through the application and test build.
  • ran: SwiftFormat and translation validation passed.
  • attempted: the broader unit run reached an unrelated AddressTypeIntegrationTests environment failure; affected transfer tests remained green.
  • attempted: the hardware journey did not reach the changed flow because the test Lightning node did not finish starting.

@ovitrif ovitrif self-assigned this Sep 10, 2026
@ovitrif
ovitrif requested review from jvsena42 and pwltr September 10, 2026 13:43
@ovitrif ovitrif changed the title fix: create the blocktank order on confirm, not on every continue fix: create channel order only on confirm Sep 10, 2026
@greptile-apps

This comment has been minimized.

Comment thread Bitkit/ViewModels/TransferViewModel.swift Outdated
@ovitrif
ovitrif marked this pull request as draft September 10, 2026 14:01
@ovitrif
ovitrif force-pushed the fix/738-order-on-confirm branch from 487c580 to 7bc4776 Compare September 10, 2026 18:02
@ovitrif ovitrif changed the title fix: create channel order only on confirm fix: create order only on swipe Sep 11, 2026
@ovitrif
ovitrif force-pushed the fix/738-order-on-confirm branch from 7bc4776 to 56a29cd Compare September 22, 2026 00:58
@ovitrif
ovitrif marked this pull request as ready for review September 22, 2026 01:01

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

One MEDIUM that real users can reach, plus a gesture regression on the HW path. Both inline and both checked by a second pass.

Checked and clean:

  • Double swipe or tap. isConfirming and isCreatingOrder are set synchronously before the first await, and the HW isCreatingOrder → isSigning handoff is synchronous.
  • Paying one order twice in a session. fundedOrderId is set first in fundPaidOrder, onEstimateReady drops a funded order, and a HW broadcast retry reuses the signed tx only when order, wallet, address and amount all match.
  • Creation failure. No order is stored and the estimate is preserved. The HW path shows hwTransferError and the defer resets the flag.
  • Advanced LSP balance. A changed client or LSP balance drops the order, and createOrder receives uiState.lspBalanceSat.
  • HW PSBT. It uses the order's address and feeSat, and an empty address is rejected.
  • Journey. The two edited actions are word-for-word identical to synonymdev/bitkit-android#1247.

Comment thread Bitkit/Views/Transfer/SpendingConfirm.swift Outdated
Comment thread Bitkit/ViewModels/TransferViewModel.swift
Comment thread Bitkit/ViewModels/TransferViewModel.swift Outdated
@piotr-iohk

Copy link
Copy Markdown
Collaborator

QA reviewed on 56a29cd.

Ran on iPhone 17: Transfer to spending estimated the fee, then created one order on confirm. Confirm and the payment were the same: 226 853 to spending, 1 477 service fee, 281 network fee, 228 611 total. Setup mined a block. After moving the funds back, a second order for the same amount was rejected by Blocktank because the node was already over the LSP capacity limit.

The confirm screen shows the estimate and the swipe pays the order fee. Both calls use the same balances and the same order options. Greptile did not file that note here. Its older note, that the pre-order fee size followed the receive-address type, is fixed: that fee is sized with a native SegWit address.

Pixel_6: same first transfer. A later hardware sign created the order, then Trezor Bridge had already dropped the session, so the device never signed.

Twin synonymdev/bitkit-android#1247: same behavior.

No findings.

Checked and clean

  • Continue and Advanced ask for a fee estimate and do not create an order
  • A failed funding retry reuses the unpaid order while the amount and receiving capacity stay the same
  • Open Trezor Connect is what creates the order
  • Pre-order fee sizing uses a native SegWit address

Also noticed, pre-existing

  • The activity for that transfer shows fee 1 400. That is the previous order's service and network fees (1 056 + 344), not this payment. The amount sent matches the confirm screen.

QA LGTM

piotr-iohk
piotr-iohk previously approved these changes Sep 22, 2026
@ovitrif

ovitrif commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator Author

Updated in cbaab0c to address the latest review:

  • If the created order has a higher fee than the estimate, confirmation refreshes to the final fee and funding waits for another confirmation.
  • Retained orders are reused only while they are created, unfunded, parseable, and more than 60 seconds from expiry.
  • Leaving the hardware flow while order creation is in flight prevents signing from starting.
  • The shared hardware journey remains synchronized with Android.

Validation passed: 71 focused transfer tests, the standard unit workflow (1,303 unit tests and 35 UI checks; 3 opt-in Trezor dashboard tests skipped), SwiftFormat, and translation validation. The hardware journey was attempted, but the test Lightning node did not finish starting, so it did not reach the changed flow.

@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 56a29cd7 (rebased, now cbaab0cd). The replies are posted on the existing threads. Still blocking:

  • HIGH: the expiry parse fails on Blocktank's .sssZ timestamps, so an order is never reused. Hardware Retry after an uncertain broadcast then creates and signs a second order.
  • MEDIUM: after the fee refresh, the swipe button stays stuck.
  • MEDIUM: the mining fee is still re-priced at swipe time and paid without re-confirmation.

Fixed and verified:

  • The LSP fee refresh updates the fee rows, the total and the mining fee sizing before the next swipe. The hardware grid updates too.
  • Leaving the Sign screen during order creation no longer starts signing (one ABA case left, LOW).
  • fundedOrderId still prevents paying the same order twice within a session.

@ovitrif
ovitrif force-pushed the fix/738-order-on-confirm branch from cbaab0c to 43af31d Compare September 22, 2026 14:50

@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 cbaab0cd (43af31d4): no findings. The HIGH, both MEDIUMs and the LOW are fixed.

  • Retry. orderForConfirmation returns the order unconditionally when pendingHwFundingBroadcast?.orderId matches, so Retry re-broadcasts the stored tx. testBroadcastRetryReusesThePendingOrderAfterItExpires pins this with an expired .175Z order.
  • Expiry parse. It tries .withFractionalSeconds first and falls back to the plain formatter, covered by the .175Z reuse test.
  • Swipe button. The fee-refresh branch now throws, so the catch toasts and SwipeButton resets.
  • Mining fee. The swipe-time recompute uses the displayed satsPerVbyte. UTXOs are re-selected for the order amount but not re-priced.
  • ABA. A per-call hwOrderCreationToken replaces the Bool. isCurrent is checked after createOrder and before uiState.order is written, the defer clears only its own token, and cancelHwSigning nils it.

Note, not a regression against master: onEstimateReady now always calls clearPendingHwFundingBroadcast(). After an uncertain broadcast, Back to Amount (the pop gesture is not gated on hasPendingBroadcast) followed by Continue discards the signed tx and creates a new order. Master did the same through onOrderCreated.

@ovitrif

ovitrif commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator Author

Updated in 43af31d to address the latest exact-head review:

  • Hardware broadcast Retry now keeps the already signed order even after expiry, and fractional-second Blocktank expiry timestamps are parsed for ordinary unpaid-order reuse.
  • A higher created-order fee now resets the swipe for another confirmation, and final UTXO selection keeps the mining-fee rate shown on confirmation.
  • Hardware order creation now uses a per-request token so an older response cannot replace or clear a newer confirmation.
  • Starting a new quote drops an unpaid order from the previous confirmation, matching Android.

This answers the expiry/retry, stuck swipe, fee-rate, and overlapping-request threads. All 74 focused transfer tests, SwiftFormat, and translation validation passed.

@ovitrif
ovitrif requested a review from jvsena42 September 22, 2026 14:55
@ovitrif
ovitrif dismissed jvsena42’s stale review September 22, 2026 16:51

should be addressed

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

tAck

Swipe creates exactly one order. Haven't tested HW flow

@jvsena42
jvsena42 enabled auto-merge September 22, 2026 17:22
@jvsena42
jvsena42 merged commit ebca659 into master Sep 22, 2026
13 checks passed
@jvsena42
jvsena42 deleted the fix/738-order-on-confirm branch September 22, 2026 18:39
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.

fix: create order only on swipe

3 participants