Skip to content

feat: add Paykit subscriptions - #685

Open
ben-kaufman wants to merge 20 commits into
masterfrom
codex/paykit-subscriptions
Open

feat: add Paykit subscriptions#685
ben-kaufman wants to merge 20 commits into
masterfrom
codex/paykit-subscriptions

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Description

Adds the payer side of Paykit subscriptions, building on the merged payment-proof work in #683.

  • Receives private subscription proposals and lets the payer review and confirm them.
  • Discloses the first billing period’s end date/time and full-period charge on Review, including short initial intervals.
  • Requires an immediately due first period to be paid before showing the subscribed success state; definitely failed payments remain due and can be retried, while uncertain submissions stay protected until their outcome is known.
  • Turns each unpaid billing period into the existing incoming payment-request flow, including automatic presentation, the bell queue, and the Payments tab.
  • Schedules local due-payment notifications and opens the corresponding request when the notification is selected.
  • Adds the Figma-aligned Subscriptions and Payments tabs, subscription detail/status views, cancellation, empty states, and fixed/open-ended summaries.
  • Shows a normalized Monthly Cost across all active subscriptions, independent of whether the current periods are already paid.
  • Associates payment proofs with their exact billing period so recurring periods remain independent while retaining private-only resolution and fresh Private Payment List requirements.
  • Uses the shared monetary text component for primary and secondary request amounts, with visible currency symbols in either display mode.
  • Keeps subscribe-and-pay inside one continuously presented sheet, including initial-payment failure and retry, without the modal dismissal/reappearance gap.
  • Retains the settled payment rail in completed request and subscription history so Lightning feedback is purple and on-chain feedback is orange, including success confetti.

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:

Linked 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

  • Receive an immediate-start private subscription proposal and verify review → payment → success stays in one continuously presented sheet.
  • Exercise initial-payment failure and retry and verify the sheet stays presented while the first period remains due.
  • Complete Lightning and on-chain subscription payments and verify purple/orange success confetti and matching completed-payment icons.
  • Receive a future-start proposal, trigger its due notification, and open the exact billing-period payment.
  • Dismiss and reopen a due request from the bell queue and Payments tab, then pay it with fresh private payment details.
  • Review active, expired, fixed-term, and open-ended subscription details and cancel an active subscription.

Automated Checks

  • 143 focused currency/formatting/confirmation/request/proof tests passed after the latest UI fixes, including the automatic/manual funding confirmation policy, on-chain pre-broadcast failure classification, and started-proof cleanup
  • Rendered and inspected both changed request amount headers in Bitcoin-primary and fiat-primary modes using the app’s dark appearance. Both units and symbols are visible; temporary rendering probes were removed.
  • E2E regtest Debug build for an arm64 iOS simulator
  • Exact build installed and cold-launched on an iOS simulator
  • SwiftFormat validation for all 13 previously changed Swift files and both files in the latest amount-header fix
  • Localization validation
  • git diff --check

Known 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 broader BitkitTests run 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.

@greptile-apps

This comment has been minimized.

Comment thread Bitkit/Services/PaykitPaymentProofService.swift
Comment thread Bitkit/Services/PaykitPaymentProofService.swift Outdated
@ben-kaufman
ben-kaufman force-pushed the codex/paykit-payment-proofs branch from fee8601 to 01c4164 Compare August 28, 2026 14:13
@ben-kaufman
ben-kaufman force-pushed the codex/paykit-subscriptions branch from a8d71d0 to 059eb83 Compare August 31, 2026 13:10
@ben-kaufman
ben-kaufman force-pushed the codex/paykit-payment-proofs branch from 01c4164 to bcf1d8a Compare August 31, 2026 14:25
@ben-kaufman
ben-kaufman force-pushed the codex/paykit-subscriptions branch from 059eb83 to 69a0746 Compare August 31, 2026 14:25
@ben-kaufman
ben-kaufman force-pushed the codex/paykit-subscriptions branch from 69a0746 to 3a1bcd2 Compare August 31, 2026 18:22
@ben-kaufman

Copy link
Copy Markdown
Contributor Author

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.

@ben-kaufman
ben-kaufman force-pushed the codex/paykit-payment-proofs branch from c3f2d8c to 281b865 Compare August 31, 2026 18:30
@ben-kaufman
ben-kaufman force-pushed the codex/paykit-subscriptions branch from 3a1bcd2 to cf4cea2 Compare August 31, 2026 18:30
@ben-kaufman

Copy link
Copy Markdown
Contributor Author

Restacked again on the final signed payment-proof hardening head 281b8654; current signed subscriptions head is cf4cea28. Focused PaykitPaymentProofServiceTests: 21/21 passing.

@ben-kaufman ben-kaufman mentioned this pull request Aug 31, 2026
2 tasks
ovitrif

