From f6db678249090ce32434d1be527b171ba4f64df5 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 20 May 2026 20:49:33 +0000 Subject: [PATCH] fix: audit follow-ups (fee retry, perf, self-pay gate) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From an independent audit + my own pass, addressing concrete issues: OnchainZapSendDialog - Fee estimate fetch now retries with bounded backoff (4 tries, 1/2/3s spacing) instead of giving up after one attempt. Covers two real boot races: LocalCache.onchainBackend not yet wired at first composition, and a flaky feeEstimates() call. Without retry the Send button stayed permanently disabled. - SplitsRecipientSection now indexes preview shares by pubkey once via remember(previewShares) { associateBy { ... } } instead of an O(N²) firstOrNull lookup per split row. - belowDustShares is now wrapped in remember(previewShares) so it doesn't re-filter the list on every recomposition. - canSend now also requires resolvedRecipient != senderPubKey in single-recipient mode, so the user can't tap Send when the only fallback recipient is themselves (would fail at the builder's "cannot zap yourself" check). - formatWeight no longer prints "50.0%" for whole-percent shares — trailing ".0" is stripped (was a Double->String artifact). OnchainZapSplitter - Added distributeUnchecked(): same allocation as distribute() but never throws on dust; returns every share so the UI preview can render the full shape in one pass. distribute() (used by the build/send path) still throws via DustRecipientException so the real send keeps its dust gate. - Added check(remainder < splits.size) before the remainder loop to pin the invariant that bounds remainder.toInt() and the k % size defensive mod. - Test for distributeUnchecked. ReactionsRow / ReusableZapButton / ZapCustomDialog - baseNote.toEventHint() is now wrapped in remember(baseNote) in all three dialog launchers so it's not allocated on every parent recomposition. --- .../ui/components/ReusableZapButton.kt | 3 +- .../amethyst/ui/note/ReactionsRow.kt | 3 +- .../amethyst/ui/note/ZapCustomDialog.kt | 3 +- .../loggedIn/wallet/OnchainZapSendDialog.kt | 70 ++++++++++--------- .../commons/onchain/OnchainZapSplitter.kt | 36 ++++++++-- .../commons/onchain/OnchainZapSplitterTest.kt | 15 ++++ 6 files changed, 88 insertions(+), 42 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ReusableZapButton.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ReusableZapButton.kt index 44e5591c0..772948a63 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ReusableZapButton.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/components/ReusableZapButton.kt @@ -211,6 +211,7 @@ fun ReusableZapButton( } if (showOnchainDialog) { + val zappedEventHint = remember(baseNote) { baseNote.toEventHint() } OnchainZapSendDialog( accountViewModel = accountViewModel, onDismiss = { @@ -218,7 +219,7 @@ fun ReusableZapButton( onchainZapAmount = null }, recipientPubKey = baseNote.author?.pubkeyHex, - zappedEvent = baseNote.toEventHint(), + zappedEvent = zappedEventHint, prefillAmountSats = onchainZapAmount, ) } 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 e24844d6f..842f23f22 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 @@ -1245,11 +1245,12 @@ fun ZapReaction( } onchainZapRequest?.let { request -> + val zappedEventHint = remember(baseNote) { baseNote.toEventHint() } OnchainZapSendDialog( accountViewModel = accountViewModel, onDismiss = { onchainZapRequest = null }, recipientPubKey = baseNote.author?.pubkeyHex, - zappedEvent = baseNote.toEventHint(), + zappedEvent = zappedEventHint, prefillAmountSats = request.amountSats, ) } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/ZapCustomDialog.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/ZapCustomDialog.kt index befdaa60e..8c4e6feff 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/ZapCustomDialog.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/ZapCustomDialog.kt @@ -354,6 +354,7 @@ fun ZapCustomDialog( } if (sendOnchain) { + val zappedEventHint = remember(baseNote) { baseNote.toEventHint() } OnchainZapSendDialog( accountViewModel = accountViewModel, onDismiss = { @@ -361,7 +362,7 @@ fun ZapCustomDialog( onClose() }, recipientPubKey = baseNote.author?.pubkeyHex, - zappedEvent = baseNote.toEventHint(), + zappedEvent = zappedEventHint, prefillAmountSats = postViewModel.value(), prefillComment = postViewModel.customMessage.text, ) 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 656df4d94..4f646d669 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 @@ -93,6 +93,7 @@ import com.vitorpamplona.quartz.nipBCOnchainZaps.builder.OnchainZapBuilder import com.vitorpamplona.quartz.nipBCOnchainZaps.chain.FeeEstimates import com.vitorpamplona.quartz.utils.BigDecimal import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.delay import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import java.text.NumberFormat @@ -184,10 +185,24 @@ fun OnchainZapSendDialog( var useSplits by remember(zappedEventId) { mutableStateOf(onchainSplits.isNotEmpty()) } val splitMode = useSplits && onchainSplits.isNotEmpty() + // Fetch fee estimates with bounded retry. Covers two boot races: + // - LocalCache.onchainBackend is null briefly while AppModules wires it up + // - feeEstimates() throws on a flaky network + // Without retry, the Send button would stay permanently disabled because + // the build path needs a fee rate. LaunchedEffect(Unit) { - val backend = LocalCache.onchainBackend ?: return@LaunchedEffect - fees = - runCatching { withContext(Dispatchers.IO) { backend.feeEstimates() } }.getOrNull() + repeat(4) { attempt -> + if (fees != null) return@LaunchedEffect + val backend = LocalCache.onchainBackend + if (backend != null) { + val newFees = runCatching { withContext(Dispatchers.IO) { backend.feeEstimates() } }.getOrNull() + if (newFees != null) { + fees = newFees + return@LaunchedEffect + } + } + if (attempt < 3) delay(1_000L * (attempt + 1)) + } } val presetAmounts by accountViewModel.account.settings.syncedSettings.zaps.onchainZapAmountChoices @@ -208,44 +223,28 @@ fun OnchainZapSendDialog( // Preview the per-recipient share allocation. Always compute the full // list (even when some shares would land below dust) so the UI can show - // every recipient's amount; the dust offenders are flagged separately - // and gate the Send button. + // every recipient's amount; below-dust offenders are flagged separately + // and gate the Send button so the user can't tap into a guaranteed + // BUILDING-stage failure. val previewShares = remember(splitMode, onchainSplits, amountSats) { if (!splitMode || amountSats == null || amountSats <= 0) { null } else { runCatching { - OnchainZapSplitter.distribute( - totalSats = amountSats, - splits = onchainSplits, - dustThresholdSats = OnchainZapBuilder.DUST_THRESHOLD_SATS, - ) - }.getOrElse { e -> - if (e is DustRecipientException) { - // Re-run with a 0 dust threshold to get the full shape - // for the preview; the real send still uses the proper - // dust check via [DustRecipientException]. - runCatching { - OnchainZapSplitter.distribute( - totalSats = amountSats, - splits = onchainSplits, - dustThresholdSats = 0L, - ) - }.getOrNull() - } else { - null - } - } + OnchainZapSplitter.distributeUnchecked(amountSats, onchainSplits) + }.getOrNull() } } val belowDustShares = - previewShares.orEmpty().filter { it.sats < OnchainZapBuilder.DUST_THRESHOLD_SATS } + remember(previewShares) { + previewShares.orEmpty().filter { it.sats < OnchainZapBuilder.DUST_THRESHOLD_SATS } + } val canSend = !sending && result == null && - (splitMode || resolvedRecipient != null) && + (splitMode || (resolvedRecipient != null && resolvedRecipient != senderPubKey)) && amountSats != null && amountSats > 0 && fees != null && @@ -748,6 +747,12 @@ private fun SplitsRecipientSection( SectionLabel("Splits among ${splits.size} recipients") val totalWeight = splits.sumOf { it.second } + // Index the preview by pubkey once — the splits list scan would otherwise + // be O(N²) for the per-row sat amount lookup. + val previewByPubKey = + remember(previewShares) { + previewShares?.associateBy { it.recipientPubKey } + } Surface( shape = MaterialTheme.shapes.medium, @@ -756,7 +761,7 @@ private fun SplitsRecipientSection( ) { Column(modifier = Modifier.padding(vertical = 4.dp)) { splits.forEach { (pubKey, weight) -> - val share = previewShares?.firstOrNull { it.recipientPubKey == pubKey } + val share = previewByPubKey?.get(pubKey) Row( modifier = Modifier.padding(horizontal = 12.dp, vertical = 6.dp), verticalAlignment = Alignment.CenterVertically, @@ -819,9 +824,10 @@ private fun formatWeight( return if (pct >= 99.95) { "100%" } else { - // One decimal place keeps "33.3%" readable without floating-point noise. - val rounded = (pct * 10).toLong() / 10.0 - "$rounded%" + // Round to a tenth of a percent. Drop the trailing ".0" so whole + // percentages render as "50%" instead of "50.0%". + val tenths = (pct * 10).toLong() + if (tenths % 10 == 0L) "${tenths / 10}%" else "${tenths / 10}.${tenths % 10}%" } } diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/onchain/OnchainZapSplitter.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/onchain/OnchainZapSplitter.kt index 6ee96927c..553bdf0b9 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/onchain/OnchainZapSplitter.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/onchain/OnchainZapSplitter.kt @@ -79,6 +79,27 @@ object OnchainZapSplitter { totalSats: Long, splits: List>, dustThresholdSats: Long, + ): List { + val shares = computeShares(totalSats, splits) + val belowDust = shares.filter { it.sats < dustThresholdSats } + if (belowDust.isNotEmpty()) throw DustRecipientException(belowDust, dustThresholdSats) + return shares + } + + /** + * Same allocation as [distribute] but never throws on dust. Returns every + * share (including below-dust ones) so a UI preview can render the full + * shape and the caller can decide what to do with offenders. Use + * [distribute] for the actual send path where below-dust must hard-fail. + */ + fun distributeUnchecked( + totalSats: Long, + splits: List>, + ): List = computeShares(totalSats, splits) + + private fun computeShares( + totalSats: Long, + splits: List>, ): List { require(totalSats > 0) { "total must be positive" } require(splits.isNotEmpty()) { "splits must be non-empty" } @@ -96,23 +117,24 @@ object OnchainZapSplitter { shares[i] = s assigned += s } + // Each floor() loses < 1 sat, so the total remainder is strictly less + // than `splits.size` — well within Int range for any plausible N. val remainder = totalSats - assigned + check(remainder < splits.size) { "remainder $remainder exceeds splits.size ${splits.size}" } if (remainder > 0) { val orderByWeight = splits.indices.sortedWith( compareByDescending { splits[it].second }.thenBy { it }, ) for (k in 0 until remainder.toInt()) { + // remainder < splits.size, so k % size is just k — kept + // defensively in case the bound ever loosens. shares[orderByWeight[k % orderByWeight.size]] += 1 } } - val result = - splits.mapIndexed { i, (pubKey, weight) -> - OnchainZapShare(recipientPubKey = pubKey, sats = shares[i], weight = weight) - } - val belowDust = result.filter { it.sats < dustThresholdSats } - if (belowDust.isNotEmpty()) throw DustRecipientException(belowDust, dustThresholdSats) - return result + return splits.mapIndexed { i, (pubKey, weight) -> + OnchainZapShare(recipientPubKey = pubKey, sats = shares[i], weight = weight) + } } } diff --git a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/onchain/OnchainZapSplitterTest.kt b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/onchain/OnchainZapSplitterTest.kt index 198204d77..bf6bc1d46 100644 --- a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/onchain/OnchainZapSplitterTest.kt +++ b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/onchain/OnchainZapSplitterTest.kt @@ -153,6 +153,21 @@ class OnchainZapSplitterTest { assertTrue(shares[2].sats in 19_900..20_100) } + @Test + fun distributeUncheckedReturnsBelowDustShares() { + // 1000 sats split 1:99 → 10 and 990. distribute() throws on the 10; + // distributeUnchecked() returns both, leaving dust handling to caller. + val shares = + OnchainZapSplitter.distributeUnchecked( + totalSats = 1000L, + splits = listOf(a to 1.0, b to 99.0), + ) + assertEquals(2, shares.size) + assertEquals(10L, shares[0].sats) + assertEquals(990L, shares[1].sats) + assertEquals(1000L, shares.sumOf { it.sats }) + } + @Test fun floatingPointWeightsSumExactly() { // 0.1 + 0.2 = 0.30000000000000004 in IEEE-754. Make sure that doesn't