fix: create order only on swipe - #1247
Conversation
This comment has been minimized.
This comment has been minimized.
f52d79c to
ec9e6f9
Compare
ec9e6f9 to
bdaea52
Compare
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
jvsena42
left a comment
There was a problem hiding this comment.
One HIGH and one MEDIUM, both reachable in release, both checked by a second pass and reproduced with unit tests (posted in a separate comment).
On greptile's fee thread: master's Confirm rendered order.feeSat - clientBalanceSat and order.feeSat + miningFeeSats from the order it then paid. The displayed-vs-paid LSP fee gap is therefore new in this PR, not pre-existing as the PR body says. The same point is raised on synonymdev/bitkit-ios#741.
Checked and clean:
- Double swipe or tap.
confirmPayJob/hwTransferSignJobandisBusyare checked, and the flags are set synchronously before launch. - Order creation failing after swipe.
orderstays null and the next swipe creates a new one. - Back while busy.
BackHandler(enabled = isBusy), the gated top-bar back and the hidden drawer on both Confirm and HW Sign. - HW path. The passphrase check runs before the order is created. The PSBT uses the order's address and
feeSat,PendingHwFundingBroadcast.matchespins order, address and fee, and a new quote clears the pending broadcast. - Sizing address. P2WPKH index 0, the same output type as the LSP funding address.
- Journey. The edited actions match the code and are identical to iOS.
|
Regression tests for the two findings in my review. They are added to Tests@Test
fun `a paid hardware order is not paid again by the next hardware transfer of the same amount`() = test {
val paidOrder = previewBtOrder()
val nextOrder = nextOrderAfter(paidOrder)
stubHwFundingSuccess(paidOrder)
quoteOrder(paidOrder)
whenever(blocktankRepo.createOrder(any(), any(), any()))
.thenReturn(Result.success(paidOrder), Result.success(nextOrder))
sut.onTransferToSpendingHwConfirm(HARDWARE_WALLET_ID)
advanceUntilIdle()
verify(cacheStore).addPaidOrder(eq(paidOrder.id), eq(TXID))
sut.onConfirmAmount(paidOrder.clientBalanceSat.toLong())
advanceUntilIdle()
sut.onTransferToSpendingHwConfirm(HARDWARE_WALLET_ID)
advanceUntilIdle()
verify(hwWalletRepo, times(1)).composeFundingTransaction(
eq(HARDWARE_WALLET_ID), eq(paidOrder.payment?.onchain?.address.orEmpty()), any(), any(),
)
verify(blocktankRepo, times(2)).createOrder(any(), any(), any())
}
@Test
fun `a paid hardware order is not paid again by the next savings transfer of the same amount`() = test {
val paidOrder = previewBtOrder()
val nextOrder = nextOrderAfter(paidOrder)
stubHwFundingSuccess(paidOrder)
stubSpendableBalances(spendable = ON_CHAIN_BALANCE)
whenever(lightningRepo.calculateTotalFee(any(), any(), any(), anyOrNull(), anyOrNull()))
.thenReturn(Result.success(1_000uL))
stubSendOnChainSuccess()
quoteOrder(paidOrder)
whenever(blocktankRepo.createOrder(any(), any(), any()))
.thenReturn(Result.success(paidOrder), Result.success(nextOrder))
sut.onTransferToSpendingHwConfirm(HARDWARE_WALLET_ID)
advanceUntilIdle()
sut.onConfirmAmount(paidOrder.clientBalanceSat.toLong())
advanceUntilIdle()
sut.onTransferToSpendingConfirm()
advanceUntilIdle()
verify(lightningRepo, never()).sendOnChain(
eq(paidOrder.payment?.onchain?.address.orEmpty()),
any(), any(), anyOrNull(), anyOrNull(), any(), anyOrNull(), any(), any(), any(), any(),
)
verify(blocktankRepo, times(2)).createOrder(any(), any(), any())
}
@Test
fun `a paid savings transfer does not block quoting the next transfer`() = test {
val paidOrder = spendingOrder(feeSat = 98_000uL)
stubSpendableBalances(spendable = 110_000u)
whenever(lightningRepo.calculateTotalFee(any(), any(), any(), anyOrNull(), anyOrNull()))
.thenReturn(Result.success(1_000uL))
stubSendOnChainSuccess()
quoteOrder(paidOrder)
sut.onTransferToSpendingConfirm()
advanceUntilIdle()
verify(cacheStore).addPaidOrder(eq(paidOrder.id), eq(TXID))
val nextAmount = paidOrder.clientBalanceSat.toLong() - 1_000
sut.onConfirmAmount(nextAmount)
advanceUntilIdle()
assertEquals(nextAmount.toULong(), sut.spendingUiState.value.clientBalanceSat)
}
private fun nextOrderAfter(order: IBtOrder): IBtOrder = order.copy(
id = "order_next",
payment = order.payment?.copy(onchain = order.payment?.onchain?.copy(address = "bcrt1qnextorderaddress")),
)
RunAt With |
|
QA reviewed on bdaea52. Ran on Pixel_6: Transfer to spending estimated the fee, then created one order on confirm. The transfer was recorded and setup mined a block. A later hardware attempt estimated again and created the order when signing started. Trezor Bridge had already dropped the session, so the device never signed. The estimates before that sign did not create orders. Greptile's created-fee note describes the code. The confirm screen shows the estimate and the swipe pays the order fee. On the completed transfers those amounts matched. A quote that moves between the estimate and the order would be paid without a second look. This PR leaves that guard out. iPhone 17: the first transfer did the same thing. Confirm and the payment were the same: 226 853 to spending, 1 477 service fee, 281 network fee, 228 611 total. Twin synonymdev/bitkit-ios#741: same behavior. No findings. Checked and clean
QA LGTM |
bdaea52 to
ca6b62c
Compare
|
Updated in ca6b62c to address the latest review:
Validation passed: |
jvsena42
left a comment
There was a problem hiding this comment.
Delta since bdaea521 (rebased, now ca6b62c80). The replies are posted on my two threads. Still blocking:
- MEDIUM: system Back from SettingUp still traps the user on SpendingConfirm after a successful pay.
- MEDIUM: an unpaid order (from the fee refresh or a failed pay) survives a same-amount re-quote and is funded without an expiry check.
Fixed and verified:
- A paid order is cleared in
fundPaidOrder, so a same-amount follow-up creates a new order. My HW → HW regression test passes at head, alongside the author's equivalents:TransferViewModelTesthas 87 tests, all passing. - A higher order fee refreshes the fee, the total and the mining plan, and the next swipe pays exactly what is shown. The HW grid updates too.
- The quote entry points gate on active jobs. A new quote cannot land while a pay or sign job is running, because each setter re-checks the job before its state update with no suspension in between.
- HW retry after a failed broadcast still reuses the same signed tx, since the order and the pending tx are kept.
ca6b62c to
19115be
Compare
|
Updated in 5620bef to address the latest exact-head review:
This answers the stale-order and back-navigation threads. |
jvsena42
left a comment
There was a problem hiding this comment.
Delta since ca6b62c80 (19115be70): no findings. Both MEDIUMs are fixed.
OnSpendingFundingPaidnow navigates withpopUpTo<Routes.SpendingConfirm> { inclusive = true }, so Back from SettingUp lands on SpendingAmount. A new Continue there is gated on active jobs only, and the resulting quote clearsisConfirmPaying.onEstimateReadyalways dropsorder, so a new quote from Amount creates a fresh order. Reuse stays within one Confirm or HW Sign visit. The Advanced and Default re-quotes still keep a matching order, which is fine because they run inside that visit. HW retry still reuses the pending signed tx, since a retry goes throughensureSpendingOrder, notonEstimateReady.
19115be to
5620bef
Compare
|
Rebased the reviewed changes onto current master in 5620bef after the base advanced. The order-reuse and back-stack fixes are unchanged. |
jvsena42
left a comment
There was a problem hiding this comment.
Delta since 19115be70 (5620bef61): rebase only. The PR diff against the new merge-base is line-for-line identical to the head I reviewed. No findings.
|
QA reviewed on 5620bef. Since the review on 22 Sep, a higher created fee waits for another confirmation, a new quote drops the previous unpaid order, and Back from the in-progress screen no longer returns to the completed confirm. Ran on Pixel_6. 25% of savings showed 11 291 to spending, 1 192 service fee, 141 network fee, 12 624 total. The swipe opened the in-progress screen. System Back landed on the amount screen, with 11 291 still filled in, not on confirm. Savings shows that transfer as 12 624. The created fee matched the estimate, so the second confirmation did not appear. Twin bitkit-ios#741 is on 43af31d. Not run on a simulator this round. FindingsNo findings. Checked and clean
QA LGTM |
jvsena42
left a comment
There was a problem hiding this comment.
tAck
Not tested: Hardware wallet
Master's synonymdev#1247 removed the order this link named, so the conflicts could only be resolved by reworking the link onto the quote flow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #1243
Description
This PR creates a Blocktank channel order only when the user confirms a transfer from savings to spending.
Counterpart: synonymdev/bitkit-ios#741
Out of Scope
Design
N/A — no design available.
Preview
android-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
TransferViewModelTest.ktcovers estimate-only navigation, higher-fee reconfirmation, same-confirmation reuse, new-quote replacement, funded-order removal, confirmation guards, and Native SegWit funding sizing.ContentViewTest.ktcovers removing Spending Confirm from the back stack after successful funding.just compile,just test, andjust lint; Detekt completed with 16 existing findings outside the PR files.@transfer_1passed with default and custom receiving capacity.