Skip to content

feat: add subscription requests - #1239

Open
ben-kaufman wants to merge 9 commits into
masterfrom
codex/paykit-subscription-requests-android
Open

feat: add subscription requests#1239
ben-kaufman wants to merge 9 commits into
masterfrom
codex/paykit-subscription-requests-android

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Description

This PR adds subscription proposals to contacts, building on the payer flow merged in #1186.

  • Create a daily, weekly, monthly or yearly subscription with an amount, name, optional description and optional public Pubky icon.
  • Reuse Payment Request amount entry, recipient rows and expiry controls; send to one saved, privately linked contact.
  • Distinguish queued from sent and pending from accepted; show creator subscriber/received-payment counts and support pending/active deletion without editing.
  • Validate the wire-size limit before icon upload/enqueue, downsample selected images, and aggregate valid billing-period proofs without creating payer prompts for creators.
  • Keep the newly merged screen map in sync and add one user-facing changelog fragment.

Discover, autopay and renewal UI are intentionally excluded. Icon hosting is public by design. Base is master; no unmerged payer dependency remains.

iOS counterpart: synonymdev/bitkit-ios#736.

Figma: Intro, Overview, Create Subscription, Choose Recipient, Sent Proposal.

Deferred custom-icon SDK work

Current Pubky image fetching buffers the complete file before passing it to the image loader. Upload-side downsampling does not bound downloads of images supplied by others.

When the forthcoming Paykit bounded-fetch and recoverable-publication support is integrated for iOS #736, include Android #1239 or its merged successor in the coordinated update and validation. This includes Kotlin bindings and Android decode/cache limits.

Shared-avatar handling and ambiguous proposal-publication outcomes remain SDK concerns. These limitations do not establish that Android currently deletes images unsafely. The current Android implementation and icons remain unchanged. The SDK integration is deferred to that follow-up.

Preview

Create → recipient → sent → overview using test wallets, silent at 4× speed. The first three seconds of the accelerated source clip were trimmed to exclude the system photo picker. All attached media was inspected and shows only Bitkit, including its keyboard; no device home screen or other app is included.

The walkthrough predates the final recipient corrections. The first screenshot below shows the final timer, Contacts spacing, Paste inset and 52dp field height.

android-pr-bitkit-only-4x.mp4
Final recipient, sent confirmation and overview screenshots Final recipient field, timer, Paste inset and Contacts spacing Subscription request sent confirmation Subscriptions overview after sending proposal

QA Notes

Manual Tests

  • 1. Subscriptions → Create Subscription → Amount: set a positive amount, name and frequency → Choose Recipient: only one saved/private-linked contact can be selected; expiry is configurable → Propose Subscription: confirmation shows recipient, amount and frequency.
  • 2. Create Subscription → upload custom icon → Choose Recipient → back: draft remains intact; the same public icon appears on the receiving device.
  • 3. Receiver → Review & Subscribe → accept and manually pay: creator shows one subscriber/one received payment attributed to the payer, without a payer payment prompt on the creator.
  • 4. Creator pending or accepted subscription → Delete → Swipe To Delete: request expires at the receiver and payment history remains available.
  • 5. Device offline → Propose Subscription: failure must not claim Sent; restore connectivity and retry the retained draft successfully.
  • 6. regression: Payment Requests → Create Payment Request: shared amount entry, contact selection and expiry remain functional.

Live regtest creation/delivery/acceptance/manual on-chain payments, custom icons and paid cancellation passed in both directions with iOS. Offline error/draft retention/retry and pending deletion also passed. Live transport-queued flushing, Lightning, mainnet and production push were not tested.

