fix(zaps): audit follow-ups on the BOLT12 fallback and profile chips

- Only retry a refused BOLT12 offer over BOLT11 when the wallet rejected it before attempting a payment (EXPIRED, NOT_FOUND, BAD_REQUEST, NOT_IMPLEMENTED, UNSUPPORTED_PAYMENT_INSTRUCTION, UNSUPPORTED_NETWORK). NIP-47 defines PAYMENT_FAILED as possibly "due to a timeout", so an HTLC can still settle after that reply; retrying on it, or on INTERNAL / OTHER / a missing code, could pay the recipient twice. The decision is now an allowlist and the test locks the non-retry set. - NwcInfoCache exposes an `updates` counter bumped on every stored entry. The zap picker keys its rail recompute on it, so a BOLT12-only recipient's bolt appears when the wallet's kind:13194 info lands after the popup opened, instead of only after closing and reopening it. - A BOLT12 refusal with neither message nor code no longer toasts the raw "%1$s" placeholder; the code name (or OTHER) fills the detail. - One abbreviateBolt12Offer() replaces three copies of the lno1 truncation in the profile chip, the offers dialog and the settings screen. - Reword the "these four" recompute-key comment so it covers the BOLT12 reads added alongside it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LCxV2YvpegwUEKUhN13Mvq
2026-10-06 11:48:24 +00:00 · 2026-09-12 20:47:05 +00:00
parent 3887b033e0
commit d4ac5149f7
9 changed files with 86 additions and 43 deletions
@@ -188,9 +188,10 @@ class AccountZapActions(
     * external-wallet or LNURL fallback because only NWC returns the proof.
     *
     * Outcomes are split by what they say about the money:
-     *  - [onNotPaid]: the wallet answered with an error, so nothing was paid. The
-     *    wallet does the offer → invoice exchange itself, so a stale or dead offer
-     *    lands here too. The caller may safely retry over another rail.
+     *  - [onNotPaid]: the wallet answered with an error. The wallet does the offer →
+     *    invoice exchange itself, so a stale or dead offer lands here too. Whether a
+     *    retry is safe depends on the code — `PAYMENT_FAILED` may be a timeout with the
+     *    HTLC still in flight — see `Bolt12LightningFallback`.
     *  - [onError]: paid but no valid receipt, or nothing conclusive. Never retry.
     *  - [onTimeout]: the wallet never answered. Unknown state — never retry.
     */
@@ -25,27 +25,30 @@ import com.vitorpamplona.quartz.nip47WalletConnect.rpc.NwcErrorCode
 /**
 * Decides whether a BOLT12 zap the wallet refused should be re-sent as a BOLT11 zap.
 *
- * Only ever consulted for a NIP-47 *error* reply, which by the spec means the wallet
- * did not pay — so the retry can never double-spend. The question left is whether a
- * second attempt over a different instruction has a chance: a refusal about the
- * offer (the wallet could not resolve it, the recipient's node is unreachable, the
- * offer expired, or our wallet does not handle `lno` at all) is worth retrying over
- * the recipient's lightning address; a refusal about *our* wallet — no balance, a
- * quota or rate limit, a permission the connection lacks — would fail the same way
- * on BOLT11 and would only produce a second error.
+ * Only ever consulted for a NIP-47 *error* reply. An allowlist, because not every
+ * error means no money moved: NIP-47 defines `PAYMENT_FAILED` as "may be due to a
+ * timeout, exhausting all routes, insufficient capacity or similar", and a wallet
+ * that gave up on a payment whose HTLC is still in flight can see it settle later.
+ * Retrying on that, or on the catch-all `INTERNAL` / `OTHER` / no-code replies,
+ * could pay the recipient twice. Only refusals the wallet raises *before* it
+ * attempts a payment qualify — the offer could not be resolved or has expired, the
+ * request was rejected as malformed, or our wallet does not handle `lno` at all.
+ * Those are the "recipient's configuration is stale" cases the fallback exists for.
+ * Refusals about our own wallet (balance, quota, permissions) are out too: BOLT11
+ * through the same wallet would fail identically and only add a second error.
 */
 object Bolt12LightningFallback {
-    /** Refusals that describe the sender's wallet, not the offer. */
-    private val senderSideCodes =
+    /** Refusals raised before any payment attempt, about the offer or the instruction. */
+    private val offerSideCodes =
        setOf(
-            NwcErrorCode.INSUFFICIENT_BALANCE,
-            NwcErrorCode.QUOTA_EXCEEDED,
-            NwcErrorCode.RATE_LIMITED,
-            NwcErrorCode.RESTRICTED,
-            NwcErrorCode.UNAUTHORIZED,
-            NwcErrorCode.UNSUPPORTED_ENCRYPTION,
+            NwcErrorCode.EXPIRED,
+            NwcErrorCode.NOT_FOUND,
+            NwcErrorCode.BAD_REQUEST,
+            NwcErrorCode.NOT_IMPLEMENTED,
+            NwcErrorCode.UNSUPPORTED_PAYMENT_INSTRUCTION,
+            NwcErrorCode.UNSUPPORTED_NETWORK,
        )

    /** True when a refusal with [code] (null when the wallet sent none) should be retried over BOLT11. */
-    fun shouldRetry(code: NwcErrorCode?): Boolean = code !in senderSideCodes
+    fun shouldRetry(code: NwcErrorCode?): Boolean = code in offerSideCodes
 }
@@ -34,6 +34,7 @@ import com.vitorpamplona.amethyst.ui.nwc.nwcTimeoutMessage
 import com.vitorpamplona.amethyst.ui.stringRes
 import com.vitorpamplona.quartz.experimental.clink.pointers.NDebit
 import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl
+import com.vitorpamplona.quartz.nip47WalletConnect.rpc.NwcErrorCode
 import com.vitorpamplona.quartz.nip47WalletConnect.rpc.NwcTransactionMetadata
 import com.vitorpamplona.quartz.nip53LiveActivities.streaming.LiveActivitiesEvent
 import com.vitorpamplona.quartz.nip57Zaps.LnZapEvent
@@ -505,13 +506,14 @@ class ZapPaymentHandler(
     * [Account.sendBolt12Zap]. Fire-and-forget like [payViaNWC]: dispatch is optimistic
     * and settlement/errors surface later through the async NWC response.
     *
-     * When the wallet answers that it did **not** pay — it resolves the offer itself,
-     * so a stale or unreachable offer fails there — and the recipient also publishes a
-     * lightning address, the same share is re-sent as a regular BOLT11 zap through
-     * [zapOverLightning], silently: the BOLT12 error is only shown when there is no
-     * BOLT11 route, or when the refusal is about our wallet rather than the offer
-     * ([Bolt12LightningFallback]). A paid-but-no-receipt outcome and a wallet that
-     * never answers are never retried, since funds may already have moved.
+     * When the wallet refuses the offer before attempting a payment — it resolves the
+     * offer itself, so a stale, expired or unsupported offer fails there — and the
+     * recipient also publishes a lightning address, the same share is re-sent as a
+     * regular BOLT11 zap through [zapOverLightning], silently. The BOLT12 error is
+     * shown when there is no BOLT11 route or when the refusal does not qualify
+     * ([Bolt12LightningFallback]: a failed payment attempt may still settle, and a
+     * refusal about our own wallet would repeat on BOLT11). A paid-but-no-receipt
+     * outcome and a wallet that never answers are never retried either.
     */
    suspend fun payViaBolt12(
        recipients: List<Bolt12Recipient>,
@@ -576,7 +578,9 @@ class ZapPaymentHandler(
                            onError(stringRes(context, R.string.error_dialog_zap_error), e.message ?: e.toString(), recipient.user)
                        }
                    } else {
-                        reportBolt12Error(R.string.bolt12_payment_failed, detail)
+                        // bolt12_payment_failed always formats a detail; a wallet may send neither
+                        // message nor a recognised code.
+                        reportBolt12Error(R.string.bolt12_payment_failed, detail ?: (code ?: NwcErrorCode.OTHER).name)
                    }
                },
                onTimeout = {
@@ -65,6 +65,7 @@ import com.vitorpamplona.amethyst.ui.insets.imePaddingSafe
 import com.vitorpamplona.amethyst.ui.navigation.navs.INav
 import com.vitorpamplona.amethyst.ui.navigation.topbars.SavingTopBar
 import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel
+import com.vitorpamplona.amethyst.ui.screen.loggedIn.profile.header.abbreviateBolt12Offer
 import com.vitorpamplona.amethyst.ui.screen.loggedIn.relays.SettingsCategory
 import com.vitorpamplona.amethyst.ui.stringRes
 import com.vitorpamplona.amethyst.ui.theme.ButtonBorder
@@ -191,7 +192,7 @@ fun Bolt12OfferEntry(
        horizontalArrangement = Arrangement.SpaceAround,
    ) {
        Text(
-            text = "${offer.take(14)}…${offer.takeLast(6)}",
+            text = abbreviateBolt12Offer(offer),
            style = MaterialTheme.typography.bodyMedium,
            fontFamily = FontFamily.Monospace,
            maxLines = 1,
@@ -226,6 +226,7 @@ import kotlinx.collections.immutable.toImmutableList
 import kotlinx.collections.immutable.toImmutableSet
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.delay
+import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.withContext
 import kotlinx.serialization.json.Json
@@ -2130,12 +2131,12 @@ fun observeZapRailCapability(
 ): RailCapability {
    val cashuState = accountViewModel.account.cashuWalletState
    val author = baseNote.author
-    // These four are deliberately read only to drive the recompute below — do NOT
-    // delete them as "unused". Each observe* call ALSO subscribes the relay fetch
-    // (so a not-yet-seen kind:0 / kind:10019 gets pulled in while the popup is
-    // open), and each value is a remember() key so railCapability recomputes when
-    // it arrives. RailCapabilityResolver.peek re-reads everything itself; these
-    // just say *when* to re-run it.
+    // Every value below up to `showOnchainWallet` is deliberately read only to drive
+    // the recompute — do NOT delete them as "unused". Each observe* call ALSO
+    // subscribes the relay fetch (so a not-yet-seen kind:0 / kind:10019 / kind:10058
+    // gets pulled in while the popup is open), and each value is a remember() key so
+    // railCapability recomputes when it arrives. RailCapabilityResolver.peek re-reads
+    // everything itself; these just say *when* to re-run it.
    val cashuMints by cashuState.mints.collectAsStateWithLifecycle()
    val cashuEntries by cashuState.tokenEntries.collectAsStateWithLifecycle()
    val recipientInfo = author?.let { observeUserInfo(it, accountViewModel).value }
@@ -2144,7 +2145,11 @@ fun observeZapRailCapability(
    // (rides in UserMetadataForKeyKinds beside kind:0) and our default NWC wallet,
    // whose `pay` support decides whether that offer makes them Lightning-payable.
    val bolt12OfferList = author?.let { observeNoteEvent<Bolt12OfferListEvent>(it.bolt12OfferListNote, accountViewModel).value }
-    val defaultWalletUri by accountViewModel.account.nip47SignerState.defaultWalletUri
+    val nip47State = accountViewModel.account.nip47SignerState
+    val defaultWalletUri by nip47State.defaultWalletUri.collectAsStateWithLifecycle()
+    // The wallet's kind:13194 info (its `pay` support) is a plain cache read inside
+    // canZapViaBolt12(); this counter is what recomputes when it lands after opening.
+    val walletInfoUpdates by remember(nip47State) { nip47State.infoCache?.updates ?: MutableStateFlow(0) }
        .collectAsStateWithLifecycle()
    // Honors the user's "show on-chain wallet" preference: off hides the on-chain
    // rail from the zap chips too, matching the wallet screen, profile chips, and
@@ -2194,6 +2199,7 @@ fun observeZapRailCapability(
        nutzapInfo,
        bolt12OfferList,
        defaultWalletUri,
+        walletInfoUpdates,
        showPayToChip,
        recipientPayTo,
        payToApps,
@@ -148,7 +148,7 @@ private fun Bolt12OfferRow(
                .padding(horizontal = 16.dp, vertical = 10.dp),
    ) {
        Text(
-            text = "${offer.take(14)}…${offer.takeLast(6)}",
+            text = abbreviateBolt12Offer(offer),
            style = MaterialTheme.typography.bodyMedium,
            fontFamily = FontFamily.Monospace,
            color = MaterialTheme.colorScheme.onSurface,
@@ -233,3 +233,10 @@ private fun Bolt12NwcAmountDialog(
        }
    }
 }
+
+/**
+ * The short form every BOLT12 surface shows for an `lno1…` offer: enough of the
+ * head to recognise the prefix, the tail to tell two offers apart. Shared by the
+ * profile chip, this dialog and the offers settings screen so they agree.
+ */
+fun abbreviateBolt12Offer(offer: String): String = "${offer.take(14)}\u2026${offer.takeLast(6)}"
@@ -234,7 +234,7 @@ private fun RailAndTargetChips(
            ProfilePaymentChip(
                color = BitcoinOrange,
                label = stringRes(Res.string.bolt12_lightning_offer),
-                detail = remember(offer) { "${offer.take(14)}\u2026${offer.takeLast(6)}" },
+                detail = remember(offer) { abbreviateBolt12Offer(offer) },
                copyValue = offer,
                onClick = { bolt12DialogOffer = offer },
            ) {
@@ -29,23 +29,29 @@ class Bolt12LightningFallbackTest {
    @Test
    fun offerSideRefusalsRetryOverLightning() {
        listOf(
-            NwcErrorCode.PAYMENT_FAILED,
            NwcErrorCode.EXPIRED,
            NwcErrorCode.NOT_FOUND,
            NwcErrorCode.BAD_REQUEST,
            NwcErrorCode.NOT_IMPLEMENTED,
            NwcErrorCode.UNSUPPORTED_PAYMENT_INSTRUCTION,
            NwcErrorCode.UNSUPPORTED_NETWORK,
-            NwcErrorCode.INTERNAL,
-            NwcErrorCode.OTHER,
        ).forEach { code ->
            assertTrue("$code should fall back to BOLT11", Bolt12LightningFallback.shouldRetry(code))
        }
    }

    @Test
-    fun aReplyWithoutACodeStillRetries() {
-        assertTrue(Bolt12LightningFallback.shouldRetry(null))
+    fun aFailedOrAmbiguousPaymentNeverRetries() {
+        // PAYMENT_FAILED "may be due to a timeout" (NIP-47): the HTLC can still settle.
+        // The catch-alls and a reply with no code say nothing about whether funds moved.
+        listOf(
+            NwcErrorCode.PAYMENT_FAILED,
+            NwcErrorCode.INTERNAL,
+            NwcErrorCode.OTHER,
+            null,
+        ).forEach { code ->
+            assertFalse("$code may already have paid; a retry could double-spend", Bolt12LightningFallback.shouldRetry(code))
+        }
    }

    @Test
@@ -30,6 +30,10 @@ import kotlinx.coroutines.CompletableDeferred
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.IO
+import kotlinx.coroutines.flow.MutableStateFlow
+import kotlinx.coroutines.flow.StateFlow
+import kotlinx.coroutines.flow.asStateFlow
+import kotlinx.coroutines.flow.update
 import kotlinx.coroutines.launch

 /**
@@ -75,6 +79,16 @@ class NwcInfoCache(

    private val cache = ConcurrentMap<HexKey, Entry>()

+    private val updatesState = MutableStateFlow(0)
+
+    /**
+     * Bumps every time a fetch stores an entry. [current] is a plain map read, so a
+     * composable that derives state from it (the zap picker's "can our wallet pay a
+     * BOLT12 offer" check) has nothing to recompose on when the info arrives after
+     * it opened; keying on this flow closes that gap.
+     */
+    val updates: StateFlow<Int> = updatesState.asStateFlow()
+
    // Fetches in progress, keyed like [cache]. Every fetching path goes through [fetchOnce].
    private val inFlight = ConcurrentMap<HexKey, CompletableDeferred<NwcInfoEvent?>>()

@@ -182,6 +196,7 @@ class NwcInfoCache(
            }

        cache[uri.pubKeyHex] = Entry(info, now())
+        updatesState.update { it + 1 }
        return info
    }