fix: audit follow-ups (fee retry, perf, self-pay gate)

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<Event>() is now wrapped in remember(baseNote)
  in all three dialog launchers so it's not allocated on every
  parent recomposition.
This commit is contained in:
Claude
2026-05-20 20:49:33 +00:00
parent 85dfe93ea7
commit f6db678249
6 changed files with 88 additions and 42 deletions
@@ -211,6 +211,7 @@ fun ReusableZapButton(
} }
if (showOnchainDialog) { if (showOnchainDialog) {
val zappedEventHint = remember(baseNote) { baseNote.toEventHint<Event>() }
OnchainZapSendDialog( OnchainZapSendDialog(
accountViewModel = accountViewModel, accountViewModel = accountViewModel,
onDismiss = { onDismiss = {
@@ -218,7 +219,7 @@ fun ReusableZapButton(
onchainZapAmount = null onchainZapAmount = null
}, },
recipientPubKey = baseNote.author?.pubkeyHex, recipientPubKey = baseNote.author?.pubkeyHex,
zappedEvent = baseNote.toEventHint<Event>(), zappedEvent = zappedEventHint,
prefillAmountSats = onchainZapAmount, prefillAmountSats = onchainZapAmount,
) )
} }
@@ -1245,11 +1245,12 @@ fun ZapReaction(
} }
onchainZapRequest?.let { request -> onchainZapRequest?.let { request ->
val zappedEventHint = remember(baseNote) { baseNote.toEventHint<Event>() }
OnchainZapSendDialog( OnchainZapSendDialog(
accountViewModel = accountViewModel, accountViewModel = accountViewModel,
onDismiss = { onchainZapRequest = null }, onDismiss = { onchainZapRequest = null },
recipientPubKey = baseNote.author?.pubkeyHex, recipientPubKey = baseNote.author?.pubkeyHex,
zappedEvent = baseNote.toEventHint<Event>(), zappedEvent = zappedEventHint,
prefillAmountSats = request.amountSats, prefillAmountSats = request.amountSats,
) )
} }
@@ -354,6 +354,7 @@ fun ZapCustomDialog(
} }
if (sendOnchain) { if (sendOnchain) {
val zappedEventHint = remember(baseNote) { baseNote.toEventHint<Event>() }
OnchainZapSendDialog( OnchainZapSendDialog(
accountViewModel = accountViewModel, accountViewModel = accountViewModel,
onDismiss = { onDismiss = {
@@ -361,7 +362,7 @@ fun ZapCustomDialog(
onClose() onClose()
}, },
recipientPubKey = baseNote.author?.pubkeyHex, recipientPubKey = baseNote.author?.pubkeyHex,
zappedEvent = baseNote.toEventHint<Event>(), zappedEvent = zappedEventHint,
prefillAmountSats = postViewModel.value(), prefillAmountSats = postViewModel.value(),
prefillComment = postViewModel.customMessage.text, prefillComment = postViewModel.customMessage.text,
) )
@@ -93,6 +93,7 @@ import com.vitorpamplona.quartz.nipBCOnchainZaps.builder.OnchainZapBuilder
import com.vitorpamplona.quartz.nipBCOnchainZaps.chain.FeeEstimates import com.vitorpamplona.quartz.nipBCOnchainZaps.chain.FeeEstimates
import com.vitorpamplona.quartz.utils.BigDecimal import com.vitorpamplona.quartz.utils.BigDecimal
import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.delay
import kotlinx.coroutines.launch import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext import kotlinx.coroutines.withContext
import java.text.NumberFormat import java.text.NumberFormat
@@ -184,10 +185,24 @@ fun OnchainZapSendDialog(
var useSplits by remember(zappedEventId) { mutableStateOf(onchainSplits.isNotEmpty()) } var useSplits by remember(zappedEventId) { mutableStateOf(onchainSplits.isNotEmpty()) }
val splitMode = useSplits && 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) { LaunchedEffect(Unit) {
val backend = LocalCache.onchainBackend ?: return@LaunchedEffect repeat(4) { attempt ->
fees = if (fees != null) return@LaunchedEffect
runCatching { withContext(Dispatchers.IO) { backend.feeEstimates() } }.getOrNull() 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 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 // Preview the per-recipient share allocation. Always compute the full
// list (even when some shares would land below dust) so the UI can show // list (even when some shares would land below dust) so the UI can show
// every recipient's amount; the dust offenders are flagged separately // every recipient's amount; below-dust offenders are flagged separately
// and gate the Send button. // and gate the Send button so the user can't tap into a guaranteed
// BUILDING-stage failure.
val previewShares = val previewShares =
remember(splitMode, onchainSplits, amountSats) { remember(splitMode, onchainSplits, amountSats) {
if (!splitMode || amountSats == null || amountSats <= 0) { if (!splitMode || amountSats == null || amountSats <= 0) {
null null
} else { } else {
runCatching { runCatching {
OnchainZapSplitter.distribute( OnchainZapSplitter.distributeUnchecked(amountSats, onchainSplits)
totalSats = amountSats, }.getOrNull()
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
}
}
} }
} }
val belowDustShares = val belowDustShares =
previewShares.orEmpty().filter { it.sats < OnchainZapBuilder.DUST_THRESHOLD_SATS } remember(previewShares) {
previewShares.orEmpty().filter { it.sats < OnchainZapBuilder.DUST_THRESHOLD_SATS }
}
val canSend = val canSend =
!sending && !sending &&
result == null && result == null &&
(splitMode || resolvedRecipient != null) && (splitMode || (resolvedRecipient != null && resolvedRecipient != senderPubKey)) &&
amountSats != null && amountSats != null &&
amountSats > 0 && amountSats > 0 &&
fees != null && fees != null &&
@@ -748,6 +747,12 @@ private fun SplitsRecipientSection(
SectionLabel("Splits among ${splits.size} recipients") SectionLabel("Splits among ${splits.size} recipients")
val totalWeight = splits.sumOf { it.second } 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( Surface(
shape = MaterialTheme.shapes.medium, shape = MaterialTheme.shapes.medium,
@@ -756,7 +761,7 @@ private fun SplitsRecipientSection(
) { ) {
Column(modifier = Modifier.padding(vertical = 4.dp)) { Column(modifier = Modifier.padding(vertical = 4.dp)) {
splits.forEach { (pubKey, weight) -> splits.forEach { (pubKey, weight) ->
val share = previewShares?.firstOrNull { it.recipientPubKey == pubKey } val share = previewByPubKey?.get(pubKey)
Row( Row(
modifier = Modifier.padding(horizontal = 12.dp, vertical = 6.dp), modifier = Modifier.padding(horizontal = 12.dp, vertical = 6.dp),
verticalAlignment = Alignment.CenterVertically, verticalAlignment = Alignment.CenterVertically,
@@ -819,9 +824,10 @@ private fun formatWeight(
return if (pct >= 99.95) { return if (pct >= 99.95) {
"100%" "100%"
} else { } else {
// One decimal place keeps "33.3%" readable without floating-point noise. // Round to a tenth of a percent. Drop the trailing ".0" so whole
val rounded = (pct * 10).toLong() / 10.0 // percentages render as "50%" instead of "50.0%".
"$rounded%" val tenths = (pct * 10).toLong()
if (tenths % 10 == 0L) "${tenths / 10}%" else "${tenths / 10}.${tenths % 10}%"
} }
} }
@@ -79,6 +79,27 @@ object OnchainZapSplitter {
totalSats: Long, totalSats: Long,
splits: List<Pair<HexKey, Double>>, splits: List<Pair<HexKey, Double>>,
dustThresholdSats: Long, dustThresholdSats: Long,
): List<OnchainZapShare> {
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<Pair<HexKey, Double>>,
): List<OnchainZapShare> = computeShares(totalSats, splits)
private fun computeShares(
totalSats: Long,
splits: List<Pair<HexKey, Double>>,
): List<OnchainZapShare> { ): List<OnchainZapShare> {
require(totalSats > 0) { "total must be positive" } require(totalSats > 0) { "total must be positive" }
require(splits.isNotEmpty()) { "splits must be non-empty" } require(splits.isNotEmpty()) { "splits must be non-empty" }
@@ -96,23 +117,24 @@ object OnchainZapSplitter {
shares[i] = s shares[i] = s
assigned += 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 val remainder = totalSats - assigned
check(remainder < splits.size) { "remainder $remainder exceeds splits.size ${splits.size}" }
if (remainder > 0) { if (remainder > 0) {
val orderByWeight = val orderByWeight =
splits.indices.sortedWith( splits.indices.sortedWith(
compareByDescending<Int> { splits[it].second }.thenBy { it }, compareByDescending<Int> { splits[it].second }.thenBy { it },
) )
for (k in 0 until remainder.toInt()) { 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 shares[orderByWeight[k % orderByWeight.size]] += 1
} }
} }
val result = return splits.mapIndexed { i, (pubKey, weight) ->
splits.mapIndexed { i, (pubKey, weight) -> OnchainZapShare(recipientPubKey = pubKey, sats = shares[i], weight = 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
} }
} }
@@ -153,6 +153,21 @@ class OnchainZapSplitterTest {
assertTrue(shares[2].sats in 19_900..20_100) 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 @Test
fun floatingPointWeightsSumExactly() { fun floatingPointWeightsSumExactly() {
// 0.1 + 0.2 = 0.30000000000000004 in IEEE-754. Make sure that doesn't // 0.1 + 0.2 = 0.30000000000000004 in IEEE-754. Make sure that doesn't