Automated Checks

  • Full suite: 2,430 unit tests passed with zero failures/skips on the final reviewed source, including ScreensMapTest.kt.
  • PaykitSubscriptionProposalTest.kt covers UTF-8/wire-size boundaries. PaykitPaymentRequestRepoSubscriptionTest.kt covers creator terms, queued delivery, oversize rejection before icon upload/enqueue, pending cancellation and duplicate/off-schedule proofs. No automated coverage was removed.
  • Nine Compose tests passed: CreateSubscriptionScreenTest.kt, SubscriptionsScreenTest.kt and existing CreatePaymentRequestScreenTest.kt regression coverage. Includes all four frequency tabs, no-Discover empty state, compact 520dp confirmation layouts, loading state, single recipient, expiry, and truthful sent/queued copy.
  • App and instrumentation builds plus detekt completed using E2E=true E2E_BACKEND=network and the dev flavor. Detekt retains its upstream advisory configuration; the cohesive shared subscription test fixture has a non-blocking LargeClass advisory, reviewed explicitly.
  • Final local review found no remaining actionable issues. The final UI changes reuse the existing Payment Request picker and shared subscription card; no changed-UI detekt findings remain.

@greptile-apps

greptile-apps Bot commented Sep 9, 2026

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 4/5

The PR should not merge until payment history for deleted creator subscriptions remains reachable.

Findings

  1. P1 Deletion hides payment history

Summary

  • Extends Paykit proposal terms and repository models to support recurring creator records.
  • Adds the multi-step Compose creation and confirmation flow.
  • Separates payer and creator subscription processing, notifications, and payment presentation.
  • Adds wire-size, repository, and Compose coverage.
  • Creator payment history becomes unreachable after deleting a paid subscription.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Create subscription details] --> B[Choose saved private-linked contact]
    B --> C[Validate proposal wire size]
    C --> D{Custom icon?}
    D -- Yes --> E[Compress and upload public icon]
    D -- No --> F[Build recurring Paykit proposal]
    E --> F
    F --> G[Enqueue proposal]
    G --> H{Delivered immediately?}
    H -- Yes --> I[Sent confirmation]
    H -- No --> J[Queued confirmation]
    G --> K[Creator subscription record]
    K --> L[Pending or active creator list]
    L --> M[Subscription details]
    M --> N[Received-payment history]
    M --> O[Delete or cancel]
    O --> P[Canceled record retained]
    P -. currently filtered from UI .-> N
Loading

val proposals = subscriptions.filter { it.isPayer && it.isProposalVisible(now) }
val active = subscriptions.filter { it.isPayer && it.isActive(now) }
val expired = subscriptions.filter { it.isPayer && it.isExpired(now) && acceptedAt(it.id) != null }
val created = subscriptions.filter { it.isCreatedVisible(now) }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Deletion hides payment history

Deleting a creator-owned subscription changes it to CANCELED, but this filter only exposes proposed or active creator subscriptions. The canceled record remains in repository state while disappearing from the only list that links to its detail screen. Because received payments are shown only on that detail screen and are excluded from global payment history, deleting a paid subscription makes its retained payment history inaccessible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Kept subscriptions with received payments in the Created list after deletion, so their payment history remains accessible. They show as expired, while canceled proposals without payments still disappear.

@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. No critical or high findings.

This adds the creator/payee side and re-tags the existing payer logic with isPayer guards. Every path that moves sats is untouched and still needs a fresh user action per period (acceptSubscriptionAndStartPayment → send flow; due periods → openIncomingPaymentRequest → send flow). Nothing auto-pays, nothing pays a cancelled or dismissed period, and the persisted-before-cleared dismissal ordering from the earlier fix is intact (dismissSubscriptionPayment, :289-322).

I specifically checked for the three bugs confirmed on the earlier subscriptions work — none repeat: creation uses a callback rather than a StateFlow, and the proof/dismissal code is unchanged, so neither the proof-kept-forever wedge nor the queue-cleared-before-persist ordering is reintroduced.

Also verified clean: amount shown vs paid is still enforced by acceptsPaymentAmount; cross-identity is guarded by expectedIdentity/generation under operationMutex with the SDK re-checking identity in uploadProfileAvatar and proposePaymentRequest, and publishCreatedSubscription gating on isCurrentState; payee records can't leak into one-off history (toPaykitPaymentRequest:1188 rejects recurrence != null); the notification scheduler, monthly-cost, proposals and accept paths are all isPayer-filtered; runSuspendCatching throughout the new suspend paths; creationMutex.tryLock guards double-submit.

