feat: add subscription requests - #1239
Conversation
The PR should not merge until payment history for deleted creator subscriptions remains reachable. Findings
|
| 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) } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 SubscriptionAvatar → PubkyImage 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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, | ||
| ) | ||
| } |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
|
conflicts |
There was a problem hiding this comment.
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.
Cross-platform test with iOS #736Android 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
🔴 Blocking: IDs that don't match iOSThese 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.
With the amount step reusing the Still open from my earlier review: top paddingIt still reproduces at Other findings
Emulator noteThe 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 |

Description
This PR adds subscription proposals to contacts, building on the payer flow merged in #1186.
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
QA Notes
Manual Tests
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
ScreensMapTest.kt.PaykitSubscriptionProposalTest.ktcovers UTF-8/wire-size boundaries.PaykitPaymentRequestRepoSubscriptionTest.ktcovers creator terms, queued delivery, oversize rejection before icon upload/enqueue, pending cancellation and duplicate/off-schedule proofs. No automated coverage was removed.CreateSubscriptionScreenTest.kt,SubscriptionsScreenTest.ktand existingCreatePaymentRequestScreenTest.ktregression coverage. Includes all four frequency tabs, no-Discover empty state, compact 520dp confirmation layouts, loading state, single recipient, expiry, and truthful sent/queued copy.E2E=true E2E_BACKEND=networkand the dev flavor. Detekt retains its upstream advisory configuration; the cohesive shared subscription test fixture has a non-blocking LargeClass advisory, reviewed explicitly.