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.
|
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>
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.
|
|
||
| private var shouldAutomaticallyPay: Bool { | ||
| preparingRequest == nil && app.contactPaymentContext?.isInitialSubscriptionPayment == true && app.selectedWalletToPayFrom == .lightning && | ||
| preparingRequest == nil && app.contactPaymentContext?.isInitialSubscriptionPayment == true && |
There was a problem hiding this comment.
[MEDIUM] Manual coin selection ends on First Payment Failed
Trigger: accept a subscription whose first period is paid on-chain while Advanced coin selection is Manual and no coins are selected.
shouldAutomaticallyPay now includes that on-chain payment. submitPayment(isAutomatic:) still calls requireManualCoinSelection, which opens the UTXO picker and throws CancellationError so a manual swipe can reset. startAutomaticPaymentIfNeeded treats every CancellationError as a failed payment and pushes First Payment Failed on top of the picker. Retry starts the same automatic path, so it fails again until coin selection is changed or the period is paid later by hand.
The subscription is accepted and no transaction is broadcast. The payer cannot choose coins for this automatic payment.
Expected: open the coin picker and continue the payment after coins are chosen, as a manual on-chain send already does and as Android does at 02420758 (prepareCoinSelection navigates to coin selection and returns).
Evidence basis: source analysis at 51e143ab. SendConfirmationSwipeTests expects this cancellation to stop submission without authorizing payment; it does not cover the automatic subscription caller.
| setupTask = Task { | ||
| defer { | ||
| if !Task.isCancelled { | ||
| isSetupPending = false |
There was a problem hiding this comment.
[MEDIUM] Deferred Lightning fallback skips the hardware wallet
Trigger: accept a subscription while Lightning channels exist but none are usable, or while the node is not running yet. Hot savings cannot cover the amount, and a paired hardware wallet can.
Scan keeps the unified invoice on Lightning in those states. setUpSend runs selectHardwareFundingSourceIfNeeded only when the wallet is already on-chain, then clears isSetupPending. The later fallback in validatePaymentAfterSync switches to on-chain without selecting hardware. Automatic payment then sends from savings and lands on First Payment Failed. Retry scans the same way while channels stay unusable, so it repeats. The balance check that kept the sheet open already counted the hardware balance.
Expected: after falling back to on-chain, select a hardware wallet that can pay and show its confirmation, which is what happens when scan chooses on-chain before the sheet opens. Android at 02420758 switches the pay method and then calls selectHardwareFundingSourceForAmount before the automatic start.
Evidence basis: source analysis at 51e143ab. The new setup wait does cover hardware selection when the payment is already on-chain at setup.
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.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 feeSendConfirmationViewTests,swiftformat --lintandnode scripts/validate-translations.js— pass locally