Three LOW notes inline, all dev/QA-facing today (isPaykitEnabled default false). Two of them also apply to the iOS twin (synonymdev/bitkit-ios#736).

every = 1u,
unit = draft.frequency.rawValue,
startsAt = timestamp,
anchor = timestamp,

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.

Low, disclosed, but worth a decision: anchoring the billing grid at proposal time lets the first billed period be arbitrarily short.

startsAt = anchor = proposal time, but the payer may accept any time up to proposalExpiresAt — and PaymentRequestExpiration goes up to 30 days. periodsThrough() bills the period containing acceptedAt in full, so with a Day plan and a Week expiry, accepting 6d23h later buys a period with an hour left, and the next full charge falls due an hour after that. Two consented payments within the hour.

It is disclosed — SubscriptionsScreen.kt:703-712 shows "First billing period ends {date}. Each period is charged in full." — and every charge needs its own consent, which is why this is LOW rather than higher. But this PR is the first Bitkit code that generates such terms, so it's the right place to decide.

Either set startsAt to the proposal's expiry so the grid begins after the acceptance window, or cap the expiry options relative to the billing unit (hide Month for monthly, Week/Month for weekly). A payer-side minimum-first-period rule would also work but is a cross-platform behaviour change.

Same line-for-line on iOS (PaykitPaymentRequestService.swift:648-653) — worth deciding once for both.

PaykitSubscriptionMetadata(description, benefits)
}.getOrDefault(PaykitSubscriptionMetadata(null, emptyList()))
val iconUri = subscription["icon_uri"]?.jsonPrimitive?.contentOrNull
?.clean(512)

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.

Low: the only validation on the payee-supplied icon is the pubky:// prefix, so it can point at any key's blob.

The URI goes to Coil via SubscriptionAvatarPubkyImage and renders in place of the counterparty's avatar on the review sheet, list rows and detail screen. A proposer setting icon_uri = pubky://<someone-else>/pub/bitkit.to/…avatar puts that person's picture on the subscription, and the payer's device fetches from that homeserver on every render.

After the prefix check, require the URI's host segment to match counterparty (PubkyPublicKeyFormat.matches(...)) and fall back to null — default icon or contact avatar — otherwise.

Same check on iOS (PaykitSubscription.swift, also prefix-only).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The cross-key icon is possible, but checking counterparty directly would also reject the creator's own icon on created records, where counterparty is the payer. A top-level host check also would not constrain a descriptor's src. Leaving icon handling unchanged here for the coordinated follow-up.

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.

Accepted — not reopening this. Your reason holds (a bare counterparty host check breaks creator-owned records where counterparty is the payer, and wouldn't constrain a descriptor's src either).

Input for the coordinated follow-up, since the iOS twin landed its half this week: synonymdev/bitkit-ios#736 fixed the display side rather than the URI side. It renders the saved contact's display name plus a locally-derived truncated public key on the review route, both resolved from subscription.counterparty and never from metadata — so the proposer-supplied icon can stay unconstrained while the sheet still carries an identity the proposer can't forge.

Relevant because the Android review sheet currently has no identity text at all: SubscriptionReview renders money, SubscriptionProviderCard (avatar + note + frequency), the period date and the swipe. I checked, and that's pre-existing rather than something this PR changed — SubscriptionReview is byte-identical to master. What this PR did change is the avatar itself (SubscriptionsScreen.kt:776, master's PubkyContactAvatar(profile = contact, …)SubscriptionAvatar), so a non-null icon_uri now suppresses the one contact-derived cue that was there.

So the two halves compound: unconstrained icon + no identity text. Either half alone is much weaker. Whichever way the follow-up goes, worth deciding both together rather than just the URI check.

contentType = "image/jpeg",
expectedIdentity = expectedIdentity,
)
}

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.

Low: the public icon blob is uploaded before the proposal is committed, so a later failure leaves an orphaned world-readable file.

uploadProfileAvatar publishes under /pub/bitkit.to/… and runs before the post-upload validateProposalExpiration/validate(proposal) and before proposePaymentRequest. Any throw after this point leaves the blob on the homeserver, and each retry uploads another. Peer discovery succeeding but proposePaymentRequest failing on an identity check or transport is the ordinary way in.

