fix: expose rejected incoming payment requests

[ovitrif]

Fixes #1209

Description

• Records stable parse, resolution, and presentation failure reasons while redacting payment-request counterparties and targets.
• Retries explicitly opened payment requests 15 times before showing localized terminal feedback and keeping the request actionable.
• Adds accessibility tags, regression coverage, documentation, and a reproducible rejected-request journey.

Preview

pr1209-terminal-recovery-preview.mp4

QA Notes

Manual Tests

• 1. Sender Receive → Payment Request Details → request 1 sat from a saved contact → Payment Request Sent: request reaches the receiver.
• 2. Sender Settings → disable Paykit → receiver Home → Payment Requests in-sheet → tap Pay: 15 attempts run at two-second intervals, then the localized unavailable toast appears.
• 3. Payment Requests in-sheet after terminal feedback → use the same row: it remains available for retry or dismissal.

Automated Checks

• Unit tests added in PaykitPaymentRequestDiagnosticsTest.kt: verify parse and resolution diagnostics redact valid and invalid counterparties and Throwable messages while retaining a stable error type.
• Unit tests modified in PaykitPaymentRequestRepoTest.kt and PublicPaykitRepoTest.kt: cover stable parse and resolution failure reasons and suppress repeated expired-record diagnostics.
• Unit tests modified in AppViewModelSendFlowTest.kt: cover 15 explicit attempts, final redacted resolution diagnostics, decoded-target log redaction, localized terminal feedback, expiration during backoff or resolution, automatic-batch continuation, and request-sheet restoration without replacing unrelated sheets.
• Instrumented tests modified in PaymentRequestsScreenTest.kt: cover stable request, Pay, and Dismiss accessibility tags; the focused class passes 5/5 on API 37 and 5/5 on API 36.
• Prior-head verification before the latest master merge: AppViewModelSendFlowTest passed 202/202; just compile, just test, and just lint passed.
• Latest master merge: tests, builds, lint, device QA, CI triggers, and CI observation were not run.

The full two-wallet journey passed on API 37 before the latest master merge: a delivered 1-sat request became unresolvable after its sender disabled Paykit, produced 15 redacted resolution_failed attempts, showed terminal feedback, and returned to the request sheet with the same row actionable. The prior head passed GitHub's full local and staging E2E matrix, and the focused payment-request UI class passed on API 37 and API 36.

[piotr-iohk]

QA LGTM.

Tested latest (95788b9) Pixel emu against iOS codex/paykit-payment-proofs, regtest. Incoming 1 sat + 27k from iOS. Tap Pay on an unresolvable request:

• 15 resolution_failed attempts at ~2s
• Stopped retrying requested incoming Paykit payment request after '15' presentation attempts
• Toast: "Payment Request" / "The payment request is no longer available."
• Row stays with Pay and Dismiss

Opening a still-resolvable request goes to Confirm with swipe disabled. That is the master isAmountInputValid hole, not this PR. Already fixed on #1178 (e04115003); standalone: #1218 / #1221. Not a blocker for this toast/retry path.

[piotr-iohk]

Please resolve conflicts.

[piotr-iohk]

QA LGTM.

Latest (800b902d) after the conflict rebase.

