Skip to content

feat(send): improve Lightning send failure recovery - #1140

Open
pwltr wants to merge 5 commits into
masterfrom
feat/reset-routing
Open

feat(send): improve Lightning send failure recovery#1140
pwltr wants to merge 5 commits into
masterfrom
feat/reset-routing

Conversation

@pwltr

@pwltr pwltr commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Description

Ports the iOS Lightning send-failure retry/support behavior to Android.

  • Shows the send failure screen for failed Lightning sends instead of relying only on toasts.
  • Maps LDK payment failure reasons to localized, user-facing strings and avoids exposing internal error values like Optional(...), NodeError, or DuplicatePayment.
  • Uses the reusable generic route failure copy for routeNotFound and retriesExhausted.
  • Preserves LDK failure reasons for payments that first enter SendPendingScreen and fail later.
  • Shows the correct failure title for Lightning vs on-chain send errors.
  • Shows Contact Support above Try Again on the Lightning send failure screen.
  • Prefills the support report with failure type, payment method, routing cache reset status, and the available Lightning payment request.
  • Resets routing caches only for routing-related failures, and only once per current send flow/payment attempt.
  • Retries non-routing failures normally without resetting graph/scorer caches.
  • Retry clears local/VSS network graph data and scorer/pathfinding score data before restarting ldk-node when a routing reset is needed.
  • Retry waits for local routing data to be usable again without requiring a newer remote RGS snapshot timestamp or waiting only on delayed graph-cache persistence.
  • Guards retry state in WalletViewModel so overlapping retry flows cannot surface stale timeout errors.
  • Resets send navigation back to the retry route instead of stacking retry screens on top of the failure screen.

Closes #829

Preview

Normal payment:

Screen.Recording.2026-08-11.at.17.59.27.mov

QuickPay:

Screen.Recording.2026-08-12.at.14.55.13.mov

QA Notes

Tested on mainnet emulator:

  • Confirmed ROUTE_NOT_FOUND surfaces user-facing payment failure copy.
  • Confirmed pending RETRIES_EXHAUSTED failures surface the localized retries-exhausted copy instead of generic failure copy.
  • Confirmed Try Again deletes local graph, VSS graph, scorer, and external scores cache for the first routing-related retry.
  • Confirmed Try Again does not repeat routing cache reset after a reset has already been attempted in the current send flow.
  • Confirmed retry restarts from an empty graph and accepts the same RGS snapshot timestamp.
  • Confirmed retry waits through RGS/scorer refresh and returns to the send confirmation path without timing out when graph cache persistence lags behind the in-memory graph.
  • Confirmed no stale retry timeout after repeated retry attempts.

Automated checks:

  • compileDevDebugKotlin
  • PaymentFailureReasonExtTest.kt
  • AppViewModelSendFlowTest.kt
  • detekt

@pwltr
pwltr force-pushed the feat/reset-routing branch 3 times, most recently from 6a53759 to 873d26e Compare August 12, 2026 12:41
@pwltr
pwltr force-pushed the feat/reset-routing branch from 873d26e to 776366e Compare August 12, 2026 12:42
@pwltr
pwltr marked this pull request as ready for review August 12, 2026 12:42
@greptile-apps

greptile-apps Bot commented Aug 12, 2026

Copy link
Copy Markdown

Greptile Summary

The PR replaces toast-only Lightning send failures with a recoverable failure screen, localized failure reasons, support-report prefilling, and routing-cache resets for routing-related retries.

  • Propagates typed LDK payment failure reasons through normal, pending, and QuickPay send flows.
  • Adds retry-route and reset-attempt state to the nested send navigation graph.
  • Clears graph and scorer caches, restarts LDK, and waits for routing sources before retrying.
  • Adds localized failure and support copy plus focused send-flow tests.

Confidence Score: 4/5

The PR should not merge until pending QuickPay failures can retry without crashing from cleared payment state.

The new Pending branch clears the only QuickPay request state, while the corresponding failure route explicitly returns to a destination that requires that state to be non-null.

