fix: expose rejected incoming payment requests

[ovitrif]

Fixes #714

Expose rejected incoming payment requests through privacy-safe diagnostics and terminal user feedback while preserving automatic recovery.

Description

• Classify malformed incoming requests with stable parse reason codes and log only the redacted counterparty.
• Log each permanent parse rejection once per record and reason.
• Classify endpoint-resolution and payment-target failures with redaction-safe error categories and codes without logging request contents or raw errors.
• Keep automatic presentation retries running every 120 seconds after the initial retry window without repeated warning logs.
• End an explicit Pay attempt after fifteen failures with the localized Payment Request unavailable toast while leaving the request actionable.
• Preserve requested expirations across suspended resolution and retry backoff, then show the localized expired toast exactly once.
• Document the failure contract and the ported Android resolution-failure journey.

Linked Issues/Tasks

• fix: expose rejected incoming payment requests #714
• Parent: epic: Pubky marketplace wallet support #712

Preview

pr721-payment-request-unavailable-2x.mp4

QA Notes

Manual Tests

• 1. iPhone 17 simulator on iOS 26.5/regtest → pair with an iPhone Air Paykit peer: both apps exchange private Paykit data.
• 2. Create profiles on both simulators → save each profile as a contact on the other → send a 1 sat incoming payment request: the request appears on the payer.
• 3. Peer contact-payment settings → disable payment endpoints → payer Payment Requests → tap Pay: explicit resolution retries begin.
• 4. Payment Requests → wait through fifteen explicit attempts: logs show category=resolution reason=no_supported_endpoint and redact the counterparty.
• 5. Fifteenth failed attempt → inspect terminal feedback: PaymentRequestUnavailableToast shows Payment Request and The payment request is no longer available.
• 6. Dismiss terminal feedback → inspect the request row for payment-request id 7abfa801-a3bd-4d74-b75a-18be91d2ddbf: PaymentRequestPay-7abfa801-a3bd-4d74-b75a-18be91d2ddbf remains enabled for another attempt.

Automated Checks

• Prior focused simulator tests after the prior base sync: 77 passed across PaykitPaymentRequestServiceTests and PublicPaykitServiceTests.
• Prior focused simulator tests: 72 passed across PaykitPaymentRequestServiceTests and PrivatePaykitServiceTests.
• Prior focused simulator tests: 73 passed across PaykitPaymentRequestServiceTests and PublicPaykitServiceTests.
• Prior simulator build-and-run: passed on iPhone 17.
• Prior SwiftFormat lint, translation validation, journey XML validation, and changed diff check: passed.
• Android #1217 verification: matching requested-resolution-failure.xml journey confirmed at b79c43d.
• Current head: 1fd7dff2, based on master 05c4fe01.
• Current-head tests, builds, simulator/device QA, and CI watching: not run at user direction.

[ovitrif]

@greptileai

[ovitrif]

The rewritten head preserves the earlier expiry fix. I found two non-blocking coverage and journey-documentation gaps.

[jvsena42]

