Skip to content

feat: add subscription requests - #736

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

feat: add subscription requests#736
ben-kaufman wants to merge 10 commits into
masterfrom
codex/paykit-subscription-requests

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 #685.

  • 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 proposals and pending from accepted subscriptions. Show creator subscribers/received payments, with delete/cancel but no edit.
  • Validate the wire-size limit before uploading/enqueueing, downsample icons, and count valid billing-period payments once without generating payer prompts for creators.

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 Final recipient field, expiry and contact spacing Name field with standard button clearance above keyboard Description scrolled into view with keyboard open
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

  • 1. Subscriptions → Create Subscription → Amount: enter a positive amount; set name and frequency; choose one privately linked contact and expiry → Propose Subscription: sent confirmation shows the recipient, amount and frequency.
  • 2. Create Subscription → upload custom icon → Choose Recipient → back: amount, name, description, frequency and icon remain intact; the receiver sees the same public icon.
  • 3. Receiver → Review & Subscribe → accept and manually pay: creator changes from pending to one subscriber/one received payment attributed to the payer; the creator does not get a payer payment prompt.
  • 4. Pending or accepted creator subscription → Delete → Swipe To Delete: cancellation reaches the receiver; existing payment history is retained; deleted creator proposal remains absent after restart.
  • 5. Send with unavailable connectivity or oversized metadata: no false Sent confirmation, useful error and retained draft; retry works after correcting the condition.
  • 6. regression: Payment Requests → Create Payment Request: shared amount, recipient selection and expiry behave as before.
  • 7. Create Subscription → focus Name and Description in either order: button keeps its standard keyboard clearance, wrapped description/caret stays visible, scrolling and keyboard dismissal work; Choose Recipient and Back preserve the draft while either field is focused.

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

  • 156 focused XCTest cases passed after the final code change. PaykitSubscriptionProposalTests.swift covers UTF-8 wire boundaries, escaped strings, reserved icon space and image downsampling/errors. PaykitPaymentRequestServiceTests.swift covers creator lifecycle, validation before upload/enqueue and proof aggregation, including fractional billing instants.
  • Existing payment-proof, send-confirmation, currency and fiat-formatting suites passed. No automated coverage was removed.
  • Simulator E2E build passed with 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.
  • Final local review found no remaining actionable issues. Keyboard behavior was verified interactively; the focused unit suite does not automate those interactions.

@greptile-apps

greptile-apps Bot commented Sep 9, 2026

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 2/5

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

  1. P1 Security Icon Upload Is Nonatomic
  2. P1 Cancellation Hides Payment History
  3. P2 Security Remote Icons Lack Limits
  4. P2 Translations Contain English Placeholders

Summary

  • Builds recurring Paykit proposal terms with wire-size validation and icon downsampling.
  • Separates payer subscriptions from creator-owned proposals and accepted subscriptions.
  • Adds creator payment history, deletion controls, proposal confirmation states, assets, and localized strings.
  • Refactors shared amount, recipient, text-entry, image, and segmented-control components for the new flow.
  • The review identified non-atomic icon publication, inaccessible history after creator cancellation, unbounded remote icon decoding, and localization-workflow issues.

Diagram

sequenceDiagram
    participant Creator
    participant UI as Subscription UI
    participant Manager as Payment Request Manager
    participant Pubky
    participant Paykit
    participant Recipient

    Creator->>UI: Enter amount, metadata, frequency and recipient
    UI->>Manager: proposeSubscription(draft, target)
    Manager->>Manager: Validate expiry, endpoints and recipient
    opt Custom icon
        Manager->>Pubky: Upload public icon
        Pubky-->>Manager: icon_uri
    end
    Manager->>Paykit: Propose recurring payment request
    Paykit-->>Manager: Proposal record
    Manager->>Paykit: Process pending private messages
    alt Delivered
        Paykit-->>Recipient: Subscription proposal
        Manager-->>UI: Sent
    else Delivery pending
        Manager-->>UI: Queued
    end
    Recipient->>Paykit: Accept and submit payment proof
    Paykit-->>Manager: Active subscription and proof
    Manager-->>UI: Subscriber and received-payment history
    Creator->>Manager: Delete subscription
    Manager->>Paykit: Cancel recurring request
Loading

Comment on lines +593 to +598
let iconURI: String? = if let iconData = draft.iconData {
try await sdk.uploadProfileAvatar(
bytes: Self.compressedSubscriptionIcon(iconData),
contentType: "image/jpeg",
expectedIdentity: expectedIdentity
)

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 security Icon Upload Is Nonatomic

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:

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.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

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.

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.

Comment on lines +470 to +472
func isCreatedVisible(at date: Date) -> Bool {
isCreatedByUser && (isProposalVisible(at: date) || isActive(at: date))
}

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 Cancellation Hides Payment History

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

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.

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 }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 security Remote Icons Lack Limits

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

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.

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.

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.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

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.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment on lines +1213 to +1238
"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";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Translations Contain English Placeholders

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!

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.

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 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 high finding. The payer-side payment paths this PR touches are only narrowedisPayer 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 #685none 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,

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.

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.

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.

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 }

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

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.

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

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.

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.

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.

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

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

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-optional let at PaykitSubscription.swift:413, assigned at :526 from record.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.

@jvsena42

Copy link
Copy Markdown
Member

Cross-platform test with Android #1239

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

Verdict: iOS → Android works end to end. Android → iOS is blocked, and the test IDs have mismatches that I'm treating as blocking.

Results against the QA list