Files Needing Attention: app/src/main/java/to/bitkit/ui/screens/wallets/send/SendQuickPayScreen.kt, app/src/main/java/to/bitkit/ui/sheets/SendSheet.kt

Important Files Changed

Filename Overview
app/src/main/java/to/bitkit/ui/screens/wallets/send/SendQuickPayScreen.kt Moves QuickPay cleanup to terminal results, but clearing state on Pending breaks the newly added pending-failure retry route.
app/src/main/java/to/bitkit/ui/sheets/SendSheet.kt Adds typed failure navigation, support reporting, and routing-aware retry behavior; its QuickPay retry destination still requires state that the Pending path clears.
app/src/main/java/to/bitkit/repositories/LightningRepo.kt Adds graph/scorer cache reset and bounded routing-data refresh orchestration without an independently established defect.
app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt Routes active Lightning failures into the send sheet and preserves typed failure details and payment requests.
app/src/main/java/to/bitkit/viewmodels/WalletViewModel.kt Serializes routing-reset retries with a mutex and exposes retry progress to the UI.
app/src/main/java/to/bitkit/ext/PaymentFailureReasonExt.kt Maps LDK failure reasons to localized user copy and sanitized compact support metadata.

Sequence Diagram

sequenceDiagram
    participant User
    participant QuickPay as QuickPay Screen
    participant AppVM as AppViewModel
    participant Pending as Pending Screen
    participant Error as Failure Screen

    QuickPay->>AppVM: resetQuickPay()
    QuickPay->>Pending: "navigate with retryRoute=QuickPay"
    Pending-->>Error: payment failure
    User->>Error: Try Again
    Error->>QuickPay: clear stack and navigate
    QuickPay->>AppVM: read quickPayData
    AppVM-->>QuickPay: null
    QuickPay--xQuickPay: requireNotNull crashes
Loading

Reviews (1): Last reviewed commit: "feat(send): improve Lightning send failu..." | Re-trigger Greptile

Comment thread app/src/main/java/to/bitkit/ui/sheets/SendSheet.kt
Comment thread app/src/main/java/to/bitkit/ui/sheets/SendSheet.kt Outdated
Comment thread app/src/main/java/to/bitkit/domain/commands/NotifyPendingPaymentResolved.kt Outdated
Comment thread app/src/main/java/to/bitkit/repositories/LightningRepo.kt Outdated
Comment thread app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt Outdated
Comment thread app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Three defects remain because the earlier review findings are unresolved:

  • Dismissing the send sheet during Try Again fails to restart Lightning because stop is NonCancellable and the reset job is cancelled.
  • Writing QuickPay invoice state into sendUiState routes a failed QuickPay retry to Confirm.
  • Report-issue prefill overwrites edits because it is reapplied on composition restart.

@pwltr

pwltr commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Addressed all comments, waiting for CI to pass.

@pwltr
pwltr requested review from jvsena42 and ovitrif August 14, 2026 13:13
Comment on lines +90 to +98
private fun String.compactFailureType(): String {
val unwrappedOptional = removeSurrounding("Optional(", ")")
val unwrappedNodeError = unwrappedOptional.removeSurrounding("NodeError(", ")")
return unwrappedNodeError
.substringBefore("(")
.substringAfterLast(".")
.trim()
.ifBlank { UNKNOWN_FAILURE_TYPE }
}

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.

Kotlin errors here comes in a different shape, like "LDK Node error: Duplicate payment."
using .substringAfterLast(".") makes the last part always
UNKNOWN_FAILURE_TYPE.

}

private val INTERNAL_PAYMENT_ERROR_MARKERS = listOf(
"Optional(",

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.

This is also iOS exclusive error shape

}