One nit, arising from reviewing the Android counterpart (#1217) rather than from this PR's own code: the iOS-only label on this suite stops being true once #1217 lands.

[jvsena42]

Reviewed this against the Android twin (synonymdev/bitkit-android#1217), where I confirmed a real regression. iOS is structurally better and does not have it — worth recording why.

Android bumped its presentation generation unconditionally when a request turned out expired, including for automatic presentations, so an expired request A invalidated the batch and request B behind it was fully resolved over the network and then discarded. Here, presentRequests snapshots activePresentationGeneration for the closure (:888-901) and isCurrentPresentation (:905-912) is checked at AppScene.swift:950 — before the beginPaymentRequest network call at :952 — and again at :953-957, :983, :1012, :1027, :1038. So even when the generation does move, no resolution work is wasted. The new expired branch also returns at :946 before the bump, and discardExpiredRequests only bumps when requestedPresentationId itself is dropped (:1110-1113), which is nil for automatic presentations.

The other two Android bugs are absent too: .expired is effectively unreachable because the SDK derives ProposalExpired at query time and parse checks state != .proposed first (returning .nonActionableState, which is excluded from logging), and synchronizationDate is sampled before the await. And the toast queues can't both be non-empty in one transition — performRefresh guards on requestedId != handledRequestedExpirationId (:1019) and deferPresentation passes it through (:942-948).

Also verified: the expiry double-now() race is fixed (one presentationDate threaded into discardExpiredRequests(at:), :940-945); all five new toast strings exist in en.lproj/Localizable.strings:1477-1481; persistPresentedRequestIds() runs before every discardExpiredRequests return path.

Two non-blocking logging notes inline.

[jvsena42]

Re-reviewed at 5e4bebd. No HIGH/MEDIUM. Nothing new to file — the two candidates I chased both died on verification (details below so they aren't re-chased).

Fund safety / authorization — clean. Amount and counterparty are pinned at approve time and this PR does not move that. SendSheet calls markPresentedIfPending(request) (clearing requestedPresentationId) and sets sendAmountSats from the request; confirmation goes prepareIncomingPaymentRequest → prepareForPayment → perform, which re-checks expiry, pending membership, and single-flight via processingRequestIds. approvedPaymentRequestIds is set only after service.accept succeeds. No path added here re-enters payment: every deferPresentation return value only affects retry scheduling and toasts, and .requestedPresentationEnded marks the request presented and drops the id, so a new Pay tap must go through requestPresentation again. Because markPresentedIfPending nils the id on send-sheet open, the perform expiry path can't enqueue a second "expired" toast alongside the send flow's own error.

Trust boundary — clean, and this was the main thing I wanted to check given the PR surfaces counterparty-side failures. Toasts render only static localization keys; logs carry enum raw values, safeCode ([a-z0-9_-], ≤64 bytes), a metatype name, and the 12-char redacted pubkey prefix. PaykitError.context is dropped everywhere. No homeserver- or counterparty-supplied string reaches the UI or a log verbatim.

Also traced clean: no persisted schema change (PresentationStore.State and PaykitPaymentRequest.ID untouched; ParseFailure and IncomingPaykitPaymentRequestFailureReason are never persisted), so no migration risk from the previously shipped build. parse accepts exactly the set base accepted for both actionable and history records — only failure reporting changed. Both feedback queues drain with while let under a single trigger comparison, so SwiftUI coalescing several increments into one onChange can't drop a toast. Every deferIncomingPaykitPaymentRequestPresentation call site is preceded by isCurrentPresentation(request) with no await in between, so there's no TOCTOU on the requester across the suspension. No seed-derived material touched.

Android parity (#1217): the sheet-restore-cancelled-by-hideSheet bug is structurally absent here — the sheet hides itself before requesting presentation and never restores, terminal feedback lands as a toast on whatever is underneath. The peer-supplied-invoice-logged-on-decode-failure issue is absent too; this PR actually removed the base Logger.warn("...: \(error)") at that site in favour of reason=invalid_payment_target, which is the right direction. Expiry-during-backoff is present on both and handled here via recordRequestedPresentationExpiration + expirationTrigger.

Still dev/QA-facing today (isUIEnabled defaults false, Dev Settings only), so none of this is user-reachable until the flag flips.

[jvsena42]

No findings. Clean at the HIGH/MEDIUM bar.

The delta since my last pass is one commit (00f15f348, "log initial payment request failure"), which takes the middle ground I offered — logging the first deferral of an automatic request rather than none. I checked it only for over-correction, not to reopen the choice:

• Shape landed as once per (request.id, reason) for automatic presentations, backed by automaticPresentationDiagnosticReasons. Bounded at 7 reasons per pending request; the dict is filtered to pending ids on every refresh, on expiry, on markPresentedIfPending, on perform success, and on clear(). No unbounded growth, nothing persisted, nothing keyed across identities.
• Requested (explicit Pay) presentations are untouched: wasRequestedPresentation is captured before the inner call can nil the id, and the guard returns false for anything requested or non-.retryScheduled. Terminal outcomes still force shouldLogDiagnostic = true in the struct init independent of the tuple, and .ignored stays silent.
• Toasts are byte-identical; only diagnosticMessage(for:) gates the log.
• testPresentationDispatcherLogsFirstAutomaticFailurePerReasonAndLifecycle now pins the effect — message text, repeat suppressed, reason change re-logs, record removal and reappearance re-logs — which addresses the "tests pin the flag not the effect" note from my last review.
• IncomingPaykitPaymentRequestFailureReason going Equatable -> Hashable is a synthesised conformance on a String-raw enum.

Gating: PaykitFeatureFlags.isUIEnabled (isUIAvailable && UserDefaults paykitUiEnabled, default false), toggled only from Dev Settings behind the hidden SupportScreen tap. The manager re-checks the flag and refreshIncomingPaykitPaymentRequests() bails without it. Dev/QA-facing today.

Checked and clean:

• Rejected request resurfacing or being paid. reject -> perform(resultingState: .rejected) -> sdk.rejectPaymentRequest happens inside operation(request) before any local mutation; if it throws, the request stays pending and the view toasts it. Only processPendingMessages() is try?-swallowed, and that's the outbound counterparty notification, retried on the next synchronize() — not local state. Every synchronize() re-parses the SDK store, and a .rejected record trips record.state != .proposed -> .nonActionableState, so it never re-enters pendingRequests. The counterparty can't rewrite local lifecycle state. Losing presentedRequestIds from the Keychain can re-show a sheet but never pay.
• Re-issue under a new id produces a new ID triple requiring a fresh Pay tap plus send-sheet confirmation. No auto-pay path exists here or on base, so content-based dedup isn't load-bearing for fund safety.
• Amount TOCTOU. The request reaching beginPaymentRequest is the snapshot from requestsForPresentation(); PaymentAmountContext is built from that object's amountValue and the same object is pinned into ContactPaymentContext. Nothing re-reads the amount from the store between display and pay.
• PaykitResolutionFailureDiagnostics. Log-only strings: case name plus safeCode ([a-z0-9_-], <=64 bytes, else unknown_code), PaykitError.context dropped in every arm, unknown types rendered as metatype name only. Sole consumer is one Logger.warn. No toast, no UI, no counterparty transmission — reject(...reason: nil). No error path leaks balance, liquidity or node id, and no rejection failure reports as success.
• Trust boundaries in PaymentRequestsView. The diff there is two accessibilityIdentifiers embedding paymentRequestId — not rendered, not a path, not a URL. Toasts render static localization keys only.
• AppScene / processing before unlock. Polling runs on .task(id: scenePhase) regardless of PIN, but sheets are hosted in MainNavView, which only renders once isPinVerified — AuthCheck replaces it while locked, so a showSheet(.send) issued while locked has no host until unlock, and the send sheet still requires explicit confirmation. handleScenePhaseChange only refreshes on .active.
• Concurrency. The manager is @Observable @MainActor; deferPresentation(_:diagnosticReason:) is synchronous with no suspension between the wasRequestedPresentation capture and the set insert. Both toast queues drain with while let under one trigger comparison, so Observable coalescing can't drop a toast. Every deferIncomingPaykitPaymentRequestPresentation call is preceded by isCurrentPresentation(request) with no await in between — no reentrancy window.
• Journey and READMEs match the code. Retry counts, the 120s steady interval, the 35s wait covering 1 + 14x2s, every accessibility identifier, and the toast title/description against Localizable.strings. "Request remains available for another attempt" holds — it stays in pendingRequests and isActionDisabled clears when requestedPresentationId nils.
• Migration. No persisted schema touched; the new dict is in-memory. No predicate was removed, so the #697 limit clause doesn't engage here.

One correction to my own framing from earlier: PrivatePaykitService+Errors.swift isn't new — it exists on master, and this PR appends PaykitResolutionFailureDiagnostics to it.

Cross-repo (synonymdev/bitkit-android#1217): the terminal unavailable toast after 15 explicit attempts is present on both. Android is missing the equivalent of requestedPresentationUnavailableTrigger for a requested request that disappears mid-retry for a non-expiry reason — filed there, not here. Your parse-rejection and presentation-failure log dedup is the better shape of the two; I've noted it on the Android side.

[jvsena42]

No HIGH, no MEDIUM. One LOW inline.

The only new code since my last verdict (00f15f34) is what the two master merges brought in. I checked merge fidelity by diffing the PR's added/removed lines before and after the merge for the three logic files: the only differences are master's context folded in (the record.state != .proposed expiry clause, paymentProofKind/billingPeriod, the didRejectScannedPaymentForInsufficientBalance branch, presentNextIncomingPaykitItem). Nothing from the PR was dropped or weakened in the resolution.

The real question was whether the PR's new logic behaves correctly over the state master added — subscription requests in pendingRequests, the new await inside performRefresh, dismissSubscriptionPayment, the SendSheet retry path. It does:

• No re-pay. Every new deferral outcome affects only retry scheduling and toasts. .requestedPresentationEnded marks presented and nils the id; a new Pay must go through requestPresentation (:1261-1273), send-sheet confirmation, then perform (:1641-1656), which re-checks expiry, pending membership and single-flight. Rejected records trip .nonActionableState on every re-parse (:120-122) and never re-enter pendingRequests. The master-added SendSheet.retryIncomingPaymentRequest (:872-890) feeds the same path; on 15 failures it gets the unavailable toast and the accepted request stays payable.
• Recurring requests under the new logic. isExpired (:270-272) is .proposed-only and subscription-derived requests have expiresAt = nil (:229), so recordRequestedPresentationExpiration can never fire for them; dismissSubscriptionPayment clears the requested id itself (:1184-1187) before any refresh, so it can't produce a spurious unavailable toast.
• Amount/counterparty pinning unchanged by the merge: ContactPaymentContext is built from the snapshot object at AppScene.swift:1048-1052, and isCurrentPresentation precedes every defer with no await between.
• Trust boundaries unchanged: toasts are static keys with no placeholders; logs carry enum raw values, safeCode, metatype name and the 12-char redacted prefix, with redactedCounterparty returning <invalid> for non-normalisable input.
• Journey arithmetic matches head: 1 + 14×2s (:839), 120 s steady (:840), and PaymentRequestsScreen (PaymentRequestsView.swift:338) is a pushed screen, not a sheet, so presentation isn't blocked by activeSheetConfiguration.

Not re-raising, but confirming it's still live at head: ben-kaufman's thread 3977405933 (AppScene:1010) is a merge product — PaykitPaymentRequestService.swift:1565 assigns pendingRequests → :1572 await subscriptionNotificationScheduler.synchronize( (master-added) → :1585 requested-id check. Any discardExpiredRequests() caller landing in that await hits :1721-1724 and nils requestedPresentationId with no unavailable record, so the branch at :1587-1592 has nothing to enqueue. Leaving it to that thread. Worth noting for cross-feeding: Android has no equivalent await inside its refresh between list replacement and the requested-id check, so that race doesn't port.

Dead branch, mentioning so nobody chases it: PaykitPaymentRequestManager.handleUnavailablePaymentRoute:1490 deferPresentation(request) is now unreachable — AppScene.swift:1073 only routes to the coordinator when didRejectScannedPaymentForInsufficientBalance is true, and the coordinator re-reads the same flag with no await between, so insufficientBalance is always true there. Cosmetic.

Parity with synonymdev/bitkit-android#1217: Android has no unsupported_recurrence equivalent on this side — iOS ParseFailure still has 12 cases and logs recurring_request for every recurring record, leaning on per-(record, reason, counterparty) dedupe. So iOS has no triage blind spot for malformed recurrences, but the two wire taxonomies now diverge (12 vs 13 reasons) and Android is silent where iOS logs once. Worth a decision on which side moves; not a defect on either.

[jvsena42]

Fix confirmed at b9279b77. The race ben's thread was about is closed, and the new test is a genuine regression test. One LOW artefact of the fix, posted as a refinement on that thread rather than a new one.

Why it works. The commit moves the requested-id check — and its presentationGeneration += 1 / unavailableRequestedPresentations.append / requestedPresentationId = nil — from after synchronize() to before it (:1572-1582, with the await now at :1583). The region :1505-1582 contains no suspension point, so the record is enqueued and the trigger incremented atomically with the pendingRequests replacement. The invariant during the await becomes: either the id is already nil with the record queued, or the request is still in pendingRequests. No interleaved caller can then reach :1721-1723 in a state that loses the toast.

I walked all five callers I named last time rather than assuming the one path was representative:

Caller during the await	Result
reconcileExpiredRequests	id already nil → :1704 guard fails, :1721 map is nil, no-op; record already queued. If it leaves pending via :1705 expiry, :1704 records it as expired first — the correct toast
requestPresentation	id non-nil → returns false; can only set a new id for a request that is in pending
markPresentedIfPending	nils the id without a toast — the success path; post-await code no longer re-checks the id, so no wrong toast
propose	invalidateRefresh() + discardExpiredRequests(), same as row 1
perform	success path nils the id deliberately; failure path starts a fresh refresh whose own pre-await check runs on fresh state

dismissSubscriptionPayment and applyCommittedSubscription (not on my original list) nil or replace only when the request is still pending, or via their own discardExpiredRequests() — same reasoning.

No inverse failure. Checked the success path too, not just the failure path: the only enqueue site is :1578, it runs once per refresh and nils the id at :1581, and the old post-await block was moved rather than duplicated — so no double toast. handledRequestedExpirationId and the unavailable check are sampled at the same refreshDate in the same synchronous region, so unavailable+expired can't pair for one Pay. And for a payment to happen the send sheet calls markPresentedIfPending first, which nils the id, so a succeeded request can't draw an unavailable toast.

Fund safety verified rather than assumed — the Bitkit-side diff is confined to performRefresh:1572-1596. perform() (:1634-1692) is byte-identical between heads: expiry guard, pending-membership guard, processingRequestIds single-flight, approvedPaymentRequestIds insertion. prepareForPayment, requestsForPresentation, isCurrentPresentation, finishPayment, paymentRequestForRetry all untouched. Feedback-only, as intended.

Also: unavailableRequestedPresentations holds a copy from previousPending and is only ever consumed by consumeUnavailableRequestedPresentation(); it is never written back into pendingRequests, so this does not widen #737.

Journey docs now spell the full composite PaymentRequestRow-<payment-request-id>-<counterparty>-<receiver-path>-one-time, which maps 1:1 onto PaymentRequestsView.swift:171-174. You went stricter than the prefix wording I suggested; it's correct, and the literal -one-time is right because the journey seeds exactly one proposed one-time request.

The test earns its keep. testRefreshRecordsUnavailableBeforeSuspendedNotificationSynchronization reproduces the interleaving exactly — suspends inside pendingNotificationRequests(), asserts mid-suspension that the id is nil and the trigger is 1, advances the clock, runs reconcileExpiredRequests(), then resumes and asserts exactly one .presentFeedback. Against d9369ecd it fails at the mid-suspension assertions. It pins the no-double case too.

[jvsena42]

@ovitrif conflicts — mergeable=CONFLICTING at head 55d0548c.

No one had flagged it on this PR yet. Master moved (#720 landed, #719's twin merged on android), which is the likely cause.

Worth folding in with the open refinement on thread 3977405933 — the pre-await requestIds snapshot at :1572 still being used by the post-await filters at :1591-1594 — so it's one push rather than two.