I already ran the full unresolvable-request journey on 95788b9 (15 × ~2s, redacted resolution_failed, unavailable toast, row stays with Pay/Dismiss). The only commit since that QA is the valid-pubky redaction test. Rebase onto master (incl. #1178) does not change the toast/retry path.

This pass:

• Installed 800b902d on Pixel_6 emu. Wallet restored (Alice / pubkyff…qyqnsuy), contact payments on, Paykit session re-signed and published btc-regtest-p2wpkh.
• Focused unit tests pass: PaykitPaymentRequestDiagnosticsTest, PaykitPaymentRequestRepoTest, PublicPaykitRepoTest, AppViewModelSendFlowTest.
• Journey XML parses.

CI green on this head, including local + staging E2E.

Happy to approve.

[piotr-iohk]

Pls resolve conflicts

[ovitrif]

@piotr-iohk Merged current master and resolved the payment-request/subscription conflicts while retaining both behaviors.

Fixed in 36c4404

[jvsena42]

All three of my earlier findings are still fixed after the master merge — I checked rather than assumed, since 36c440497 brought in #1186 and touched these exact blocks:

• The generation bump is still inside the guard (AppViewModel.kt:1116-1120); the merge only renamed clearRequestedPaymentRequestPresentation() → clearRequestedPaymentRequest(), and finishUnavailablePaymentRequestPresentation:1132 mirrors it. Automatic-batch behaviour intact: expired A → no bump → B still passes isCurrentPaymentRequestPresentation and presents.
• Expired("expired", shouldLogIncomingRejection = false) survives at :97.
• if (clearIncomingPaymentRequestTarget()) return still precedes the generic toast at :3317.

fix: simplify expiry condition is behaviour-identical, which I traced end to end: synchronizeLocked samples clock.now() once (:643) for both paths, and expiresAt != null && expiresAt <= now ≡ expiresAt?.let { it <= now } == true — same operator, same clock. It actually restores verbatim what master already had; the merge had hand-expanded it, which is what tripped detekt. PaykitPaymentRequest.isExpired:111-112 uses the same predicate, and scheduleExpirationLocked delays by (nextExpiration - clock.now()).coerceAtLeast(ZERO) so the discard fires at or after expiresAt, never before. No boundary shift, and iOS's parsedExpiration > now keep-condition is the same boundary. Injected Clock throughout, no System.currentTimeMillis() in the Paykit files.

Two LOW notes inline. Both port to the iOS twin (synonymdev/bitkit-ios#721).

[jvsena42]

No HIGH, no MEDIUM. One LOW inline — a merge artefact, not a security issue.

This pass focused on the delta since my last one (486fbf7 expiry simplification, 9c1fa8c unsupported_recurrence split) and on whether the new logic behaves over the state the master merge brought in.

What I traced clean:

• toPaykitSubscription() now runs inside parsePaykitPaymentRequest on every recurring record (PaykitPaymentRequestRepo.kt:1099). It cannot throw and take down refresh(): every validation exits return null, every.toInt() is guarded by the > Int.MAX_VALUE.toUInt() check, and the only throwing call (metadata.exportText()) sits inside runCatching. A hostile recurring record cannot DoS the pending list.
• The recurring split is behaviour-preserving for the accept set. Both RecurringRequest and UnsupportedRecurrence return Rejected; a malformed recurrence still never reaches oneTimeIncoming. The history path maps to UnsupportedRecurrence → null, identical to base's recurrence != null → null. Only logging changed.
• Fund safety re-traced after the merge. Amount and counterparty are pinned at open time (beginContactPayment passes PaymentAmountContext(it.amountValue, "btc") from the pinned request, :555); pay time checks equality against _sendUiState.amount (:3994), bolt11 msat match (:4003-4006), isPending (:4019), single-flight (:4770), and updateRequest moves the id into processingRequestIds and out of _pendingRequests before the send (:844-861). A failed SDK reject leaves the request pending and visible; nothing auto-pays.
• Redaction is closed on every path. scanLogId returns a fixed string whenever the context carries a request (:2183, :2227); decode-failure, decode-success and unhandled-variant branches all use fixed messages; logPresentationFailure emits only error::class.simpleName. No peer-supplied bytes reach the log, both toasts are fixed resources, and no failure reason is sent back to the counterparty.
• StateFlow conflation on pendingRequests is safe — retainPaymentRequestPresentationState is state-based (id !in requestIds), not event-based, so a dropped intermediate emission cannot produce a false unavailable toast.
• Docs and journey match head. 1 + 14 × 2 s (:5408) = 15 attempts / 28 s, inside the journey's 35 s; automatic presentation falls back to REFRESH_INTERVALS.last() = 120 s; silent reasons match shouldLogIncomingRejection = false; toast title and PaymentRequestsScreen tag both resolve. The journey starts on the full-screen route where shouldRestorePaymentRequestSheet is false, so "remains visible" is the correct assertion.
• runCatching in the diff is only on non-suspend calls (Instant.parse, Bolt11Invoice.fromStr); suspend paths use runSuspendCatching.

Parity with synonymdev/bitkit-ios#721: iOS has no unsupported_recurrence — its ParseFailure still has 12 cases (PaykitPaymentRequestService.swift:5-22), and its shouldLogIncomingRejection silences only outgoingRequest/nonActionableState, so it logs recurring_request for every recurring record and leans on per-(record, reason, counterparty) dedupe to stay quiet. Net: iOS has no triage blind spot for malformed recurrences, but the two wire taxonomies now diverge (13 vs 12 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 20f8c55e. wallet__payment_request_timestamp is gone from strings.xml:1284, git grep payment_request_timestamp -- app/src returns zero hits at the new head, and alphabetical ordering is preserved around the deletion (..._time → ..._unavailable). Nothing else changed in the commit.

That was the only finding I had. Clean from my side.

[jvsena42]

fix: cancel expired payment request scan reviewed — this is new work rather than a response to my findings (the dead string was the only one and it's already gone). The change is right in intent; one LOW inline about collateral damage, gated.

The swap at :1118 from a bare paymentRequestPresentationGeneration++ to invalidatePaymentRequestPresentation() means an expired requested payment request now also tears down a scan that's still queued for it, instead of leaving one to fire later against a request the user was just told had expired. That's the correct fix for the case the new test pins.

I checked the three remaining bare paymentRequestPresentationGeneration++ sites to see whether the fix was applied incompletely, and I don't think it was:

• :929 — the Send sheet is visible and markPresented just succeeded. This is the success path; there shouldn't be a queued scan to cancel, and cancelling one would be wrong.
• :1076 — retries exhausted, "unavailable" toast. Arguable, but the request stays pending and the automatic presentation path can still pick it up, so a hard scan teardown would be more aggressive than the state warrants.
• :2953 — interrupted request with retryIncomingRequest == false, which immediately markPresenteds. Same reasoning.

So the single call-site change looks deliberate rather than partial. The helper itself and its other eight references pre-date this commit; only :1118 changed.

[jvsena42]

@ovitrif conflicts again — this one re-conflicted after your 09-09 resolution, so the earlier "resolved" note no longer holds.

mergeable=CONFLICTING at head ca1144f4. Master moved since (#1216 and #1236 merged), which is the likely cause.

Flagging because I reviewed 20f8c55e earlier today and there's an open LOW on AppViewModel.kt:1118 — worth folding into the same push rather than a separate one.

[jvsena42]

Rebased and mergeable again at 97a5c34c — and the LOW I raised is fixed, in exactly the narrow shape rather than a broad one.

invalidatePaymentRequestPresentation now takes a requestId and scopes the teardown through a local predicate (:1205-1223):

1209        fun targetsRequest(context: ContactPaymentContext?): Boolean {
1210            val scanRequestId = context?.incomingPaymentRequest?.id ?: return false
1211            return requestId == null || scanRequestId == requestId
1212        }

The requestId == null branch preserves the old broad behaviour, so the seven pre-existing call sites are unchanged — that was the part I cared about, since a blanket narrowing there could have been its own regression. And the expiry site now passes the id (:1118), so request A expiring can no longer nil a deferred scan belonging to request B. The scenario I described is closed.

You also added behaviour I didn't ask for and it looks right: the helper returns whether the expired request owned the active context with the Send sheet open, and :1120 hides the sheet in that case. ownsActiveContext compares against the same requestId, so it can't dismiss a sheet belonging to a different request — worth stating explicitly since that was the obvious way to get it wrong.

Rebase looks clean: the PR-authored delta against master is intact (14 files), and nothing from the earlier review was dropped.