fun Throwable.toCompactFailureType(): String {
val rawValue = message?.trim()?.takeIf { it.isNotEmpty() }

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.

Android can derive the type from exception class NodeException.DuplicatePayment -> DuplicatePayment, could use it instead of depending on the message

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

Test-coverage note on compactFailureType(): the assertions only exercise iOS-shaped strings, so the shape Android actually produces is untested — and it currently resolves to Unknown. Details inline.

val unwrappedNodeError = unwrappedOptional.removeSurrounding("NodeError(", ")")
return unwrappedNodeError
.substringBefore("(")
.substringAfterLast(".")

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.

substringAfterLast(".") assumes the iOS input shape. This function is a port of compactFailureType in bitkit-ios (Bitkit/Views/Wallets/Send/SendFailure.swift:41), where the input is String(describing: someType) and the dot split strips a Swift module/type qualifier (LDKNode.NodeError -> NodeError), the same way the Optional( and ( steps handle Swift's optional and associated-value rendering.

On Android the input is Throwable.message, which is a human-readable sentence: Errors.kt:120 builds "LDK Node error: $it" over strings like "Duplicate payment.". So the dot split lands on sentence punctuation rather than a qualifier:

  • "LDK Node error: Duplicate payment." -> substringAfterLast(".") == "" -> ifBlank { UNKNOWN_FAILURE_TYPE } -> "Unknown"
  • "LDK Node error: Invalid custom TLVs" (no trailing period) -> no . at all -> the whole "LDK Node error: ..." prefix leaks through as the failure type

Net effect: Failure type: Unknown in the prefilled support ticket for essentially every synchronous LN send failure (insufficient funds, duplicate payment, invalid invoice...). The this::class.simpleName fallback at :61 is already unqualified, so the dot-stripping is vestigial on that path too.

Minimum regression test that pins the trigger without prescribing an implementation:

assertNotEquals("Unknown", Exception("Payment sending failed.").toCompactFailureType())

See the test-file comment for the fuller block. Getting a real type name out of an LdkError means reading the inner NodeException class rather than parsing its message — note LdkError.inner is currently private (Errors.kt:31), so that would need widening.

Comment on lines +45 to +47
fun `compact failure types omit optional and node error wrappers`() {
assertEquals("routeNotFound", PaymentFailureReason.ROUTE_NOT_FOUND.toCompactFailureType())
assertEquals("DuplicatePayment", Exception("Optional(NodeError(DuplicatePayment))").toCompactFailureType())

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.

Both inputs here are iOS-shaped, so this passes for the wrong reason. "Optional(NodeError(DuplicatePayment))" is Swift String(describing:) output and happens to carry no trailing period — exactly the case substringAfterLast(".") handles correctly. The Android shape ("LDK Node error: <sentence>." from Errors.kt:120) is never asserted, and it returns "Unknown" today.

Suggested additions:

@Test
fun `compact failure types survive android ldk error messages`() {
    // Errors.kt formats NodeException as "LDK Node error: <sentence>."
    val withPeriod = LdkError(NodeException.DuplicatePayment("Duplicate payment.")).toCompactFailureType()
    val withoutPeriod = LdkError(NodeException.InvalidCustomTlvs("Invalid custom TLVs")).toCompactFailureType()

    assertNotEquals("Unknown", withPeriod)
    assertFalse(withoutPeriod.contains("LDK Node error"))
}

@Test
fun `compact failure types fall back to the exception class name`() {
    assertEquals("IllegalStateException", IllegalStateException().toCompactFailureType())
}

@Test
fun `compact failure types ignore sentence punctuation`() {
    assertNotEquals("Unknown", Exception("Payment sending failed.").toCompactFailureType())
}

The first and third fail on the current implementation; the second pins the this::class.simpleName path at PaymentFailureReasonExt.kt:61, which is what the ticket falls back to for app-level errors (note it only kicks in when message is null — PaymentRoutingRefreshTimeoutError etc. carry a message and so go down the parsing path).

They are deliberately written as assertNotEquals/assertFalse rather than exact strings so they do not lock in a specific naming scheme. If the fix ends up unwrapping LdkError.inner, these can tighten to assertEquals("DuplicatePayment", ...) / assertEquals("InvalidCustomTlvs", ...).

Needs kotlin.test.assertNotEquals plus to.bitkit.utils.LdkError and org.lightningdevkit.ldknode.NodeException imports. UNKNOWN_FAILURE_TYPE is file-private in the main source, hence the "Unknown" literal.

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.

Improve Send Error Sheet UX

3 participants