Repository navigation
Conversation
fbc5a5b to
e69f9ed
Compare
|
talosmachina
left a comment
There was a problem hiding this comment.
No findings. iOS port of synonymdev/bitkit-android#1422: shouldAutomaticallyPay and requiresManualConfirmation no longer require Lightning funding, and InitialSubscriptionPaymentProgress now renders the shared SubscriptionReviewContent with a confirmed swipe. Reviewed e69f9ed, full tier, reasoned from the code and CI: iOS does not build on this box.
What I checked, and 4 candidates I ruled out
Read in full: InitialSubscriptionPaymentProgress.swift, SwipeButton.swift, the new SubscriptionReviewContent and the review route in SubscriptionsView.swift, the automatic-payment path of SendConfirmationView.swift (startAutomaticPaymentIfNeeded, submitPayment, performPayment)
CI: Greptile and change detection green; Run Tests and build-local were still pending at review time
Ruled out
- On-chain auto-pay with no fee rate:
submitPaymentcallswallet.setFeeRatewhenisFeeRateMissing, and a failure there throws into the.failureroute ofstartAutomaticPaymentIfNeeded, so it never sends at an unset rate. - Hardware wallet paying without the device:
shouldAutomaticallyPaystill excludeshwSend.isActive, andrequiresManualConfirmationreturns true for a hardware payment, asSendConfirmationViewTestsasserts. - Missing environment for the new
@Environment(PaykitPaymentRequestManager.self): both hosts,SendConfirmationViewandLnurlPayConfirm, already read the same environment object, so it is in scope wherever the progress view is shown. isConfirmedleaking into other swipes: it defaults to false and onlySubscriptionReviewContentsets it, fromisPaying.
Merge confidence: 4/5, no findings, but Run Tests and build-local were still pending and the branch was not built here.
a58b830 to
8ef8bc1
Compare
|
Two independent reviews, nothing blocking a merge. worth doing, does not block
nits
|
8ef8bc1 to
d3a9962
Compare
|
Went through the review.
|
d3a9962 to
51e143a
Compare
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Re-review of the diff since e69f9ed: the branch was rebased onto the new base, and the four original commits are unchanged (git range-diff), so the delta is 1d5cffd9, 177782d8, 9935eb07 and the journey and changelog commits. Reviewed 51e143a, full tier, reasoned from the code and CI: iOS does not build on this box.
What I checked, and 4 candidates I ruled out
Read in full: setUpSend and cleanup in SendSheet.swift (the new isSetupPending), in SendConfirmationView.swift the .task and the new .onChange(of: isSetupPending), startAutomaticPaymentIfNeeded, submitPayment(isAutomatic:), settleTransactionFee, calculateTransactionFee, automaticPaymentLacksFee and showManualConfirmation, the SubscriptionProviderCard change, and both subscription journeys
Earlier findings: I had none. Greptile's two P1s on e69f9ed are fixed in the code. The automatic start now waits for isSetupPending to clear, which happens after selectHardwareFundingSourceIfNeeded. The fee warnings now run after settleTransactionFee(), and a zero on-chain fee falls back to manual confirmation
@coreyphillips's missing-fee-rate point: submitPayment now calls showManualConfirmation() before setFeeRate when isFeeRateMissing, so an outage no longer lands on First Payment Failed
CI: validate and change detection green; Run Tests, Run Integration Tests and build-local still pending at review time
Ruled out
- Automatic payment re-firing after the manual fallback:
showManualConfirmationsetsrequiresPaymentConfirmation, which makesshouldAutomaticallyPayfalse, so theisSetupPendingchange and later fee updates cannot restart it. isSetupPendingstuck true: thedeferskips the reset only when the task was cancelled. Both cancel sites either start a new task that sets it again (setUpSend) or tear the sheet down (cleanup).settleTransactionFeenever converging: after three tries it returns with whatever fee it has, and a fee of 0 on-chain goes to manual confirmation rather than sending.- Hardware wallet picked after the decision: the
.taskseesisSetupPendingand does not start; theonChangere-evaluatesshouldAutomaticallyPaywithhwSendalready active and opens the manual confirmation.
Merge confidence: 4/5, no findings, but the fee-rate fallback has no unit test (the PR says so), the test and build jobs were still pending, and the branch was not built here.
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Reviewed the full PR diff against its merge base, at 51e143ab. There was no saved baseline. Android bitkit-android#1422 at 02420758 was compared only for this first-subscription payment, not reviewed as a whole.
2 actionable findings — resolve or provide an evidence-backed rebuttal.
The fee-rate fallback and the wait for hardware selection when the payment is already on-chain both hold in this revision. Two gaps remain where that automatic path still does not match a manual send or the Android port: manual coin selection, and hardware selection after Lightning is deferred and later falls back to on-chain.
Run Tests, Run Integration Tests, and build-local passed on this revision. This review inspected SendConfirmationViewTests and did not run them locally. Device testing: not performed in this review.
Suggested additional test cases
- iOS. Advanced coin selection set to Manual. On-chain savings cover a 5,000 sat subscription plus fee, with no usable Lightning balance. Accept a proposal whose first period is due. The coin picker opens, and First Payment Failed does not. After coins are chosen, the payment sends and Subscribed appears.
- iOS. Lightning channels exist but none are usable. Hot savings are below the amount, and a paired hardware wallet can cover the amount plus fee. Accept the same proposal. The hardware send confirmation opens, and First Payment Failed does not. Retry while those channels stay unusable does not repeat the failure.
- iOS. On-chain savings only, with fee estimates unavailable. Accept the proposal. The manual confirmation opens with SendConfirmRetryFeeRate, and First Payment Failed does not.
Findings
- [MEDIUM] Manual coin selection ends on First Payment Failed — inline at
Bitkit/Views/Wallets/Send/SendConfirmationView.swift:195. - [MEDIUM] Deferred Lightning fallback skips the hardware wallet — inline at
Bitkit/Views/Wallets/Send/SendSheet.swift:384.
|
Manual coin selection after 83f77dd (iPhone 17 simulator, regtest, savings only, 5,000 sat proposal): swipe → coin picker → choose a coin → Continue → regular confirmation waiting for ios_manual_coins.mp4 |
|
Retested on current head The new diagnostic logs identify the fallback trigger for this reproduction (UTC): The fee rate becomes available 133 ms after the fallback. In Please keep the automatic flow waiting while fee setup is still in progress, reserving manual fallback for a genuine failure or a case that requires another confirmation. This normal one-swipe case remains failing on the current head; I am holding iOS approval pending a fix and retest. Recording of this retest: ios-double-swipe.mp4 |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
83f77dd to
f907ff9
Compare
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Re-reviewed the full PR at f907ff97 after the comparison base changed since the saved assessment. No prior coverage was inherited.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
A normal on-chain savings payment can still leave the acceptance swipe and open the detailed confirmation while the fee rate is loading. SendConfirmationView.swift and SendSheet.swift are unchanged from 83f77dd4, where that second swipe was recorded. Choosing coins still opens the picker, and the confirmation after that choice waits for its own swipe. Android bitkit-android#1422 at f729fab8 was compared only for this first payment: awaitAutomaticOnchainFee waits for the in-flight fee refresh before it requires confirmation.
Run Tests and Run Integration Tests passed on this revision. This review read SendConfirmationViewTests and did not run them. Device testing: not performed in this review.
Suggested additional test cases
- iOS. Lightning channels exist and none are usable. Savings cannot cover 5,000 sats plus the fee, and a paired hardware wallet can. Accept a proposal whose first period is due. The hardware confirmation opens, and no savings transaction is broadcast before the device signs.
- iOS. On-chain savings only, after fee estimates have failed and are not still loading. Accept the same proposal. The manual confirmation opens with SendConfirmRetryFeeRate. A fee rate that is only slow to arrive stays on the loading review.
Findings
- [MEDIUM] On-chain subscription payment asks for a second swipe while the fee is still loading — inline at
Bitkit/Views/Wallets/Send/SendConfirmationView.swift:786.
| private func submitPayment(isAutomatic: Bool = false) async throws { | ||
| guard preparingRequest == nil, walletSwitchContext == nil else { throw CancellationError() } | ||
| if isAutomatic, isFeeRateMissing { | ||
| showManualConfirmation(reason: "fee rate missing") |
There was a problem hiding this comment.
[MEDIUM] On-chain subscription payment asks for a second swipe while the fee is still loading
Trigger: accept a subscription whose first period is paid from on-chain savings with Autopilot coin selection, while the send sheet has not finished selecting a fee rate.
isSetupPending starts false, and setUpSend sets it true only after the first render. startAutomaticPaymentIfNeeded treats false as permission to decide. The confirmation .task uses preparingRequest?.id as its identity, so it keeps running with the isSetupPending value from the render that started it. It can enter submitPayment while loadFeeRateWithRetry is still in flight. There, a nil on-chain rate takes the fee rate missing fallback. The swipe loading state treats that same nil rate as loading until feeRateLoadFailed is set; this fallback does not. showManualConfirmation sets requiresPaymentConfirmation, so when setup later finishes and the rate is present, onChange finds automatic payment already disabled and logs not automatic after setup.
The acceptance swipe ends on the detailed savings confirmation, and a second swipe is required. The subscription is already accepted. In the reported run the second swipe paid 5,000 sats plus the fee once.
Expected: keep waiting while fee setup is in progress, including funding selection. Open the manual confirmation when the load has failed or finished without a rate, or when hardware signing, manual coins, or a fee warning requires another confirmation. A normal savings payment should reach Subscribed from the original swipe.
Evidence basis: source analysis at f907ff9. Those two send files match 83f77dd, where the simulator log shows fee rate missing at 19:12:14.723 and Selected fee rate 133 ms later, then not automatic after setup (comment). This review did not run that path. SendConfirmationViewTests covers the fee and coin predicates and does not cover this setup ordering.
There was a problem hiding this comment.
Confirmed and fixed in 3cfc918 and b2ae7e1. Your fee rate missing → Selected fee rate → not automatic after setup log on 83f77dd matches the code path.
Cause. isSetupPending is a plain property of SendConfirmationView, so the .task that starts the automatic payment reads the value from the render that created it. SendSheet only set it to true inside setUpSend, which runs from its own .task after the first render, so that first render carried false and the guard in startAutomaticPaymentIfNeeded passed while the fee rate was still loading. submitPayment then took the missing-rate fallback, which sets requiresPaymentConfirmation for good.
Changes.
- 3cfc918:
isSetupPendingstarts true inSendSheet, so a confirmation rendered beforesetUpSendruns does not read setup as finished. The automatic start then comes fromonChange(of: isSetupPending)once the fee rate has loaded and hardware funding selection is done. - b2ae7e1: in
submitPayment, an automatic payment with a missing rate opens the manual confirmation only whenwallet.feeRateLoadFailedis set. While the rate is loading it returns without disabling automatic payment, and the start runs again when setup finishes. This is the same missing-and-not-failed condition the swipe already treats as loading.
A failed fee load still opens the manual confirmation with SendConfirmRetryFeeRate: setup ends, the start runs, the rate is missing and feeRateLoadFailed is true. Hardware wallet, manual coins and the fee warning are unchanged.
Test. testAutomaticPaymentAwaitsFeeRateOnlyWhileItIsLoading in SendConfirmationViewTests.swift. 31 tests across SendConfirmationViewTests, SendConfirmationSwipeTests and WalletViewModelSendTests pass on the iPhone 17 simulator (checked in the xcresult that the new case executed). It pins the wait/fallback decision only. The render ordering lives in the views and has no unit test.
Not verified on a device. I did not drive the flow this round: it needs the Android issuer, and the emulators are in use. I also never reproduced the second swipe locally, so a passing run of mine would not prove much. Leaving this thread open for your retest; the expected log after one swipe has no Opening manual confirmation line.
Android. bitkit-android#1422 does not have this: awaitAutomaticOnchainFee joins the in-flight fee refresh before it decides.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| .onChange(of: isSetupPending) { _, isPending in | ||
| guard !isPending, preparingRequest == nil else { return } | ||
| Task { @MainActor in | ||
| if app.contactPaymentContext?.isInitialSubscriptionPayment == true, !shouldAutomaticallyPay { | ||
| showManualConfirmation(reason: "not automatic after setup") | ||
| } | ||
| await startAutomaticPaymentIfNeeded() |
There was a problem hiding this comment.
First payment starts too early
When an initial subscription payment falls back from Lightning to on-chain while confirmation is already open, funding selection and onSwitchToOnchainWallet run separately. Funding selection can finish while the wallet switch is still choosing coins. Clearing isSetupPending then starts automatic payment, but submitPayment throws because walletSwitchContext is still set. The user sees First Payment Failed even though setup is simply unfinished.
Wait for both tasks before starting payment, and retry the automatic start when the wallet switch finishes.
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up review of the changes since f907ff97, at b2ae7e15. Baseline: previous QA review. This pass rechecked the initial on-chain fee wait, the setup-pending flag, the new predicate test, and Android's fee wait at f729fab8. Inherited coverage: manual coin selection and the confirmation after coins are chosen; review sheet layout, swipe label, and in-payment overlay; payment warnings, PIN, and hardware confirmation on the automatic path; journeys review-and-subscribe and cancellation-during-confirmation.
1 actionable finding — resolve or provide an evidence-backed rebuttal.
The second-swipe report is closed for the first setup. isSetupPending starts true, and a missing rate opens the manual confirmation only after feeRateLoadFailed is set. That matches Android awaitAutomaticOnchainFee, which joins the in-flight fee refresh before it decides. Run Tests passed on this revision. This review read the new predicate and did not run it. Device testing: not performed in this review.
Retry after First Payment Failed clears the fee rate and leaves the automatic start waiting for a setup pass that does not run again. The open wallet-switch comment still matches this head: clearing the funding wait can start payment while walletSwitchContext is set, and that cancellation opens First Payment Failed. These two commits leave that path as it was.
Suggested additional test cases
- iOS simulator, regtest, on-chain savings only, Autopilot coin selection. Accept a proposal whose first period is due while fee estimates are slow. One swipe keeps the review layout loading, the log has no
Opening manual confirmation for first subscription payment, and Subscribed appears. - iOS simulator, regtest, on-chain savings only, with payment PIN enabled. Accept the same proposal, cancel the PIN prompt so First Payment Failed appears, then tap Retry. The fee rate loads again and the payment continues, or the manual confirmation shows
SendConfirmRetryFeeRateand can complete. The review overlay does not remain loading.
Findings
- [MEDIUM] Retry leaves the first payment waiting after the fee rate is cleared — inline at
Bitkit/Views/Wallets/Send/SendConfirmationView.swift:788.
| if isAutomatic, isFeeRateMissing { | ||
| guard !Self.automaticPaymentAwaitsFeeRate(isFeeRateMissing: isFeeRateMissing, feeRateLoadFailed: wallet.feeRateLoadFailed) else { | ||
| // The rate is still loading; the automatic start runs again when send setup finishes. | ||
| hasStartedAutomaticPayment = false |
There was a problem hiding this comment.
[MEDIUM] Retry leaves the first payment waiting after the fee rate is cleared
Trigger: the first on-chain subscription payment reaches First Payment Failed, including when the payer cancels the PIN prompt or a fee warning, and they tap Retry.
Mechanism: retryEmbeddedInitialSubscriptionPayment calls wallet.resetSendState, which sets the selected fee rate to nil and feeRateLoadFailed to false. It does not run setUpSend again, so isSetupPending stays false. The recreated confirmation calls startAutomaticPaymentIfNeeded. automaticPaymentAwaitsFeeRate is then true, and this branch clears hasStartedAutomaticPayment and returns. The restart path is onChange(of: isSetupPending) when setup finishes. Setup does not run again, and the fee-rate onChange only recalculates the fee. testAutomaticPaymentAwaitsFeeRateOnlyWhileItIsLoading checks the predicate and does not cover a completed setup that will not run again.
Consequence: the Review & Subscribe overlay stays in its loading state. requiresPaymentConfirmation stays false, so the manual confirmation and SendConfirmRetryFeeRate stay hidden. The accepted subscription's first payment does not send from this retry. The payer can dismiss the sheet and pay that period later from the payment-due notification.
Expected: retry loads the fee rate again, or a missing rate with no load in progress opens the manual confirmation and its fee retry.
Evidence basis: source analysis at b2ae7e15. This review did not run the retry path.

Closes #920
Stacked on #880. iOS port of synonymdev/bitkit-android#1422.
This PR makes accepting a subscription a single swipe when the first payment is paid on-chain, matching the Subscriptions flow in Figma and the existing Lightning behaviour, and brings the Review sheet closer to the frame.
Description
Swipe To Subscribe & Pay. Figma shows one swipe from Review & Subscribe to Subscribed, and its flow notes do not limit that to Lightning.SendConfirmationView.shouldAutomaticallyPayandrequiresManualConfirmationno longer require Lightning funding, so the first payment due on acceptance starts by itself for on-chain savings too. A hardware wallet still opens the send confirmation, because the device has to sign.InitialSubscriptionPaymentProgressreplaced it with a bare progress indicator; that remains only as the fallback when the subscription cannot be found.Swipe To Subscribe, as in Figma, whether or not a first payment is due.First billing period ends … Each period is charged in full.line, which is not in the Figma frame.SubscriptionCounterpartyidentifier.SwipeButtongainsisConfirmed, which draws the knob at the end of the track for a swipe that was completed on a previous screen.Opening manual confirmation for first subscription payment: …).Out of Scope
SubscriptionsView.swift: the FigmaAutomatically pay this subscriptionswitch. Follow-up tracked in Subscriptions: unbuilt controls and the swipe colour rule #755 (item 3): neither platform has an auto-pay setting yet, so later periods are still paid by hand from the Subscription Payment Due notification.SubscriptionsView.swift: sending the first payment in the background so the swipe only waits for the Paykit request. Follow-up, noted in Subscriptions: unbuilt controls and the swipe colour rule #755, as on Android.Design
Preview
iPhone 17 simulator on regtest, fresh wallet with on-chain savings only, proposal sent from an Android emulator wallet. 5,000 sats plus a 143 sat fee left the wallet with one swipe.
Recording of the single swipe on the current head, in real time: just under a minute from the swipe to Subscribed on the simulator. It took about three minutes before the Paykit runtime updates.
ios_subscription_single_swipe_v4.mp4
QA Notes
Journeys
review-and-subscribe.xml— with the first period due on acceptance and on-chain savings only, one swipe keeps the Review and Subscribe layout with the swipe loading and ends on Subscribed with no second swipecancellation-during-confirmation.xml— the first period is now paid on acceptance, so the journey advances the subscription clock and cancels while the next unpaid period is on the send confirmation; not driven on a deviceManual Tests
N/A
Automated Checks
SendConfirmationViewTests.swift— an automatic payment requires manual confirmation only for hardware fundingSendConfirmationViewTests.swift— an automatic payment lacks a fee only on-chain with no calculated fee, and manual coin selection is needed only for on-chain savings without selected coins, and manually chosen coins are detected only for on-chain savings with manual selectionSendConfirmationViewTests.swift— an automatic payment waits for the fee rate only while it is loading, not after the load failed; the setup ordering in the views has no unit test and the fix at b2ae7e1 was not driven on a deviceSendConfirmationViewTests,swiftformat --lintandnode scripts/validate-translations.js— pass locallytestCustomFeeWalletSwitchSurvivesFeeScreenNavigationinSendConfirmationLifecycleTests.swiftfails locally on this branch and on master alike