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, target-log redaction, localized terminal feedback, expiration during backoff or resolution, automatic-batch continuation, and request-sheet restoration.
• 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 full local validation: just compile, just test, and just lint passed before the latest feedback batch.
• Current-head local verification: AppViewModelSendFlowTest passes 199/199 and PaykitPaymentRequestRepoTest passes 30/30 after rebasing onto master; Kotlin compilation completed as part of the focused run.

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 full connected suite remains blocked by the unrelated existing OnchainServiceTests.testDeriveRegtestDescriptorsForSupportedAccountTypes failure; the focused payment-request UI class passes on API 37 and API 36.

[greptile-apps]

Greptile Summary

The PR adds categorized, redacted diagnostics for rejected incoming Paykit requests and bounded retries with localized terminal feedback while preserving the request for manual action.

• Refactors incoming-request parsing into explicit success and rejection results.
• Adds reason-specific diagnostics for parsing, resolution, and presentation failures.
• Retries explicitly opened requests up to 15 times and restores the originating request sheet when appropriate.
• Adds stable accessibility tags, regression tests, documentation, and a rejected-request journey.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
app/src/main/java/to/bitkit/repositories/PaykitPaymentRequestRepo.kt	Replaces nullable request parsing with categorized results and redacted diagnostics while preserving active-request and history eligibility.
app/src/main/java/to/bitkit/repositories/PublicPaykitRepo.kt	Maps payment-opening outcomes to stable resolution and presentation failure reasons.
app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt	Adds bounded manual presentation retries, reason-specific diagnostics, terminal feedback, and conditional request-sheet restoration.
app/src/main/java/to/bitkit/ui/screens/paymentrequests/PaymentRequestsScreen.kt	Adds stable test and accessibility tags to request rows and incoming-request actions.
app/src/test/java/to/bitkit/viewmodels/AppViewModelSendFlowTest.kt	Covers the 15-attempt terminal path, diagnostics, localized feedback, sheet restoration, and full-screen behavior.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Incoming Paykit request] --> B[Parse request terms]
    B -->|Rejected| C[Log redacted parse reason]
    B -->|Accepted| D[Show actionable request]
    D --> E[User selects Pay]
    E --> F[Resolve payment details]
    F -->|Opened| G[Normal send flow]
    F -->|Unavailable| H[Log redacted failure]
    H --> I{Attempts remaining?}
    I -->|Yes| J[Retry after delay]
    J --> F
    I -->|No| K[Show localized terminal feedback]
    K --> L[Restore request surface when applicable]
    L --> M[Request remains available for retry or dismissal]

Loading

_{Reviews (3): Last reviewed commit: "test: cover valid pubky redaction" | Re-trigger Greptile}

[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]

Diffed this against the iOS counterpart (#721) semantically. The port is faithful in the parts that carry weight: same 12-reason parse taxonomy and wire strings, same two suppressed reasons, same ULong.MAX/1000 cap, exhaustive when on the presentation taxonomy with no permissive default, same redaction (counterparty is the only peer-supplied value that reaches a log, and it is bounded). No rejection reason is sent back to the sender, so there is no privacy leak to the requesting party, and no attacker-supplied error text reaches the UI unescaped.

The one behavioural gap I found was already filed by @ben-kaufman on the AppViewModel.kt:882 thread, so I have not duplicated it. One low convention item below.

[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]

Non-blocking. This finishExpiredPaymentRequestPresentation call can restore the PaymentRequests sheet on top of an unrelated sheet the user is mid-flow in.

The other two callers (:870, :896) run downstream of isPaymentRequestPresentationBlocked(), so currentSheet == null is guaranteed there. This one runs inside the pendingRequests.drop(1).collect collector with no sheet check. Sequence: user taps Pay on R, it doesn't resolve, goes to backoff; user then pastes an unrelated invoice and is in Sheet.Send(Confirm); the retry fires and is dropped as blocked, but requestedPaymentRequestId/shouldRestorePaymentRequestSheet stay set; R's expiresAt passes, the repo prunes, and this path reaches showSheet(Sheet.PaymentRequests) — which nulls _currentSheet and tears down the user's Send flow. No fund impact (a swipe already in proceedWithPayment continues in viewModelScope), they just lose the result screen.

One line: if (restorePaymentRequestSheet && currentSheet.value == null) showSheet(Sheet.PaymentRequests). The toast still fires, and if the open sheet was R's own Send sheet the :808-815 collector already hides it. The tests at AppViewModelSendFlowTest.kt:866/906 only cover the no-other-sheet case.

[jvsena42]

Non-blocking, and it's the adjacent branch to the one already fixed. The decode-failure log at :2612 is now redacted, but this unhandled-scan branch still interpolates the whole Scanner value.

A counterparty can publish, under a supported MethodId, a string that decodes successfully to a variant this when doesn't handle — a lightning address landing as Scanner.LnurlAddress(address=…), say. beginContactPayment → openContactPayment → handleScan → coreService.decode succeeds, falls through to else, and the peer-supplied payload gets written to the log on each of the 15 retries. Same class as the thread you closed on docs/payment-requests.md:11, so the doc's claim is slightly ahead of the code here.

if (hasIncomingPaymentRequest) "Received unhandled incoming Paykit payment request target" else … covers it. Separately and pre-existing: :2618's Logger.info("Handling decoded scan data: $it") logs the full decoded target on the success path too, which is outside that doc's rejection claim — noting only.