It also holds operationMutex across a network upload, blocking refresh/dismiss/accept for the duration.

Reordering so the upload is the last fallible step before proposePaymentRequest fixes both, or delete the blob on failure (the SDK exposes path). Worth considering doing the upload outside the mutex and re-validating identity afterwards.

(For the record, reservedIconUri is sized against the staging namespace, which is longer than mainnet bitkit.to, so that preflight bound is conservative — not a bug.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A failed proposal can leave a public upload behind, and the upload holds the shared operation lock. Moving it later does not make upload and proposal publication atomic, and deleting on a failed return needs care when publication may have succeeded. Leaving this for the recoverable-publication SDK follow-up documented in the description.

@jvsena42

Copy link
Copy Markdown
Member

conflicts

@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 70629e09. No new findings.

Attribution: the delta since my last pass (29cecb8a) is exactly one commit — 70629e095 chore: merge master into subscription requests — touching 34 files, of which 30 are byte-identical to master (#1216, #1235, #1236 landing) and 4 differ only on master's side. Net: this PR authored zero new lines since I last looked, so I re-verified the PR's own subscription code at head against the fund-safety list rather than reviewing a delta.

One merge interaction I checked because it's the kind of thing that slips through: master's #1216 made MethodId.fromRawValue(value, network = Env.network) network-aware, but PaykitSubscription.kt:313 still calls the one-arg form. The default parameter makes it behaviourally identical in production, and creator-side acceptedPaymentEndpointIdentifiers uses MethodId.rawValue = rawValueForNetwork(Env.network), so proposer and payer agree on identifiers. Not a finding, but worth a glance if the default ever changes.

Fund safety — the reason this PR gets the scrutiny it does. The creator-side role split only narrows the payer paths rather than adding new ones: requestsThrough returns empty for payee (:240), accept() requires it == subscription && it.isPayer && isProposalActionable and pins the displayed value (repo:690), the scheduler is gated on isPayer (scheduler:70), and restoredAcceptances is filtered on isPayer (repo:767). No auto-start charge path exists on the creator side. Period arithmetic holds: every == 0u rejected at :296, every.toLong() * index stays inside Long, and the plusX calls are wrapped in non-suspend runCatching — correct usage. No new ULong arithmetic was introduced.

Also clean: nothing seed-derived written, sent or logged; uploadProfileAvatar(expectedIdentity) and proposePaymentRequest(expectedIdentity) both check live identity, and publishCreatedSubscription gates on isCurrentState(generation, expectedIdentity); inbound proposal fields are all bounded (description 1024, benefits 8×160, note 256, icon_uri 512); every suspend path uses runSuspendCatching, with runCatching only on non-suspend helpers; dismissSubscriptionPayment persists before mutating in-memory state.

Recorded, not filed — the creator-side ledger counts payer-asserted proofs. PaykitSubscription.kt:345-354 reads only billingPeriod and paymentEndpointIdentifier; the SDK's proof payload is never inspected and nothing correlates against LDK or on-chain receipts, so a subscriber can make "Payments: N" and N +amount rows appear on the creator's detail screen without paying. Android is actually stricter than iOS here (the periodsThrough grid check drops off-grid periods) and derives the rows on the fly rather than persisting them. One Android-specific consequence worth knowing: isCreatedVisible (:216) includes || paidPeriods.isNotEmpty(), so a single bogus proof pins a cancelled subscription in the Created list.

This is the same pre-existing Paykit-wide trust model I recorded on the iOS twin (synonymdev/bitkit-ios#736) rather than filing against it — the SDK does stateless correlation only and delegates settlement to the caller. Not this PR's defect; this is just the first Android code to render the number. If it's ever addressed, the narrow interim is to keep paidPeriods.isNotEmpty() out of the delete-visibility rule, or label those rows as reported rather than settled.

Status of my open threads: the short-first-billing-period one (PaykitPaymentRequestRepo.kt:576) is still unanswered — head still has startsAt = timestamp, anchor = timestamp with timestamp = proposal time. The icon-namespace and orphaned-icon-blob threads you replied to today are both deferred with reasons I accept; I've left a note on the first with input from the iOS side for whenever the coordinated follow-up happens.

Gating unchanged: dev/QA-facing today.

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

Sent proposal sheet his not respecting top padding

Image

@jvsena42

Copy link
Copy Markdown
Member

Cross-platform test with iOS #736

Android 902bb335f (devDebug, Pixel 9 emulator) against iOS d2f0585f (iPhone 17 sim, Debug without E2E_BUILD → staging homeserver + staging regtest). Fresh identities: Android AA, iOS BB, saved as each other's contacts. Times are UTC.

Verdict: the Android side behaved correctly everywhere I could test it. The test IDs don't match iOS, which I'm treating as blocking, and the top-padding issue from my earlier review is still there.

Results against the QA list

# Test Result
1 Create & propose ✅ Received from iOS: Review & Subscribe opened on its own about 11 s after iOS sent (₿5,000, Weekly, first billing period correct). ✅ Android → iOS: the Sent screen shows BB, ₿1,000, Daily. deliveryStatus was Sent, but iOS never showed it (iOS blocker, details on #736).
2 Draft kept + icon ✅ Amount, Daily, name, description and the custom icon all survive Choose Recipient → Back. Couldn't check the icon on iOS because of the iOS blocker.
3 Accept & pay ✅ Paying an iOS-created subscription: Review & Subscribe → on-chain confirm (₿141 fee) → Subscribed. The overview shows Active 1 and a monthly cost of ₿21,667, which is correct for ₿5,000/week. The payment proof was queued and reached iOS about 25 s later, credited to AA.
4 Creator cancels ✅ iOS's delete reached Android in about 32 s: the status changed to Expired, Cancel disappeared, and the payment row (BB · −₿5,000) was kept under Expired.
5 Offline / oversized Not tested
6 Payment request regression ✅ Create Payment Request → BB → ₿1,000 → Send → Sent. It never appeared on iOS (iOS blocker).
7 Keyboard Not applicable on the emulator run

🔴 Blocking: IDs that don't match iOS

These have to match, per the shared journey vocabulary. AGENTS.md makes Android the reference, so most of the fixes may land on iOS (#736). Please settle each one with that PR so both land together.

Element Android iOS
Frequency tabs Tab-day/week/month/year (CustomTabRowWithSpacing.kt:58) Tab-daily/weekly/monthly/yearly (also changes with the device language on iOS)
Subscription amount step Reuses PaymentRequestAmountContent (CreateSubscriptionScreen.kt:151), so it exposes PaymentRequestAmount, PaymentRequestAmountContinue, PaymentRequestNumberPad SubscriptionAmount… prefix
Recipient contact row SubscriptionContact<pubky>, no separator (CreatePaymentRequestScreen.kt:490) SubscriptionContact-<pubky>

With the amount step reusing the PaymentRequest* tags, a journey can't tell the subscription amount step from the payment-request one on Android. Worth deciding on purpose.

Still open from my earlier review: top padding

It still reproduces at 902bb335f. The Sent proposal sheet's drag handle and "Sent" title are drawn inside the status bar. The Create Subscription sheet has the same problem: its title sits at status-bar height, right under the clock.

Other findings

  • The first review step doesn't name the sender. SubscriptionReviewSubscriptionProviderCard(subscription, contact) (SubscriptionsScreen.kt:702) shows only the contact's avatar ("B") next to the subscription name. The sender's name first appears on the on-chain confirm step. iOS fixed the equivalent in 123ccf51 (saved-contact name on review).
  • Link state for the iOS blocker, seen from Android. After iOS's link to AA got stuck ("Encrypted Link Handshake is still in progress"), Android kept calling restore_encrypted_link for BB and never started a new handshake. Removing and re-adding BB on Android didn't change that either; the SDK link state is independent of the contact list. There's no in-app recovery on either side. Full details and a repro are on Generic error message for Lightning payment failures hides actual cause #736.

Emulator note

The icon picker on the emulator is Google Photos' picker: tapping a tile only selects it, and it isn't applied until Done is tapped. The app correctly launches a single, image-only PickVisualMedia (GetContent is only the pre-Android-13 fallback), so this isn't a bug. It's just a trap when automating.

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.

2 participants