feat: add subscription requests - #736
Conversation
 This PR is not safe to merge until subscription creation avoids orphaned public icon uploads and canceled creator subscriptions retain an accessible payment history. Findings
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let iconURI: String? = if let iconData = draft.iconData { | ||
| try await sdk.uploadProfileAvatar( | ||
| bytes: Self.compressedSubscriptionIcon(iconData), | ||
| contentType: "image/jpeg", | ||
| expectedIdentity: expectedIdentity | ||
| ) |
There was a problem hiding this comment.
The icon is published before the final expiry check and proposal write. If the draft expires or proposePaymentRequest fails after the upload, the UI reports that no proposal was created but the selected image remains publicly hosted, with no rollback or cleanup path. The upload also separates the initial recipient and payment-rail checks from the final proposal, so unlinking the contact or disabling a rail during the upload can submit stale terms because the final SDK call rechecks only the identity. Revalidate eligibility and endpoint settings immediately before proposing, and remove an uploaded blob after failure or defer publication until the proposal can be committed.
How this was verified: The icon upload is an externally visible write performed before the final expiry check and proposal call, while the later proposal boundary validates the identity but not current recipient eligibility or endpoint settings.
Knowledge Base Used:
There was a problem hiding this comment.
Confirmed that a failed proposal can leave the icon public. The SDK reuses content-based avatar names and can fail after enqueueing, so deleting on every error could break a shared image or a queued proposal. This remains open, including the payment-option recheck after upload.
There was a problem hiding this comment.
Agreed. Because avatar names are content-addressed and an upload may already be referenced by a queued proposal, unconditional cleanup on failure is unsafe. The remaining actionable fix is to revalidate expiry, current recipient eligibility, and accepted payment endpoints immediately after the upload and before constructing/enqueueing the proposal; if any changed, fail without enqueueing rather than using the stale pre-upload snapshot. The orphaned-icon case likely needs an SDK-level ownership/cleanup mechanism to resolve safely, so I’m keeping this finding open for the post-upload validation.
There was a problem hiding this comment.
Added a final check of expiry, recipient eligibility and payment options after upload. A changed recipient, session or payment option now stops the proposal before enqueueing. Safe cleanup of the uploaded image remains deferred.
| func isCreatedVisible(at date: Date) -> Bool { | ||
| isCreatedByUser && (isProposalVisible(at: date) || isActive(at: date)) | ||
| } |
There was a problem hiding this comment.
Canceling a creator-owned subscription makes its retained payment history unreachable. A canceled subscription is neither proposed nor active, so isCreatedVisible removes it from the Created section. The Expired section accepts only payer-side subscriptions, and creator payments are exposed only through receivedPaymentRequests() on the now-hidden detail screen. Deleting an accepted subscription with prior payments therefore removes the user's only route to those payments, contrary to the stated retained-history behavior. Keep canceled creator subscriptions with payments in a historical section or persist their payment rows into accessible history.
Knowledge Base Used: Payment request management
There was a problem hiding this comment.
Canceled or ended creator subscriptions with payments now appear in the Expired section, where their payment history remains accessible. Deleted proposals without payments still disappear.
| .prefix(8) | ||
| .compactMap { Self.trimmed($0, limit: 160) } | ||
| iconURI = Self.trimmed(subscription["icon_uri"] as? String, limit: 512) | ||
| .flatMap { $0.hasPrefix("pubky://") ? $0 : nil } |
There was a problem hiding this comment.
Incoming subscription metadata can reference any pubky:// resource, and the image loader downloads and fully decodes that resource without a byte or pixel limit. A malicious proposer can point icon_uri at an oversized or decompression-heavy public image; opening the subscription list then downloads it, decodes it with UIImage(data:), and caches the raw data, potentially causing excessive memory and storage use. Bind the URI to the proposal creator where appropriate and enforce download and decoded-dimension limits before constructing the image.
How this was verified: The proposal-controlled URI is accepted solely by its scheme, then fetched and decoded at full resolution without a size check before being cached.
Knowledge Base Used: Contacts and Pubky identity
There was a problem hiding this comment.
Confirmed that the loader has no download or decoded-image limit. The pinned SDK returns the full response before the app can inspect it, so an app-side size check alone would not cap the download. This remains open pending a bounded fetch path.
There was a problem hiding this comment.
The binding half of this thread is still open, and it now closes more than a fetch-limit issue — worth doing rather than dropping.
PaykitSubscription.swift:135-136 accepts icon_uri on the bare "pubky://" prefix with no namespace check, so a proposer can point straight at a third party's real pubky-hosted avatar blob. No re-hosting needed.
What changed in this PR is what that buys them. SubscriptionsView.swift:309-318 made the proposer-supplied icon win the precedence:
if let iconURI = subscription.metadata.iconURI {
PubkyImage(uri: iconURI, size: size, cornerRadius: size / 5)
} else if subscription.isCreatedByUser {
...
} else if let contact {
PubkyContactAvatar(contact: contact, size: size)On master (:279-285) the contact avatar always won. And the review sheet has no other identity cue — review() and SubscriptionProviderCard (:931-939) show note + frequency, no name and no truncated key — so after this change every visual signal on "Review & Subscribe" is proposer-controlled.
Capping severity honestly: master already lets the proposer control note, which is the card's primary text, so this strengthens an attacker-controlled surface rather than creating one, and it needs an established private link. The charge-time confirm does resolve the real contact (SendConfirmationView.swift:171-174), though it renders only a 32pt avatar with no name, and :155-158 shouldAutomaticallyPay can auto-pay the initial charge when PIN isn't required — so the payer may barely see it.
Either fix is small: bind icon_uri to the counterparty's namespace at PaykitSubscription.swift:136 (this thread's original ask), or pass the contact's display name as subtitle on the review-route SubscriptionProviderCard — that parameter already exists and is already used at :830.
There was a problem hiding this comment.
Agreed — this is a distinct identity-binding issue, not merely the deferred fetch-limit problem. The minimal fix should validate the icon namespace for incoming payer proposals against subscription.counterparty before using metadata.iconURI. That check must be role-aware: a locally created payee record’s uploaded icon belongs to the local creator namespace, so it must not be rejected by comparing it to the recipient counterparty. PubkyImage’s existing redirect check only protects descriptor → blob redirects; it cannot prevent the initial URI from targeting another user’s blob. Adding the contact display name as the review-card subtitle would be useful defense-in-depth, but it does not prevent the spoofed avatar, so namespace binding should remain the primary fix.
There was a problem hiding this comment.
Added the contact name and public-key fingerprint to Review & Subscribe, while keeping the subscription title and frequency visible. Icon namespace binding remains open, so this does not make the custom image a verified identity signal.
There was a problem hiding this comment.
That is useful defense-in-depth: the contact name and truncated public-key fingerprint give the payer independent identity cues while retaining the subscription title and frequency. It does not resolve the original issue, though—icon_uri is still accepted based only on the pubky:// scheme and can reference another user's public avatar, so the image remains an unverified, proposer-controlled signal. I agree this specific namespace-binding finding should remain open (or be explicitly tracked as a deferred risk) until incoming payer icons are constrained to the proposal creator's namespace.
| "subscriptions__choose_recipient" = "Choose Recipient"; | ||
| "subscriptions__create" = "Create"; | ||
| "subscriptions__content_too_long" = "Shorten the subscription name or description and try again."; | ||
| "subscriptions__icon_error" = "Could not load this image. Choose another image and try again."; | ||
| "subscriptions__create_subscription" = "Create Subscription"; | ||
| "subscriptions__created" = "Created"; | ||
| "subscriptions__created_summary" = "1 subscriber · {count} payments"; | ||
| "subscriptions__created_summary_single_payment" = "1 subscriber · 1 payment"; | ||
| "subscriptions__custom_icon" = "Icon"; | ||
| "subscriptions__custom_icon_description" = "Tap to upload a custom icon"; | ||
| "subscriptions__delete_subscription" = "Delete Subscription"; | ||
| "subscriptions__description" = "Description"; | ||
| "subscriptions__description_placeholder" = "What is this subscription for?"; | ||
| "subscriptions__name" = "Subscription Name"; | ||
| "subscriptions__name_placeholder" = "Subscription name"; | ||
| "subscriptions__pending" = "Pending"; | ||
| "subscriptions__proposal_queued_description" = "Your subscription proposal is queued and will send automatically."; | ||
| "subscriptions__proposal_queued_headline" = "Queued\n<accent>Proposal</accent>"; | ||
| "subscriptions__proposal_queued_title" = "Queued"; | ||
| "subscriptions__proposal_queued_status" = "Proposal queued"; | ||
| "subscriptions__proposal_sent_description" = "You have sent a subscription proposal to"; | ||
| "subscriptions__proposal_sent_headline" = "Sent\n<accent>Proposal</accent>"; | ||
| "subscriptions__proposal_sent_status" = "Proposal sent"; | ||
| "subscriptions__propose_subscription" = "Propose Subscription"; | ||
| "subscriptions__subscribers" = "Subscribers"; | ||
| "subscriptions__swipe_to_delete" = "Swipe To Delete"; |
There was a problem hiding this comment.
The new subscription strings were copied as English values into every non-English localization file instead of going through the repository's translation workflow. Because validation checks key presence rather than translated content, these placeholders silently pass CI and become indistinguishable from completed translations. This leaves the entire flow untranslated and prevents missing-key warnings from tracking the work. Keep only the English source strings until genuine translations are pulled, or add actual translations for each locale. The same pattern appears in the Arabic, Catalan, Czech, German, Greek, Latin American Spanish, Spanish, Italian, Dutch, Polish, Brazilian Portuguese, Portuguese, and Russian localization files.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Removed the new English placeholders from the non-English files. The flow uses the existing English fallback until translations arrive, and missing-translation warnings can track those keys again.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Reviewed for fund draining specifically. No high finding. The payer-side payment paths this PR touches are only narrowed — isPayer now gates presentation, accept, requests and notifications — so payee records can't be materialised as payable requests, and one-time inits still require terms.recurrence == nil (:84-87). Identity is re-checked at upload and propose (PubkyService.swift:489-497).
Things I broke on purpose and couldn't: the new proof filter recurrence.contains(_:) (PaykitSubscription.swift:208-211, applied at :528-536) always re-matches a locally generated period because PaykitPreciseInstant.timestamp is already canonical, and Android applies the identical filter with identical month/year clamping — so there's no cross-platform hiding of received payments. The timestamp parser change (:18-29) is safe: fractionalSeconds(from:) returns the literal substring, so the replacingOccurrences strip can't corrupt it. The 1000-byte pre-validation field set matches PaymentRequestWire exactly. Amount strings round-trip through en_US_POSIX.
I also checked the three bugs I confirmed on #685 — none repeat: isDefiniteOnchainPreBroadcastFailure and the pending-screen detach logic are untouched, and the review sheet now shows the period end.
On assets: timer-outline.svg carries Figma-export markers. I can't demonstrate the PNG illustrations or asterisk.svg were substituted rather than exported, so I'm not flagging them — just noting I checked, since the repo rule forbids lookalikes.
One schedule note and two LOWs inline. All dev/QA-facing today (PaykitFeatureFlags.isUIEnabled default off), so nothing blocking.
| let metadataText = String(decoding: metadataData, as: UTF8.self) | ||
| let timestamp = Self.timestamp(proposalDate) | ||
| let recurrence = Paykit.PaymentRequestRecurrence( | ||
| every: 1, |
There was a problem hiding this comment.
The creator anchors the billing grid at proposal time while the proposal can stay open for a full period, so the payer's first charge can buy a near-zero window and be followed immediately by a second full charge.
With startsAt == anchor == proposalDate, the first period is [proposalDate, proposalDate + 1 unit). paymentDueOnAcceptance(at:) (PaykitSubscription.swift:570-574) charges the full period containing the acceptance instant and only skips periods that already ended. Meanwhile CreateSubscriptionView.swift:251-253 offers PaymentRequestExpiration.allCases, so the proposal expiry can equal the billing unit.
Concretely: 100k sats/month proposed Jan 15 08:00 with "1 month" expiry. Payer accepts Feb 15 07:50 → 100k for [Jan 15, Feb 15). At 08:00 the refresh materialises [Feb 15, Mar 15) as pending plus a due-payment notification → a second 100k ten minutes later. The payer bought ten minutes for a full month. Even the default 7-day expiry allows ~23% loss on a monthly plan.
Not higher severity because it's disclosed — SubscriptionsView.swift:616-623 shows "First billing period ends {date}. Each period is charged in full." — and every charge is a separate explicit confirmation. It's a disclosed trap, not an unauthorised debit.
Cleanest fix stays local to this PR: filter the expiry options so expiry < billing unit (hide .month for monthly, .week/.month for weekly), or cap draft.expiresAt in proposeSubscription. A payer-side minimum-first-period rule would also work and stays schedule-compatible with the payee's contains filter, but it's a cross-platform behaviour change.
Identical on Android (PaykitPaymentRequestRepo.kt:570-575, same expiry options) — synonymdev/bitkit-android#1239. Worth deciding once for both.
There was a problem hiding this comment.
Agreed that accepting near a billing boundary can leave a very short first period at full price. The end date and full-period charge are disclosed, but limiting proposal expiry alone would not guarantee a minimum first period. This remains open for a coordinated billing-policy decision across iOS and Android.
| guard let selectedTarget else { return } | ||
| do { | ||
| let subscription = try await paymentRequests.proposeSubscription(draft, to: selectedTarget) | ||
| guard paymentRequests.subscriptions.contains(where: { $0.id == subscription.id }) else { return } |
There was a problem hiding this comment.
Low: a successful proposal is silently dropped from the UI if the manager's post-await guards fail, which invites a duplicate.
The guard at PaykitPaymentRequestService.swift:1109-1113 compares savedPublicKeysSnapshot == savedPublicKeys — an order-sensitive [String] compare. The proposal window is long (icon upload + propose + processPendingMessages), so a contacts refresh re-emitting savedPublicKeys in a different order during it makes the manager return the subscription without appending it. This view's guard … contains then returns without onSent: no toast, isCreatingRequest resets, the button re-enables — and the user taps "Propose Subscription" again, so the counterparty receives two recurring proposals.
Each still needs the payer's explicit acceptance, so there's no silent debit — hence LOW.
On guard failure, still route to .proposalSent(subscription) (the SDK did create it) or await paymentRequests.refresh() before navigating; at minimum surface a toast. Same pattern exists in the one-time flow (CreatePaymentRequestView.swift:488), so it's pre-existing in kind.
There was a problem hiding this comment.
Created proposals now stay in the current session's subscription list when contacts refresh during delivery, so the confirmation screen can still open. Clearing or switching the session still discards the stale UI result.
| @@ -0,0 +1,369 @@ | |||
| import PhotosUI | |||
There was a problem hiding this comment.
Nit: the icon is JPEG-compressed twice, and once on the main actor.
loadIcon calls compressedSubscriptionIcon from the view's .task — main actor, so CGImageSourceCreateThumbnailAtIndex on a full camera photo hitches the UI — stores the ≤400px JPEG in draft.iconData, and then proposeSubscription (PaykitPaymentRequestService.swift:598-600) re-runs compressedSubscriptionIcon on that already-compressed JPEG.
Harmless but lossy. Compress once, service-side, off-main via Task.detached, and keep the raw picker bytes in the draft.
There was a problem hiding this comment.
Confirmed. The picker compresses on the main actor and the service compresses the result again. This optimization remains deferred with the custom-icon work.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
No HIGH, no MEDIUM. Subscriptions are the highest-risk shape in this codebase, so I went at the recurring-charge semantics hard and came away with nothing blocking. One reply on greptile's open icon thread rather than a new finding, and one trust-model observation below that is explicitly not a defect in this PR.
Fund draining — the thing I most expected to find, and didn't. There is no auto-start path: acceptance requires the swipe, and the first charge and every renewal are separate pendingRequests entries that go through the normal send confirm. The amount is immutable in the SDK record, so a changed amount is a new request ID needing fresh acceptance. Cancellation is committed in the SDK record and recurringPending filters on lifecycleState == .activeRecurring (:1587), so a cancelled subscription cannot resurface; the SDK rejects proofs after cancel. Creator cancel correctly skips the payer-only protectedRequestIds check (:1339-1347). Period arithmetic holds up: the wholeSecondDate refactor re-attaches anchor nanoseconds consistently, contains(_:) yields exactly the period containing the proof start, every is bounded, minute/hour are unsupported, and overflow returns nil and breaks rather than wrapping.
The payer-guard removal in PaykitSubscription.init?(record:) (:505-513) is safe. It now admits payee records, so I traced every consumer of manager.subscriptions: presentation (:1236-1254), accept (:1293), refresh (:1562-1573), applyCommittedSubscription (:1658-1669), discardExpiredRequests (:1762), the notification scheduler (PaykitSubscription.swift:677), monthly cost, and AppScene.swift:1023. All payer-gated — historyRequests and outgoingRequests never receive creator-side rows.
Key material and privacy. PubkyService.swift:489-500 only adds an identity check inside the same lock, calling sdk.identityStatus() directly with no re-entrant operationLock. Nothing secret logged. Worth noting the icon path does the right thing: compressedSubscriptionIcon re-encodes via CGImageSourceCreateThumbnailAtIndex → UIImage.jpegData, so EXIF and GPS from the photo library are not published, and SDK avatar names are content-addressed.
Trust-boundary fields are all bounded: description ≤1024, benefits ≤8×160, icon_uri ≤512 + scheme check, note ≤256, amount via strict sats(fromBitcoinAmount:), endpoints filtered by network. The proposal path rechecks eligibility, identity, endpoints and expiry after the upload (:602-609), and isCreatingRequest + interactiveDismissDisabled prevent double-submit.
Recorded, deliberately not filed against this PR — a Paykit-level trust-model gap.
The creator-side ledger counts payer-asserted proofs with no settlement check. PaykitSubscription.swift:545-548 guards only "parses as a billing period" and "is on the schedule grid"; proof.proof — the actual preimage or txid — is never read on the payee path, and Payment (:408-411) doesn't even carry it. So a linked contact running a modified client can make "1 subscriber · N payments" and "+amount" rows appear having paid nothing. I confirmed the SDK half rather than assuming it: Package.resolved pins paykit-rs 09e388d8 (rc46), and paykit-lib/src/payment_request/types.rs:370 says in its own doc comment that validate_for_request "checks stateless correlation only … Caller state still owns lifecycle, role, dedupe, settlement". The caller never does it. recurrence.contains anchors on the period's own start, so future periods count too, up to maximumPeriods.
Why it isn't this PR's defect: the same trust model is already live verbatim on master for one-time requests — origin/master:PaykitPaymentRequestService.swift:66-74 accepts both roles, :127-129 ignores .proof, and PaymentRequestsView.swift:163/:307 already render an asserted proof as a green "+amount received". This PR introduces the creator role and the aggregates, but inherits the model. Blast radius is also narrower than it first looks: these rows never reach historyRequests, the Payment Requests ledger, the activity list, or balance — receivedPaymentRequests() has exactly one call site (SubscriptionsView.swift:461), the detail sheet. It's a display artifact in one dev-gated sheet.
So: no change requested here. If you want it addressed, the right shape is a Paykit-level issue covering both the one-time and recurring flows, not settlement reconciliation bolted onto this PR — that would be real machinery (inbound LN payment-hash reconciliation, on-chain txid→output→own-address matching, plus persistence for proofs arriving before the wallet sees settlement) for an unshipped feature. A cheap interim if you want one: drop the "N payments" headline from rowSubtitle (:1021-1023) and the detail cell (:426-428), keeping the per-period cards where they read as "the subscriber says they paid."
Not re-raising: the three threads I opened (short-first-period, deferred cross-platform; silent drop, fixed in 6a82651c by dropping the order-sensitive savedPublicKeys guard; double-compress, accepted as deferred), the four greptile threads, or your accepted limitations on public icon hosting, unbounded icon fetch and the orphaned icon on failure.
Gating: PaykitFeatureFlags.isUIEnabled reads UserDefaults["paykitUiEnabled"], default false (PaykitFeatureFlags.swift:14-16) — dev/QA-facing today, user-facing when the flag flips.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Fix confirmed at 123ccf51, and you went further than I asked — greptile thread 3969695337 is addressed.
I'd offered the subtitle parameter as one option. You added both the saved-contact name and an unconditional truncated-key line (:946-953), gated by a new showsCounterparty that only the review call site passes (:609). I traced the provenance of both values, since a "fix" that rendered a proposer-supplied name would have been worse than none:
subscription.counterparty— non-optionalletatPaykitSubscription.swift:413, assigned at:526fromrecord.counterparty. The FFI binding documents that as the private-stream peer identity established by the transport, not a field parsed from the proposal terms. Same provenance one-time requests already rely on (PaykitPaymentRequestService.swift:117).contact.displayName— resolved from the local contacts store by normalized key match against that transport-derived counterparty (:924). Either a user-edited override, the counterparty's own profile with a label fallback, or a placeholder built from the truncated key. None of it readable from the proposal payload.
So neither value touches subscription.metadata. That's the right shape.
Also checked: the key line is unconditional and on its own row with its own lineLimit(1), so a long contact name truncates in its own row and can't push identity out of view. A non-contact counterparty still renders the truncated key rather than going blank. The other two SubscriptionProviderCard call sites (:790, :832) pass no showsCounterparty and default false, so they render exactly as before. The new @EnvironmentObject ContactsManager is already injected at AppScene.swift:203 and the card's own child SubscriptionAvatar already required it, so no new missing-environment crash surface.
Stating one thing rather than leaving it assumed: icon precedence is unchanged. :310 still lets a proposer-supplied iconURI win over the contact avatar. That's fine now the text rows carry locally-derived identity, but it is text-only that changed.
Non-blocking, take it or leave it: note is still the first and only bold line on the card (:941), with the identity rows third and fourth in dimmed caption. A proposer setting note = "Synonym" gets a bold line that reads like a name, with the real key beneath it — detectable now, but subordinate. PaymentRequestsView.swift:613 uses the inverse hierarchy (contact?.displayName ?? displayTruncated(counterparty) as the primary BodyMSBText). If you want the review route to match that convention, promote identity to primary and demote note to a caption. I wouldn't hold the PR for it.
Gating unchanged: isUIEnabled default false — dev/QA-facing today.
2 participants
Description
This PR adds subscription proposals to contacts, building on the payer flow merged in #685.
Discover, autopay and renewal UI are intentionally excluded. Icon hosting is public by design. Adds one changelog fragment.
Base:
master, including the merged payer PR. No additional unmerged branch dependency.Deferred custom icon limits
Custom subscription icons remain enabled. The current Paykit SDK buffers the complete image download before iOS can enforce limits, so oversized images can exhaust memory or storage. Bounded SDK fetching and iOS decode/cache limits remain follow-up work.
Safe publication and cleanup also remain unresolved. Avatar filenames are shared by content, and a proposal call can fail after enqueueing. Deleting an uploaded image on every error could break another profile or a queued proposal. These limitations are deferred to later Paykit work and coordinated Swift/Kotlin SDK integration in iOS and Android, including Android #1239 or its merged successor. This PR does not fix these risks or claim security readiness.
Linked Issues/Tasks
Screenshot / Video
Recordings are silent at 4× speed using test wallets. All attached media was inspected and shows only Bitkit, including its keyboard; no device home screen, photo picker or other app is included.
Final keyboard and recipient behavior:
ios-keyboard-fix-4x.mp4
Final recipient and keyboard screenshots
Earlier create → recipient → sent → overview walkthrough
This walkthrough predates the final keyboard and recipient sizing/spacing corrections. The recording and screenshots above show those final corrections.
ios-figma-audit-4x.mp4
QA Notes
Manual Tests
Live regtest creation/delivery/acceptance/manual on-chain payment, public icon transfer and cancellation passed in both directions with Android. Latest-build confirmation, pending deletion and restart also passed. Offline draft retention/retry was tested on Android. Live Lightning, mainnet and production push were not tested.
Automated Checks
PaykitSubscriptionProposalTests.swiftcovers UTF-8 wire boundaries, escaped strings, reserved icon space and image downsampling/errors.PaykitPaymentRequestServiceTests.swiftcovers creator lifecycle, validation before upload/enqueue and proof aggregation, including fractional billing instants.DEBUG E2E_BUILD,E2E_BACKEND=network,E2E_NETWORK=regtest; SwiftFormat lint passed for the touched Swift files. This was a focused test run, not the entire iOS suite.