fix: create order only on swipe - #741
Conversation
This comment has been minimized.
This comment has been minimized.
487c580 to
7bc4776
Compare
7bc4776 to
56a29cd
Compare
jvsena42
left a comment
There was a problem hiding this comment.
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.
isConfirmingandisCreatingOrderare set synchronously before the first await, and the HWisCreatingOrder→isSigninghandoff is synchronous. - Paying one order twice in a session.
fundedOrderIdis set first infundPaidOrder,onEstimateReadydrops 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
hwTransferErrorand the defer resets the flag. - Advanced LSP balance. A changed client or LSP balance drops the order, and
createOrderreceivesuiState.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.
|
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
Also noticed, pre-existing
QA LGTM |
56a29cd to
cbaab0c
Compare
|
Updated in cbaab0c to address the latest review:
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
left a comment
There was a problem hiding this comment.
Delta since 56a29cd7 (rebased, now cbaab0cd). The replies are posted on the existing threads. Still blocking:
- HIGH: the expiry parse fails on Blocktank's
.sssZtimestamps, 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).
fundedOrderIdstill prevents paying the same order twice within a session.
cbaab0c to
43af31d
Compare
jvsena42
left a comment
There was a problem hiding this comment.
Delta since cbaab0cd (43af31d4): no findings. The HIGH, both MEDIUMs and the LOW are fixed.
- Retry.
orderForConfirmationreturns the order unconditionally whenpendingHwFundingBroadcast?.orderIdmatches, so Retry re-broadcasts the stored tx.testBroadcastRetryReusesThePendingOrderAfterItExpirespins this with an expired.175Zorder. - Expiry parse. It tries
.withFractionalSecondsfirst and falls back to the plain formatter, covered by the.175Zreuse 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
hwOrderCreationTokenreplaces the Bool.isCurrentis checked aftercreateOrderand beforeuiState.orderis written, thedeferclears only its own token, andcancelHwSigningnils 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.
|
Updated in 43af31d to address the latest exact-head review:
This answers the expiry/retry, stuck swipe, fee-rate, and overlapping-request threads. All 74 focused transfer tests, SwiftFormat, and translation validation passed. |
jvsena42
left a comment
There was a problem hiding this comment.
tAck
Swipe creates exactly one order. Haven't tested HW flow
Closes #738
Description
This PR creates a Blocktank channel order only when the user confirms a transfer from savings to spending.
Counterpart: synonymdev/bitkit-android#1247
Out of Scope
Design
N/A — no design available.
Preview
ios-transfer-preview.mp4
QA Notes
Journeys
@transfer_1in transfer.e2e.ts covers the existing software-wallet transfer with default and custom receiving capacity.Manual Tests
regression:run@transfer_1with default and custom receiving capacity → the software-wallet transfer remains user-facing equivalent to the existing flow.Automated Checks
TransferViewModelTests.swiftcovers estimate-only navigation, higher-fee reconfirmation, same-confirmation reuse, new-quote replacement, fractional-second expiry parsing, and funding-address sizing.TransferViewModelHwTests.swiftcovers pending signed-order retry after expiry, cancellation during hardware order creation, and overlapping request isolation.AddressTypeIntegrationTestsenvironment failure; affected transfer tests remained green.