feat: add Paykit subscriptions - #1186
Conversation
This comment has been minimized.
This comment has been minimized.
|
could test the PR stack feature for this case |
f189535 to
a1a7907
Compare
2eecc75 to
baf9894
Compare
9e4ee27 to
19823e6
Compare
baf9894 to
5a876f8
Compare
5a876f8 to
bb2f12f
Compare
bb2f12f to
050b32b
Compare
82d6c73 to
e1053b0
Compare
1cbcc6f to
4cfdf4e
Compare
e1053b0 to
850c252
Compare
4cfdf4e to
dd53772
Compare
|
@jvsena42 Fixed in dd53772. The duplicated fiat value came from replacing the subscription review sheet while its content was still composed. The flow now reuses the existing dismiss-delay-present sheet transition and waits for that transition before validating the destination. The focused send-flow regression passes. |
850c252 to
bac8656
Compare
dd53772 to
9bd7677
Compare
bac8656 to
6bb8501
Compare
|
Pushed 90b6c09, matching the iOS safeguard. Uncertain Lightning submission errors now retain the payment proof and open the existing Pending screen. An already-started proof also blocks another attempt before private details are consumed, including switching to a hardware payment. Definite failures still clear normally, and unrelated proof-delivery errors remain best-effort. All 2,380 unit tests pass. Compilation and detekt completed, with only the existing lint findings. Full crash recovery remains a separate LDK Node issue: synonymdev/ldk-node#116. Missing payment history is not treated as failure, and crash-injection E2E has not been run for this gap. |
jvsena42
left a comment
There was a problem hiding this comment.
Traced proposal → accept → auto-started first payment → period materialisation → presentation → proof lifecycle → cancellation, and cross-checked the period math against iOS #685.
All nine threads from my earlier pass are genuinely fixed at head — queue emptied before the dismissal is persisted (now persist-first with a bail, :279-287), the cross-identity keychain write (isCurrentState guard precedes every write, :632; activate()/clear() bump the generation before taking the mutex), _onchainPaymentResolution coalescing (now a list consumed per item, :471-484), and the retry hijack (gated on isInitialSubscriptionPayment, SendSheet.kt:520-527). Not re-raising the total-debit-on-auto-start one you closed as a cross-platform product decision.
Fund safety checks out. No auto-pay path beyond the first period — shouldAutomaticallyPay (:5332-5333) requires isInitialSubscriptionPayment, which only acceptSubscriptionAndStartPayment / retryInitialSubscriptionPaymentInCurrentSheet / showInitialSubscriptionPaymentFailure set; every later period goes through presentIncomingPaymentRequestOrStop and needs a swipe. QuickPay can't short-circuit a presented request (:3348, :3400). The amount is pinned msat-exact by acceptsPaymentAmount/acceptsLightningInvoice (:3836-3850) before every send. Double-pay is guarded three ways (isSubmittingPaymentRequest, prepare() throwing OperationInProgress on a started proof, and in-flight/completed ids filtered out of the due queue). Acceptance is persisted before the queue is mutated (:512-514).
One medium and two low notes inline.
Regression test — pre-dispatch node timeout leaves a permanently started proofTest-only, applies cleanly onto Fails on head (32 tests, 1 failed; the other 31 pass): Line 352 is Passes with For the downstream consequence the test uses the store's own predicate rather than new mocking: after the failure a fresh
pr1186-regression.diffdiff --git a/app/src/test/java/to/bitkit/repositories/PaykitPaymentProofRepoTest.kt b/app/src/test/java/to/bitkit/repositories/PaykitPaymentProofRepoTest.kt
index 20a3064c7..e406beba1 100644
--- a/app/src/test/java/to/bitkit/repositories/PaykitPaymentProofRepoTest.kt
+++ b/app/src/test/java/to/bitkit/repositories/PaykitPaymentProofRepoTest.kt
@@ -342,6 +342,20 @@ class PaykitPaymentProofRepoTest : BaseUnitTest(StandardTestDispatcher()) {
}
}
+ @Test
+ fun `node run timeout before dispatch clears proof`() = test {
+ val request = paymentRequest(MethodId.Bolt11.rawValue)
+ val repo = paymentProofRepo()
+ repo.prepare(request, MethodId.Bolt11.rawValue, PaykitPaymentProofKind.Lightning).getOrThrow()
+ repo.associateLightningPayment(request, PAYMENT_HASH, MethodId.Bolt11.rawValue).getOrThrow()
+
+ assertTrue(repo.failLightningPayment(PAYMENT_HASH, NodeRunTimeoutError("payInvoice")))
+
+ assertTrue(storedProofs.isEmpty())
+ repo.prepare(request, MethodId.Bolt11.rawValue, PaykitPaymentProofKind.Lightning).getOrThrow()
+ assertTrue(storedProofs.none { it.requestId == request.id && it.paymentStarted })
+ }
+
@Test
fun `onchain proof uses selected endpoint and transaction id`() = test {
val txid = "ab".repeat(32) |
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed at 60bbd450f. No HIGH/MEDIUM. One trivial parity nit inline; everything else I chased died on verification.
The iOS twin's rail-flip defect is absent here, and the reason is worth recording. On #685 the initial-subscription auto-pay can be dispatched on the Lightning rail and then broadcast on-chain with no swipe and no fee shown, because two validators check different amounts. Android doesn't have that pair. The rail is set once at :2906-2910 and corrected once at :2919-2923, and the correction uses the requested incomingPaymentRequest.amountSats, not the invoice amount — the zero-amount canSend at :2029 only decides whether a Lightning invoice is offered, never which rail is used. The bolt11 and LNURL insufficient-capacity paths abort (:3121-3134, :3176-3185) rather than falling back to on-chain. shouldAutomaticallyPay is decided once at :3431 exactly as on iOS, but only after navigateToSendRoute(SendRoute.Confirm) at :2966, so it already sees the final rail; the only post-navigation writer is the funding-source switch button, which isn't composed while the auto-pay spinner is up (SendConfirmScreen.kt:245), and handleSanityChecks never writes payMethod. So the balance dialog that provides the suspension window on iOS can't flip anything here.
No background payment path, which was my first check given a scheduler is involved. The scheduler is fed only by PaykitPaymentRequestRepo, whose synchronizeLocked sits behind isAvailable() and whose activate() is only called when isPaykitEnabled; turning the flag off runs clear() → clearStateLocked() → subscriptionNotificationScheduler.cancel(). The worker only posts a notification whose tap lands on the normal swipe-to-pay confirm.
Recurring-spend safety traced clean. acceptsPaymentAmount and acceptsLightningInvoiceAmountMsats are msat-exact and evaluated on every send, plus onScanLightning rejects a fixed-amount invoice that differs. Period caps hold: one request id per period keyed on billingPeriodStartsAt, periodsThrough only emits started periods with end > acceptedAt, the due queue excludes paid/locally-completed/in-flight/dismissed, prepare() throws OperationInProgress on any started proof, and submitReady dedups by exact billing period. Nothing auto-pays beyond the first Lightning period — isInitialSubscriptionPayment is only set by the accept/retry/failure paths, and a notification tap or external intent lands on the swipe screen. Cancellation refuses while a started proof exists and the scheduler diff cancels stale unique work names.
Migration is safe: every new persisted field has a default, a master-era proof decodes with paymentStarted=false and is still reconciled via paymentIdentifier, and corrupt blobs decode to empty rather than crashing.
All the earlier fixes verified correct, not merely present — the typed NodeNotRunningError/NodeRunTimeoutError classifier (with asNodeException() returning the AppError itself so the when actually matches), the pending-collector auto-dismiss guard, the null-identity fallback, dismiss-persisted-before-queue-mutation, the isCurrentState guard preceding every keychain write, resolution list+consume, the retry-hijack guard ordering, and the hardware sign/cancel window where the results collector completes the proof before onSendSuccess clears the context.
Three candidates I dropped rather than post: an unpinned auto-pay amount (real as a code fact, but the binding exposes no amend-terms API, so it's defensive against a protocol feature that doesn't exist — and it contradicts a verdict I already left on this PR); a claimed double-pay when proof prep fails (premise is wrong — associateLightningPayment creates a started proof when none exists, and completeOnchainPayment has the same fallback, so the period leaves the due queue); and a supposedly-dead subscriptionAcceptedAt (it's called via a :: method reference at SubscriptionsScreen.kt:122, so deleting it would break the build).
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed the delta at f0ec557c3. Single commit, one file, and it's correct — details on the thread. No HIGH/MEDIUM; nothing further from me on this PR.
|
@jvsena42 fixed |
There was a problem hiding this comment.
QA Notes
Pixel_10_Pro_XL emulator, Android 17 / API 37, dev flavor on regtest. All six Manual Tests passed, driven on 5a61869 immediately before the latest hardware-send and payment-request fixes landed.
The on-chain subscription payment settled for real: LDK broadcast c907aeeb13e5e68091af8744c337e7d6094bd0e795615addc0e3762f42916f0d paid 1000 sats to the payee's private p2wpkh endpoint, the success screen rendered orange confetti, and the completed-payment row showed a matching orange entry. The Lightning half of that same comparison did not settle on this regtest topology, because the wallet's only channel is with the Blocktank LSP while the test invoice came from a standalone lnd node, so no route exists; that selector's purple branch is unchanged on this branch and rendered correctly in an earlier run.
I read the commits since that run as code. The queued-versus-sent confirmation copy is total over the two-value PaykitPaymentRequestDeliveryStatus, the new BackHandler reuses the exact guard the on-screen back already applied, handleFailure now releases isBroadcastUnresolved on every failure path rather than one branch, and cancelling a hardware signature falls back to hiding the sheet when there is no back entry.
Approve.
Coverage
- Journeys: 0%
- No agent-journey suite covers the changed screens.
- Unit tests: 2%
- New HwSendViewModelTest regressions plus updated CreatePaymentRequestScreenTest.
- QA: 6%
- All six Manual Tests driven on emulator at the preceding head.
- Total: 3%
—
Reviewed by Claude Code (claude-opus-5 high) via gh-pr-review-loop skill
jvsena42
left a comment
There was a problem hiding this comment.
The duplicated amount display is fixed — 2311655a46 removes the right one. NumberPadTextField still renders the secondary amount exactly once via its default showSecondaryField = true, and the new instrumented test pins a single $/₿ node across the unit toggle. No over-correction. My earlier CHANGES_REQUESTED is resolved; all nine of my earlier threads plus the two later inline notes remain fixed at head.
One LOW inline, on the cancel path. Everything is behind isPaykitEnabled (default false, Dev Settings only) — isAvailable() gates synchronizeLocked, and turning the flag off runs clear() -> clearStateLocked() -> subscriptionNotificationScheduler.cancel(). Dev/QA-facing today, so not blocking, but the LOW ships unchanged the day the flag flips.
Delta checked hunk by hunk: e39a3d4637 selects queued vs sent description — both string resources exist, and request is the creation-time snapshot, so the copy won't flip if delivery completes while the screen is up. The IncomingPaymentRequestDetailsScreen scroll change is sound: the scrolled column contains no weight/FillHeight/lazy list (the Modifier.weight(1f) uses are RowScope weights inside a Row), and the Pay/Dismiss row sits outside the scroll region, so the buttons stay reachable with a 256-char note. The master merge only brought SendSheet.kt:326 into PR-touched files, which doesn't interact with the subscription retry guard.
Checked and clean — the fund-drain surface specifically:
- Caps. Amount is fixed by SDK terms and copied verbatim per period, with msat-exact pinning before every send. End date is enforced in period generation with a hard
MAX_PERIODS = 10_000, and one request id per period keyed onbillingPeriod.startsAt. - Approver pinned (TOCTOU).
takeIf { it == displayedSubscription }at the ViewModel, and again under the mutex in the repo withit == subscription && it.isProposalActionable(now). Data-class equality coversamountSats,recurrenceand endpoints, so an amended proposal can't be approved against the displayed one. - Double payment. Period boundaries are computed purely in UTC from the payee anchor, so device timezone and DST changes can't shift
billingPeriodStartsAtand mint a second id for the same period — that was my main worry going in. The due queue excludes paid, locally-completed, in-flight and dismissed;prepare()throwsOperationInProgresson a started proof; and the initial-payment retry re-looks-uppendingRequest(id), so an in-flight first attempt can't be retried. The restoration fallback only ever movesacceptedAtlater, never earlier, so no extra period can appear. - Cancellation. The SDK cancel is the durable step and runs first; local state then drops all unpaid periods, and the scheduler diff cancels stale unique work. A notification already posted before cancel is harmless — the tap only sets
requestedPaymentRequestId, and presentation does a livependingRequest()lookup that clears when absent. Nothing is reconstructed from intent extras. - Auto-start paths. The only one is the initial Lightning software payment, requiring
isInitialSubscriptionPayment && LIGHTNING && hardwareWalletId == null; on-chain and hardware go through the normal fee/swipe screen. PIN-for-payments and biometrics are honoured on that path at the screen level, independent of the spinner branch, and PIN failure cancels the payment. A notification tap while locked is queued behindisPaymentRequestPresentationBlocked(), which includes!_isAuthenticated. Per-notificationrequestCode = idmeans two due notifications can't share aFLAG_ONE_SHOTPendingIntent and swap extras. - Proofs. Derived locally (txid from broadcast, or LDK payment hash) and submitted with the exact billing period. Paid periods come from the payer's own SDK record, so a counterparty marking a period paid could only reduce charges.
- Trust boundaries. Issuer strings are bounded and clipped — note
take(256), description 1024, benefits 8x160 — and rendered withmaxLines/ellipsis. TheMoney.ktdiff adds only aprefix/showSymbolpassthrough andTag.ktaddsAddTagButtonwith a string resource, so no untrusted input reaches either. Amounts are clamped before display. - Concurrency.
PaykitPaymentRequestPresentationStoredoes every read-modify-write under oneMutex, a corrupt blob decodes to empty,dismissSubscriptionPaymentpersists before mutating_pendingRequests,synchronizeLockedguards withisCurrentStatebefore any write, and the scheduler is@Synchronized.
Cross-repo: the inline finding does not apply to synonymdev/bitkit-ios#685. Its SwipeButton runs onComplete in an unstructured Task { @MainActor in … } that survives an interactive dismiss, so the cancel completes and updates local state even if the sheet is swiped away. No need to reopen that one.
| loading = loading, | ||
| onConfirm = { | ||
| loading = true | ||
| scope.launch { |
There was a problem hiding this comment.
Swipe-to-cancel runs the repo call in the sheet's Compose scope with no dismiss guard, so dismissing mid-cancel leaves the SDK record CANCELED while the period stays due and payable for up to 120s.
scope here is rememberCoroutineScope() (:852), which dies with the composition. Trace: swipe -> loading = true -> scope.launch { onCancel() } -> PaykitPaymentRequestRepo.cancel -> updateSubscription -> paykitSdkService.cancelPaymentRequest (FFI under operationMutex, completes). While that's in flight the sheet is still dismissable — scrim tap, back, or drag — so hideSheet() nulls _currentSheet, SubscriptionSheet stops composing, and the scope is cancelled. processPendingMessages() then throws CancellationException (correctly rethrown by runSuspendCatching) before applySubscriptionRecordLocked and synchronizeAfterSubscriptionAction.
Result: the SDK has it CANCELED, but _subscriptions still shows Active and _pendingRequests still lists the current period as due — in the Payments tab, the bell, and pendingRequest(id) lookups — until the next poll, which is 30/60/120s. In that window "Pay" on the stale row goes through the full swipe flow (accept(request) for a billing period only checks pending) and submits a proof against a CANCELED request; "Cancel" again reaches the SDK a second time and toasts.
LOW because no sats move without a deliberate swipe and the view self-heals on the next poll. But it's the same shape as the dismissal-before-persist issue we already fixed, just on the cancel path — and acceptSubscriptionAndStartPayment already got the guard it needs via isAcceptingSubscription in ContentView.kt:502. Cancel just didn't get the same treatment.
Narrowest fix: add an isCancellingSubscription StateFlow set around cancelSubscription in AppViewModel and include it in dismissEnabled alongside isAcceptingSubscription — that also blocks the BackHandler and scrim in SheetHost. Alternatively run the repo call in viewModelScope and await() it from the sheet.
There was a problem hiding this comment.
Moved cancellation into the ViewModel scope so it finishes updating the subscription and due payments even if the sheet is dismissed.
There was a problem hiding this comment.
Confirmed fixed in c3769567b, via the second option — viewModelScope.async { … }.await(). The repo call now outlives the composition, so applySubscriptionRecordLocked and synchronizeAfterSubscriptionAction still run when the sheet is dismissed mid-cancel, while the caller keeps the result. viewModelScope uses a SupervisorJob, so a failed cancel can't take the scope down with it, and .onFailure(::toast) still reports. Test added.

Description
Adds the payer side of Paykit subscriptions, building on the merged payment-proof work in #1199 and #1178.
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-rc51Preview
Final Android and cross-platform proof recordings were completed locally; they are not attached to the PR.
QA Notes
Manual Tests
Automated Checks
testDevDebugUnitTestsuite: 2,421 tests passed on the latest UI fixesPaykitPaymentRequestRepoTest,PaykitPaymentRequestRepoSubscriptionTest,SubscriptionsScreenTest, andAppViewModelSendFlowTestCreatePaymentRequestScreenTest: all 4 emulator UI tests passed. The extended amount test verifies a single secondary amount in Bitcoin and fiat modes. The sent-screen test also verifies queued versus sent delivery with a nonempty note, and failed before the wording fix.compileDevDebugKotlincompileDevDebugAndroidTestKotlindetekt --rerun-tasksassembleDevDebug, install, and cold launch on API 36 emulatorCodexBullTlsUiApi36git 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.