feat: add Paykit subscriptions - #685
Conversation
This comment has been minimized.
This comment has been minimized.
fee8601 to
01c4164
Compare
a8d71d0 to
059eb83
Compare
01c4164 to
bcf1d8a
Compare
059eb83 to
69a0746
Compare
69a0746 to
3a1bcd2
Compare
|
Restacked on the updated payment-proof branch at c3f2d8c. The subscription layer is unchanged; all commits remain signed, and the complete payment-proof suite (including recurring periods and uncertain on-chain recovery) passes 22/22. |
c3f2d8c to
281b865
Compare
3a1bcd2 to
cf4cea2
Compare
|
Restacked again on the final signed payment-proof hardening head |
281b865 to
e66771b
Compare
d68a602 to
c274b0a
Compare
|
The subscriptions branch is now correctly restacked on the current signed #683 head ( |
c274b0a to
e641682
Compare
|
Restacked onto #683 signed head Post-restack simulator verification passes 134/134 across |
|
@ben-kaufman conflicts |
4656cef to
9136de4
Compare
jvsena42
left a comment
There was a problem hiding this comment.
Reviewed this as unattended-spending risk and tried hard to break the consent model. It holds. Despite the framing, this is not unattended: every billing period is materialised as a pending payment request and paid through the normal send sheet with a swipe. The single auto-submit (two sites, SendConfirmationView:302 and LnurlPayConfirm:158) is the first period, immediately after the user swipes "Subscribe and pay".
Specifically verified: the amount is pinned at proposal and re-checked msat-exact at send time on both rails and cannot be bypassed by the auto path; accept(_:) requires full struct equality so terms cannot drift between review and acceptance; a paymentStarted proof is persisted before the send on both rails and a persistence failure aborts the broadcast; fiat-denominated subscriptions are rejected; catch-up after a long absence yields N swipe-gated requests, never a batch payment; restore from seed resumes subscriptions only as pending requests. A post-accept TOCTOU on the terms is not exploitable — paykit-rs marks a record invalid when a second proposal reuses an ID.
Port fidelity against Android #1186 is high: recurrence math, caps, retry ladder, lifecycle transitions and notification scheme all match semantically.
One medium on the auto-start's cancellation behaviour, plus low items. Note the medium rests on NavigationStack cancelling the root view's .task on push, which I reasoned about but could not execute — worth a device check with PIN-for-payments on and biometrics off.
|
Review fixes pushed in signed commit The orphan started-proof recovery thread remains open intentionally: absence from LDK history is not authoritative evidence that payment was never dispatched, so clearing it on a timer is unsafe. The matching Android fee-confirmation change is still local because the pinned private Maven dependencies return HTTP 401 during compile/tests. Latest CI is pending; this is not a claim that all remaining validation/recovery issues are closed. |
ovitrif
left a comment
There was a problem hiding this comment.
Two non-blocking issues in the send flow, both in code this PR adds.
The uncertain on-chain branch routes to the pending screen with a Paykit payment request id in the paymentHash slot, so the activity lookup it drives can never match. The recipient row in CreatePaymentRequestView renders the same truncated pubkey twice.
The orphaned-Lightning-proof concern raised earlier on PaykitPaymentProofService reads as addressed at this head: beginLightningPayment now pivots on whether the submission error is definite, sendWithTimeout only reports afterListening once lightningService.send has returned, and both directions have test coverage.
jvsena42
left a comment
There was a problem hiding this comment.
Reviewed for fund-draining specifically, cross-checking against the Android twin (synonymdev/bitkit-android#1186) where I confirmed a wedged-proof bug. No fund-draining bug here, and all 12 of my earlier threads are genuinely fixed at head — I adversarially spot-checked two rather than taking it on faith:
SendPendingScreen.paymentHashis nowString?(:27) withsearchForActivityreturning early at:133andapplyPendingResolutionIfNeededguarding at:106, while the request-ID path still resolves viaonchainPaymentResolutionPublisher— no 24s futile lookup.PaykitPreciseInstant.init?(timestamp:)(PaykitSubscription.swift:14-29) now parses the whole canonical string with fractional-seconds then plain fallback and derivessecondsfrom the parsed instant, so+01:00with a fraction yields the correct instant.
The fund-safety properties I tried hardest to break, all sound: amount is pinned at pay time (validateIncomingPaymentRequestAmounts pins sats and bolt11 msat; the LNURL path is covered by bitkit-core's validate_lnurl_pay_invoice); no renewal auto-pays — shouldAutomaticallyPay is Lightning-and-initial-only and every later period needs a swipe; prepare() throws operationInProgress while a started proof exists; proofs are identity-tagged and re-checked live; dismissSubscriptionPayment persists before mutating; and the derived acceptedAt fallback can never back-bill earlier than the first paid period.
One non-blocking classifier gap inline. Deliberately not re-raising the wedge itself (settled on my PaykitPaymentProofService.swift:500 thread), the Lightning auto-pay product call (settled in 8338720a), the decode-error wipe (my own review body), or the partial-first-period question (asked once on the Android twin — one cross-repo thread is enough).
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed at 92376ab6. One MEDIUM inline — not blocking, since paykitUiEnabled defaults off. It's a residual gap in 8338720a rather than a reversal of it: that commit stops auto-pay from starting on the on-chain rail, and this is auto-pay already dispatched with the rail flipping underneath it.
The 92376ab6 fix itself is correct. isDefiniteOnchainPreBroadcastFailure moved into the service with .WalletOperationFailed, .PersistenceFailed added, while the Lightning classifier still lists only .NotRunning, .InvalidInvoice, .InvalidAmount, .PaymentSendingFailed — so PersistenceFailed stays uncertain on the LN side, which is the right asymmetry and is pinned by the table test. The hardware path keeps its own definite/uncertain split.
No background payment path, which was the first thing I checked given a scheduler is involved. The only reconcile() caller is inside refreshIncomingPaykitPaymentRequests, which guards on the flag; polling is scenePhase == .active only; the notification path schedules local UNNotificationRequests and a tap funnels back through the same gated refresh.
Recurring-spend safety traced clean. Amount is pinned at pay time on all three rails, with acceptsPaymentAmount exact and the bolt11 msat comparison overflow-guarded. No renewal ever auto-pays — shouldAutomaticallyPay requires isInitialSubscriptionPayment, which is only set by SubscriptionSheet.accept or an embedded retry after a swipe or tap. Double-pay is blocked by prepare() throwing operationInProgress on any started or completed proof, and completeOnchainPayment requires paymentStarted. Each period has its own identity via billingPeriodStartsAt, paid periods drop out through record.paymentProofs plus local completedRequestProofKinds, and cancellation purges pending periods while a concurrent associate/markStarted then throws pre-dispatch. Terms can't drift because accept requires full struct equality, and every/amount are overflow-guarded with the boundary index bounded.
Also clean: identity is pinned across awaits by generation plus PubkyPublicKeyFormat.matches at every commit point, the notification target checks payerIdentity, isSigning covers the hardware beforeBroadcast window so no dismissal can cancel between mark-started and broadcast, and nothing seed-derived reaches persisted state, userInfo, or logs. PaykitSubscriptionState is enum-free, so there's no unknown-case decode failure.
Two minor things I'm noting rather than filing: markOnchainPaymentStarted does 1+N FFI calls per on-chain request send (matchingTransactionIds calls getTransactionDetails per outbound payment, all serialised on the core queue) — fail-closed so no fund risk, latency only, and the Android twin already fixed it in 050b32b36 by using a single filtered activity query. And initialSubscriptionPaymentRequestIds is unreachable: it's inserted only via requestPresentation(_, isInitialSubscriptionPayment: true), whose sole production caller needs a non-embedded sheet whose context already carries the flag, and the only non-embedded source reads from this same set — circular, so never populated. Android deleted its equivalent in ed030dac7.
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed the delta at 559e3008d. The rail-switch defect is fixed — trace on the resolved thread. No HIGH/MEDIUM. Two notes, neither blocking.
The 16 lines removed from PaykitPaymentRequestService are safe to remove, and I checked this specifically because deleting tracking state while fixing a race is where collateral usually hides. initialSubscriptionPaymentRequestIds and its consumers only served the non-embedded retry branch in retryIncomingPaymentRequest. At head every isInitialSubscriptionPayment: true context is created inside the .subscription sheet, and those retries take the embedded branch; the only non-embedded SendSheet gets its contexts from AppScene, which no longer sets the flag. So that state was dead by construction — which also means my earlier AppScene.swift:904 thread is moot rather than regressed. Worth noting since it read as a behaviour change in the diff.
On the new test — SendConfirmationViewTests exhausts the 2x2x2 truth table of requiresManualConfirmation, so the predicate is locked. It doesn't cover what actually made this a bug, which was the placement of the check relative to the suspension points; that's protected only by reading the code. I don't think a view-level ordering test is practical here, so I'd just flag it as the thing to be careful of in future edits: moving either call site earlier than its current position reopens the window.
One optional nit: :649 runs after the PIN/biometric block, and since the flip typically lands while parked on the balance alert, the flipped path is alert → PIN → dropped to on-chain confirm → swipe → PIN again. Evaluating requiresManualConfirmation once at the top of submitPayment() as well would short-circuit the common case before auth, keeping :649 and :716 as the authoritative checks. Purely a UX smoothing — the current placement is the correct one for safety, and it's what I asked for.
There was a problem hiding this comment.
QA Notes
Ran the six Manual Tests on an iPhone 17 simulator (iOS 26.5) paired with a second iPhone 17 simulator as the Paykit counterparty, both on regtest, from a build of this head.
- Immediate-start proposal stays in one continuously presented sheet from review through payment to the subscribed state.
- Initial-payment failure keeps the sheet presented with the first period still due, and
Retry Paymentre-enters the flow in place. - Lightning and on-chain subscription payments both settle with the matching purple and orange success treatment.
- A future-start proposal correctly takes nothing on acceptance, schedules its due notifications on the billing boundaries, and tapping the delivered
Subscription Payment Duenotification opens exactly that billing period's payment, which then completes. - A due request dismissed from the failure sheet reopens from both the bell queue and the Payments tab and pays with fresh private payment details.
- Active, expired, fixed-term and open-ended subscription details all render, and cancelling an active subscription works.
Both findings from the previous review are fixed. The uncertain on-chain branch no longer passes a Paykit request id as a payment hash, so the pending screen stops polling for an id that could never match, and the recipient row without a saved contact now renders the truncated key once instead of twice.
Approve.
—
Reviewed by Claude Code (claude-opus-5 high) via gh-pr-review-loop skill
ovitrif
left a comment
There was a problem hiding this comment.
signing off on the bot approval
Description
Adds the payer side of Paykit subscriptions, building on the merged payment-proof work in #683.
This PR intentionally covers payer-side receipt only. Subscription creation, Discover, automatic payment without confirmation, and renewal are not included because they are outside this MVP or are not currently exposed by Paykit.
Dependencies:
0.1.0-rc46Linked Issues/Tasks
Screenshot / Video
Final iOS and cross-platform proof recordings were completed locally; they are not attached to the PR.
QA Notes
Manual Tests
Automated Checks
git diff --checkKnown limitation
Full recovery of orphaned Lightning submissions is tracked in LDK Node #116. A missing payment-history record is not proof of failure, so unknown started attempts remain protected and may block subscription cancellation. The latest submission-error fixes and first-period disclosure are covered by automated regressions. The earlier recordings/manual checks predate these review fixes; no new crash-injection E2E or UI recording was performed for this update.
Latest review follow-up (
559e3008): an automatic first Lightning payment that changes to savings or hardware during warnings, authentication, or request preparation returns to the existing fee-details screen for a fresh swipe. The broaderBitkitTestsrun was stopped because the existing address-type integration tests received HTTP 404 from the Blocktank regtest funding endpoint. The focused suite passed; the live timing reproduction was not rerun for this update.