From 6d34d3d9f9467bd02c54b0b669e3f897f7111d1e Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 16:33:56 +0000 Subject: [PATCH 1/7] feat(profile): show BOLT12 offers as payment pills, drop the header wallet buttons BOLT12 offers saved in Settings were only reachable through a small bolt button in the profile header action row, next to a second NIP-A3 wallet button, while every other way to pay a profile (Lightning, CLINK, on-chain, Cashu, NIP-A3 targets) rendered as a chip in the payment rail below the bio. Render one chip per NIP-B1 offer in that rail: tap opens the existing copy / pay-with-wallet / pay-via-intent dialog for that offer, long-press copies the raw lno1 string. Remove both header buttons, since the rail already lists every NIP-A3 target with the same tap-to-pay and long-press copy behaviour the dialog rows have. The two dialogs stay (the reaction row still opens the NIP-A3 one), so their files are renamed after what is left. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01LCxV2YvpegwUEKUhN13Mvq --- ...lt12PayButton.kt => Bolt12OffersDialog.kt} | 62 ------------ ...ymentButton.kt => PaymentTargetsDialog.kt} | 79 --------------- .../loggedIn/profile/header/ProfileActions.kt | 4 - .../profile/header/ProfilePaymentRailChips.kt | 98 +++++++++++++++---- .../composeResources/values/strings.xml | 1 + 5 files changed, 78 insertions(+), 166 deletions(-) rename amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/{Bolt12PayButton.kt => Bolt12OffersDialog.kt} (82%) rename amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/{PaymentButton.kt => PaymentTargetsDialog.kt} (76%) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/Bolt12PayButton.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/Bolt12OffersDialog.kt similarity index 82% rename from amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/Bolt12PayButton.kt rename to amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/Bolt12OffersDialog.kt index f0a50ef5a5..e6ff960a77 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/Bolt12PayButton.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/Bolt12OffersDialog.kt @@ -31,7 +31,6 @@ import androidx.compose.foundation.layout.width import androidx.compose.foundation.shape.RoundedCornerShape import androidx.compose.foundation.text.KeyboardOptions import androidx.compose.material3.Button -import androidx.compose.material3.FilledTonalButton import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme import androidx.compose.material3.OutlinedTextField @@ -55,80 +54,19 @@ import androidx.compose.ui.window.Dialog import com.vitorpamplona.amethyst.R import com.vitorpamplona.amethyst.commons.icons.symbols.Icon import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbols -import com.vitorpamplona.amethyst.commons.model.User import com.vitorpamplona.amethyst.commons.resources.Res import com.vitorpamplona.amethyst.commons.resources.bolt12_pay_with_wallet import com.vitorpamplona.amethyst.commons.resources.bolt12_payment_amount_sats -import com.vitorpamplona.amethyst.service.relayClient.reqCommand.event.EventFinderFilterAssemblerSubscription -import com.vitorpamplona.amethyst.service.relayClient.reqCommand.event.observeNoteEvent import com.vitorpamplona.amethyst.ui.components.M3ActionDialog import com.vitorpamplona.amethyst.ui.components.M3ActionSection import com.vitorpamplona.amethyst.ui.components.util.setText -import com.vitorpamplona.amethyst.ui.note.LoadAddressableNote import com.vitorpamplona.amethyst.ui.note.payViaBolt12Intent import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel import com.vitorpamplona.amethyst.ui.stringRes import com.vitorpamplona.amethyst.ui.theme.ButtonBorder import com.vitorpamplona.amethyst.ui.theme.Size20Modifier -import com.vitorpamplona.amethyst.ui.theme.ZeroPadding -import com.vitorpamplona.quartz.nipB1Bolt12Zaps.offer.Bolt12OfferListEvent import kotlinx.coroutines.launch -@Composable -fun Bolt12PayButton( - user: User, - accountViewModel: AccountViewModel, -) { - val address = - remember(user.pubkeyHex) { - Bolt12OfferListEvent.createAddress(user.pubkeyHex) - } - - LoadAddressableNote(address, accountViewModel) { note -> - if (note != null) { - EventFinderFilterAssemblerSubscription(note, accountViewModel) - val event by observeNoteEvent(note, accountViewModel) - val offers = - remember(event) { - event?.offers() ?: emptyList() - } - if (offers.isNotEmpty()) { - Bolt12PayButtonWithOffers(offers, accountViewModel) - } - } - } -} - -@Composable -fun Bolt12PayButtonWithOffers( - offers: List, - accountViewModel: AccountViewModel, -) { - var expanded by remember { mutableStateOf(false) } - - FilledTonalButton( - modifier = - Modifier - .padding(horizontal = 3.dp) - .width(50.dp), - onClick = { expanded = true }, - contentPadding = ZeroPadding, - ) { - Icon( - symbol = MaterialSymbols.Bolt, - contentDescription = stringRes(R.string.bolt12_offers), - ) - } - - if (expanded) { - Bolt12OffersDialog( - offers = offers, - accountViewModel = accountViewModel, - onDismiss = { expanded = false }, - ) - } -} - @Composable fun Bolt12OffersDialog( offers: List, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/PaymentButton.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/PaymentTargetsDialog.kt similarity index 76% rename from amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/PaymentButton.kt rename to amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/PaymentTargetsDialog.kt index c35e73e5c4..86bea970f0 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/PaymentButton.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/PaymentTargetsDialog.kt @@ -32,7 +32,6 @@ import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.size import androidx.compose.foundation.layout.width import androidx.compose.foundation.shape.RoundedCornerShape -import androidx.compose.material3.FilledTonalButton import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Surface @@ -54,98 +53,20 @@ import androidx.compose.ui.window.Dialog import com.vitorpamplona.amethyst.R import com.vitorpamplona.amethyst.commons.icons.symbols.Icon import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbols -import com.vitorpamplona.amethyst.commons.model.User import com.vitorpamplona.amethyst.commons.resources.Res import com.vitorpamplona.amethyst.commons.resources.no_payment_targets_message import com.vitorpamplona.amethyst.commons.resources.show_qr -import com.vitorpamplona.amethyst.service.relayClient.reqCommand.event.EventFinderFilterAssemblerSubscription -import com.vitorpamplona.amethyst.service.relayClient.reqCommand.event.observeNoteEvent import com.vitorpamplona.amethyst.ui.components.M3ActionDialog import com.vitorpamplona.amethyst.ui.components.M3ActionRow import com.vitorpamplona.amethyst.ui.components.M3ActionSection import com.vitorpamplona.amethyst.ui.components.util.setText -import com.vitorpamplona.amethyst.ui.navigation.navs.INav import com.vitorpamplona.amethyst.ui.note.ErrorMessageDialog -import com.vitorpamplona.amethyst.ui.note.LoadAddressableNote -import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel import com.vitorpamplona.amethyst.ui.screen.loggedIn.qrcode.QrCodeDrawer import com.vitorpamplona.amethyst.ui.stringRes import com.vitorpamplona.amethyst.ui.theme.Size20Modifier -import com.vitorpamplona.amethyst.ui.theme.ZeroPadding import com.vitorpamplona.quartz.experimental.nipA3.PaymentTarget -import com.vitorpamplona.quartz.experimental.nipA3.PaymentTargetsEvent import kotlinx.coroutines.launch -@Composable -fun PaymentButton( - user: User, - accountViewModel: AccountViewModel, - nav: INav, -) { - val address = - remember(user.pubkeyHex) { - PaymentTargetsEvent.createAddress(user.pubkeyHex) - } - - LoadAddressableNote(address, accountViewModel) { note -> - if (note != null) { - EventFinderFilterAssemblerSubscription(note, accountViewModel) - val event by observeNoteEvent(note, accountViewModel) - val targets = - remember(event) { - event?.paymentTargets() ?: emptyList() - } - if (targets.isNotEmpty()) { - PaymentButtonWithTargets(user, targets, nav) - } - } - } -} - -@Composable -fun PaymentButtonWithTargets( - user: User, - targets: List, - nav: INav, -) { - var expanded by remember { mutableStateOf(false) } - - FilledTonalButton( - modifier = - Modifier - .padding(horizontal = 3.dp) - .width(50.dp), - onClick = { expanded = true }, - contentPadding = ZeroPadding, - ) { - Icon( - symbol = MaterialSymbols.AccountBalanceWallet, - contentDescription = stringRes(R.string.payment_targets), - ) - } - - if (expanded) { - PaymentTargetsDialog( - targets = targets, - onDismiss = { expanded = false }, - payInApp = { target -> - // Targets one of the user's wallets can pay (lightning, bitcoin) - // go to the Send Payment screen, which collects the amount and - // confirms in place — no extra dialog. Returns false when no - // in-app wallet applies so the dialog falls back to payto://. - val route = inAppPaymentRouteFor(user.pubkeyHex, target) - if (route != null) { - expanded = false - nav.nav(route) - true - } else { - false - } - }, - ) - } -} - @Composable fun PaymentTargetsDialog( targets: List, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/ProfileActions.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/ProfileActions.kt index 3501530049..5a1a29969b 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/ProfileActions.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/ProfileActions.kt @@ -40,10 +40,6 @@ fun ProfileActions( ) { MessageButton(baseUser, accountViewModel, nav) - PaymentButton(baseUser, accountViewModel, nav) - - Bolt12PayButton(baseUser, accountViewModel) - val isMe by remember(accountViewModel) { derivedStateOf { accountViewModel.userProfile() == baseUser } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/ProfilePaymentRailChips.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/ProfilePaymentRailChips.kt index eeda4bccf3..61c6fbda35 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/ProfilePaymentRailChips.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/ProfilePaymentRailChips.kt @@ -36,8 +36,10 @@ import androidx.compose.material3.Surface import androidx.compose.material3.Text import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.rememberCoroutineScope +import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.graphics.Color @@ -56,6 +58,7 @@ import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbols import com.vitorpamplona.amethyst.commons.model.User import com.vitorpamplona.amethyst.commons.model.nip01Core.UserInfo import com.vitorpamplona.amethyst.commons.resources.Res +import com.vitorpamplona.amethyst.commons.resources.bolt12_lightning_offer import com.vitorpamplona.amethyst.commons.resources.clink_lightning_offer import com.vitorpamplona.amethyst.commons.resources.send_payment_method_cashu import com.vitorpamplona.amethyst.commons.resources.send_payment_method_lightning @@ -76,6 +79,7 @@ import com.vitorpamplona.amethyst.ui.theme.Size16Modifier import com.vitorpamplona.quartz.experimental.clink.pointers.NOffer import com.vitorpamplona.quartz.experimental.nipA3.PaymentTarget import com.vitorpamplona.quartz.experimental.nipA3.PaymentTargetsEvent +import com.vitorpamplona.quartz.nipB1Bolt12Zaps.offer.Bolt12OfferListEvent import com.vitorpamplona.quartz.nipBCOnchainZaps.taproot.TaprootAddress import kotlinx.coroutines.launch import androidx.compose.material3.Icon as M3Icon @@ -84,9 +88,10 @@ private val CashuPurple = Color(0xFFA855F7) /** * One FlowRow of tappable chips for every way to pay this profile: Lightning - * (lud16, long-press copies the address), the CLINK offer, the NIP-BC on-chain - * wallet, Cashu nutzaps (shown only when the logged-in user's cashu wallet - * shares a mint the recipient accepts), and the NIP-A3 payment-target chips. + * (lud16, long-press copies the address), the CLINK offer, the NIP-B1 BOLT12 + * offers (one chip each), the NIP-BC on-chain wallet, Cashu nutzaps (shown only + * when the logged-in user's cashu wallet shares a mint the recipient accepts), + * and the NIP-A3 payment-target chips. * A single FlowRow so the chips wrap together with uniform spacing instead of * stacking as separately padded rows. */ @@ -113,35 +118,57 @@ fun DisplayPaymentRailChips( .collectAsStateWithLifecycle() val onchainAvailable = showOnchainWallet && LocalCache.onchainBackend != null - val address = + val targetsAddress = remember(baseUser.pubkeyHex) { PaymentTargetsEvent.createAddress(baseUser.pubkeyHex) } + val bolt12Address = + remember(baseUser.pubkeyHex) { + Bolt12OfferListEvent.createAddress(baseUser.pubkeyHex) + } - LoadAddressableNote(address, accountViewModel) { note -> + LoadAddressableNote(targetsAddress, accountViewModel) { targetsNote -> val targets = - if (note != null) { - EventFinderFilterAssemblerSubscription(note, accountViewModel) - val event by observeNoteEvent(note, accountViewModel) + if (targetsNote != null) { + EventFinderFilterAssemblerSubscription(targetsNote, accountViewModel) + val event by observeNoteEvent(targetsNote, accountViewModel) remember(event) { event?.paymentTargets() ?: emptyList() } } else { emptyList() } - if (lud16.isNullOrEmpty() && clinkOffer == null && !onchainAvailable && cashuMintUrl == null && targets.isEmpty()) { - return@LoadAddressableNote - } + LoadAddressableNote(bolt12Address, accountViewModel) { bolt12Note -> + val bolt12Offers = + if (bolt12Note != null) { + EventFinderFilterAssemblerSubscription(bolt12Note, accountViewModel) + val event by observeNoteEvent(bolt12Note, accountViewModel) + remember(event) { event?.offers() ?: emptyList() } + } else { + emptyList() + } - RailAndTargetChips( - baseUser = baseUser, - lud16 = lud16, - clinkOffer = clinkOffer, - onchainAvailable = onchainAvailable, - cashuMintUrl = cashuMintUrl, - targets = targets, - accountViewModel = accountViewModel, - nav = nav, - ) + if (lud16.isNullOrEmpty() && + clinkOffer == null && + bolt12Offers.isEmpty() && + !onchainAvailable && + cashuMintUrl == null && + targets.isEmpty() + ) { + return@LoadAddressableNote + } + + RailAndTargetChips( + baseUser = baseUser, + lud16 = lud16, + clinkOffer = clinkOffer, + bolt12Offers = bolt12Offers, + onchainAvailable = onchainAvailable, + cashuMintUrl = cashuMintUrl, + targets = targets, + accountViewModel = accountViewModel, + nav = nav, + ) + } } } @@ -151,6 +178,7 @@ private fun RailAndTargetChips( baseUser: User, lud16: String?, clinkOffer: NOffer?, + bolt12Offers: List, onchainAvailable: Boolean, cashuMintUrl: String?, targets: List, @@ -161,6 +189,9 @@ private fun RailAndTargetChips( nav.nav(Route.SendPayment(baseUser.pubkeyHex, method.routeKey)) } + // The BOLT12 offer whose pay/copy dialog is open, if any. + var bolt12DialogOffer by remember { mutableStateOf(null) } + FlowRow( horizontalArrangement = Arrangement.spacedBy(6.dp), verticalArrangement = Arrangement.spacedBy(6.dp), @@ -199,6 +230,23 @@ private fun RailAndTargetChips( } } + bolt12Offers.forEach { offer -> + ProfilePaymentChip( + color = BitcoinOrange, + label = stringRes(Res.string.bolt12_lightning_offer), + detail = remember(offer) { "${offer.take(14)}\u2026${offer.takeLast(6)}" }, + copyValue = offer, + onClick = { bolt12DialogOffer = offer }, + ) { + Icon( + symbol = MaterialSymbols.Bolt, + contentDescription = null, + tint = BitcoinOrange, + modifier = Size16Modifier, + ) + } + } + if (onchainAvailable) { ProfilePaymentChip( color = BitcoinOrange, @@ -239,6 +287,14 @@ private fun RailAndTargetChips( PaymentTargetChip(baseUser, target, accountViewModel, nav) } } + + bolt12DialogOffer?.let { offer -> + Bolt12OffersDialog( + offers = listOf(offer), + accountViewModel = accountViewModel, + onDismiss = { bolt12DialogOffer = null }, + ) + } } /** diff --git a/commons/src/commonMain/composeResources/values/strings.xml b/commons/src/commonMain/composeResources/values/strings.xml index 6cee50140c..cdb9318316 100644 --- a/commons/src/commonMain/composeResources/values/strings.xml +++ b/commons/src/commonMain/composeResources/values/strings.xml @@ -258,6 +258,7 @@ Lightning Invoice Expired CLINK Offer + BOLT12 Offer To Confirm payment Amount (sats) From 5e8fdedb839005e98e534479f8f1b9eb8fef78f8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 18:49:03 +0000 Subject: [PATCH 2/7] fix(zaps): offer the Lightning rail to BOLT12-only recipients MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The zap picker gated its Lightning bolt on the recipient's lud16/lud06, but the send path has routed a recipient with a kind:10058 offer over BOLT12 for a while (when our default NWC wallet advertises `pay`). A recipient who published only an offer therefore had no bolt in the popup and no one-tap zap, even though ZapPaymentHandler could pay them. Teach RailCapabilityResolver about the BOLT12 route: hasLightning is now true for a recipient with an offer when our wallet can pay offers. The rail keeps its single bolt — which flavour is used stays a send-time decision. The popup observes the recipient's offer list and the default wallet URI so the bolt appears as those load, and the one-tap fast path uses the same check. The sender-side test moves into AccountZapActions.canZapViaBolt12 so the handler, the picker and the profile dialog share it. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01LCxV2YvpegwUEKUhN13Mvq --- .../amethyst/model/AccountZapActions.kt | 10 +++++++ .../amethyst/model/zap/RailCapability.kt | 16 +++++++++- .../amethyst/service/ZapPaymentHandler.kt | 5 +--- .../amethyst/ui/note/ReactionsRow.kt | 30 +++++++++++++++---- .../ui/screen/loggedIn/AccountViewModel.kt | 2 +- 5 files changed, 52 insertions(+), 11 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountZapActions.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountZapActions.kt index ac39759a4c..3f9bd894c3 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountZapActions.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountZapActions.kt @@ -162,6 +162,16 @@ class AccountZapActions( ?.supportsMethod(NwcMethod.PAY) == true } + /** + * True when this account can settle a BOLT12 zap at all: an NWC wallet is + * configured and the default one advertises `pay`. The sender-side half of the + * BOLT12 route; the recipient-side half is a published kind:10058 offer. + */ + fun canZapViaBolt12(): Boolean = + account.settings.nwcWallets.value + .isNotEmpty() && + defaultWalletSupportsBolt12Pay() + /** * Sends a NIP-B1 BOLT12 zap to [recipientPubKey] over the default NWC wallet. * diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/zap/RailCapability.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/zap/RailCapability.kt index f53cc2144a..0c3b1593c8 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/zap/RailCapability.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/zap/RailCapability.kt @@ -46,6 +46,12 @@ import com.vitorpamplona.quartz.nip57Zaps.splits.zapSplitSetup * through that rail — matching the existing best-effort behaviour of the * actual send paths (Lightning skips pubkeys with no `lnAddress`; on-chain * separately warns about lnAddress-only splits that can't be paid on-chain). + * + * [hasLightning] is the whole Lightning rail, not just BOLT11: a recipient with + * no `lnAddress` but a published kind:10058 BOLT12 offer counts when our own + * NWC wallet can pay offers, because the zap send path routes them over BOLT12 + * (see `ZapPaymentHandler`). The chip stays one bolt either way — which flavour + * gets used is decided at send time, not in the picker. */ @Immutable data class RailCapability( @@ -143,6 +149,12 @@ object RailCapabilityResolver { baseNote: Note, cashuState: CashuWalletState, payToEnabled: Boolean = false, + /** + * Whether our default NWC wallet can pay BOLT12 offers + * (`AccountZapActions.canZapViaBolt12`). When true, a recipient's published + * offer makes them payable on the Lightning rail even without an lnAddress. + */ + bolt12Payable: Boolean = false, ): RailCapability { val author = baseNote.author?.pubkeyHex val splits = baseNote.event?.zapSplitSetup().orEmpty() @@ -170,7 +182,9 @@ object RailCapabilityResolver { val hasLightning = lnAddressOnlySplits.isNotEmpty() || pubKeyRecipients.any { pk -> - LocalCache.getUserIfExists(pk)?.lnAddress() != null + val user = LocalCache.getUserIfExists(pk) + user?.lnAddress() != null || + (bolt12Payable && user?.bolt12Offers()?.isNotEmpty() == true) } // On-chain pays the pubkey directly; an event with only lnAddress diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/ZapPaymentHandler.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/ZapPaymentHandler.kt index cd44293231..cdec670729 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/ZapPaymentHandler.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/ZapPaymentHandler.kt @@ -167,10 +167,7 @@ class ZapPaymentHandler( // BOLT12 when our default NWC wallet advertises the nwc#2 `pay` method (needed for // the payer proof). Otherwise — no wallet, or a wallet without `pay` — the recipient // stays on lightning, so an unsupported wallet degrades gracefully instead of erroring. - val canBolt12 = - account.settings.nwcWallets.value - .isNotEmpty() && - account.zaps.defaultWalletSupportsBolt12Pay() + val canBolt12 = account.zaps.canZapViaBolt12() val bolt12Recipients = unverifiedZapsToSend.mapNotNull { diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/ReactionsRow.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/ReactionsRow.kt index a394481d99..40642ee2cb 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/ReactionsRow.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/ReactionsRow.kt @@ -216,6 +216,7 @@ import com.vitorpamplona.quartz.nip30CustomEmoji.CustomEmoji import com.vitorpamplona.quartz.nip57Zaps.zapraiser.zapraiserAmount import com.vitorpamplona.quartz.nip61Nutzaps.info.NutzapInfoEvent import com.vitorpamplona.quartz.nipA0VoiceMessages.BaseVoiceEvent +import com.vitorpamplona.quartz.nipB1Bolt12Zaps.offer.Bolt12OfferListEvent import com.vitorpamplona.quartz.utils.TimeUtils import kotlinx.collections.immutable.ImmutableList import kotlinx.collections.immutable.ImmutableSet @@ -1480,10 +1481,15 @@ fun zapClick( choices.size == 1 -> { // One-tap fast path is Lightning-only. If the recipient can't - // receive Lightning (no lud16/lud06), firing a zap here would just - // fail — open the picker instead so the rail-aware chip can route to - // cashu / on-chain / reload. - val caps = RailCapabilityResolver.peek(baseNote, accountViewModel.account.cashuWalletState) + // receive Lightning (no lud16/lud06, and no BOLT12 offer our wallet + // can pay), firing a zap here would just fail — open the picker + // instead so the rail-aware chip can route to cashu / on-chain / reload. + val caps = + RailCapabilityResolver.peek( + baseNote, + accountViewModel.account.cashuWalletState, + bolt12Payable = accountViewModel.account.zaps.canZapViaBolt12(), + ) if (caps.hasLightning) { onZapStarts() accountViewModel.zap( @@ -2134,6 +2140,12 @@ fun observeZapRailCapability( val cashuEntries by cashuState.tokenEntries.collectAsStateWithLifecycle() val recipientInfo = author?.let { observeUserInfo(it, accountViewModel).value } val nutzapInfo = author?.let { observeNoteEvent(it.nutzapInfoNote, accountViewModel).value } + // BOLT12 route inputs, same contract: the recipient's kind:10058 offer list + // (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(it.bolt12OfferListNote, accountViewModel).value } + val defaultWalletUri by accountViewModel.account.nip47SignerState.defaultWalletUri + .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 // Send Payment screen. @@ -2180,11 +2192,19 @@ fun observeZapRailCapability( cashuEntries, recipientInfo, nutzapInfo, + bolt12OfferList, + defaultWalletUri, showPayToChip, recipientPayTo, payToApps, ) { - val rc = RailCapabilityResolver.peek(baseNote, cashuState, showPayToChip) + val rc = + RailCapabilityResolver.peek( + baseNote, + cashuState, + showPayToChip, + bolt12Payable = accountViewModel.account.zaps.canZapViaBolt12(), + ) if (onchainEnabled) { rc.copy(onchainMaxSpendableSats = onchainFunds?.maxSpendableSats) } else { diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountViewModel.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountViewModel.kt index cda7e35f72..d3fa91fa09 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountViewModel.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/AccountViewModel.kt @@ -1196,7 +1196,7 @@ class AccountViewModel( .isNotEmpty() /** True when a BOLT12 offer can be paid in-app: an NWC wallet is set and advertises `pay` (nwc#2). */ - fun canPayBolt12ViaNwc(): Boolean = hasNwcWallet() && account.zaps.defaultWalletSupportsBolt12Pay() + fun canPayBolt12ViaNwc(): Boolean = account.zaps.canZapViaBolt12() /** * Pays a recipient's BOLT12 [offer] over the default NWC wallet using the nwc#2 From 6a4ddfa9747c6e39c620b1f04619f16f013c6555 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 19:21:27 +0000 Subject: [PATCH 3/7] fix(blossom): re-check the cache after winning the read-auth in-flight slot aFastSignerStillSharesOneSignature still failed about two runs in five: a straggler that read the in-flight map and the cache as empty could then win putIfAbsent because the leader had already signed, cached and retired its entry in between, and would sign a second token. The leader's cache put happens-before its removal of the same key, so once a caller has claimed the slot a cached token, if any, is visible. Look once more there: hand the cached token to this caller and to any follower that already picked up the fresh deferred, retire the entry, and skip the signature. Ten reruns of the class pass where two in five failed before. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01LCxV2YvpegwUEKUhN13Mvq --- .../service/http/BlossomReadAuthTokenProvider.kt | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/service/http/BlossomReadAuthTokenProvider.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/service/http/BlossomReadAuthTokenProvider.kt index 7c635ce442..80814a484b 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/service/http/BlossomReadAuthTokenProvider.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/service/http/BlossomReadAuthTokenProvider.kt @@ -129,6 +129,18 @@ class BlossomReadAuthTokenProvider( val fresh = CompletableDeferred() inFlight.putIfAbsent(host, fresh)?.let { return it } + // Third look, now that this caller holds the entry. Between the second look and + // the `putIfAbsent` a leader can run its whole cycle — sign, cache, retire — and + // that retirement is exactly what let this caller claim the slot. The leader's + // put to [cache] happens-before its removal of the same key, so any token it + // minted is visible here: hand it out (to this caller and to any follower that + // already picked up [fresh]) and retire the entry, instead of signing again. + cachedHeader(host)?.let { + inFlight.remove(host, fresh) + fresh.complete(it) + return fresh + } + scope .launch { val header = From 3887b033e00a773c3c8c2de2e94bb6b57fb080e0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 20:17:09 +0000 Subject: [PATCH 4/7] feat(zaps): fall back to a BOLT11 zap when the wallet refuses a BOLT12 offer The wallet resolves the offer itself, so a stale or unreachable kind:10058 offer surfaces as a NIP-47 error reply, which by the spec means nothing was paid. When that recipient also publishes a lightning address, re-send the same share as a regular zap through the BOLT11 lane instead of toasting the BOLT12 error; the toast stays for a recipient with no other route. Bolt12LightningFallback keeps the decision pure and tested: every refusal retries except the ones about our own wallet (insufficient balance, quota, rate limit, restricted, unauthorized, unsupported encryption), which would fail the same way over BOLT11. A paid-but-no-receipt outcome is never retried, and neither is a wallet that never answers: sendBolt12Zap now passes a timeout handler, so a silent wallet reports a timeout and steps the progress instead of leaving the zap hanging. The BOLT11 lane moves into zapOverLightning so the main zap and the fallback share one path, and Bolt12Recipient carries the lnAddress and relay hint the retry needs. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01LCxV2YvpegwUEKUhN13Mvq --- .../amethyst/model/AccountZapActions.kt | 34 +++- .../service/Bolt12LightningFallback.kt | 51 +++++ .../amethyst/service/ZapPaymentHandler.kt | 192 +++++++++++++----- .../service/Bolt12LightningFallbackTest.kt | 64 ++++++ 4 files changed, 289 insertions(+), 52 deletions(-) create mode 100644 amethyst/src/main/java/com/vitorpamplona/amethyst/service/Bolt12LightningFallback.kt create mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/service/Bolt12LightningFallbackTest.kt diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountZapActions.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountZapActions.kt index 3f9bd894c3..560ccee5ce 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountZapActions.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountZapActions.kt @@ -37,7 +37,10 @@ import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl import com.vitorpamplona.quartz.nip01Core.signers.NostrSignerInternal import com.vitorpamplona.quartz.nip47WalletConnect.Nip47WalletConnect import com.vitorpamplona.quartz.nip47WalletConnect.rpc.IErrorResponseLike +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.NwcErrorCode +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.NwcErrorResponse import com.vitorpamplona.quartz.nip47WalletConnect.rpc.NwcMethod +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.PayInvoiceErrorResponse import com.vitorpamplona.quartz.nip47WalletConnect.rpc.PayMethod import com.vitorpamplona.quartz.nip47WalletConnect.rpc.PaySuccessResponse import com.vitorpamplona.quartz.nip47WalletConnect.rpc.Request @@ -183,6 +186,13 @@ class AccountZapActions( * still happened; [onError] reports "paid, no receipt"). [zappedEvent] is null for * a profile zap. Requires an NWC wallet (see [hasNwcWallet]); BOLT12 zaps have no * 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. + * - [onError]: paid but no valid receipt, or nothing conclusive. Never retry. + * - [onTimeout]: the wallet never answered. Unknown state — never retry. */ suspend fun sendBolt12Zap( zappedEvent: Event?, @@ -193,15 +203,21 @@ class AccountZapActions( zapType: LnZapEvent.ZapType, // (messageResId, detail) — the caller localizes; detail carries a wallet error, if any. onError: (Int, String?) -> Unit, + // (code, detail) — the wallet refused or failed the payment; no funds moved. + onNotPaid: suspend (NwcErrorCode?, String?) -> Unit, + onTimeout: () -> Unit, onProcessed: () -> Unit, ) { // NONZAP means "pay, but publish no receipt" — settle the offer without binding // a zap intent or emitting a 9736, matching the privacy of a bolt11 NONZAP. if (zapType == LnZapEvent.ZapType.NONZAP) { - sendNwcRequest(PayMethod.create("bitcoin:?lno=$offer", amountMillisats)) { response -> + sendNwcRequest(PayMethod.create("bitcoin:?lno=$offer", amountMillisats), onTimeout) { response -> account.scope.launch { - if (response is IErrorResponseLike) onError(R.string.bolt12_payment_failed, response.errorMessage()) - onProcessed() + try { + if (response is IErrorResponseLike) onNotPaid(response.nwcErrorCode(), response.errorMessage()) + } finally { + onProcessed() + } } } return @@ -221,7 +237,7 @@ class AccountZapActions( val payerNote = Bolt12ZapBuilder.payerNote(intent) - sendNwcRequest(PayMethod.create("bitcoin:?lno=$offer", amountMillisats, payerNote)) { response -> + sendNwcRequest(PayMethod.create("bitcoin:?lno=$offer", amountMillisats, payerNote), onTimeout) { response -> account.scope.launch { // try/finally so a failure while assembling/publishing the receipt (e.g. a // remote signer error) still steps progress and surfaces an error, instead @@ -244,7 +260,7 @@ class AccountZapActions( } } - is IErrorResponseLike -> onError(R.string.bolt12_payment_failed, response.errorMessage()) + is IErrorResponseLike -> onNotPaid(response.nwcErrorCode(), response.errorMessage()) else -> onError(R.string.bolt12_zap_paid_no_receipt, null) } @@ -384,3 +400,11 @@ class AccountZapActions( return this } } + +/** The NIP-47 error code on a failed reply, whichever error shape the wallet used. */ +private fun Response.nwcErrorCode(): NwcErrorCode? = + when (this) { + is NwcErrorResponse -> error?.code + is PayInvoiceErrorResponse -> error?.code + else -> null + } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/Bolt12LightningFallback.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/Bolt12LightningFallback.kt new file mode 100644 index 0000000000..60815ca778 --- /dev/null +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/Bolt12LightningFallback.kt @@ -0,0 +1,51 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.service + +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. + */ +object Bolt12LightningFallback { + /** Refusals that describe the sender's wallet, not the offer. */ + private val senderSideCodes = + setOf( + NwcErrorCode.INSUFFICIENT_BALANCE, + NwcErrorCode.QUOTA_EXCEEDED, + NwcErrorCode.RATE_LIMITED, + NwcErrorCode.RESTRICTED, + NwcErrorCode.UNAUTHORIZED, + NwcErrorCode.UNSUPPORTED_ENCRYPTION, + ) + + /** 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 +} diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/ZapPaymentHandler.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/ZapPaymentHandler.kt index cdec670729..df1b584156 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/ZapPaymentHandler.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/ZapPaymentHandler.kt @@ -43,6 +43,7 @@ import com.vitorpamplona.quartz.nip57Zaps.splits.ZapSplitSetupLnAddress import com.vitorpamplona.quartz.nip57Zaps.splits.zapSplitSetup import com.vitorpamplona.quartz.nip57Zaps.validate.LnurlForm import com.vitorpamplona.quartz.nip89AppHandlers.definition.AppDefinitionEvent +import com.vitorpamplona.quartz.utils.Log import com.vitorpamplona.quartz.utils.mapNotNullAsync import kotlinx.collections.immutable.ImmutableList import kotlinx.collections.immutable.toImmutableList @@ -84,11 +85,17 @@ class ZapPaymentHandler( val user: User? = null, ) - /** A recipient routed over BOLT12 (NIP-B1): they publish a kind:10058 [offer] and we hold an NWC wallet. */ + /** + * A recipient routed over BOLT12 (NIP-B1): they publish a kind:10058 [offer] and we + * hold an NWC wallet. [lnAddress] is their BOLT11 route, kept so a refused offer can + * fall back to a regular zap (see [payViaBolt12]); null when they publish none. + */ data class Bolt12Recipient( val user: User, val offer: String, val weight: Double = 1.0, + val lnAddress: String? = null, + val relay: NormalizedRelayUrl? = null, ) suspend fun zap( @@ -173,7 +180,7 @@ class ZapPaymentHandler( unverifiedZapsToSend.mapNotNull { val user = it.user if (canBolt12 && it.bolt12Offer != null && user != null) { - Bolt12Recipient(user, it.bolt12Offer, it.weight) + Bolt12Recipient(user, it.bolt12Offer, it.weight, it.lnAddress, it.relay) } else { null } @@ -230,48 +237,20 @@ class ZapPaymentHandler( // --- Lightning lane ----------------------------------------------------------- if (zapsToSend.isNotEmpty()) { - val splitZapRequests = signAllZapRequests(note, pollOption, message, zapType, zapsToSend, amountMilliSats, totalWeight) - - if (splitZapRequests.isNotEmpty()) { - onProgress(0.05f) - - val payables = - assembleAllInvoices( - requests = splitZapRequests, - totalAmountMilliSats = amountMilliSats, - message = message, - okHttpClient = okHttpClient, - onError = onError, - onProgress = { onProgress(it * 0.7f + 0.05f) }, - context = context, - totalWeight = totalWeight, - ) - - if (payables.isNotEmpty()) { - onProgress(0.75f) - - // Route through the user's selected default payment source. A CLINK debit takes - // precedence over NWC when it is the chosen default; NWC-only users are unaffected - // (defaultPaymentSource() resolves to their NWC wallet). No source -> wallet app. - when (val source = account.settings.defaultPaymentSource()) { - is PaymentSource.ClinkDebit -> { - payViaClinkDebit(payables, source.wallet.pointer, onError = onError, onProgress = { - onProgress(it * 0.25f + 0.75f) - }, context) - } - - is PaymentSource.Nwc -> { - payViaNWC(payables, note, onError = onError, onProgress = { - onProgress(it * 0.25f + 0.75f) // keeps within range. - }, context) - } - - null -> { - onPayViaIntent(payables.toImmutableList()) - } - } - } - } + zapOverLightning( + zapsToSend = zapsToSend, + note = note, + pollOption = pollOption, + message = message, + zapType = zapType, + totalAmountMilliSats = amountMilliSats, + totalWeight = totalWeight, + okHttpClient = okHttpClient, + onError = onError, + onProgress = onProgress, + onPayViaIntent = onPayViaIntent, + context = context, + ) } // --- BOLT12 lane -------------------------------------------------------------- @@ -279,12 +258,15 @@ class ZapPaymentHandler( payViaBolt12( recipients = bolt12Recipients, note = note, + pollOption = pollOption, totalAmountMilliSats = amountMilliSats, totalWeight = totalWeight, message = message, zapType = zapType, + okHttpClient = okHttpClient, onError = onError, onProgress = { onProgress(it * 0.25f + 0.75f) }, + onPayViaIntent = onPayViaIntent, context = context, ) } @@ -292,6 +274,68 @@ class ZapPaymentHandler( onProgress(1f) } + /** + * The BOLT11 lane: signs one kind 9734 per recipient, fetches each invoice from + * the recipient's LNURL, then settles through the default payment source. Used + * for every lnAddress recipient of a zap, and again by [payViaBolt12] for a + * recipient whose offer the wallet refused. [onProgress] spans 0.05..1.0. + */ + private suspend fun zapOverLightning( + zapsToSend: List, + note: Note, + pollOption: Int?, + message: String, + zapType: LnZapEvent.ZapType, + totalAmountMilliSats: Long, + totalWeight: Double, + okHttpClient: (String) -> OkHttpClient, + onError: (String, String, User?) -> Unit, + onProgress: (percent: Float) -> Unit, + onPayViaIntent: (ImmutableList) -> Unit, + context: Context, + ) { + val splitZapRequests = signAllZapRequests(note, pollOption, message, zapType, zapsToSend, totalAmountMilliSats, totalWeight) + if (splitZapRequests.isEmpty()) return + + onProgress(0.05f) + + val payables = + assembleAllInvoices( + requests = splitZapRequests, + totalAmountMilliSats = totalAmountMilliSats, + message = message, + okHttpClient = okHttpClient, + onError = onError, + onProgress = { onProgress(it * 0.7f + 0.05f) }, + context = context, + totalWeight = totalWeight, + ) + if (payables.isEmpty()) return + + onProgress(0.75f) + + // Route through the user's selected default payment source. A CLINK debit takes + // precedence over NWC when it is the chosen default; NWC-only users are unaffected + // (defaultPaymentSource() resolves to their NWC wallet). No source -> wallet app. + when (val source = account.settings.defaultPaymentSource()) { + is PaymentSource.ClinkDebit -> { + payViaClinkDebit(payables, source.wallet.pointer, onError = onError, onProgress = { + onProgress(it * 0.25f + 0.75f) + }, context) + } + + is PaymentSource.Nwc -> { + payViaNWC(payables, note, onError = onError, onProgress = { + onProgress(it * 0.25f + 0.75f) // keeps within range. + }, context) + } + + null -> { + onPayViaIntent(payables.toImmutableList()) + } + } + } + private fun calculateZapValue( amountMilliSats: Long, weight: Double, @@ -460,21 +504,40 @@ class ZapPaymentHandler( * and (if the returned proof validates) publishes a 9736 zap — see * [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. */ suspend fun payViaBolt12( recipients: List, note: Note, + pollOption: Int?, totalAmountMilliSats: Long, totalWeight: Double, message: String, zapType: LnZapEvent.ZapType, + okHttpClient: (String) -> OkHttpClient, onError: (String, String, User?) -> Unit, onProgress: (percent: Float) -> Unit, + onPayViaIntent: (ImmutableList) -> Unit, context: Context, ) { val progress = PaymentProgress(recipients.size, onProgress) mapNotNullAsync(recipients) { recipient: Bolt12Recipient -> + fun reportBolt12Error( + msgRes: Int, + detail: String?, + ) { + val msg = if (detail != null) stringRes(context, msgRes, detail) else stringRes(context, msgRes) + onError(stringRes(context, R.string.bolt12_zap_error), msg, recipient.user) + } + account.zaps.sendBolt12Zap( zappedEvent = note.event, recipientPubKey = recipient.user.pubkeyHex, @@ -482,9 +545,44 @@ class ZapPaymentHandler( amountMillisats = calculateZapValue(totalAmountMilliSats, recipient.weight, totalWeight), message = message, zapType = zapType, - onError = { msgRes, detail -> - val msg = if (detail != null) stringRes(context, msgRes, detail) else stringRes(context, msgRes) - onError(stringRes(context, R.string.bolt12_zap_error), msg, recipient.user) + onError = ::reportBolt12Error, + onNotPaid = { code, detail -> + val lnAddress = recipient.lnAddress + if (lnAddress != null && Bolt12LightningFallback.shouldRetry(code)) { + Log.i("ZapPaymentHandler") { "BOLT12 offer refused ($code: $detail); re-sending over BOLT11 to $lnAddress" } + try { + zapOverLightning( + zapsToSend = listOf(MyZapSplitSetup(lnAddress, recipient.weight, recipient.relay, recipient.user)), + note = note, + pollOption = pollOption, + message = message, + zapType = zapType, + totalAmountMilliSats = totalAmountMilliSats, + totalWeight = totalWeight, + okHttpClient = okHttpClient, + onError = onError, + // The zap's own progress finished when the BOLT12 request was + // dispatched; the retry settles in the background like NWC does. + onProgress = {}, + onPayViaIntent = onPayViaIntent, + context = context, + ) + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + // Nothing was paid on either rail. Report it as the lightning failure it + // is, rather than letting [sendBolt12Zap]'s catch call it "paid, no receipt". + Log.w("ZapPaymentHandler", "BOLT11 fallback failed after a refused BOLT12 offer", e) + onError(stringRes(context, R.string.error_dialog_zap_error), e.message ?: e.toString(), recipient.user) + } + } else { + reportBolt12Error(R.string.bolt12_payment_failed, detail) + } + }, + onTimeout = { + // No response callback will fire, so account for the settlement step here. + reportBolt12Error(R.string.bolt12_payment_failed, nwcTimeoutMessage(context)) + progress.step() }, onProcessed = { progress.step() }, ) diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/Bolt12LightningFallbackTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/Bolt12LightningFallbackTest.kt new file mode 100644 index 0000000000..059085c571 --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/Bolt12LightningFallbackTest.kt @@ -0,0 +1,64 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.service + +import com.vitorpamplona.quartz.nip47WalletConnect.rpc.NwcErrorCode +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +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)) + } + + @Test + fun senderSideRefusalsDoNotRetry() { + listOf( + NwcErrorCode.INSUFFICIENT_BALANCE, + NwcErrorCode.QUOTA_EXCEEDED, + NwcErrorCode.RATE_LIMITED, + NwcErrorCode.RESTRICTED, + NwcErrorCode.UNAUTHORIZED, + NwcErrorCode.UNSUPPORTED_ENCRYPTION, + ).forEach { code -> + assertFalse("$code is about our wallet, BOLT11 would fail the same way", Bolt12LightningFallback.shouldRetry(code)) + } + } +} From d4ac5149f75249fce257a27ea40426fb01208aca Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 20:47:05 +0000 Subject: [PATCH 5/7] 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 Claude-Session: https://claude.ai/code/session_01LCxV2YvpegwUEKUhN13Mvq --- .../amethyst/model/AccountZapActions.kt | 7 ++-- .../service/Bolt12LightningFallback.kt | 37 ++++++++++--------- .../amethyst/service/ZapPaymentHandler.kt | 20 ++++++---- .../bolt12Offers/Bolt12OffersScreen.kt | 3 +- .../amethyst/ui/note/ReactionsRow.kt | 20 ++++++---- .../profile/header/Bolt12OffersDialog.kt | 9 ++++- .../profile/header/ProfilePaymentRailChips.kt | 2 +- .../service/Bolt12LightningFallbackTest.kt | 16 +++++--- .../model/nip47WalletConnect/NwcInfoCache.kt | 15 ++++++++ 9 files changed, 86 insertions(+), 43 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountZapActions.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountZapActions.kt index 560ccee5ce..eb854749ea 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountZapActions.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountZapActions.kt @@ -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. */ diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/Bolt12LightningFallback.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/Bolt12LightningFallback.kt index 60815ca778..4f73a3b2c2 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/Bolt12LightningFallback.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/Bolt12LightningFallback.kt @@ -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 } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/ZapPaymentHandler.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/ZapPaymentHandler.kt index df1b584156..0d08176f3c 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/ZapPaymentHandler.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/ZapPaymentHandler.kt @@ -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, @@ -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 = { diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/actions/bolt12Offers/Bolt12OffersScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/actions/bolt12Offers/Bolt12OffersScreen.kt index 354d0a435e..0a0ef7d197 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/actions/bolt12Offers/Bolt12OffersScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/actions/bolt12Offers/Bolt12OffersScreen.kt @@ -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, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/ReactionsRow.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/ReactionsRow.kt index 40642ee2cb..b469b4091e 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/ReactionsRow.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/ReactionsRow.kt @@ -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(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, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/Bolt12OffersDialog.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/Bolt12OffersDialog.kt index e6ff960a77..e377c20473 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/Bolt12OffersDialog.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/Bolt12OffersDialog.kt @@ -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)}" diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/ProfilePaymentRailChips.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/ProfilePaymentRailChips.kt index 61c6fbda35..43e0949b2c 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/ProfilePaymentRailChips.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/profile/header/ProfilePaymentRailChips.kt @@ -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 }, ) { diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/Bolt12LightningFallbackTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/Bolt12LightningFallbackTest.kt index 059085c571..98fe9c90fc 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/Bolt12LightningFallbackTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/Bolt12LightningFallbackTest.kt @@ -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 diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/nip47WalletConnect/NwcInfoCache.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/nip47WalletConnect/NwcInfoCache.kt index 51c1c1e6df..3782bdfe66 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/nip47WalletConnect/NwcInfoCache.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/model/nip47WalletConnect/NwcInfoCache.kt @@ -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() + 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 = updatesState.asStateFlow() + // Fetches in progress, keyed like [cache]. Every fetching path goes through [fetchOnce]. private val inFlight = ConcurrentMap>() @@ -182,6 +196,7 @@ class NwcInfoCache( } cache[uri.pubKeyHex] = Entry(info, now()) + updatesState.update { it + 1 } return info } From 285a51e98fe11f1fa8a6c7f21a8fbed2d1375f24 Mon Sep 17 00:00:00 2001 From: mstrofnone Date: Sat, 12 Sep 2026 14:38:38 +1000 Subject: [PATCH 6/7] fix(desktop): stop silent account wipe on keychain errors and upgrade races Three independent bugs in DesktopAccountStorage / SecureKeyStorage could turn one ambiguous macOS Keychain reply, one transient read error, or one Homebrew upgrade race into permanent account-metadata loss on ~/.amethyst/accounts.json.enc. 1. Silent AES metadata-key rotation on ambiguous keychain miss. getOrCreateKey() treated null from getPrivateKey("account-metadata- key") as "no key exists" and generated a fresh AES key. On macOS javakeyring collapses errSecItemNotFound (-25300), errSecAuthFailed (-25293), errSecUserCanceled (-128), and errSecInteractionNotAllowed (-25308) into the same PasswordAccessException; getFromKeyring turns them all into null. A single Deny click on the OS Keychain dialog silently rotated the AES key and destroyed the ability to decrypt the existing accounts.json.enc. Fix: add a new strict SecureKeyStorage.getPrivateKeyOrThrow(npub) on the common expect. On JVM/macOS it wraps /usr/bin/security find-generic-password whose exit codes (0 = found, 44 = not found, others = ambiguous) are documented and unambiguous. On JVM Windows/Linux it uses javakeyring but throws on any PasswordAccessException from the strict path. On Android it uses EncryptedSharedPreferences.contains(). On iOS it mirrors the existing "pending (iOS Phase 4)" stub. getOrCreateKey now calls getPrivateKeyOrThrow and propagates SecureStorageException without ever rotating the key. The permissive getPrivateKey(npub) is unchanged; its callers (per-account nsec, ephemeral bunker keys) still tolerate null on any error. 2. Any read failure resets the file. readMetadataFromDisk() used to rename to accounts.json.enc.corrupt. and return empty AccountMetadata() on any exception, including transient IO and the newly-throwing keychain path from bug 1. Fix: distinguish exception types. - AEADBadTagException / BadPaddingException: back up to .corrupt., reset, fire StorageCorruption.FileCorrupted (unchanged). - JacksonException: back up but to .jsonerror. so it is distinguishable from ciphertext corruption; fire StorageCorruption.JsonMalformed. - Anything else (IO error, OOM, thrown keychain path): do NOT rename; rethrow to caller and fire a new StorageCorruption.TransientError(cause) subtype. The file stays untouched. AccountManager.loadSavedAccount already wraps in try/catch and turns the throw into Result.failure. - Truncated file (size < GCM IV size): still backup + reset, genuinely unusable. 3. No cross-process advisory lock. Homebrew replacing the .app while the old process is mid-save, or an accidental double-launch of Compose Desktop (no built-in single-instance guard), could produce a truncated file that trips bug 2. Fix: withAccountsFileLock helper (mirrors SecureKeyStorage. withFileLock) wraps read + write in a RandomAccessFile(lockFile, "rw").channel.lock() on ~/.amethyst/accounts.json.enc.lock (0600). Because FileChannel.lock is per-JVM, an in-process Mutex is held before acquiring the channel lock. A separate stateMutex guards the read-modify-write cycle in saveAccount / deleteAccount / setCurrentAccount so two concurrent writers cannot each read the same base metadata and each rewrite it. Backward compatibility: existing keychain items are read unchanged; no schema migration for accounts.json.enc; the file lock adds a .lock sidecar older builds ignore. Tests: new SecureKeyStorageOrThrowTest (pure exit-code parser, mac lookup Found/NotFound/Ambiguous, non-mac keyring hit and throw-on-PasswordAccessException). DesktopAccountStorageTest gains five cases: getOrCreateKey ambiguous-error preserves file and does not rotate; getOrCreateKey definitive-not-found happy path; readMetadataFromDisk transient-IO preserves file with no backup sibling; GCM tag mismatch keeps .corrupt. backup; JSON malformed uses new .jsonerror. suffix; eight concurrent saveAccount calls serialize under the file lock with no lost updates. All existing AccountManager* MockK setups extended to also stub getPrivateKeyOrThrow. Local verify: :desktopApp:test + :commons:jvmTest, 2490 tests, all pass. Spotless clean. --- .../commons/keystorage/SecureKeyStorage.kt | 20 ++ .../commons/keystorage/SecureKeyStorage.kt | 22 ++ .../keystorage/SecureKeyStorage.ios.kt | 2 + .../commons/keystorage/SecureKeyStorage.kt | 161 +++++++++++++ .../keystorage/SecureKeyStorageOrThrowTest.kt | 212 +++++++++++++++++ .../desktop/account/DesktopAccountStorage.kt | 176 ++++++++++---- .../account/AccountManagerKeyLoginTest.kt | 1 + .../account/AccountManagerLoadAccountTest.kt | 1 + .../AccountManagerLoadStateTransitionsTest.kt | 1 + .../account/AccountManagerLogoutTest.kt | 1 + .../AccountManagerNip46IsolationTest.kt | 1 + .../AccountManagerStateTransitionTest.kt | 1 + .../account/DesktopAccountStorageTest.kt | 217 ++++++++++++++++++ .../desktop/benchmark/LaunchScenario.kt | 1 + .../desktop/ui/AppStateMachineTest.kt | 1 + 15 files changed, 777 insertions(+), 41 deletions(-) create mode 100644 commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorageOrThrowTest.kt diff --git a/commons/src/androidMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt b/commons/src/androidMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt index 74b279d621..dbb4efb401 100644 --- a/commons/src/androidMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt +++ b/commons/src/androidMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt @@ -107,6 +107,26 @@ actual class SecureKeyStorage private actual constructor() { } } + /** + * Android backend: EncryptedSharedPreferences.contains + getString has no + * ambiguous-error state comparable to macOS Keychain user-cancel/deny, so + * "key not present" and "key present" are the only two null outcomes. + * Any thrown exception is a genuine failure and propagates. + */ + actual suspend fun getPrivateKeyOrThrow(npub: String): String? = + withContext(Dispatchers.IO) { + try { + val key = KEY_PREFIX + npub + if (!encryptedPrefs.contains(key)) { + null + } else { + encryptedPrefs.getString(key, null) + } + } catch (e: Exception) { + throw SecureStorageException("Failed to retrieve private key", e) + } + } + actual suspend fun deletePrivateKey(npub: String): Boolean = withContext(Dispatchers.IO) { try { diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt index efb9d7342e..2ee7e124cf 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt @@ -76,12 +76,34 @@ expect class SecureKeyStorage private constructor() { * **Security Warning:** The returned String cannot be securely zeroed from memory (JVM limitation). * Dereference the returned value immediately after use to minimize exposure time. * + * Callers that MUST distinguish "key does not exist" from "backend refused/locked/failed" + * (for example, before generating a replacement key on disk) should use + * [getPrivateKeyOrThrow] instead. This method returns null on any error and cannot + * safely be used as an "is this the first launch?" probe. + * * @param npub The public key in npub (Bech32) format * @return The private key in hexadecimal format, or null if not found * @throws SecureStorageException if retrieval operation fails */ suspend fun getPrivateKey(npub: String): String? + /** + * Retrieves a private key for the given npub, distinguishing "definitively absent" + * from any other failure mode. + * + * On success, returns the key. When the backend confirms the item does not exist, + * returns null. Any other outcome (backend unavailable, user denied the OS prompt, + * keychain locked, I/O error) throws [SecureStorageException]. This is the safe + * primitive for compare-and-swap style flows where a null must not be interpreted + * as permission to generate and persist a replacement. + * + * @param npub The public key in npub (Bech32) format + * @return The private key in hexadecimal format, or null only when the backend + * confirms the item does not exist + * @throws SecureStorageException on any ambiguous or transient failure + */ + suspend fun getPrivateKeyOrThrow(npub: String): String? + /** * Deletes a private key for the given npub. * diff --git a/commons/src/iosMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.ios.kt b/commons/src/iosMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.ios.kt index d67627da12..d48ac93cfd 100644 --- a/commons/src/iosMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.ios.kt +++ b/commons/src/iosMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.ios.kt @@ -34,6 +34,8 @@ actual class SecureKeyStorage private actual constructor() { actual suspend fun getPrivateKey(npub: String): String? = throw SecureStorageException("Keychain Services binding pending (iOS Phase 4)") + actual suspend fun getPrivateKeyOrThrow(npub: String): String? = throw SecureStorageException("Keychain Services binding pending (iOS Phase 4)") + actual suspend fun deletePrivateKey(npub: String): Boolean = throw SecureStorageException("Keychain Services binding pending (iOS Phase 4)") actual suspend fun hasPrivateKey(npub: String): Boolean = throw SecureStorageException("Keychain Services binding pending (iOS Phase 4)") diff --git a/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt b/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt index 85c8b9c95e..31a27b98fb 100644 --- a/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt +++ b/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt @@ -149,6 +149,75 @@ actual class SecureKeyStorage private actual constructor() { } } + /** + * Strict variant that distinguishes "backend confirms item not found" from every + * other outcome. This matters on macOS: `javakeyring` collapses `errSecItemNotFound` + * (-25300), `errSecAuthFailed` (-25293), `errSecUserCanceled` (-128), and + * `errSecInteractionNotAllowed` (-25308) into the same `PasswordAccessException`. + * A caller that mistook "user clicked Deny" for "first launch, generate a fresh + * key" would silently rotate the metadata AES key and permanently destroy the + * accounts.json.enc it was supposed to unlock. + * + * On macOS this shells out to `/usr/bin/security find-generic-password`, whose + * exit codes are documented and unambiguous (44 = not found, 128 = user cancel / + * dialog dismissed, others = backend failure). On Windows / Linux, javakeyring + * has no such ambiguity for the equivalent flows in practice, but we still treat + * any `PasswordAccessException` here as ambiguous (throw) to keep the contract + * strict on the getOrCreate path. + */ + actual suspend fun getPrivateKeyOrThrow(npub: String): String? = + withContext(Dispatchers.IO) { + try { + if (!keyringAvailable) { + return@withContext getFromFallback(npub) + } + if (isMacOs()) { + return@withContext getFromMacSecurityCli(SERVICE_NAME, npub) + } + try { + keyring().getPassword(SERVICE_NAME, npub) + } catch (e: PasswordAccessException) { + // Non-mac backends: keep the strict contract by refusing to + // treat this as "definitively absent". A caller that needs a + // permissive lookup should use getPrivateKey() instead. + throw SecureStorageException( + "Keyring backend refused access or returned ambiguous not-found", + e, + ) + } + } catch (e: SecureStorageException) { + throw e + } catch (e: BackendNotSupportedException) { + keyringAvailable = false + println("OS keyring not available, using fallback encrypted storage") + getFromFallback(npub) + } catch (e: Exception) { + throw SecureStorageException("Failed to retrieve private key (strict)", e) + } + } + + /** + * Test seam: overridable strategy for the strict macOS lookup. Production wires + * to [defaultMacSecurityLookup] which spawns `/usr/bin/security`. Tests replace + * this with a stub so unit tests run hermetically on any OS. + */ + internal var macSecurityLookup: (String, String) -> MacSecurityResult = + ::defaultMacSecurityLookup + + private fun getFromMacSecurityCli( + service: String, + account: String, + ): String? { + val result = macSecurityLookup(service, account) + return when (result) { + is MacSecurityResult.Found -> result.password + is MacSecurityResult.NotFound -> null + is MacSecurityResult.Ambiguous -> throw SecureStorageException( + "macOS Keychain access failed (${result.reason}, exit=${result.exitCode})", + ) + } + } + actual suspend fun deletePrivateKey(npub: String): Boolean = withContext(Dispatchers.IO) { try { @@ -466,6 +535,98 @@ internal interface KeyringHandle { ) } +/** + * Outcome of a strict macOS `/usr/bin/security find-generic-password` lookup. + * Kept as a sealed hierarchy so [SecureKeyStorage.getPrivateKeyOrThrow] can + * cleanly translate to `null` versus `SecureStorageException`. + */ +internal sealed class MacSecurityResult { + data class Found( + val password: String, + ) : MacSecurityResult() + + object NotFound : MacSecurityResult() + + /** + * Any exit code other than 0 (found) or 44 (item not found). Reason is a short + * human string derived from stderr / documented codes: + * 128 = user cancelled or dismissed the Keychain Access dialog + * -25293 (errSecAuthFailed) surfaces as exit 51 in practice + * -25308 (errSecInteractionNotAllowed) surfaces when Keychain is locked + */ + data class Ambiguous( + val exitCode: Int, + val reason: String, + ) : MacSecurityResult() +} + +/** + * Pure parser split out for testability on non-macOS CI runners. Maps the + * documented exit code contract of `/usr/bin/security find-generic-password` + * to a [MacSecurityResult]. `stdout` is the raw password body (`-w` prints it + * followed by a newline; strip the trailing newline only). `stderr` is used + * as a hint for the ambiguous [MacSecurityResult.Ambiguous.reason] string. + */ +internal fun parseMacSecurityFindResult( + exitCode: Int, + stdout: String, + stderr: String, +): MacSecurityResult = + when (exitCode) { + 0 -> MacSecurityResult.Found(stdout.trimEnd('\n', '\r')) + 44 -> MacSecurityResult.NotFound + else -> { + val reason = + when { + exitCode == 128 -> "user cancelled Keychain dialog" + stderr.contains("-25293") -> "errSecAuthFailed" + stderr.contains("-25308") -> "errSecInteractionNotAllowed" + stderr.contains("-128") -> "user cancelled Keychain dialog" + stderr.isNotBlank() -> + stderr + .lineSequence() + .first() + .trim() + .take(120) + else -> "unknown" + } + MacSecurityResult.Ambiguous(exitCode, reason) + } + } + +private fun isMacOs(): Boolean = System.getProperty("os.name").orEmpty().startsWith("Mac") + +/** + * Production implementation: spawn `/usr/bin/security` and read exit code + streams. + * Kept package-private so tests can also reach it if they want to run the real path + * on a mac host, but production always goes through the [SecureKeyStorage.macSecurityLookup] + * indirection. + */ +internal fun defaultMacSecurityLookup( + service: String, + account: String, +): MacSecurityResult { + val process = + try { + ProcessBuilder( + "/usr/bin/security", + "find-generic-password", + "-s", + service, + "-a", + account, + "-w", + ).redirectErrorStream(false).start() + } catch (e: Exception) { + return MacSecurityResult.Ambiguous(-1, "failed to spawn /usr/bin/security: ${e.message ?: e::class.simpleName ?: "unknown"}") + } + process.outputStream.close() + val stdout = process.inputStream.bufferedReader().use { it.readText() } + val stderr = process.errorStream.bufferedReader().use { it.readText() } + val exitCode = process.waitFor() + return parseMacSecurityFindResult(exitCode, stdout, stderr) +} + internal class RealKeyringHandle( private val keyring: Keyring, ) : KeyringHandle { diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorageOrThrowTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorageOrThrowTest.kt new file mode 100644 index 0000000000..980d6c718f --- /dev/null +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorageOrThrowTest.kt @@ -0,0 +1,212 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.commons.keystorage + +import com.github.javakeyring.PasswordAccessException +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Assert.fail +import org.junit.Test + +/** + * Unit tests for the strict `getPrivateKeyOrThrow` lookup and its pure macOS + * `/usr/bin/security` exit-code parser. + * + * These tests must never touch the OS keychain and must run on any host, so: + * - the macOS integration paths are gated behind [MacSecurityResult] stubs + * injected via `SecureKeyStorage.macSecurityLookup`; + * - the parser test operates on captured stdout/stderr/exitCode triples; + * - non-mac backends are exercised via the `KeyringHandle` test seam already + * used by [SecureKeyStorageKeyringCacheTest]. + */ +class SecureKeyStorageOrThrowTest { + private class ExplodingKeyring( + private val onGet: () -> Nothing, + ) : KeyringHandle { + override fun getPassword( + service: String, + account: String, + ): String = onGet() + + override fun setPassword( + service: String, + account: String, + password: String, + ) { + // unused in these tests + } + + override fun deletePassword( + service: String, + account: String, + ) { + // unused in these tests + } + } + + private class StaticKeyring( + private val map: Map, String>, + ) : KeyringHandle { + override fun getPassword( + service: String, + account: String, + ): String = map[service to account] ?: throw PasswordAccessException("no entry") + + override fun setPassword( + service: String, + account: String, + password: String, + ) {} + + override fun deletePassword( + service: String, + account: String, + ) {} + } + + // --- macOS security(1) parser --- + + @Test + fun `parser exit 0 returns Found with trimmed password`() { + val result = parseMacSecurityFindResult(0, "hunter2\n", "") + assertTrue(result is MacSecurityResult.Found) + assertEquals("hunter2", (result as MacSecurityResult.Found).password) + } + + @Test + fun `parser exit 0 preserves internal newlines and only strips trailing`() { + val result = parseMacSecurityFindResult(0, "line1\nline2\n", "") + assertEquals("line1\nline2", (result as MacSecurityResult.Found).password) + } + + @Test + fun `parser exit 44 returns NotFound`() { + val result = + parseMacSecurityFindResult( + 44, + "", + "security: SecKeychainSearchCopyNext: The specified item could not be found in the keychain.\n", + ) + assertTrue(result is MacSecurityResult.NotFound) + } + + @Test + fun `parser exit 128 flagged as user-cancelled`() { + val result = parseMacSecurityFindResult(128, "", "security: dismissed\n") + assertTrue(result is MacSecurityResult.Ambiguous) + val ambig = result as MacSecurityResult.Ambiguous + assertEquals(128, ambig.exitCode) + assertTrue(ambig.reason.contains("cancel")) + } + + @Test + fun `parser stderr -25293 mapped to errSecAuthFailed`() { + val result = parseMacSecurityFindResult(51, "", "security: SecKeychainItemCopyContent (-25293)\n") + val ambig = result as MacSecurityResult.Ambiguous + assertEquals("errSecAuthFailed", ambig.reason) + } + + @Test + fun `parser unknown exit falls back to first stderr line`() { + val result = parseMacSecurityFindResult(9999, "", "security: mystery: line 1\nline 2\n") + val ambig = result as MacSecurityResult.Ambiguous + assertEquals("security: mystery: line 1", ambig.reason) + } + + // --- getPrivateKeyOrThrow: strict semantics via injected macOS lookup --- + // (Enabled unconditionally: the macSecurityLookup indirection is exercised + // via a stub, so no `security` binary is invoked. The `isMacOs()` check + // means this test only takes the mac path on macOS runners; on Linux it + // takes the javakeyring path, which we validate separately below.) + + private fun newStorage(): SecureKeyStorage = SecureKeyStorage.create() + + @Test + fun `mac lookup Found returns password without ambiguity`() = + runBlocking { + if (!System.getProperty("os.name").orEmpty().startsWith("Mac")) return@runBlocking + val storage = newStorage() + storage.macSecurityLookup = { _, _ -> MacSecurityResult.Found("secretval") } + assertEquals("secretval", storage.getPrivateKeyOrThrow("account-metadata-key")) + } + + @Test + fun `mac lookup NotFound returns null`() = + runBlocking { + if (!System.getProperty("os.name").orEmpty().startsWith("Mac")) return@runBlocking + val storage = newStorage() + storage.macSecurityLookup = { _, _ -> MacSecurityResult.NotFound } + assertNull(storage.getPrivateKeyOrThrow("account-metadata-key")) + } + + @Test + fun `mac lookup Ambiguous throws SecureStorageException with reason`() = + runBlocking { + if (!System.getProperty("os.name").orEmpty().startsWith("Mac")) return@runBlocking + val storage = newStorage() + storage.macSecurityLookup = { _, _ -> + MacSecurityResult.Ambiguous(128, "user cancelled Keychain dialog") + } + try { + storage.getPrivateKeyOrThrow("account-metadata-key") + fail("Expected SecureStorageException") + } catch (e: SecureStorageException) { + assertTrue(e.message?.contains("user cancelled") == true) + assertTrue(e.message?.contains("128") == true) + } + } + + // --- non-mac backend: PasswordAccessException must throw, never null --- + + @Test + fun `non-mac keyring PasswordAccessException throws not returns null`() = + runBlocking { + if (System.getProperty("os.name").orEmpty().startsWith("Mac")) return@runBlocking + val storage = newStorage() + storage.keyringFactory = { ExplodingKeyring { throw PasswordAccessException("locked") } } + try { + storage.getPrivateKeyOrThrow("account-metadata-key") + fail("Expected SecureStorageException") + } catch (e: SecureStorageException) { + assertTrue( + e.message?.contains("ambiguous", ignoreCase = true) == true || + e.message?.contains("refused", ignoreCase = true) == true, + ) + } + } + + @Test + fun `non-mac keyring hit returns password`() = + runBlocking { + if (System.getProperty("os.name").orEmpty().startsWith("Mac")) return@runBlocking + val storage = newStorage() + storage.keyringFactory = { + StaticKeyring( + mapOf( + ("amethyst-desktop" to "account-metadata-key") to "abc123", + ), + ) + } + assertEquals("abc123", storage.getPrivateKeyOrThrow("account-metadata-key")) + } +} diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt index 3f6ff53ec2..9ef274a2ad 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt @@ -28,7 +28,10 @@ import com.vitorpamplona.amethyst.commons.model.account.AccountStorage import com.vitorpamplona.amethyst.commons.model.account.SignerType import com.vitorpamplona.amethyst.commons.util.deleteOrWarn import com.vitorpamplona.quartz.utils.Log +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import java.io.File +import java.io.RandomAccessFile import java.nio.file.Files import java.nio.file.StandardCopyOption import java.nio.file.attribute.PosixFilePermission @@ -58,6 +61,16 @@ sealed class StorageCorruption( class JsonMalformed( backupPath: String?, ) : StorageCorruption(backupPath) + + /** + * A transient failure surfaced from the read path (I/O error, keychain refused + * or otherwise ambiguous access, OOM, etc). No backup was written and the + * on-disk file is untouched. Callers should retry or surface an error UI rather + * than treating this as data loss. See [DesktopAccountStorage.readMetadataFromDisk]. + */ + class TransientError( + val cause: Throwable, + ) : StorageCorruption(backupPath = null) } class DesktopAccountStorage( @@ -68,6 +81,7 @@ class DesktopAccountStorage( companion object { private const val METADATA_KEY_ALIAS = "account-metadata-key" private const val ACCOUNTS_FILE = "accounts.json.enc" + private const val ACCOUNTS_LOCK_FILE = "accounts.json.enc.lock" private const val AES_KEY_SIZE = 32 // 256 bits private const val GCM_IV_SIZE = 12 private const val GCM_TAG_BITS = 128 @@ -76,38 +90,51 @@ class DesktopAccountStorage( private val mapper = jacksonObjectMapper() private val amethystDir by lazy { File(homeDir, ".amethyst") } - // In-memory cache — read from disk once, then serve from memory + // In-memory cache: read from disk once, then serve from memory private var cachedMetadata: AccountMetadata? = null + // In-process mutex around the cross-process file lock. Two callers inside + // the same JVM would otherwise fail with OverlappingFileLockException from + // FileChannel.lock(), since JVM file locks are per-JVM not per-thread. + private val fileLockMutex = Mutex() + + // Guards read-modify-write cycles on [cachedMetadata]. Distinct from + // [fileLockMutex] so we can hold it across a full read + mutate + write + // sequence (the file lock is taken and released inside each disk op). + private val stateMutex = Mutex() + // --- AccountStorage interface --- override suspend fun loadAccounts(): List = getCachedMetadata().accounts.map { it.toAccountInfo() } - override suspend fun saveAccount(info: AccountInfo) { - val metadata = getCachedMetadata() - val dto = AccountInfoDto.from(info) - val updated = metadata.accounts.filter { it.npub != info.npub } + dto - writeCachedMetadata(metadata.copy(accounts = updated)) - } + override suspend fun saveAccount(info: AccountInfo) = + stateMutex.withLock { + val metadata = getCachedMetadata() + val dto = AccountInfoDto.from(info) + val updated = metadata.accounts.filter { it.npub != info.npub } + dto + writeCachedMetadata(metadata.copy(accounts = updated)) + } - override suspend fun deleteAccount(npub: String) { - val metadata = getCachedMetadata() - val updated = metadata.accounts.filter { it.npub != npub } - val newActive = - if (metadata.activeNpub == npub) { - updated.firstOrNull()?.npub - } else { - metadata.activeNpub - } - writeCachedMetadata(metadata.copy(accounts = updated, activeNpub = newActive)) - } + override suspend fun deleteAccount(npub: String) = + stateMutex.withLock { + val metadata = getCachedMetadata() + val updated = metadata.accounts.filter { it.npub != npub } + val newActive = + if (metadata.activeNpub == npub) { + updated.firstOrNull()?.npub + } else { + metadata.activeNpub + } + writeCachedMetadata(metadata.copy(accounts = updated, activeNpub = newActive)) + } override suspend fun currentAccount(): String? = getCachedMetadata().activeNpub - override suspend fun setCurrentAccount(npub: String) { - val metadata = getCachedMetadata() - writeCachedMetadata(metadata.copy(activeNpub = npub)) - } + override suspend fun setCurrentAccount(npub: String) = + stateMutex.withLock { + val metadata = getCachedMetadata() + writeCachedMetadata(metadata.copy(activeNpub = npub)) + } // --- Cached I/O --- @@ -129,9 +156,17 @@ class DesktopAccountStorage( val file = getAccountsFile() if (!file.exists()) return AccountMetadata() + ensureDir() + return withAccountsFileLock { + readMetadataFromDiskLocked(file) + } + } + + private suspend fun readMetadataFromDiskLocked(file: File): AccountMetadata { val encrypted = file.readBytes() if (encrypted.size < GCM_IV_SIZE) { - val backup = backupCorruptFile(file) + // Genuinely unusable: not enough bytes for the IV. Back up and reset. + val backup = backupCorruptFile(file, ".corrupt") onCorruption(StorageCorruption.FileCorrupted(backup)) return AccountMetadata() } @@ -140,31 +175,43 @@ class DesktopAccountStorage( val decrypted = decrypt(encrypted) mapper.readValue(decrypted) } catch (e: javax.crypto.AEADBadTagException) { - Log.e("DesktopAccountStorage", "GCM auth tag mismatch — file corrupted or key lost", e) - val backup = backupCorruptFile(file) + // Genuine ciphertext corruption or lost/rotated AES key. + Log.e("DesktopAccountStorage", "GCM auth tag mismatch, file corrupted or key lost", e) + val backup = backupCorruptFile(file, ".corrupt") onCorruption(StorageCorruption.FileCorrupted(backup)) AccountMetadata() } catch (e: javax.crypto.BadPaddingException) { - Log.e("DesktopAccountStorage", "Decryption failed — file corrupted", e) - val backup = backupCorruptFile(file) + // Genuine ciphertext corruption. + Log.e("DesktopAccountStorage", "Decryption failed, file corrupted", e) + val backup = backupCorruptFile(file, ".corrupt") onCorruption(StorageCorruption.FileCorrupted(backup)) AccountMetadata() } catch (e: com.fasterxml.jackson.core.JacksonException) { + // Schema mismatch: decrypted cleanly but the JSON does not fit our shape. + // Distinct suffix so operators can tell it apart from ciphertext corruption. Log.e("DesktopAccountStorage", "JSON malformed after decryption", e) - val backup = backupCorruptFile(file) + val backup = backupCorruptFile(file, ".jsonerror") onCorruption(StorageCorruption.JsonMalformed(backup)) AccountMetadata() + } catch (e: kotlin.coroutines.cancellation.CancellationException) { + throw e } catch (e: Exception) { - Log.e("DesktopAccountStorage", "Failed to read accounts metadata", e) - val backup = backupCorruptFile(file) - onCorruption(StorageCorruption.FileCorrupted(backup)) - AccountMetadata() + // Transient failure: I/O error, keychain refused / ambiguous, OOM, etc. + // DO NOT rename the on-disk file; the ciphertext is intact and the next + // launch may succeed (for example after the user re-approves the + // Keychain Access prompt). Surface up for the caller to decide. + Log.e("DesktopAccountStorage", "Transient error reading accounts metadata; file preserved", e) + onCorruption(StorageCorruption.TransientError(e)) + throw e } } - private fun backupCorruptFile(file: File): String? = + private fun backupCorruptFile( + file: File, + suffix: String, + ): String? = try { - val backup = File(file.parent, "accounts.json.enc.corrupt.${System.currentTimeMillis()}") + val backup = File(file.parent, "${file.name}$suffix.${System.currentTimeMillis()}") java.nio.file.Files .copy(file.toPath(), backup.toPath()) file.deleteOrWarn("DesktopAccountStorage", "corrupt accounts file") @@ -178,31 +225,78 @@ class DesktopAccountStorage( val json = mapper.writeValueAsBytes(metadata) val encrypted = encrypt(json) - // Atomic write via temp file val file = getAccountsFile() - val temp = File(amethystDir, "${ACCOUNTS_FILE}.tmp") - temp.writeBytes(encrypted) - Files.move(temp.toPath(), file.toPath(), StandardCopyOption.REPLACE_EXISTING) - - setFilePermissions(file) + withAccountsFileLock { + // Atomic write via temp file, under the cross-process lock so two + // Amethyst instances (Homebrew upgrade race, accidental double-launch) + // cannot interleave writes and truncate the file. + val temp = File(amethystDir, "$ACCOUNTS_FILE.tmp") + temp.writeBytes(encrypted) + Files.move( + temp.toPath(), + file.toPath(), + StandardCopyOption.REPLACE_EXISTING, + StandardCopyOption.ATOMIC_MOVE, + ) + setFilePermissions(file) + } } + /** + * Cross-process advisory lock + in-process mutex around the accounts.json.enc + * read/write critical section. The mutex is required because JVM + * `FileChannel.lock()` is a per-JVM lock and would throw + * `OverlappingFileLockException` on the second acquire from the same JVM. + * The channel lock is required to keep two Amethyst processes serial (upgrade + * race, accidental double-launch, cron-style relaunch). + * + * Mirrors the pattern used in SecureKeyStorage.withFileLock; kept private + * to this class so the two lock lifecycles stay independent. + */ + private suspend inline fun withAccountsFileLock(crossinline block: suspend () -> T): T = + fileLockMutex.withLock { + val lockFile = File(amethystDir, ACCOUNTS_LOCK_FILE) + if (!lockFile.exists()) { + lockFile.createNewFile() + setFilePermissions(lockFile) + } + RandomAccessFile(lockFile, "rw").use { raf -> + raf.channel.lock().use { _ -> + block() + } + } + } + private fun getAccountsFile() = File(amethystDir, ACCOUNTS_FILE) // --- AES-256-GCM encryption --- private var cachedKey: ByteArray? = null + /** + * Reads (or creates on first launch) the metadata AES key. + * + * Distinguishes: + * - key exists in keychain: use it + * - keychain confirms definitively absent: generate + persist a fresh key + * - any other outcome (user cancelled/denied prompt, keychain locked, + * backend transient error): propagate the exception, do NOT rotate. + * + * Rotating the AES key on an ambiguous miss silently destroys the ability + * to decrypt the existing accounts.json.enc, wiping the logged-in accounts + * on next launch. That is the bug this method exists to prevent. + */ private suspend fun getOrCreateKey(): ByteArray { cachedKey?.let { return it } - val existing = secureStorage.getPrivateKey(METADATA_KEY_ALIAS) + val existing = secureStorage.getPrivateKeyOrThrow(METADATA_KEY_ALIAS) if (existing != null) { val key = Base64.getDecoder().decode(existing) cachedKey = key return key } + // Definitively absent: safe to create and persist a fresh key. val key = ByteArray(AES_KEY_SIZE).also { SecureRandom().nextBytes(it) } secureStorage.savePrivateKey(METADATA_KEY_ALIAS, Base64.getEncoder().encodeToString(key)) cachedKey = key diff --git a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerKeyLoginTest.kt b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerKeyLoginTest.kt index 4d511c57f7..6059f961fe 100644 --- a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerKeyLoginTest.kt +++ b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerKeyLoginTest.kt @@ -58,6 +58,7 @@ class AccountManagerKeyLoginTest { val keySlot = slot() val valueSlot = slot() coEvery { storage.getPrivateKey(capture(keySlot)) } answers { keyStore[keySlot.captured] } + coEvery { storage.getPrivateKeyOrThrow(capture(keySlot)) } answers { keyStore[keySlot.captured] } coEvery { storage.savePrivateKey(capture(keySlot), capture(valueSlot)) } answers { keyStore[keySlot.captured] = valueSlot.captured } diff --git a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerLoadAccountTest.kt b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerLoadAccountTest.kt index 8b01b71677..5aad247a29 100644 --- a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerLoadAccountTest.kt +++ b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerLoadAccountTest.kt @@ -49,6 +49,7 @@ class AccountManagerLoadAccountTest { storage = mockk(relaxed = true) // Return null so DesktopAccountStorage generates a fresh AES key coEvery { storage.getPrivateKey("account-metadata-key") } returns null + coEvery { storage.getPrivateKeyOrThrow("account-metadata-key") } returns null tempDir = createTempDirectory("acctmgr-load-test").toFile() amethystDir = File(tempDir, ".amethyst") amethystDir.mkdirs() diff --git a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerLoadStateTransitionsTest.kt b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerLoadStateTransitionsTest.kt index f98a4c3407..602016ed5f 100644 --- a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerLoadStateTransitionsTest.kt +++ b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerLoadStateTransitionsTest.kt @@ -63,6 +63,7 @@ class AccountManagerLoadStateTransitionsTest { fun setup() { storage = mockk(relaxed = true) coEvery { storage.getPrivateKey("account-metadata-key") } returns null + coEvery { storage.getPrivateKeyOrThrow("account-metadata-key") } returns null tempDir = createTempDirectory("acctmgr-load-state").toFile() File(tempDir, ".amethyst").mkdirs() manager = AccountManager(storage, tempDir) diff --git a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerLogoutTest.kt b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerLogoutTest.kt index 2f74a42390..741b79044d 100644 --- a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerLogoutTest.kt +++ b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerLogoutTest.kt @@ -46,6 +46,7 @@ class AccountManagerLogoutTest { fun setup() { storage = mockk(relaxed = true) coEvery { storage.getPrivateKey("account-metadata-key") } returns null + coEvery { storage.getPrivateKeyOrThrow("account-metadata-key") } returns null tempDir = createTempDirectory("acctmgr-logout-test").toFile() manager = AccountManager(storage, tempDir) } diff --git a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerNip46IsolationTest.kt b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerNip46IsolationTest.kt index f2688a2819..6cb2a59e15 100644 --- a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerNip46IsolationTest.kt +++ b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerNip46IsolationTest.kt @@ -56,6 +56,7 @@ class AccountManagerNip46IsolationTest { fun setup() { storage = mockk(relaxed = true) coEvery { storage.getPrivateKey("account-metadata-key") } returns null + coEvery { storage.getPrivateKeyOrThrow("account-metadata-key") } returns null tempDir = createTempDirectory("acctmgr-nip46-iso-test").toFile() amethystDir = File(tempDir, ".amethyst") amethystDir.mkdirs() diff --git a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerStateTransitionTest.kt b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerStateTransitionTest.kt index 6734d49793..62e5eb08c0 100644 --- a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerStateTransitionTest.kt +++ b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerStateTransitionTest.kt @@ -56,6 +56,7 @@ class AccountManagerStateTransitionTest { fun setup() { storage = mockk(relaxed = true) coEvery { storage.getPrivateKey("account-metadata-key") } returns null + coEvery { storage.getPrivateKeyOrThrow("account-metadata-key") } returns null tempDir = createTempDirectory("acctmgr-state-test").toFile() amethystDir = File(tempDir, ".amethyst") amethystDir.mkdirs() diff --git a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorageTest.kt b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorageTest.kt index 9ffd9ac1b3..c1c4a88993 100644 --- a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorageTest.kt +++ b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorageTest.kt @@ -21,18 +21,28 @@ package com.vitorpamplona.amethyst.desktop.account import com.vitorpamplona.amethyst.commons.keystorage.SecureKeyStorage +import com.vitorpamplona.amethyst.commons.keystorage.SecureStorageException import com.vitorpamplona.amethyst.commons.model.account.AccountInfo import com.vitorpamplona.amethyst.commons.model.account.SignerType import io.mockk.coEvery import io.mockk.coVerify import io.mockk.mockk import io.mockk.slot +import kotlinx.coroutines.asCoroutineDispatcher +import kotlinx.coroutines.async +import kotlinx.coroutines.awaitAll +import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.withContext import java.io.File +import java.util.concurrent.Executors import kotlin.test.AfterTest import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFails +import kotlin.test.assertFalse +import kotlin.test.assertNotNull import kotlin.test.assertNull import kotlin.test.assertTrue @@ -56,6 +66,9 @@ class DesktopAccountStorageTest { coEvery { secureStorage.getPrivateKey(capture(keySlot)) } answers { keyStore[keySlot.captured] } + coEvery { secureStorage.getPrivateKeyOrThrow(capture(keySlot)) } answers { + keyStore[keySlot.captured] + } coEvery { secureStorage.savePrivateKey(capture(keySlot), capture(valueSlot)) } answers { keyStore[keySlot.captured] = valueSlot.captured } @@ -197,4 +210,208 @@ class DesktopAccountStorageTest { // Encrypted content should NOT contain the npub in plaintext assertTrue(!content.contains("npub1secret")) } + + // --- Bug 1: silent AES key rotation --- + + @Test + fun `getOrCreateKey keyring throws ambiguous error does not rotate key or touch file`() = + runTest { + // First launch: seed a real metadata key + an existing accounts.json.enc + storage.saveAccount(AccountInfo("npub1existing", SignerType.Internal)) + val file = File(File(tempDir, ".amethyst"), "accounts.json.enc") + val originalBytes = file.readBytes() + val originalMetadataKey = keyStore["account-metadata-key"] + assertNotNull(originalMetadataKey) + + // Fresh storage instance simulating a relaunch: the keychain now + // returns an ambiguous error (user cancelled the Keychain dialog). + val throwingStorage: SecureKeyStorage = mockk() + coEvery { throwingStorage.getPrivateKeyOrThrow("account-metadata-key") } throws + SecureStorageException("user cancelled Keychain dialog") + // Legacy permissive read should still return the key; production + // must not fall back to it on the getOrCreate path. + coEvery { throwingStorage.getPrivateKey(any()) } answers { keyStore[firstArg()] } + coEvery { throwingStorage.hasPrivateKey(any()) } answers { keyStore.containsKey(firstArg()) } + + val relaunched = DesktopAccountStorage(throwingStorage, tempDir) + + // Any operation that needs the metadata key must fail loudly, not + // rotate the key or write a fresh empty file. + assertFails { runBlocking { relaunched.loadAccounts() } } + + // Bug 1 invariant: no new savePrivateKey call for the metadata key. + coVerify(exactly = 0) { + throwingStorage.savePrivateKey("account-metadata-key", any()) + } + // Bug 1 + Bug 2 invariant: on-disk ciphertext untouched. + assertTrue(file.exists()) + assertContentEquals(originalBytes, file.readBytes()) + // Bug 1 invariant: keyStore metadata key unchanged. + assertEquals(originalMetadataKey, keyStore["account-metadata-key"]) + } + + @Test + fun `getOrCreateKey keyring returns definitive not-found creates and persists new key`() = + runTest { + // Happy path first launch: getPrivateKeyOrThrow returns null, + // storage generates + persists a fresh AES key exactly once. + assertNull(keyStore["account-metadata-key"]) + + storage.saveAccount(AccountInfo("npub1first", SignerType.Internal)) + + assertNotNull(keyStore["account-metadata-key"]) + coVerify(exactly = 1) { + secureStorage.savePrivateKey("account-metadata-key", any()) + } + } + + // --- Bug 2: read failure must not silently reset the file --- + + @Test + fun `readMetadataFromDisk transient IO error does not backup file`() = + runTest { + // Seed a real file we can inspect. + storage.saveAccount(AccountInfo("npub1existing", SignerType.Internal)) + val amethystDir = File(tempDir, ".amethyst") + val file = File(amethystDir, "accounts.json.enc") + val originalBytes = file.readBytes() + val originalName = file.name + + // Fresh storage that surfaces a transient error from the keychain. + val throwingStorage: SecureKeyStorage = mockk() + coEvery { throwingStorage.getPrivateKeyOrThrow("account-metadata-key") } throws + SecureStorageException("transient keychain error") + coEvery { throwingStorage.getPrivateKey(any()) } returns null + coEvery { throwingStorage.hasPrivateKey(any()) } returns false + + val corruptions = mutableListOf() + val relaunched = + DesktopAccountStorage(throwingStorage, tempDir, onCorruption = { corruptions += it }) + + assertFails { runBlocking { relaunched.loadAccounts() } } + + // File preserved, no .corrupt.* or .jsonerror.* sibling created. + assertTrue(file.exists()) + assertContentEquals(originalBytes, file.readBytes()) + val siblings = amethystDir.listFiles().orEmpty().map { it.name } + assertFalse(siblings.any { it != originalName && it.startsWith("accounts.json.enc") && (it.contains(".corrupt.") || it.contains(".jsonerror.")) }) + + // The callback fired with the transient subtype so the app can retry. + assertTrue(corruptions.any { it is StorageCorruption.TransientError }) + } + + @Test + fun `readMetadataFromDisk gcm tag mismatch backs up and resets`() = + runTest { + // Seed a valid file so we have a real metadata key in the mock keystore. + storage.saveAccount(AccountInfo("npub1a", SignerType.Internal)) + val amethystDir = File(tempDir, ".amethyst") + val file = File(amethystDir, "accounts.json.enc") + assertTrue(file.exists()) + + // Overwrite with random bytes that pass the length check but fail + // GCM auth tag verification. Prefix with a fresh IV, then garbage. + val garbage = ByteArray(64) { it.toByte() } + file.writeBytes(garbage) + + val corruptions = mutableListOf() + val relaunched = + DesktopAccountStorage(secureStorage, tempDir, onCorruption = { corruptions += it }) + + val loaded = relaunched.loadAccounts() + assertTrue(loaded.isEmpty()) + + // Backup exists with the .corrupt. suffix; original file was + // removed (and will be re-created on next save). + val siblings = amethystDir.listFiles().orEmpty().map { it.name } + assertTrue(siblings.any { it.startsWith("accounts.json.enc.corrupt.") }) + assertTrue(corruptions.any { it is StorageCorruption.FileCorrupted }) + } + + @Test + fun `readMetadataFromDisk json malformed uses jsonerror suffix`() = + runTest { + // Build an accounts.json.enc whose plaintext decrypts fine but is + // not the expected AccountMetadata shape. Easiest path: reuse the + // production encrypt via a lightweight helper storage that lets us + // control the plaintext. + storage.saveAccount(AccountInfo("npub1a", SignerType.Internal)) + val amethystDir = File(tempDir, ".amethyst") + val file = File(amethystDir, "accounts.json.enc") + + // Encrypt an unrelated JSON payload with the same AES key the mock + // keystore holds so decryption succeeds but Jackson rejects the shape. + val key = + java.util.Base64 + .getDecoder() + .decode(keyStore["account-metadata-key"]!!) + val iv = ByteArray(12) { 7 } + val cipher = javax.crypto.Cipher.getInstance("AES/GCM/NoPadding") + cipher.init( + javax.crypto.Cipher.ENCRYPT_MODE, + javax.crypto.spec.SecretKeySpec(key, "AES"), + javax.crypto.spec.GCMParameterSpec(128, iv), + ) + val badPayload = cipher.doFinal("\"not an object\"".toByteArray()) + file.writeBytes(iv + badPayload) + + val corruptions = mutableListOf() + val relaunched = + DesktopAccountStorage(secureStorage, tempDir, onCorruption = { corruptions += it }) + + val loaded = relaunched.loadAccounts() + assertTrue(loaded.isEmpty()) + + val siblings = amethystDir.listFiles().orEmpty().map { it.name } + assertTrue(siblings.any { it.startsWith("accounts.json.enc.jsonerror.") }) + assertTrue(corruptions.any { it is StorageCorruption.JsonMalformed }) + } + + // --- Bug 3: cross-process file lock --- + + @Test + fun `writeMetadataToDisk concurrent saves serialize under file lock`() { + val executor = Executors.newFixedThreadPool(4) + try { + runBlocking { + withContext(executor.asCoroutineDispatcher()) { + val jobs = + (1..8).map { idx -> + async { + storage.saveAccount( + AccountInfo( + npub = "npub1parallel$idx", + signerType = SignerType.Internal, + ), + ) + } + } + jobs.awaitAll() + } + } + // All eight accounts present, file not truncated. + val loaded = runBlocking { storage.loadAccounts() } + assertEquals(8, loaded.size) + val npubs = loaded.map { it.npub }.toSet() + assertEquals((1..8).map { "npub1parallel$it" }.toSet(), npubs) + + // Lock sidecar exists and is respected. + val lockFile = File(File(tempDir, ".amethyst"), "accounts.json.enc.lock") + assertTrue(lockFile.exists()) + } finally { + executor.shutdownNow() + } + } + + private fun assertContentEquals( + expected: ByteArray, + actual: ByteArray, + ) { + assertEquals(expected.size, actual.size, "byte size mismatch") + for (i in expected.indices) { + if (expected[i] != actual[i]) { + throw AssertionError("byte differs at index $i: expected=${expected[i]} actual=${actual[i]}") + } + } + } } diff --git a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/benchmark/LaunchScenario.kt b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/benchmark/LaunchScenario.kt index 7edddfa185..3b017fff94 100644 --- a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/benchmark/LaunchScenario.kt +++ b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/benchmark/LaunchScenario.kt @@ -95,6 +95,7 @@ object LaunchScenario { val tempHome = createTempDirectory("launch-scenario").toFile() val storage = mockk(relaxed = true) coEvery { storage.getPrivateKey(any()) } returns null + coEvery { storage.getPrivateKeyOrThrow(any()) } returns null File(tempHome, ".amethyst").mkdirs() val account = AccountManager(storage, tempHome) diff --git a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/ui/AppStateMachineTest.kt b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/ui/AppStateMachineTest.kt index e645ca4cfc..cd98c7ea9c 100644 --- a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/ui/AppStateMachineTest.kt +++ b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/ui/AppStateMachineTest.kt @@ -89,6 +89,7 @@ class AppStateMachineTest { File(tempDir, ".amethyst").mkdirs() storage = mockk(relaxed = true) coEvery { storage.getPrivateKey(any()) } returns null + coEvery { storage.getPrivateKeyOrThrow(any()) } returns null harnessScope = CoroutineScope(Dispatchers.Default + SupervisorJob()) relay = LaunchFixtureRelay.open(LaunchFixture.build(noteCount = 0).events) } From 1ea820e699543db083c44c97432ddb3d10c22db7 Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Sat, 12 Sep 2026 17:43:12 -0400 Subject: [PATCH 7/7] fix(desktop): allow first-launch key bootstrap and stop caching unwritten state Two defects found reviewing the strict-keychain fix. 1. Fresh Linux/Windows installs could never persist an account. getPrivateKeyOrThrow turns any PasswordAccessException into a SecureStorageException on non-macOS backends, but every backend java-keyring ships throws that exact exception for a *genuinely absent* credential: - WinCredentialStoreBackend: CredReadA false (ERROR_NOT_FOUND) -> throw - FreedesktopKeyringBackend: empty object paths -> throwNoExistingCredentialException - KWalletBackend: hasEntry false -> "Password is not in wallet" So the strict lookup structurally cannot report "definitively absent" there, the create branch in getOrCreateKey was unreachable, and nothing between it and AccountManager.addAccountToStorage catches the throw. getOrCreateKey now bootstraps a fresh key when the strict lookup fails *and* accounts.json.enc does not exist. With no ciphertext on disk there is nothing a new key can orphan, so the invariant the strict contract protects is untouched: once the file exists the exception propagates exactly as before. 2. writeCachedMetadata updated the in-memory cache before the disk write, so a failed write (keychain refusal, I/O error, disk full) left the session serving accounts that were never persisted -- a save that reported success and vanished on the next launch. Persist first, cache second. Both are pinned by new tests, and both were mutation-checked: reverting either fix fails exactly one of them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VgVDQQXAg4cmzsWHoJj61k --- .../desktop/account/DesktopAccountStorage.kt | 42 +++++++++++-- .../account/DesktopAccountStorageTest.kt | 61 +++++++++++++++++++ 2 files changed, 99 insertions(+), 4 deletions(-) diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt index 9ef274a2ad..e1813ff197 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt @@ -23,6 +23,7 @@ package com.vitorpamplona.amethyst.desktop.account import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper import com.fasterxml.jackson.module.kotlin.readValue import com.vitorpamplona.amethyst.commons.keystorage.SecureKeyStorage +import com.vitorpamplona.amethyst.commons.keystorage.SecureStorageException import com.vitorpamplona.amethyst.commons.model.account.AccountInfo import com.vitorpamplona.amethyst.commons.model.account.AccountStorage import com.vitorpamplona.amethyst.commons.model.account.SignerType @@ -145,9 +146,17 @@ class DesktopAccountStorage( return loaded } + /** + * Persists first, caches second. + * + * If the disk write fails (keychain refused, I/O error, disk full) the in-memory + * cache must NOT be left claiming a state that was never written: the rest of the + * session would serve accounts that vanish on the next launch, and the user would + * see a successful save that silently did nothing. + */ private suspend fun writeCachedMetadata(metadata: AccountMetadata) { - cachedMetadata = metadata writeMetadataToDisk(metadata) + cachedMetadata = metadata } // --- Encrypted file I/O --- @@ -280,7 +289,9 @@ class DesktopAccountStorage( * - key exists in keychain: use it * - keychain confirms definitively absent: generate + persist a fresh key * - any other outcome (user cancelled/denied prompt, keychain locked, - * backend transient error): propagate the exception, do NOT rotate. + * backend transient error): propagate the exception, do NOT rotate -- + * unless there is no accounts.json.enc yet, in which case there is no + * ciphertext to orphan and we bootstrap a fresh key (see below). * * Rotating the AES key on an ambiguous miss silently destroys the ability * to decrypt the existing accounts.json.enc, wiping the logged-in accounts @@ -289,14 +300,37 @@ class DesktopAccountStorage( private suspend fun getOrCreateKey(): ByteArray { cachedKey?.let { return it } - val existing = secureStorage.getPrivateKeyOrThrow(METADATA_KEY_ALIAS) + val existing = + try { + secureStorage.getPrivateKeyOrThrow(METADATA_KEY_ALIAS) + } catch (e: SecureStorageException) { + // Bootstrap escape. Every non-macOS backend java-keyring ships + // (Windows Credential Store, Freedesktop Secret Service, KWallet) + // throws PasswordAccessException for a *genuinely absent* credential, + // so the strict lookup structurally cannot report "definitively + // absent" there. Without this branch a fresh Linux/Windows install + // could never mint the key and could never persist an account. + // + // Minting is only safe while there is no accounts.json.enc: with no + // ciphertext on disk there is nothing a new key can orphan. Once the + // file exists the strict contract applies and we propagate. + if (getAccountsFile().exists()) throw e + Log.w( + "DesktopAccountStorage", + "Keychain lookup failed and no accounts file exists; bootstrapping a fresh metadata key", + e, + ) + null + } + if (existing != null) { val key = Base64.getDecoder().decode(existing) cachedKey = key return key } - // Definitively absent: safe to create and persist a fresh key. + // Definitively absent (or bootstrapping with nothing on disk): safe to + // create and persist a fresh key. val key = ByteArray(AES_KEY_SIZE).also { SecureRandom().nextBytes(it) } secureStorage.savePrivateKey(METADATA_KEY_ALIAS, Base64.getEncoder().encodeToString(key)) cachedKey = key diff --git a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorageTest.kt b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorageTest.kt index c1c4a88993..a4d2e9a260 100644 --- a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorageTest.kt +++ b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorageTest.kt @@ -265,6 +265,67 @@ class DesktopAccountStorageTest { } } + @Test + fun `getOrCreateKey ambiguous error with no accounts file bootstraps a fresh key`() = + runTest { + // Every non-macOS backend java-keyring ships (Windows Credential Store, + // Freedesktop Secret Service, KWallet) throws PasswordAccessException for a + // *genuinely absent* credential, which the strict lookup surfaces as + // SecureStorageException. With no accounts.json.enc there is no ciphertext + // a new key could orphan, so a fresh install must still be able to mint one + // -- otherwise Linux/Windows can never persist an account at all. + val saved = mutableMapOf() + val throwingStorage: SecureKeyStorage = mockk() + coEvery { throwingStorage.getPrivateKeyOrThrow("account-metadata-key") } throws + SecureStorageException("Keyring backend refused access or returned ambiguous not-found") + coEvery { throwingStorage.savePrivateKey(any(), any()) } answers { + saved[firstArg()] = secondArg() + } + coEvery { throwingStorage.getPrivateKey(any()) } answers { saved[firstArg()] } + coEvery { throwingStorage.hasPrivateKey(any()) } answers { saved.containsKey(firstArg()) } + + val file = File(File(tempDir, ".amethyst"), "accounts.json.enc") + assertFalse(file.exists()) + + val fresh = DesktopAccountStorage(throwingStorage, tempDir) + fresh.saveAccount(AccountInfo("npub1freshinstall", SignerType.Internal)) + + assertNotNull(saved["account-metadata-key"]) + assertTrue(file.exists()) + assertEquals(listOf("npub1freshinstall"), fresh.loadAccounts().map { it.npub }) + // The escape is bootstrap-only: once the file exists the strict contract + // applies again -- pinned by `getOrCreateKey keyring throws ambiguous error + // does not rotate key or touch file` above. + } + + // --- Cache must never claim a state that was not persisted --- + + @Test + fun `failed disk write does not poison the in-memory cache`() = + runTest { + storage.saveAccount(AccountInfo("npub1persisted", SignerType.Internal)) + assertEquals(listOf("npub1persisted"), storage.loadAccounts().map { it.npub }) + + // Block the atomic-write temp path so writeMetadataToDisk fails. + val temp = File(File(tempDir, ".amethyst"), "accounts.json.enc.tmp") + assertTrue(temp.mkdirs()) + + assertFails { + runBlocking { + storage.saveAccount(AccountInfo("npub1phantom", SignerType.Internal)) + } + } + + // Same instance: the cache must still reflect only what reached the disk, + // not the account the failed save handed it. + assertEquals(listOf("npub1persisted"), storage.loadAccounts().map { it.npub }) + + // And the on-disk file agrees. + temp.delete() + val relaunched = DesktopAccountStorage(secureStorage, tempDir) + assertEquals(listOf("npub1persisted"), relaunched.loadAccounts().map { it.npub }) + } + // --- Bug 2: read failure must not silently reset the file --- @Test