From 45aa6044b7cac791d6260b04e8b32690149f3491 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 20 May 2026 20:14:44 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20on-chain=20zap=20splits=20=E2=80=94=20dr?= =?UTF-8?q?op=20sender=20from=20splits,=20merge=20duplicates,=20gate=20Sen?= =?UTF-8?q?d=20on=20dust?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit findings from an independent code review: - HIGH: When the user zaps their own post (a common flow), every split that included the post author put the sender on the recipient list, and OnchainZapBuilder.buildSplit refused the whole tx with "cannot zap yourself". Fix: new OnchainZapSplitter.prepare() filters the sender's pubkey out of the splits before they reach the builder. - HIGH: NIP-57 lets the same pubkey appear in zap-split tags more than once (additive weights). buildSplit rejected duplicate recipients. Same prepare() helper merges duplicates by summing weights, in first-seen order. - HIGH: The dialog's live preview only showed amounts for recipients whose share was BELOW dust (because DustRecipientException only carries belowDust). Fix: parent composable computes shares with a zero dust threshold for the preview, gating the Send button on a separate belowDustShares check so the user can see all amounts and can't tap Send into a guaranteed BUILDING-stage failure. - MEDIUM: OnchainZapSendResult.Failure didn't carry the ids of receipts that successfully published before a partial-publish failure. Added publishedReceiptEventIds: List. - LOW: useSplits state was keyed by zappedEvent reference; re-emitted bundles would silently reset the toggle. Now keyed on the event id. Tests added: - splitter: prepare() drops sender, merges duplicates, filters non-positive weights; floating-point weights (0.1 + 0.2) sum exactly - builder: buildSplit produces N recipient outputs + 1 change at index N, conserves sats, rejects duplicates and below-dust shares - sender: sendSplit publishes one receipt per recipient sharing the txid with correct per-recipient amount; partial-publish failure carries the broadcast txid and the ids of receipts that did publish --- .../loggedIn/wallet/OnchainZapSendDialog.kt | 110 +++++++++++------- .../commons/onchain/OnchainZapSender.kt | 7 ++ .../commons/onchain/OnchainZapSplitter.kt | 25 ++++ .../commons/onchain/OnchainZapSenderTest.kt | 82 +++++++++++++ .../commons/onchain/OnchainZapSplitterTest.kt | 40 +++++++ .../builder/OnchainZapBuilderTest.kt | 83 +++++++++++++ 6 files changed, 303 insertions(+), 44 deletions(-) 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 5129f487b..647188878 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 @@ -70,6 +70,7 @@ import com.vitorpamplona.amethyst.commons.icons.symbols.MaterialSymbols import com.vitorpamplona.amethyst.commons.onchain.DustRecipientException 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.LocalCache import com.vitorpamplona.amethyst.model.User @@ -161,24 +162,25 @@ fun OnchainZapSendDialog( // Pull pubkey-based zap splits off the zapped event. Lightning-address-only // splits are dropped because we can't derive a Taproot output from an // lnAddress — they appear in [skippedLnSplits] so the UI can warn the user - // that those recipients won't be paid on-chain. + // that those recipients won't be paid on-chain. The sender's own pubkey is + // also dropped (zapping your own post is common; the on-chain builder + // refuses self-pays) and duplicate pubkeys are merged. + val senderPubKey = accountViewModel.account.signer.pubKey + val zappedEventId = zappedEvent?.event?.id + val rawSplits = + remember(zappedEventId) { + zappedEvent?.event?.zapSplitSetup().orEmpty() + } val onchainSplits = - remember(zappedEvent) { - zappedEvent - ?.event - ?.zapSplitSetup() - .orEmpty() - .filterIsInstance() + remember(zappedEventId, senderPubKey) { + val raw = rawSplits.filterIsInstance().map { it.pubKeyHex to it.weight } + OnchainZapSplitter.prepare(raw, senderPubKey) } val skippedLnSplits = - remember(zappedEvent) { - zappedEvent - ?.event - ?.zapSplitSetup() - .orEmpty() - .filterIsInstance() + remember(zappedEventId) { + rawSplits.filterIsInstance() } - var useSplits by remember(zappedEvent) { mutableStateOf(onchainSplits.isNotEmpty()) } + var useSplits by remember(zappedEventId) { mutableStateOf(onchainSplits.isNotEmpty()) } val splitMode = useSplits && onchainSplits.isNotEmpty() LaunchedEffect(Unit) { @@ -202,13 +204,51 @@ fun OnchainZapSendDialog( ?: searchInput.trim().takeIf { it.isNotEmpty() }?.let { decodePublicKeyAsHexOrNull(it) } ?: nip05Resolved?.pubkeyHex val amountSats = amountInput.trim().toLongOrNull() + + // 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. + 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 + } + } + } + } + val belowDustShares = + previewShares.orEmpty().filter { it.sats < OnchainZapBuilder.DUST_THRESHOLD_SATS } + val canSend = !sending && result == null && (splitMode || resolvedRecipient != null) && amountSats != null && amountSats > 0 && - fees != null + fees != null && + (!splitMode || (previewShares != null && belowDustShares.isEmpty())) ModalBottomSheet( onDismissRequest = { if (!sending) onDismiss() }, @@ -261,8 +301,8 @@ fun OnchainZapSendDialog( if (splitMode) { SplitsRecipientSection( splits = onchainSplits, + previewShares = previewShares, skippedLnSplits = skippedLnSplits, - amountSats = amountSats, onDisable = { useSplits = false }, accountViewModel = accountViewModel, ) @@ -345,7 +385,7 @@ fun OnchainZapSendDialog( try { OnchainZapSplitter.distribute( totalSats = amount, - splits = onchainSplits.map { it.pubKeyHex to it.weight }, + splits = onchainSplits, dustThresholdSats = OnchainZapBuilder.DUST_THRESHOLD_SATS, ) } catch (e: DustRecipientException) { @@ -698,32 +738,15 @@ private fun SendButton( @Composable private fun SplitsRecipientSection( - splits: List, + splits: List>, + previewShares: List?, skippedLnSplits: List, - amountSats: Long?, onDisable: () -> Unit, accountViewModel: AccountViewModel, ) { SectionLabel("Splits among ${splits.size} recipients") - // Compute per-recipient shares for the live preview. Below-dust splits are - // surfaced inline so the user sees why Send might fail before they tap it. - val shares = - remember(splits, amountSats) { - if (amountSats == null || amountSats <= 0) { - null - } else { - runCatching { - OnchainZapSplitter.distribute( - totalSats = amountSats, - splits = splits.map { it.pubKeyHex to it.weight }, - dustThresholdSats = OnchainZapBuilder.DUST_THRESHOLD_SATS, - ) - }.getOrElse { e -> - if (e is DustRecipientException) e.belowDust else null - } - } - } + val totalWeight = splits.sumOf { it.second } Surface( shape = MaterialTheme.shapes.medium, @@ -731,21 +754,21 @@ private fun SplitsRecipientSection( modifier = Modifier.fillMaxWidth(), ) { Column(modifier = Modifier.padding(vertical = 4.dp)) { - splits.forEach { split -> - val share = shares?.firstOrNull { it.recipientPubKey == split.pubKeyHex } + splits.forEach { (pubKey, weight) -> + val share = previewShares?.firstOrNull { it.recipientPubKey == pubKey } Row( modifier = Modifier.padding(horizontal = 12.dp, vertical = 6.dp), verticalAlignment = Alignment.CenterVertically, ) { UserPicture( - userHex = split.pubKeyHex, + userHex = pubKey, size = 28.dp, accountViewModel = accountViewModel, nav = EmptyNav(), ) Spacer(Modifier.size(8.dp)) Text( - text = "${formatWeight(split.weight, splits)}", + text = formatWeight(weight, totalWeight), style = MaterialTheme.typography.bodySmall, color = MaterialTheme.colorScheme.onSurfaceVariant, modifier = Modifier.weight(1f), @@ -789,10 +812,9 @@ private fun SplitsRecipientSection( private fun formatWeight( weight: Double, - all: List, + totalWeight: Double, ): String { - val total = all.sumOf { it.weight } - val pct = (weight / total) * 100.0 + val pct = (weight / totalWeight) * 100.0 return if (pct >= 99.95) { "100%" } else { diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/onchain/OnchainZapSender.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/onchain/OnchainZapSender.kt index 12c05a958..a9953abb6 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/onchain/OnchainZapSender.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/onchain/OnchainZapSender.kt @@ -87,6 +87,12 @@ sealed interface OnchainZapSendResult { val cause: Throwable? = null, /** Non-null when the payment was broadcast but a later stage failed. */ val broadcastTxid: String? = null, + /** + * Receipt ids that DID publish before the failure, in the order they + * were sent. Empty for non-publishing failures and for single-recipient + * publishes that fail on the first receipt. + */ + val publishedReceiptEventIds: List = emptyList(), ) : OnchainZapSendResult } @@ -359,6 +365,7 @@ object OnchainZapSender { "but the next receipt could not be published", cause = e, broadcastTxid = txid, + publishedReceiptEventIds = publishedIds.toList(), ) } } 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 01e76462d..6ee96927c 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 @@ -50,6 +50,31 @@ class DustRecipientException( * - any share that lands below dust is reported via [DustRecipientException] */ object OnchainZapSplitter { + /** + * Clean raw `["zap", pubkey, relay, weight]` splits for the on-chain path: + * + * 1. drop the sender's own pubkey (the on-chain builder refuses self-pays, + * and a self-share would otherwise abort the whole zap when the user + * zaps their own post — common, since most splits include the author) + * 2. merge duplicates by pubkey, summing their weights (Lightning splits + * can repeat a pubkey; on-chain we want exactly one output per pubkey + * so each recipient gets one receipt with one consolidated amount) + * + * The returned list preserves first-seen input order. + */ + fun prepare( + rawSplits: List>, + senderPubKey: HexKey, + ): List> { + val merged = linkedMapOf() + for ((pubKey, weight) in rawSplits) { + if (pubKey == senderPubKey) continue + if (weight <= 0.0) continue + merged[pubKey] = (merged[pubKey] ?: 0.0) + weight + } + return merged.entries.map { it.key to it.value } + } + fun distribute( totalSats: Long, splits: List>, diff --git a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/onchain/OnchainZapSenderTest.kt b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/onchain/OnchainZapSenderTest.kt index 85849ff0c..8f439de48 100644 --- a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/onchain/OnchainZapSenderTest.kt +++ b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/onchain/OnchainZapSenderTest.kt @@ -272,4 +272,86 @@ class OnchainZapSenderTest { assertEquals(OnchainZapSendStage.SIGNING, result.stage) assertEquals(null, backend.broadcastedHex) } + + @Test + fun sendSplitProducesOneReceiptPerRecipient() = + runTest { + val r1 = xOnly("000000000000000000000000000000000000000000000000000000000000000d") + val r2 = xOnly("000000000000000000000000000000000000000000000000000000000000000e") + val r3 = xOnly("000000000000000000000000000000000000000000000000000000000000000f") + val backend = FakeBackend(listOf(Utxo("1".repeat(64), 0, 1_000_000L, 6))) + val publishedReceipts = mutableListOf() + + val result = + OnchainZapSender.sendSplit( + backend = backend, + signer = senderSigner, + senderPubKey = senderPubKey, + recipients = + listOf( + OnchainZapShare(r1, 50_000L, 5.0), + OnchainZapShare(r2, 30_000L, 3.0), + OnchainZapShare(r3, 20_000L, 2.0), + ), + feeRateSatPerVByte = 5.0, + comment = "thanks all", + zappedEvent = null, + ) { template -> + val event = senderSigner.sign(template) + publishedReceipts += event + event + } + + assertIs(result) + assertEquals(3, publishedReceipts.size) + assertEquals(2, result.extraReceiptEventIds.size) + + // All receipts must reference the SAME txid. + val broadcastTxid = BitcoinTransaction.parse(backend.broadcastedHex!!).txid() + assertEquals(broadcastTxid, result.txid) + assertTrue(publishedReceipts.all { it.txid() == broadcastTxid }) + + // Per-recipient receipts carry the right pubkey + sat amount. + val byRecipient = publishedReceipts.associateBy { it.recipient() } + assertEquals(50_000L, byRecipient[r1]?.claimedAmountInSats()) + assertEquals(30_000L, byRecipient[r2]?.claimedAmountInSats()) + assertEquals(20_000L, byRecipient[r3]?.claimedAmountInSats()) + } + + @Test + fun sendSplitPartialPublishFailureKeepsBroadcastedReceiptIds() = + runTest { + val r1 = xOnly("000000000000000000000000000000000000000000000000000000000000000d") + val r2 = xOnly("000000000000000000000000000000000000000000000000000000000000000e") + val r3 = xOnly("000000000000000000000000000000000000000000000000000000000000000f") + val backend = FakeBackend(listOf(Utxo("1".repeat(64), 0, 1_000_000L, 6))) + var calls = 0 + + val result = + OnchainZapSender.sendSplit( + backend = backend, + signer = senderSigner, + senderPubKey = senderPubKey, + recipients = + listOf( + OnchainZapShare(r1, 50_000L, 1.0), + OnchainZapShare(r2, 30_000L, 1.0), + OnchainZapShare(r3, 20_000L, 1.0), + ), + feeRateSatPerVByte = 5.0, + comment = "", + zappedEvent = null, + ) { template -> + calls++ + if (calls == 2) throw RuntimeException("relay rejected receipt") + senderSigner.sign(template) + } + + assertIs(result) + assertEquals(OnchainZapSendStage.PUBLISHING, result.stage) + // Tx is on-chain. + assertTrue(result.broadcastTxid != null) + // Exactly one receipt was published before the failure. + assertEquals(1, result.publishedReceiptEventIds.size) + } } 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 c82275a30..198204d77 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 @@ -111,6 +111,33 @@ class OnchainZapSplitterTest { assertEquals(50_000L, shares[0].sats) } + @Test + fun prepareDropsSenderAndMergesDuplicates() { + val cleaned = + OnchainZapSplitter.prepare( + rawSplits = listOf(a to 1.0, b to 2.0, a to 1.0, c to 0.0, b to 1.0), + senderPubKey = c, + ) + // sender c is dropped, a is merged (1+1=2), b is merged (2+1=3), c=0 filtered too. + assertEquals(listOf(a to 2.0, b to 3.0), cleaned) + } + + @Test + fun prepareDropsSenderEvenIfOnlyEntry() { + val cleaned = OnchainZapSplitter.prepare(listOf(a to 1.0), senderPubKey = a) + assertTrue(cleaned.isEmpty()) + } + + @Test + fun prepareSkipsNegativeOrZeroWeights() { + val cleaned = + OnchainZapSplitter.prepare( + rawSplits = listOf(a to 1.0, b to 0.0, c to -1.0), + senderPubKey = "deadbeef".repeat(8), + ) + assertEquals(listOf(a to 1.0), cleaned) + } + @Test fun fractionalWeightsWork() { val shares = @@ -125,4 +152,17 @@ class OnchainZapSplitterTest { assertTrue(shares[1].sats in 29_900..30_100) assertTrue(shares[2].sats in 19_900..20_100) } + + @Test + fun floatingPointWeightsSumExactly() { + // 0.1 + 0.2 = 0.30000000000000004 in IEEE-754. Make sure that doesn't + // leak a missing or extra sat. + val shares = + OnchainZapSplitter.distribute( + totalSats = 1_000_000L, + splits = listOf(a to 0.1, b to 0.2), + dustThresholdSats = 330L, + ) + assertEquals(1_000_000L, shares.sumOf { it.sats }) + } } diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nipBCOnchainZaps/builder/OnchainZapBuilderTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nipBCOnchainZaps/builder/OnchainZapBuilderTest.kt index 3de8ede56..4e8646fb5 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nipBCOnchainZaps/builder/OnchainZapBuilderTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nipBCOnchainZaps/builder/OnchainZapBuilderTest.kt @@ -196,4 +196,87 @@ class OnchainZapBuilderTest { val result = OnchainZapBuilder.build(senderPubKey, recipientPubKey, 25_000L, 2.0, utxos) assertTrue(result.selectedUtxos.all { it.confirmations > 0 }, "must not select the 0-conf UTXO") } + + @Test + fun buildSplitProducesOneOutputPerRecipientPlusChange() { + // Three distinct recipients derived from low-entropy private keys; not + // for production use but fine for shape assertions. + val r1 = + Secp256k1Instance + .compressedPubKeyFor("000000000000000000000000000000000000000000000000000000000000000b".hexToByteArray()) + .copyOfRange(1, 33) + .toHexKey() + val r2 = + Secp256k1Instance + .compressedPubKeyFor("000000000000000000000000000000000000000000000000000000000000000c".hexToByteArray()) + .copyOfRange(1, 33) + .toHexKey() + val r3 = + Secp256k1Instance + .compressedPubKeyFor("000000000000000000000000000000000000000000000000000000000000000d".hexToByteArray()) + .copyOfRange(1, 33) + .toHexKey() + + val utxos = listOf(utxo(1_000_000L, 1)) + val result = + OnchainZapBuilder.buildSplit( + senderPubKey = senderPubKey, + recipients = listOf(r1 to 50_000L, r2 to 30_000L, r3 to 20_000L), + feeRateSatPerVByte = 5.0, + availableUtxos = utxos, + ) + + assertEquals(100_000L, result.recipientSats) + assertTrue(result.changeSats > 0, "should have a change output") + + // Conservation: inputs == sum(outputs) + fee. + val inputSum = result.selectedUtxos.sumOf { it.valueSats } + assertEquals(inputSum, result.recipientSats + result.changeSats + result.feeSats) + + // 3 recipient outputs + 1 change. + val tx = result.psbt.unsignedTx + assertEquals(4, tx.outputs.size) + assertEquals(50_000L, tx.outputs[0].valueSats) + assertEquals(TaprootAddress.scriptPubKeyHexForRecipient(r1).lowercase(), tx.outputs[0].scriptPubKey.toHexKey()) + assertEquals(30_000L, tx.outputs[1].valueSats) + assertEquals(TaprootAddress.scriptPubKeyHexForRecipient(r2).lowercase(), tx.outputs[1].scriptPubKey.toHexKey()) + assertEquals(20_000L, tx.outputs[2].valueSats) + assertEquals(TaprootAddress.scriptPubKeyHexForRecipient(r3).lowercase(), tx.outputs[2].scriptPubKey.toHexKey()) + // Change is always the last output. + assertEquals(result.changeSats, tx.outputs[3].valueSats) + assertEquals(senderScriptHex, tx.outputs[3].scriptPubKey.toHexKey()) + } + + @Test + fun buildSplitRejectsDuplicateRecipients() { + val utxos = listOf(utxo(1_000_000L, 1)) + val ex = + assertFailsWith { + OnchainZapBuilder.buildSplit( + senderPubKey = senderPubKey, + recipients = listOf(recipientPubKey to 10_000L, recipientPubKey to 5_000L), + feeRateSatPerVByte = 5.0, + availableUtxos = utxos, + ) + } + assertTrue(ex.message!!.contains("distinct")) + } + + @Test + fun buildSplitRejectsBelowDustRecipient() { + val r2 = + Secp256k1Instance + .compressedPubKeyFor("000000000000000000000000000000000000000000000000000000000000000c".hexToByteArray()) + .copyOfRange(1, 33) + .toHexKey() + val utxos = listOf(utxo(1_000_000L, 1)) + assertFailsWith { + OnchainZapBuilder.buildSplit( + senderPubKey = senderPubKey, + recipients = listOf(recipientPubKey to 50_000L, r2 to 100L), + feeRateSatPerVByte = 5.0, + availableUtxos = utxos, + ) + } + } }