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.
• Local verification: AppViewModelSendFlowTest passes 202/202; just compile, just test, and just lint pass after syncing with master.

The full two-wallet journey passed on API 37: 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 current head passes GitHub's full local and staging E2E matrix, and the focused payment-request UI class passes 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.

[jvsena42]

The parsePaykitPaymentRequest refactor is behaviour-preserving against base for every reason (role/state/terms/asset/amount/endpoints/expiry all produce the same accept-reject set), IncomingPaykitPaymentRequestFailureReason covers all PublicPaykitPaymentResult cases, and the diagnostics logger correctly redacts the counterparty and never emits the Throwable message.

One regression worth fixing before merge, plus two low notes.

[jvsena42]

Regression test — expired request discards the payable request behind it

Reproduces the finishExpiredPaymentRequestPresentation generation bump. Test-only, applies cleanly onto codex/1209-payment-request-errors.

./gradlew :app:testDevDebugUnitTest --tests 'to.bitkit.viewmodels.AppViewModelSendFlowTest.expired request does not discard a later payable request'

Fails on head:

java.lang.AssertionError: expired request invalidated the automatic presentation, so the payable
request was resolved again instead of being shown expected:<1> but was:<2>
  at AppViewModelSendFlowTest.kt:1543

Passes once paymentPresentationGeneration++ moves inside the if (requestedPaymentRequestId == request.id) block — full class green at 196 tests. detekt clean.

Harness note: on unfixed code the re-entry loop recurses forever because the mocked pendingPaykitPaymentRequests never prunes the expired entry, so advanceUntilIdle() would hang rather than fail. The test bounds it inside the mock instead — beginPaymentRequest(payableRequest) uses a doSuspendableAnswer that parks on awaitCancellation() past the first attempt, so the count assertion reports the regression in plain language.

pr1217-regression.diff

diff --git a/app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt b/app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt
index 30fa70582..3a8a36478 100644
--- a/app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt
+++ b/app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt
@@ -1509,6 +1509,49 @@ class AppViewModelSendFlowTest : BaseUnitTest() {
         assertEquals(Sheet.Send(SendRoute.Confirm), sut.currentSheet.value)
     }
 
+    @Test
+    fun `expired request does not discard a later payable request`() = test {
+        val expiredRequest = paymentRequest()
+        val payableRequest = expiredRequest.copy(paymentRequestId = "payable-request")
+        val bolt11 = "lnbcrt1payableafterexpired"
+        val privateContext = PrivatePaykitPaymentContext("bitkit/server", 7uL)
+        var payableAttempts = 0
+        whenever(privatePaykitRepo.beginPaymentRequest(expiredRequest))
+            .thenReturn(Result.failure(PaykitPaymentRequestError.RequestExpired))
+        whenever(privatePaykitRepo.beginPaymentRequest(payableRequest)).doSuspendableAnswer {
+            payableAttempts++
+            if (payableAttempts > 1) awaitCancellation()
+            Result.success(
+                PublicPaykitPaymentResult.Opened(
+                    paymentRequest = bolt11,
+                    privatePaymentContext = privateContext,
+                ),
+            )
+        }
+        stubLightningScan(bolt11 = bolt11, amountSats = 0u)
+        balanceState.value = BalanceState(maxSendLightningSats = 100_000u)
+        pendingPaykitPaymentRequests.value = listOf(expiredRequest, payableRequest)
+        isPaykitEnabled.value = true
+        pubkyPublicKey.value = testPublicKey
+        whenever(paykitPaymentRequestRepo.refresh()).thenReturn(Result.success(Unit))
+
+        sut.startPaykitPaymentRequestPolling()
+        advanceTimeBy(30.seconds.inWholeMilliseconds)
+        runCurrent()
+        sut.stopPaykitPaymentRequestPolling()
+
+        assertEquals(
+            expected = 1,
+            actual = payableAttempts,
+            message = "expired request invalidated the automatic presentation, so the payable request " +
+                "was resolved again instead of being shown",
+        )
+        verify(privatePaykitRepo).beginPaymentRequest(expiredRequest)
+        verify(privatePaykitRepo).beginPaymentRequest(payableRequest)
+        assertEquals(payableRequest, activeContactPaymentContext()?.incomingPaymentRequest)
+        assertEquals(Sheet.Send(SendRoute.Confirm), sut.currentSheet.value)
+    }
+
     @Test
     fun `cancelled request resolution releases the presentation guard`() = test {
         val request = paymentRequest()

[jvsena42]

Re-reviewed at 73df07f. No HIGH/MEDIUM — not blocking. Two LOW observations inline; both are dev-gated, take or leave them.

I spent most of this pass confirming your pushed fixes are actually correct rather than merely present, and they are:

• Generation bump is now inside if (requestedPaymentRequestId == request.id). I traced the case I was worried about — an automatic batch with expired A ahead of payable B: finishExpired(A) no longer bumps, clearPaymentRequestPresentationRetry(A) returns false, the loop reaches B, isCurrentPaymentRequestPresentation(B) passes, openContactPayment(B) runs. B is no longer swallowed.
• Expiry during backoff and in-flight both resolve to exactly one toast. Backoff: the retry job is cancelled, then one toast + restore. In-flight: the generation bump makes beginPaymentRequest return early so openContactPayment is never called and no second toast fires. The reverse race (RequestExpired thrown before the repo prunes) clears the requested id, so the later emission finds requestedRequest == null. No duplicate either way.
• hideSheet after restore: clearIncomingPaymentRequestTarget snapshots currentSheet is Send before clearing, and retry attempts only run with currentSheet == null, so the 15th-failure showSheet(PaymentRequests) is never followed by a hideSheet() that would undo it.
• Final-layer logging: logPresentationFailure now emits only the error class name plus the redacted pubkey, no Throwable.message, and scanLogId returns a fixed string whenever the context carries a request — so safeLogInput ?: input is only reachable when isPaymentRequest == false. That closes what I raised.

Fund safety traced clean. Amount and counterparty are pinned at open time in ContactPaymentContext; at pay time onConfirmPay single-flights on isSubmittingPaymentRequest, validateIncomingPaymentRequest re-checks acceptsPaymentAmount by equality plus the bolt11 msat match plus isPending, and accept() → updateRequest adds to processingRequestIds under operationMutex and removes from _pendingRequests before the send. A request can't be paid twice, after expiry, or at an amount other than the one shown — structurally equivalent to what iOS #721 does with markPresentedIfPending + processingRequestIds. A 15th-failure request stays in the sheet but any re-tap goes through the full Send confirm and pay-time validation again.

Also clean: both new toasts are fixed string resources, so no counterparty or error text reaches the UI; nothing seed-derived is touched; ParseFailure and the failure-reason enum aren't persisted, so there's no migration concern; the new runCatching uses are all non-suspend (Instant.parse, Bolt11Invoice.fromStr) with the suspend paths on runSuspendCatching; and synchronizePaykitContacts clears requested state before repo.clear(), so the new expiry path can't toast for a previous identity's request.

One pre-existing thing I'm noting rather than filing: handleScan runs on bgDispatcher, so clearIncomingPaymentRequestTarget → deferPaymentRequestPresentation mutates the plain mutableMapOf retry maps and the generation counter off-main while the collectors run on main. Base already did this; this PR adds two more fields to the same unsynchronised set without changing the shape. Worth a separate look someday, not here.

[jvsena42]

Reviewed the delta at 759ca617a. No HIGH/MEDIUM. One LOW as a follow-up on ben's docs/payment-requests.md:11 thread rather than a new one, since it's the same ask half-applied.

Both of my earlier findings are addressed, and I checked the guard still restores when it should — that was the risk with the fix I asked for, since a guard that's too broad silently stops doing its job. It doesn't: immediate RequestExpired, expiry during backoff, and expiry during in-flight resolution all still restore the sheet, each asserted. And the new explicit request expiring during backoff keeps an unrelated send sheet open pins the case I raised — Sheet.Send(Confirm) survives with the expired toast last of exactly two enqueues.

The unhandled-target WARN branch selection is sound too: clearIncomingPaymentRequestTarget() captures activeIncomingPaymentRequest() != null before clearing the context and returns it, so the boolean at :2658 reflects the state before teardown rather than after. The remaining leak is the INFO line, on the thread.

One candidate I chased and dropped: I thought the restore guard might miss the window where showSheet has started a transition but _currentSheet is still null. It doesn't — getBolt11() and getOnchainAddress() are plain _walletState.value reads with no suspension, and viewModelScope is Main.immediate, so a user tap sets _currentSheet synchronously before showSheet returns and the guard correctly rejects. The only residue is the 300 ms SCREEN_TRANSITION_DELAY that runs when replacing an already-open sheet, which needs three conditions to line up and ends in a replaced sheet with no fund impact. Not worth changing.

Nothing else in the scoped delta — no new runCatching, no new flows, no off-main map mutation, no key material logged. The three strings.xml hunks in the range are already on master, so they're rebase noise rather than part of this PR.

[jvsena42]

Reviewed the delta since my last pass (08386077a plus two clean master merges). The redaction commit closes my docs/payment-requests.md thread as asked — isPaymentRequest is captured at :2529 before handleDecodedScan can tear the context down, so the branch reflects pre-teardown state, and the normal-scan path keeps its existing log line. Nothing over-corrected.

One LOW observation inline. Everything here is behind PaykitFeatureFlags.isUiEnabled (!BuildConfig.FEATURE_PAYKIT_UI_DISABLED && localFlag), and isAvailable() short-circuits refresh() to clearStateLocked() when it's off, so no incoming request is parsed, presented or logged in a release build. Dev/QA-facing today. Not blocking.

Checked and clean:

• Rejection durability. The SDK write in updateRequest lands before _pendingRequests is updated, under operationMutex, so durable state precedes the UI. On the next refresh a REJECTED record maps to NonActionableState and never re-enters pending. The PaykitPaymentRequestId(paymentRequestId, counterparty, counterpartyReceiverPath) triple means a counterparty can't re-mint the same record under a new id without it being a genuinely new proposal that still needs Pay + Confirm. No auto-pay path exists in the presentation machinery.
• A 15th-failure request is markPresented, which removes it only from automatic presentation; it stays payable by design, and presentedRequestIds is persisted per identity so a restart doesn't re-auto-present.
• Amount TOCTOU. The object handed to openContactPayment is the one the loop started with and is pinned in ContactPaymentContext. A refresh that swapped in a same-id/different-amount record can't change what's paid.
• Diagnostics logging. All three PaykitPaymentRequestDiagnostics methods emit only enum logValues, error::class.simpleName, and a redacted counterparty — no Throwable.message, no note/amount/endpoint/request id. PaykitPaymentRequestDiagnosticsTest pins the raw pubky, an arbitrary counterparty and the Throwable message all absent.
• Trust boundary. The only counterparty-supplied text rendered is request.note, single-line and ellipsised, in a separate column from MoneyCell — a note can't fake the amount cell. Both new toasts are fixed string resources.
• No leak back to the counterparty. Failure reasons are consumed only by Logger; reject()/accept() carry no reason.
• Journey + docs match the code. 15 attempts x 2s = 28s inside the journey's 35s wait; the silent reasons (outgoing_request, non_actionable_state, expired) match shouldLogIncomingRejection = false; accessibility ids and toast copy match strings.xml. The journey correctly starts on the full-screen route, where shouldRestorePaymentRequestSheet is false.

Cross-repo note (synonymdev/bitkit-ios#721): iOS dedupes its parse-rejection warn per (record, reason, counterparty) via IncomingPaykitPaymentRequestRejectionLog, and gates presentation-failure warns to terminal-only for requested presentations plus first-per-reason for automatic ones. Android's logParseRejection and logPresentationRejection both fire on every sync/retry. That's the per-attempt log noise ben-kaufman had removed on the iOS side — worth matching, but it's noise, not a defect, so I'm not filing it.

Also worth recording: iOS's PaykitResolutionFailureDiagnostics classifies resolution errors into stable storage/<code>-style reasons behind a 64-byte [a-z0-9_-] allowlist, where Android logs error::class.simpleName only. Android's is strictly less informative but equally safe — no attacker-influenced bytes can reach the log at all — so no change needed.