From b1961ea19cd2bb72e347b565964ae4b26360fb92 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 29 May 2026 20:23:00 +0000 Subject: [PATCH] fix(zap): address reload-mint audit findings (double-submit, premature done, fee/poll robustness) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correctness: - ReloadMintViewModel.confirm() now guards against re-entry (only starts from Configuring/Failed and flips to Working synchronously), so a double-tap or a Failed-state Retry can't launch two pipelines that double-spend the source / double-mint the target. - sendNutzapAndFinish awaits the real CashuWalletState.sendNutzap (throws on failure) and reports Done only on success — a send that fails after a successful reload now surfaces as Failed instead of popping the screen on a premature "done" and silently stranding the moved funds. - The reload pipeline runs on the long-lived AccountViewModel scope; the VM now holds its Job and cancels it in onCleared(), so leaving the screen stops the (up to 3-minute) Lightning poll instead of hammering the mint unobserved. - rebalance() poll budget widened to a steady ~60s so a merely-slow mint no longer strands funds that already left the source. - Mint a small headroom buffer (RELOAD_FEE_BUFFER_SATS) above the bare shortfall so the follow-up nutzap's own swap fee doesn't leave the target a sat short; the source-feasibility gate accounts for it. Regressions from the settings merge: - mergeZapAmounts / the picker no longer .sorted() the amounts — a user's saved preset order is preserved instead of being silently reordered ascending. - The on-chain send dialog falls back to DEFAULT_ONCHAIN_ZAP_SATS when the unified list has nothing above the on-chain minimum, restoring the guaranteed quick-pick preset. - zapClick's one-tap (single-amount) path now checks rail capability and opens the picker when the recipient can't receive Lightning, instead of firing a doomed Lightning zap. Cleanup: - Centralized the mint-quote "settled" predicate as MintQuoteBolt11ResponseDto .isSettled(); rebalance, ReloadMintViewModel and CashuWalletViewModel now share it instead of three copies of paid==true || PAID || ISSUED. https://claude.ai/code/session_01HNE2z7CSYZ2G8KwC5fziJn --- .../amethyst/model/AccountSyncedSettings.kt | 2 +- .../model/AccountSyncedSettingsInternal.kt | 7 ++ .../model/nip60Cashu/CashuWalletState.kt | 13 ++- .../amethyst/ui/note/ReactionsRow.kt | 35 ++++-- .../loggedIn/wallet/CashuWalletViewModel.kt | 4 +- .../loggedIn/wallet/OnchainZapSendDialog.kt | 9 +- .../loggedIn/wallet/ReloadMintViewModel.kt | 109 +++++++++++++----- .../quartz/nip60Cashu/mintApi/MintApiDtos.kt | 10 +- 8 files changed, 140 insertions(+), 49 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountSyncedSettings.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountSyncedSettings.kt index e5f6367fe7..f10791a1d5 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountSyncedSettings.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountSyncedSettings.kt @@ -189,7 +189,7 @@ class AccountZapPreferences( * legacy `onchainZapAmountChoices`. Idempotent: [AccountSyncedSettings.toInternal] * re-derives the on-chain field as a subset of this list. */ -internal fun mergeZapAmounts(zaps: AccountZapPreferencesInternal): List = (zaps.zapAmountChoices + zaps.onchainZapAmountChoices).distinct().sorted() +internal fun mergeZapAmounts(zaps: AccountZapPreferencesInternal): List = (zaps.zapAmountChoices + zaps.onchainZapAmountChoices).distinct() @Stable class AccountLanguagePreferences( diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountSyncedSettingsInternal.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountSyncedSettingsInternal.kt index 8b8aa10b2b..bdc1ab134e 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountSyncedSettingsInternal.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountSyncedSettingsInternal.kt @@ -51,6 +51,13 @@ val DefaultReportWarningThreshold = 5 */ const val MIN_ONCHAIN_ZAP_SATS = 1_000L +/** + * Default on-chain quick-pick amount, used when the user's unified zap-amount + * list contains nothing at or above [MIN_ONCHAIN_ZAP_SATS]. Matches the legacy + * dedicated on-chain default so the send dialog always offers one preset. + */ +const val DEFAULT_ONCHAIN_ZAP_SATS = 5_000L + @Serializable enum class ReactionRowAction { Reply, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip60Cashu/CashuWalletState.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip60Cashu/CashuWalletState.kt index 6cc59d458e..9218037079 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip60Cashu/CashuWalletState.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/nip60Cashu/CashuWalletState.kt @@ -1209,16 +1209,19 @@ class CashuWalletState( meltToLightning(sourceMintUrl, meltQuote) // 4. The melt paid the invoice; confirm + issue proofs at the target. + // The melt settled synchronously, but a healthy mint can still lag a + // while before flipping the quote to PAID, so give it a generous + // ~60s steady budget rather than a tight escalating one — a merely + // slow mint shouldn't strand funds that have already left the source. onProgress?.invoke(0.75f) - val pollAttempts = 8 - val pollDelayMs = 1_000L + val pollAttempts = 30 + val pollDelayMs = 2_000L var paid = false var attempt = 0 while (!paid && attempt < pollAttempts) { - val status = ops.checkMintQuote(targetMintUrl, mintFlow.mintQuote.quote) - paid = status.paid == true || status.state == "PAID" || status.state == "ISSUED" + paid = ops.checkMintQuote(targetMintUrl, mintFlow.mintQuote.quote).isSettled() if (!paid) { - delay(pollDelayMs * (attempt + 1)) + delay(pollDelayMs) attempt++ } } 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 a5f0c19f0e..ece972c6f8 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 @@ -1387,17 +1387,26 @@ fun zapClick( } choices.size == 1 -> { - onZapStarts() - accountViewModel.zap( - baseNote, - choices.first() * 1000, - null, - "", - context, - onError = onError, - onProgress = { onZappingProgress(it) }, - onPayViaIntent = onPayViaIntent, - ) + // 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) + if (caps.hasLightning) { + onZapStarts() + accountViewModel.zap( + baseNote, + choices.first() * 1000, + null, + "", + context, + onError = onError, + onProgress = { onZappingProgress(it) }, + onPayViaIntent = onPayViaIntent, + ) + } else { + onMultipleChoices() + } } choices.size > 1 -> { @@ -1948,7 +1957,9 @@ fun ZapAmountChoicePopup( } val amountChoices = remember(zapAmountChoices) { - zapAmountChoices.distinct().sorted().toImmutableList() + // Keep the user's saved order (already de-duped at the settings + // layer); don't re-sort, that would override a deliberate ordering. + zapAmountChoices.distinct().toImmutableList() } val visibilityState = rememberVisibilityState(onDismiss) ZapAmountChoicePopup( diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/wallet/CashuWalletViewModel.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/wallet/CashuWalletViewModel.kt index bae29cdb97..63323e2f0a 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/wallet/CashuWalletViewModel.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/wallet/CashuWalletViewModel.kt @@ -426,7 +426,7 @@ class CashuWalletViewModel : ViewModel() { vm.launchSigner { try { val status = ops.checkMintQuote(current.mintUrl, current.flow.mintQuote.quote) - val paid = status.paid == true || status.state == "PAID" || status.state == "ISSUED" + val paid = status.isSettled() if (!paid) { // Roll back to AwaitingPayment so the polling // LaunchedEffect picks up again on the next tick. @@ -480,7 +480,7 @@ class CashuWalletViewModel : ViewModel() { val amountSats = runCatching { LnInvoiceUtil.getAmountInSats(status.request).toLong() } .getOrElse { 0L } - val paid = status.paid == true || status.state == "PAID" || status.state == "ISSUED" + val paid = status.isSettled() if (paid && amountSats > 0) { ops.completeMintFromLightning(mintUrl, quoteEvent, amountSats) _mintState.value = CashuMintFlowState.Completed(amountSats) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/wallet/OnchainZapSendDialog.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/wallet/OnchainZapSendDialog.kt index 3693ab853f..79f6ab4651 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/wallet/OnchainZapSendDialog.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/wallet/OnchainZapSendDialog.kt @@ -73,6 +73,7 @@ import com.vitorpamplona.amethyst.commons.onchain.OnchainZapSendResult import com.vitorpamplona.amethyst.commons.onchain.OnchainZapSendStage import com.vitorpamplona.amethyst.commons.onchain.OnchainZapShare import com.vitorpamplona.amethyst.commons.onchain.OnchainZapSplitter +import com.vitorpamplona.amethyst.model.DEFAULT_ONCHAIN_ZAP_SATS import com.vitorpamplona.amethyst.model.LocalCache import com.vitorpamplona.amethyst.model.MIN_ONCHAIN_ZAP_SATS import com.vitorpamplona.amethyst.model.User @@ -211,7 +212,13 @@ fun OnchainZapSendDialog( // that clear the on-chain minimum as quick presets. val zapAmountChoices by accountViewModel.account.settings.syncedSettings.zaps.zapAmountChoices .collectAsStateWithLifecycle() - val presetAmounts = remember(zapAmountChoices) { zapAmountChoices.filter { it >= MIN_ONCHAIN_ZAP_SATS } } + // Presets that clear the on-chain minimum. If the user's unified list has + // none (e.g. only small Lightning amounts), fall back to a sensible default + // so the dialog still offers a quick-pick chip instead of an empty row. + val presetAmounts = + remember(zapAmountChoices) { + zapAmountChoices.filter { it >= MIN_ONCHAIN_ZAP_SATS }.ifEmpty { listOf(DEFAULT_ONCHAIN_ZAP_SATS) } + } // Mirror the dropdown's NIP-05 / Namecoin (.bit) resolution so Send can // enable as soon as the typed name resolves, without forcing the user to diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/wallet/ReloadMintViewModel.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/wallet/ReloadMintViewModel.kt index 43d44b3d35..986e16c0b4 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/wallet/ReloadMintViewModel.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/wallet/ReloadMintViewModel.kt @@ -26,10 +26,13 @@ import com.vitorpamplona.amethyst.model.Note import com.vitorpamplona.amethyst.model.nip60Cashu.CashuWalletState import com.vitorpamplona.amethyst.model.nip60Cashu.describeMintError import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel +import com.vitorpamplona.quartz.nip01Core.core.Event import com.vitorpamplona.quartz.nip47WalletConnect.rpc.PayInvoiceMethod import kotlinx.collections.immutable.ImmutableList import kotlinx.collections.immutable.persistentListOf import kotlinx.collections.immutable.toImmutableList +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.Job import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow @@ -105,6 +108,11 @@ class ReloadMintViewModel : ViewModel() { private var baseNote: Note? = null private val state: CashuWalletState? get() = accountViewModel?.account?.cashuWalletState + /** The in-flight reload pipeline, so a second tap can be rejected and the + * whole flow cancelled when the screen leaves (it runs on the long-lived + * AccountViewModel scope, not this VM's). */ + private var job: Job? = null + private val _uiState = MutableStateFlow(ReloadUiState()) val uiState: StateFlow = _uiState.asStateFlow() @@ -146,14 +154,20 @@ class ReloadMintViewModel : ViewModel() { val balances = base.balances.associate { it.mintUrl to it.balanceSats } val targetBalance = balances[target] ?: 0L val shortfall = (base.amountSats - targetBalance).coerceAtLeast(0L) - // Rough fee cushion for enabling a source; the melt quote sets the real - // fee at execution time. 1% (min 1 sat). - val estFee = (shortfall / 100L).coerceAtLeast(1L) + // Rough fee cushion for *enabling* a source — the real Lightning + // feeReserve is only known once rebalance() fetches the melt quote, so + // this is a heuristic (1%, min 2 sat). A source that clears the estimate + // but not the real quote fails recoverably (Failed → pick another), it + // doesn't move funds. + val estFee = (shortfall / 100L).coerceAtLeast(2L) + // A source must cover what we actually mint (shortfall + headroom buffer) + // plus that melt fee. + val needFromSource = shortfall + RELOAD_FEE_BUFFER_SATS + estFee val mintSources = base.balances .filter { it.mintUrl != target } - .map { ReloadSource.Mint(it.mintUrl, it.balanceSats, canCover = it.balanceSats >= shortfall + estFee) } + .map { ReloadSource.Mint(it.mintUrl, it.balanceSats, canCover = it.balanceSats >= needFromSource) } val sources: List = mintSources + ReloadSource.Lightning // Prefer the richest mint that can cover (no new sats); else Lightning. @@ -183,28 +197,43 @@ class ReloadMintViewModel : ViewModel() { val s = _uiState.value val source = s.selectedSource ?: return - vm.launchSigner { - try { - when (source) { - is ReloadSource.Mint -> rebalanceThenZap(source.mintUrl, s.selectedTarget, s.shortfallSats, note, s.amountSats) - ReloadSource.Lightning -> reloadFromLightningThenZap(s.selectedTarget, s.shortfallSats, note, s.amountSats) + // In-flight guard: only start from a resting state. Without it a double + // tap (or the Failed-state Retry) launches a second pipeline — two mint + // quotes at the target and two melts at the source, double-spending / + // double-minting. Flip to Working synchronously so the second call bails. + if (s.status !is ReloadStatus.Configuring && s.status !is ReloadStatus.Failed) return + setStatus(ReloadStatus.Working("Starting", 0.05f)) + + // Mint a hair more than the bare shortfall so the follow-up nutzap's own + // swap fee doesn't leave the target a sat short (see #RELOAD_FEE_BUFFER). + val moveSats = s.shortfallSats + RELOAD_FEE_BUFFER_SATS + + job?.cancel() + job = + vm.launchSigner { + try { + when (source) { + is ReloadSource.Mint -> rebalanceThenZap(source.mintUrl, s.selectedTarget, moveSats, note, s.amountSats) + ReloadSource.Lightning -> reloadFromLightningThenZap(s.selectedTarget, moveSats, note, s.amountSats) + } + } catch (e: CancellationException) { + throw e // screen left mid-flow — don't mask as a Failed state + } catch (e: Exception) { + setStatus(ReloadStatus.Failed(describeMintError(e))) } - } catch (e: Exception) { - setStatus(ReloadStatus.Failed(describeMintError(e))) } - } } private suspend fun rebalanceThenZap( sourceMint: String, targetMint: String, - shortfall: Long, + moveSats: Long, note: Note, amount: Long, ) { val st = state ?: return setStatus(ReloadStatus.Working("Moving funds", 0.1f)) - st.rebalance(sourceMint, targetMint, shortfall) { p -> + st.rebalance(sourceMint, targetMint, moveSats) { p -> setStatus(ReloadStatus.Working("Moving funds", p.coerceIn(0.1f, 0.9f))) } sendNutzapAndFinish(note, amount) @@ -212,7 +241,7 @@ class ReloadMintViewModel : ViewModel() { private suspend fun reloadFromLightningThenZap( targetMint: String, - shortfall: Long, + moveSats: Long, note: Note, amount: Long, ) { @@ -221,7 +250,7 @@ class ReloadMintViewModel : ViewModel() { val ops = st.ops setStatus(ReloadStatus.Working("Requesting invoice", 0.1f)) - val flow = ops.startMintFromLightning(targetMint, shortfall) + val flow = ops.startMintFromLightning(targetMint, moveSats) val walletUri = vm.account.settings.defaultZapPaymentRequest() if (walletUri != null) { @@ -233,16 +262,18 @@ class ReloadMintViewModel : ViewModel() { } } else { // No NWC — surface the invoice for an external wallet and keep polling. - setStatus(ReloadStatus.AwaitingInvoice(flow.invoice, shortfall)) + setStatus(ReloadStatus.AwaitingInvoice(flow.invoice, moveSats)) } + // External payment can take a while; the poll runs on a job tied to the + // screen (cancelled in onCleared), so leaving stops it instead of + // hammering the mint for 3 minutes with nobody watching. val attempts = 90 val delayMs = 2_000L var paid = false var attempt = 0 while (!paid && attempt < attempts) { - val status = ops.checkMintQuote(targetMint, flow.mintQuote.quote) - paid = status.paid == true || status.state == "PAID" || status.state == "ISSUED" + paid = ops.checkMintQuote(targetMint, flow.mintQuote.quote).isSettled() if (!paid) { delay(delayMs) attempt++ @@ -253,23 +284,30 @@ class ReloadMintViewModel : ViewModel() { return } setStatus(ReloadStatus.Working("Issuing ecash", 0.85f)) - ops.completeMintFromLightning(targetMint, flow.quoteEvent, shortfall) + ops.completeMintFromLightning(targetMint, flow.quoteEvent, moveSats) sendNutzapAndFinish(note, amount) } - private fun sendNutzapAndFinish( + /** + * Send the nutzap and only report [ReloadStatus.Done] once it actually + * succeeds. Suspends on the real send (throwing on failure) instead of the + * fire-and-forget AccountViewModel.sendNutzap, so a send that fails after a + * successful reload surfaces as Failed here rather than silently stranding + * the just-moved funds while the screen pops on a premature "done". + */ + private suspend fun sendNutzapAndFinish( note: Note, amount: Long, ) { - val vm = accountViewModel ?: return + val st = state ?: return + val recipient = note.author?.pubkeyHex ?: throw IllegalStateException("Recipient has no pubkey") + val zappedEvent = note.toEventHint() ?: throw IllegalStateException("Nothing to zap") setStatus(ReloadStatus.Working("Sending zap", 0.95f)) - // The destination is now funded; fire the nutzap (its own error path - // toasts) and report Done so the screen can pop. - vm.sendNutzap( - baseNote = note, + st.sendNutzap( amountSats = amount, + recipientPubKey = recipient, + zappedEvent = zappedEvent, message = "", - onError = { _, msg, _ -> setStatus(ReloadStatus.Failed(msg)) }, ) setStatus(ReloadStatus.Done) } @@ -277,4 +315,21 @@ class ReloadMintViewModel : ViewModel() { private fun setStatus(status: ReloadStatus) { _uiState.update { it.copy(status = status) } } + + override fun onCleared() { + // The pipeline runs on the AccountViewModel scope, not this VM's, so it + // would outlive the screen — cancel it when the screen goes away. + job?.cancel() + super.onCleared() + } + + companion object { + /** + * Extra sats minted at the target beyond the bare shortfall, so the + * follow-up nutzap's own swap fee doesn't leave the mint a hair short. + * Most mints charge no input fee; this small cushion covers the ones + * that do without meaningfully over-minting. + */ + private const val RELOAD_FEE_BUFFER_SATS = 2L + } } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip60Cashu/mintApi/MintApiDtos.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip60Cashu/mintApi/MintApiDtos.kt index ff1f8929d6..1d451a5c23 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip60Cashu/mintApi/MintApiDtos.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip60Cashu/mintApi/MintApiDtos.kt @@ -192,7 +192,15 @@ data class MintQuoteBolt11ResponseDto( val state: String, val expiry: Long? = null, val paid: Boolean? = null, -) +) { + /** + * True once the mint considers the quote's invoice settled — either the + * legacy `paid` boolean (NUT-04 v0) or the `state` machine reaching PAID + * (invoice settled, proofs not yet issued) or ISSUED (proofs minted). + * Centralized so every poll loop agrees on what "paid" means. + */ + fun isSettled(): Boolean = paid == true || state == "PAID" || state == "ISSUED" +} @Serializable data class MintBolt11RequestDto(