# Test Result
1 Create & propose ✅ iOS → Android. The Sent screen shows AA, ₿5,000, Weekly. Android got Review & Subscribe about 11 s later. ❌ Android → iOS never arrived (see blocker).
2 Draft kept + icon ⚠️ Android keeps amount, frequency, name, description and icon across Choose Recipient → Back. Couldn't check the icon on the iOS receiver because of the blocker.
3 Accept & pay ✅ iOS creator → Android payer, paid on-chain. About 25 s later the iOS row showed 1 subscriber · 1 payment, credited to AA. The creator got only the normal Received Bitcoin sheet, no payer prompt. ❌ Reverse direction blocked.
4 Delete / cancel ⚠️ The cancellation reached Android in about 32 s: Expired, payment history kept. The iOS Activity entry is kept too. But after an iOS relaunch the creator subscription disappeared entirely: the overview was empty and Payments said "No payment history". isExpiredVisible (PaykitSubscription.swift:474) is meant to keep a cancelled creator subscription with payments visible. May share a cause with the blocker; not confirmed.
5 Offline / oversized Not tested
6 Payment request regression ❌ Android sent one; iOS never received it. iOS couldn't create one to AA (see blocker).
7 Keyboard Not checked: the simulator uses a hardware keyboard, so the on-screen keyboard never appeared.

🔴 Blocking: accessibility identifiers

a) Container identifiers override their children's IDs. An .accessibilityIdentifier on a container without .accessibilityElement(children: .contain) hands its ID down to child controls. I confirmed each of these with axe describe-ui:

  • CreateSubscriptionView.swift:209 SubscriptionAmount: Continue, 000, 0 and Backspace all report SubscriptionAmount instead of SubscriptionAmountContinue / N000 / N0 / NRemove.
  • CreateSubscriptionView.swift:77 CreateSubscription: Choose Recipient reports CreateSubscription instead of SubscriptionChooseRecipient.
  • CreatePaymentRequestView.swift:191 (PaykitRecipientPicker): Propose Subscription, Paste and Expires In report SubscriptionRecipient instead of SubscriptionPropose / SubscriptionRecipientPaste. This one is shared, so the Payment Request recipient screen is affected too.

b) IDs that don't match Android. AGENTS.md says iOS should use the Android ID strings.

Element iOS Android
Frequency tabs Tab-daily/weekly/monthly/yearly Tab-day/week/month/year (CustomTabRowWithSpacing.kt:58, from the enum name)
Amount Continue SubscriptionAmountContinue (CreatePaymentRequestView.swift:322) PaymentRequestAmountContinue (subscription reuses PaymentRequestAmountContent)
Recipient contact row SubscriptionContact-<pubky> (CreatePaymentRequestView.swift:249) SubscriptionContact<pubky>, no hyphen

The iOS frequency IDs also change with the device language. SegmentedControl.swift:74 builds Tab-\(description.lowercased()), and SubscriptionFrequencyOption.description is the translated label (CreateSubscriptionView.swift:361), so in any other language the ID changes. It needs a stable key, such as the raw value.

🔴 Blocker: iOS stops receiving from Android after a delete + relaunch

  1. At 17:10 the link worked in both directions: Android's acceptance and payment proof reached iOS.
  2. iOS deleted the accepted creator subscription (17:12:44) and was relaunched (17:13:40).
  3. From then on, iOS received nothing from AA:
    • Android's subscription proposal (17:22:49): Android marked it deliveryStatus == Sent (proposalWasSent → the SDK sent the queued message).
    • A plain payment request (17:31:17): same.
    • Foregrounding the app and re-entering Subscriptions (which runs refresh()) didn't help.
  4. Tapping Send on AA logged the cause (PrivatePaykitService+Payments.swift:135):
    RecoveryRequired(code: "recovery_required", context: "Encrypted Link Handshake is still in progress for counterparty sgkjeu68g6pximn1yu5ijqbtyneic1s1gwtpibmzsxhgk6jnpmxy")
    
  5. AA then falls out of eligibleTargets, which only keeps .linked peers. Send skips the Request-or-Pay chooser, and incoming items from AA are never shown. Nothing is logged along this path. eligibleTargets returns [] quietly and performRefresh only logs errors, so this looks exactly like "no messages".
  6. Meanwhile Android keeps calling restore_encrypted_link for BB; its last handshake was at 17:02. The two sides disagree about the link.
  7. Removing and re-adding the contact on both apps didn't recover it. Android never starts a new handshake. The iOS contact removal also logged Failed to remove private Paykit endpoints for deleted contact …: privateUnavailable and Failed to prune private Paykit endpoints …: privateUnavailable. paykitContactSharingCleanupPending stays false, so the private endpoints published for AA are left in place with no retry queued.

There's no in-app recovery. .recoveryRequired only sets a status label. This looks like Paykit SDK link state rather than code in this PR, but it's reachable through this PR's delete flow, and I can't tell whether the delete or the relaunch caused it.

Repro: iOS creates a subscription → Android accepts and pays → iOS deletes it (Swipe To Delete) → relaunch iOS → Android sends anything to iOS → it never shows up. Then tap Send on the contact and check the app-group log for RecoveryRequired.

Suggestion: log once when a saved contact is left out of eligibleTargets because of its link state. It would have turned hours of silence into one line.

Minor

  • The name field autocorrects ("Weekly" → "Wellington"). That's normal for a text field, but it's worth deciding whether subscription names should autocorrect at all.

@ben-kaufman

ben-kaufman commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the accessibility ID issues. The container IDs now preserve their child controls, and the subscription flow uses Tab-day/week/month/year, PaymentRequestAmountContinue, and SubscriptionContact<pubky> to match Android.

I could not tie the recovery_required link state to subscription deletion in this PR. The subscription delete path calls cancelPaymentRequest; it does not mutate contacts or encrypted-link state. The failed Android-to-iOS delivery and post-relaunch history loss remain open for Paykit recovery and persistence work. I left name autocorrection unchanged.

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