This comment was marked as resolved.

@ben-kaufman
ben-kaufman requested a review from ovitrif September 1, 2026 03:20
@ben-kaufman
ben-kaufman force-pushed the codex/paykit-payment-proofs branch from 281b865 to e66771b Compare September 1, 2026 12:44
@ben-kaufman
ben-kaufman force-pushed the codex/paykit-subscriptions branch 3 times, most recently from d68a602 to c274b0a Compare September 1, 2026 16:38
@ben-kaufman

Copy link
Copy Markdown
Contributor Author

The subscriptions branch is now correctly restacked on the current signed #683 head (6bcff553); the subscription-only head is c274b0a4, and every commit is verified. After restacking, PaykitPaymentProofServiceTests, PaykitPaymentRequestServiceTests, and PaymentNavigationHelperTests pass 112/112. The earlier details-flow and payment-sheet sequencing fixes remain present. @ovitrif @jvsena42 please re-review the current head; fresh CI is still running.

ovitrif

This comment was marked as resolved.

@ben-kaufman
ben-kaufman force-pushed the codex/paykit-subscriptions branch from c274b0a to e641682 Compare September 1, 2026 17:18
@ben-kaufman

Copy link
Copy Markdown
Contributor Author

Restacked onto #683 signed head 489b1e61; the new subscriptions head is e6416820. The one conflict was in the shared secondary button and was resolved by keeping the subscriptions/Figma-correct shared height plus #683’s shouldExpand behavior. All five subscriptions commits were re-signed.

Post-restack simulator verification passes 134/134 across HwFundingSignerTests, PaykitPaymentProofServiceTests, PaykitPaymentRequestServiceTests, and PaymentNavigationHelperTests.

@ben-kaufman
ben-kaufman requested a review from ovitrif September 1, 2026 17:18
ovitrif

This comment was marked as resolved.

@ben-kaufman
ben-kaufman requested a review from ovitrif September 1, 2026 18:45
ovitrif
ovitrif previously approved these changes Sep 1, 2026

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Base automatically changed from codex/paykit-payment-proofs to master September 2, 2026 10:57
@jvsena42

jvsena42 commented Sep 4, 2026

Copy link
Copy Markdown
Member

@ben-kaufman conflicts

@ben-kaufman
ben-kaufman force-pushed the codex/paykit-subscriptions branch from 4656cef to 9136de4 Compare September 4, 2026 16:32

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread Bitkit/Views/Wallets/Send/SendConfirmationView.swift Outdated
Comment thread Bitkit/Services/PaykitPaymentRequestService.swift Outdated
Comment thread Bitkit/Services/PaykitPaymentProofService.swift
Comment thread Bitkit/Views/Wallets/Send/SendConfirmationView.swift Outdated
Comment thread BitkitTests/PaykitPaymentRequestServiceTests.swift

Copy link
Copy Markdown
Contributor Author

Review fixes pushed in signed commit 8338720a: PIN navigation no longer cancels the authorized initial Lightning payment task; failed subscription-dismissal writes keep the period queued; the initial on-chain/hardware payment uses the existing fee/details confirmation instead of auto-paying; and the two requested manager-level regressions are covered. Final focused suite: 110 tests passed. SwiftFormat, localization validation, and diff checks also passed. The PIN cancellation was reproduced on iOS 26.5 and the fixed navigation probe passed; that probe stopped before dispatch and is not a new full payment E2E proof.

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 ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread Bitkit/Views/Wallets/Send/SendConfirmationView.swift Outdated
Comment thread Bitkit/Views/PaymentRequests/CreatePaymentRequestView.swift Outdated
@ben-kaufman
ben-kaufman requested a review from ovitrif September 6, 2026 15:18

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.paymentHash is now String? (:27) with searchForActivity returning early at :133 and applyPendingResolutionIfNeeded guarding at :106, while the request-ID path still resolves via onchainPaymentResolutionPublisher — no 24s futile lookup.
  • PaykitPreciseInstant.init?(timestamp:) (PaykitSubscription.swift:14-29) now parses the whole canonical string with fractional-seconds then plain fallback and derives seconds from the parsed instant, so +01:00 with 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).

Comment thread Bitkit/Views/Wallets/Send/SendConfirmationView.swift Outdated
@ben-kaufman
ben-kaufman requested a review from jvsena42 September 7, 2026 17:26

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread Bitkit/Views/Wallets/Send/SendConfirmationView.swift
@ben-kaufman
ben-kaufman requested a review from jvsena42 September 7, 2026 18:11

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 testSendConfirmationViewTests 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.

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Payment re-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 Due notification 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
ovitrif previously approved these changes Sep 8, 2026

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

signing off on the bot approval

@ovitrif ovitrif added this to the 2.6.0 milestone Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants