From 04d343e7c9b3e44e34115fe7ca32c27c8b71e544 Mon Sep 17 00:00:00 2001 From: andrew Date: Mon, 6 Jul 2026 03:24:21 -0300 Subject: [PATCH 1/8] add zswap test utils --- contracts/package.json | 3 +- contracts/test-utils/zswap.ts | 122 ++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 1 deletion(-) create mode 100644 contracts/test-utils/zswap.ts diff --git a/contracts/package.json b/contracts/package.json index 68324f6f..72d20a64 100644 --- a/contracts/package.json +++ b/contracts/package.json @@ -21,7 +21,8 @@ "homepage": "https://docs.openzeppelin.com/contracts-compact/", "type": "module", "imports": { - "#test-utils/address.js": "./test-utils/address.js" + "#test-utils/address.js": "./test-utils/address.js", + "#test-utils/zswap.js": "./test-utils/zswap.js" }, "scripts": { "compact": "compact-compiler --exclude '*/archive/*'", diff --git a/contracts/test-utils/zswap.ts b/contracts/test-utils/zswap.ts new file mode 100644 index 00000000..8526594b --- /dev/null +++ b/contracts/test-utils/zswap.ts @@ -0,0 +1,122 @@ +/** + * @description Test helpers for inspecting the Zswap inputs (coin spends / + * nullifiers) and outputs (coin commitments) a circuit produces in the + * simulator. + * + * The dry (in-memory) simulator does NOT enforce the ledger's nullifier set, + * so it will happily let a circuit spend the same coin twice — a node would + * reject that as a double spend. But the simulator DOES record every spend and + * every created coin in the circuit's `currentZswapLocalState`. Reading that + * lets a unit test catch a whole class of coin-handling bugs without a node: + * in particular, a circuit that emits a change coin, immediately re-spends it + * (revealing its nullifier), and then persists/returns that same coin as if it + * were still spendable. See the `ShieldedTreasury` / `ForwarderPrivate` + * change-handling regression tests. + */ + +type ShieldedCoinInfo = { nonce: Uint8Array; color: Uint8Array; value: bigint }; + +/** A coin consumed (spent) by a circuit; carries the Merkle-tree index. */ +export type ZswapInput = ShieldedCoinInfo & { mt_index: bigint }; + +/** A coin produced by a circuit, together with its recipient. */ +export type ZswapOutput = { + coinInfo: ShieldedCoinInfo; + recipient: { + is_left: boolean; + left: { bytes: Uint8Array }; + right: { bytes: Uint8Array }; + }; +}; + +/** The coins consumed and produced so far in a simulator's session. */ +export type ZswapLocalState = { + inputs: ZswapInput[]; + outputs: ZswapOutput[]; +}; + +/** + * @description Reads the accumulated Zswap local state out of a simulator. + * + * The state accumulates across circuit calls in a session (it is only reset + * when a per-call caller override is used), so callers that want the effect of + * a single circuit invocation should snapshot lengths before the call and slice + * off the newly appended entries (see {@link zswapDelta}). + * + * @param sim - A simulator produced by `createSimulator`. + * @returns The current `{ inputs, outputs }`. + */ +export function zswapLocalState(sim: unknown): ZswapLocalState { + // The circuit context lives on the wrapped synchronous simulator inside the + // backend; the public simulator surface intentionally does not re-export it. + const state = ( + sim as { + _backend?: { + sim?: { circuitContext?: { currentZswapLocalState?: ZswapLocalState } }; + }; + } + )?._backend?.sim?.circuitContext?.currentZswapLocalState; + if (!state) { + throw new Error( + 'Could not read currentZswapLocalState from simulator. This helper ' + + 'targets the dry (in-memory) backend produced by createSimulator.', + ); + } + return state as ZswapLocalState; +} + +/** Hex-encodes a byte string — handy for comparing coin/recipient bytes. */ +export const bytesToHex = (b: Uint8Array): string => + Buffer.from(b).toString('hex'); + +const toHex = bytesToHex; + +/** + * @description Captures a snapshot of the current input/output counts so a + * later {@link zswapDelta} can isolate the coins a single call produced. + * + * @param sim - A simulator produced by `createSimulator`. + * @returns The counts to pass to {@link zswapDelta}. + */ +export function zswapSnapshot(sim: unknown): { + inputs: number; + outputs: number; +} { + const { inputs, outputs } = zswapLocalState(sim); + return { inputs: inputs.length, outputs: outputs.length }; +} + +/** + * @description Returns the inputs and outputs appended since a snapshot — i.e. + * the coins consumed and produced by the call(s) made in between. + * + * @param sim - A simulator produced by `createSimulator`. + * @param snapshot - A snapshot from {@link zswapSnapshot} taken before the call. + * @returns The newly added `{ inputs, outputs }`. + */ +export function zswapDelta( + sim: unknown, + snapshot: { inputs: number; outputs: number }, +): ZswapLocalState { + const { inputs, outputs } = zswapLocalState(sim); + return { + inputs: inputs.slice(snapshot.inputs), + outputs: outputs.slice(snapshot.outputs), + }; +} + +/** + * @description Whether a coin with the given nonce appears among the spent + * inputs. A coin a contract intends to keep (a stored or returned change coin) + * must never be spent in the same transaction that creates it — if it is, its + * nullifier is already revealed and a node rejects the next spend as a double + * spend. + * + * @param inputs - The Zswap inputs to search (typically a {@link zswapDelta}). + * @param nonce - The nonce of the coin that is supposed to remain unspent. + * @returns `true` if the coin was spent. + */ +export function isNonceSpent(inputs: ZswapInput[], nonce: Uint8Array): boolean { + const target = toHex(nonce); + return inputs.some((i) => toHex(i.nonce) === target); +} From b2fefdee8026adf05e91f0ec840d15d30745bf26 Mon Sep 17 00:00:00 2001 From: andrew Date: Mon, 6 Jul 2026 04:12:07 -0300 Subject: [PATCH 2/8] fix change with drain and send --- .../src/multisig/ForwarderPrivate.compact | 22 +++++++++-------- .../src/multisig/ShieldedTreasury.compact | 18 +++++++------- .../ShieldedTreasuryStateless.compact | 24 +++++++++---------- 3 files changed, 33 insertions(+), 31 deletions(-) diff --git a/contracts/src/multisig/ForwarderPrivate.compact b/contracts/src/multisig/ForwarderPrivate.compact index a4c3d85c..e1159ab0 100644 --- a/contracts/src/multisig/ForwarderPrivate.compact +++ b/contracts/src/multisig/ForwarderPrivate.compact @@ -97,8 +97,10 @@ module ForwarderPrivate { * @description Spends a previously-deposited shielded coin to the parent, * a coin public key. The caller proves knowledge of `(parent, opSecret)` * matching the stored commitment; the coin is sent as a plain shielded note, - * so the parent stays hidden on-chain. If `coin.value` exceeds `value`, the - * change is re-emitted back to the contract for future drains. + * so the parent stays hidden on-chain. If `coin.value` exceeds `value`, + * `sendShielded` routes the change back to this contract as a self-owned + * output (dwelling at the contract address for future drains); it is + * returned untouched via `result.change` and must not be re-spent here. * * The parent is a `ZswapCoinPublicKey`, never a `ContractAddress`: a shielded * send to a contract publishes that contract's address in cleartext on the @@ -160,14 +162,14 @@ module ForwarderPrivate { disclose(value) ); - if (disclose(result.change.is_some)) { - sendImmediateShielded( - disclose(result.change.value), - Utils_selfAsRecipient(), - disclose(result.change.value.value) - ); - } - + // `sendShielded` already emits any change as a fresh output owned by this + // contract (dwelling at the contract address for future drains). `_drain` + // returns it untouched: it must not re-spend the change here and then hand + // that same (now-nullified) coin back via `result.change` — that is the + // double spend a node rejects on the next drain. A caller is free to spend + // the returned change onward itself (e.g. route it to another recipient); + // the rule is only that the change must never be re-spent AND still treated + // as held. return result; } diff --git a/contracts/src/multisig/ShieldedTreasury.compact b/contracts/src/multisig/ShieldedTreasury.compact index e6418fbf..6180cc56 100644 --- a/contracts/src/multisig/ShieldedTreasury.compact +++ b/contracts/src/multisig/ShieldedTreasury.compact @@ -85,8 +85,9 @@ module ShieldedTreasury { * @description Sends shielded tokens from the treasury. * * Looks up the stored coin by color, verifies sufficient value, - * and executes the shielded send. If the send produces change, - * it is sent back to the contract via `sendImmediateShielded`. + * and executes the shielded send. `sendShielded` already routes any + * change back to this contract as a self-owned output; the change coin + * is simply recorded in the UTXO map for the next spend. * * @notice Access control is NOT enforced here. * The consuming contract must gate this behind its own @@ -119,14 +120,13 @@ module ShieldedTreasury { const result = sendShielded(coin, disclose(recipient), disclose(amount)); + // `sendShielded` spends the stored coin (revealing its nullifier) and, + // when there is change, emits it as a fresh output owned by this + // contract. That change coin is already spendable. Do NOT re-spend it + // with `sendImmediateShielded`, which would reveal its nullifier in this + // same transaction and leave the recorded coin permanently double-spent. if (disclose(result.change.is_some)) { - const changeCoin = result.change.value; - sendImmediateShielded( - changeCoin, - Utils_selfAsRecipient(), - changeCoin.value - ); - _coins.insertCoin(disclose(color), changeCoin, Utils_selfAsRecipient()); + _coins.insertCoin(disclose(color), result.change.value, Utils_selfAsRecipient()); } else { _coins.remove(disclose(color)); } diff --git a/contracts/src/multisig/ShieldedTreasuryStateless.compact b/contracts/src/multisig/ShieldedTreasuryStateless.compact index fd334b22..4a058b13 100644 --- a/contracts/src/multisig/ShieldedTreasuryStateless.compact +++ b/contracts/src/multisig/ShieldedTreasuryStateless.compact @@ -20,7 +20,6 @@ pragma language_version >= 0.23.0; */ module ShieldedTreasuryStateless { import CompactStandardLibrary; - import { selfAsRecipient } from "../utils/Utils" prefix Utils_; // ─── Deposit ──────────────────────────────────────────────────── @@ -38,9 +37,11 @@ module ShieldedTreasuryStateless { * * The coin is supplied by the caller rather than looked up from * on-chain state; the token color is derived from `coin.color`. - * Executes the shielded send and, if change is produced, returns it - * to the contract via `sendImmediateShielded`. No balance or - * received/sent accounting is tracked on the ledger. + * Executes the shielded send and returns the result. `sendShielded` + * already routes any change back to this contract as a self-owned + * output; the caller must persist `result.change` and pass it back as + * the `coin` for the next spend. No balance or received/sent + * accounting is tracked on the ledger. * * @notice Access control is NOT enforced here. * The consuming contract must gate this behind its own @@ -63,14 +64,13 @@ module ShieldedTreasuryStateless { ): ShieldedSendResult { const result = sendShielded(disclose(coin), disclose(recipient), disclose(amount)); - if (disclose(result.change.is_some)) { - sendImmediateShielded( - disclose(result.change.value), - Utils_selfAsRecipient(), - disclose(result.change.value.value) - ); - } - + // `sendShielded` already emits any change as a fresh, self-owned output. + // `_send` returns it untouched: it must not re-spend the change here and + // then hand that same (now-nullified) coin back via `result.change`. That + // is the double spend a node rejects on the next spend. The implementing + // contract decides what to do with the returned change: persist it and + // spend it later, or route it onward itself. The rule is only that the + // change must never be re-spent AND still treated as held. return result; } } From 45dba91852b7a7318bbc13b2a5e3c4f0b43abc4d Mon Sep 17 00:00:00 2001 From: andrew Date: Mon, 6 Jul 2026 04:14:52 -0300 Subject: [PATCH 3/8] add route change circuit to mocks --- .../test/mocks/MockForwarderPrivate.compact | 24 +++++++++++++++++++ .../MockShieldedTreasuryStateless.compact | 24 +++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/contracts/src/multisig/test/mocks/MockForwarderPrivate.compact b/contracts/src/multisig/test/mocks/MockForwarderPrivate.compact index c6a62ff9..887a354a 100644 --- a/contracts/src/multisig/test/mocks/MockForwarderPrivate.compact +++ b/contracts/src/multisig/test/mocks/MockForwarderPrivate.compact @@ -40,6 +40,30 @@ export circuit drain( return ForwarderPrivate__drain(coin, parent, opSecret, value); } +// Demonstrates how an implementing contract drains to the parent, +// then uses the returned `result.change` to route the change onward +// to a *different* recipient in the same transaction (the burn()-style +// pattern). Only possible because `_drain` hands back a live, unspent change +// coin. Returns the onward send's result, whose `sent` is the coin delivered to +// `changeRecipient`. +export circuit drainAndRouteChange( + coin: QualifiedShieldedCoinInfo, + parent: ZswapCoinPublicKey, + opSecret: Bytes<32>, + value: Uint<128>, + changeRecipient: Either +): ShieldedSendResult { + const drainRes = ForwarderPrivate__drain(coin, parent, opSecret, value); + if (disclose(drainRes.change.is_some)) { + return sendImmediateShielded( + disclose(drainRes.change.value), + disclose(changeRecipient), + disclose(drainRes.change.value.value) + ); + } + return drainRes; +} + export pure circuit calculateParentCommitment( parentAddr: Bytes<32>, opSecret: Bytes<32> diff --git a/contracts/src/multisig/test/mocks/MockShieldedTreasuryStateless.compact b/contracts/src/multisig/test/mocks/MockShieldedTreasuryStateless.compact index 403ca14e..9e6255f5 100644 --- a/contracts/src/multisig/test/mocks/MockShieldedTreasuryStateless.compact +++ b/contracts/src/multisig/test/mocks/MockShieldedTreasuryStateless.compact @@ -22,3 +22,27 @@ export circuit _send( return Treasury__send(coin, recipient, amount); } +// Demonstrates how an implementing contract drains to the parent, +// Demonstrates how an implementing contract sends, then uses the +// returned `result.change` to route the change onward to a *different* +// recipient in the same transaction (the burn()-style pattern). Only +// possible because `_send` hands back a live, unspent change coin. Returns +// the onward send's result, whose `sent` is the coin delivered to +// `changeRecipient`. +export circuit _sendAndRouteChange( + coin: QualifiedShieldedCoinInfo, + recipient: Either, + amount: Uint<128>, + changeRecipient: Either +): ShieldedSendResult { + const sendRes = Treasury__send(coin, recipient, amount); + if (disclose(sendRes.change.is_some)) { + return sendImmediateShielded( + disclose(sendRes.change.value), + disclose(changeRecipient), + disclose(sendRes.change.value.value) + ); + } + return sendRes; +} + From fad04ccb2c9c70aa31ee9df5cecf81cdcbd71f66 Mon Sep 17 00:00:00 2001 From: andrew Date: Mon, 6 Jul 2026 04:27:22 -0300 Subject: [PATCH 4/8] add zswap i/o tests --- .../multisig/test/ForwarderPrivate.test.ts | 148 ++++++++++++++++++ .../MockForwarderPrivateSimulator.ts | 20 +++ 2 files changed, 168 insertions(+) diff --git a/contracts/src/multisig/test/ForwarderPrivate.test.ts b/contracts/src/multisig/test/ForwarderPrivate.test.ts index 92ffc2e6..ff6d871c 100644 --- a/contracts/src/multisig/test/ForwarderPrivate.test.ts +++ b/contracts/src/multisig/test/ForwarderPrivate.test.ts @@ -1,6 +1,12 @@ import fc from 'fast-check'; import { beforeEach, describe, expect, it } from 'vitest'; import * as utils from '#test-utils/address.js'; +import { + bytesToHex, + isNonceSpent, + zswapDelta, + zswapSnapshot, +} from '#test-utils/zswap.js'; import { MockForwarderPrivateSimulator } from './simulators/MockForwarderPrivateSimulator.js'; // The drain parent is a `ZswapCoinPublicKey` (coin public key only). A contract @@ -282,6 +288,148 @@ describe('ForwarderPrivate module', () => { }); }); + // Regression: a partial drain must return a change coin that is still + // spendable. `sendShielded` routes change back to the contract as a + // self-owned output; re-spending it here with `sendImmediateShielded` would + // reveal its nullifier in the same tx, so the `result.change` handed + // back would be a double-spent coin a node rejects on the next drain. The dry + // simulator does not enforce nullifiers, so we assert on the recorded Zswap + // I/O: the change coin's nonce must not appear among the spent inputs. + describe('drain — change coin is spendable (no double spend)', () => { + // A non-zero deploy address so the change output (routed to self for future + // drains) carries a recognizable address rather than the zero + // `dummyContractAddress()` default. + const FORWARDER_ADDRESS = '3e'.repeat(32); + let mock: MockForwarderPrivateSimulator; + + beforeEach(async () => { + mock = await MockForwarderPrivateSimulator.create( + commitment(PARENT_BYTES, OP_SECRET), + true, + { contractAddress: FORWARDER_ADDRESS }, + ); + await mock.deposit(makeCoin(COLOR, AMOUNT)); + }); + + it('should send the note to the parent and route the change back to itself on a partial drain', async () => { + const snap = zswapSnapshot(mock); + const result = await mock.drain( + makeQualifiedCoin(COLOR, AMOUNT, 0n), + key(PARENT_BYTES), + OP_SECRET, + 400n, + ); + const { inputs, outputs } = zswapDelta(mock, snap); + + // One coin consumed: the drained coin (nonce 0, full value). + expect(inputs).toHaveLength(1); + expect(inputs[0].value).toBe(AMOUNT); + expect(inputs[0].color).toStrictEqual(COLOR); + expect(inputs[0].nonce).toStrictEqual(new Uint8Array(32).fill(0)); + + // Two coins produced: the note to the parent key (`left` arm) and the + // change back to this contract (`right`/self arm). + expect(outputs).toHaveLength(2); + const toParent = outputs.filter((o) => o.recipient.is_left); + const toSelf = outputs.filter((o) => !o.recipient.is_left); + expect(toParent).toHaveLength(1); + expect(toSelf).toHaveLength(1); + + // Note: 400 of COLOR to the parent key; equals result.sent. + expect(toParent[0].coinInfo.value).toBe(400n); + expect(toParent[0].coinInfo).toStrictEqual(result.sent); + expect(toParent[0].recipient.left.bytes).toStrictEqual(PARENT_BYTES); + + // Change: the remainder back to THIS contract's address (for future + // drains), identical to the returned change coin, and NOT spent in this + // same tx. + expect(result.change.is_some).toBe(true); + expect(toSelf[0].coinInfo.value).toBe(AMOUNT - 400n); + expect(toSelf[0].coinInfo).toStrictEqual(result.change.value); + expect(bytesToHex(toSelf[0].recipient.right.bytes)).toBe( + FORWARDER_ADDRESS, + ); + expect(isNonceSpent(inputs, result.change.value.nonce)).toBe(false); + }); + + it('should spend the coin and produce only the note when draining in full', async () => { + const snap = zswapSnapshot(mock); + const result = await mock.drain( + makeQualifiedCoin(COLOR, AMOUNT, 0n), + key(PARENT_BYTES), + OP_SECRET, + AMOUNT, + ); + const { inputs, outputs } = zswapDelta(mock, snap); + + // No change: one input (the drained coin), one output (the note to parent). + expect(result.change.is_some).toBe(false); + expect(inputs).toHaveLength(1); + expect(inputs[0].value).toBe(AMOUNT); + expect(outputs).toHaveLength(1); + expect(outputs[0].recipient.is_left).toBe(true); + expect(outputs[0].recipient.left.bytes).toStrictEqual(PARENT_BYTES); + expect(outputs[0].coinInfo.value).toBe(AMOUNT); + expect(outputs[0].coinInfo).toStrictEqual(result.sent); + }); + }); + + // Tests that `_drain` (inner call in the mock `drainAndRouteChange`) returns a live, + // unspent change coin, so an implementing contract can route it onward to a different + // recipient in the same tx (the burn()-style pattern). This would be impossible if `_drain` + // handed back a coin it had already spent. + describe('drain — implementing contract routes the change onward', () => { + const CHANGE_DEST = utils.createEitherTestUser('CHANGE_DEST'); + let mock: MockForwarderPrivateSimulator; + + beforeEach(async () => { + mock = await freshMock(PARENT_BYTES); + }); + + it('should let the caller send the change to a different recipient using the drain result', async () => { + const snap = zswapSnapshot(mock); + const routed = await mock.drainAndRouteChange( + makeQualifiedCoin(COLOR, AMOUNT, 0n), + key(PARENT_BYTES), + OP_SECRET, + 400n, + CHANGE_DEST, + ); + const { inputs, outputs } = zswapDelta(mock, snap); + + // The onward send delivered the whole change to `changeRecipient`, so + // nothing is left over. + expect(routed.change.is_some).toBe(false); + expect(routed.sent.value).toBe(AMOUNT - 400n); + + // Two coins are consumed: the drained coin, then the change coin (spent + // to route it onward — spending it here is correct, unlike the keep-change + // path). + expect(inputs).toHaveLength(2); + + // The note still went to the parent for the drained `value`... + const toParent = outputs.filter( + (o) => + o.recipient.is_left && + bytesToHex(o.recipient.left.bytes) === bytesToHex(PARENT_BYTES), + ); + expect(toParent).toHaveLength(1); + expect(toParent[0].coinInfo.value).toBe(400n); + + // ...and the change was routed onward to `changeRecipient`, matching the + // returned coin. + const toChangeDest = outputs.filter( + (o) => + o.recipient.is_left && + bytesToHex(o.recipient.left.bytes) === + bytesToHex(CHANGE_DEST.left.bytes), + ); + expect(toChangeDest).toHaveLength(1); + expect(toChangeDest[0].coinInfo.value).toBe(AMOUNT - 400n); + expect(toChangeDest[0].coinInfo).toStrictEqual(routed.sent); + }); + }); + describe('property: change arithmetic', () => { it('should preserve change.value == coin.value - drain.value on partial drain', async () => { await fc.assert( diff --git a/contracts/src/multisig/test/simulators/MockForwarderPrivateSimulator.ts b/contracts/src/multisig/test/simulators/MockForwarderPrivateSimulator.ts index 7f3e2db7..a0e0d4c7 100644 --- a/contracts/src/multisig/test/simulators/MockForwarderPrivateSimulator.ts +++ b/contracts/src/multisig/test/simulators/MockForwarderPrivateSimulator.ts @@ -70,6 +70,26 @@ export class MockForwarderPrivateSimulator extends MockForwarderPrivateSimulator return this.circuits.impure.drain(coin, parent, opSecret, value); } + public drainAndRouteChange( + coin: QualifiedShieldedCoinInfo, + parent: ZswapCoinPublicKey, + opSecret: Uint8Array, + value: bigint, + changeRecipient: { + is_left: boolean; + left: { bytes: Uint8Array }; + right: { bytes: Uint8Array }; + }, + ): Promise { + return this.circuits.impure.drainAndRouteChange( + coin, + parent, + opSecret, + value, + changeRecipient, + ); + } + public getParentCommitment(): Promise { return this.circuits.impure.getParentCommitment(); } From 67d1115e4e13e05bbc6e5609e98c383567c511af Mon Sep 17 00:00:00 2001 From: andrew Date: Mon, 6 Jul 2026 04:29:07 -0300 Subject: [PATCH 5/8] add zswap i/o tests to shielded treasury --- .../multisig/test/ShieldedTreasury.test.ts | 110 +++++++++++++++++- 1 file changed, 109 insertions(+), 1 deletion(-) diff --git a/contracts/src/multisig/test/ShieldedTreasury.test.ts b/contracts/src/multisig/test/ShieldedTreasury.test.ts index 6295e7e2..e620c061 100644 --- a/contracts/src/multisig/test/ShieldedTreasury.test.ts +++ b/contracts/src/multisig/test/ShieldedTreasury.test.ts @@ -1,11 +1,23 @@ import { beforeEach, describe, expect, it } from 'vitest'; import * as utils from '#test-utils/address.js'; +import { + bytesToHex, + isNonceSpent, + zswapDelta, + zswapSnapshot, +} from '#test-utils/zswap.js'; import { ShieldedTreasurySimulator } from './simulators/ShieldedTreasurySimulator.js'; const COLOR = new Uint8Array(32).fill(1); const COLOR2 = new Uint8Array(32).fill(2); const AMOUNT = 1000n; +// A non-zero deploy address so the change output (routed to self via +// `selfAsRecipient()`) carries a recognizable address instead of the zero +// `dummyContractAddress()` default — lets the tests assert change returns to +// THIS contract, not merely to "some contract arm". +const TREASURY_ADDRESS = '7a'.repeat(32); + const Z_RECIPIENT = utils.createEitherTestUser('RECIPIENT'); function makeCoin( @@ -24,7 +36,9 @@ let treasury: ShieldedTreasurySimulator; describe('ShieldedTreasury', () => { beforeEach(async () => { - treasury = await ShieldedTreasurySimulator.create(); + treasury = await ShieldedTreasurySimulator.create({ + contractAddress: TREASURY_ADDRESS, + }); }); describe('initial state', () => { @@ -124,6 +138,100 @@ describe('ShieldedTreasury', () => { }); }); + // Regression: a partial `_send` must not re-spend the change coin it stores. + // + // `sendShielded` already emits change as a self-owned output; re-spending it + // with `sendImmediateShielded` reveals the stored coin's nullifier in the + // same transaction, so the next spend of that stored coin is a double spend a + // node rejects with `Zswap(NullifierAlreadyPresent)`. The dry simulator does + // not enforce nullifiers, so we assert on the recorded Zswap I/O instead: the + // spend of the change coin shows up as an extra input carrying its nonce. + describe('_send — change coin is spendable (no double spend)', () => { + beforeEach(async () => { + await treasury._deposit(makeCoin(COLOR, 400n)); + }); + + it('should spend the stored coin and route the change back to itself on a partial send', async () => { + const snap = zswapSnapshot(treasury); + const result = await treasury._send(Z_RECIPIENT, COLOR, 150n); + const { inputs, outputs } = zswapDelta(treasury, snap); + + // Exactly one coin is consumed: the stored 400 balance (nonce 0). + expect(inputs).toHaveLength(1); + expect(inputs[0].value).toBe(400n); + expect(inputs[0].color).toStrictEqual(COLOR); + expect(inputs[0].nonce).toStrictEqual(new Uint8Array(32).fill(0)); + + // Two coins are produced: the payment (to the recipient key, `left` arm) + // and the change (back to this contract, `right`/self arm). + expect(outputs).toHaveLength(2); + const toRecipient = outputs.filter((o) => o.recipient.is_left); + const toSelf = outputs.filter((o) => !o.recipient.is_left); + expect(toRecipient).toHaveLength(1); + expect(toSelf).toHaveLength(1); + + // Payment: 150 of COLOR to the intended recipient key; equals result.sent. + expect(toRecipient[0].coinInfo.value).toBe(150n); + expect(toRecipient[0].coinInfo.color).toStrictEqual(COLOR); + expect(toRecipient[0].coinInfo).toStrictEqual(result.sent); + expect(toRecipient[0].recipient.left.bytes).toStrictEqual( + Z_RECIPIENT.left.bytes, + ); + + // Change: 250 of COLOR routed back to THIS contract's address; identical + // to the returned change coin, and crucially NOT spent in this same tx. + expect(result.change.is_some).toBe(true); + expect(toSelf[0].coinInfo.value).toBe(250n); + expect(toSelf[0].coinInfo).toStrictEqual(result.change.value); + expect(bytesToHex(toSelf[0].recipient.right.bytes)).toBe( + TREASURY_ADDRESS, + ); + expect(isNonceSpent(inputs, result.change.value.nonce)).toBe(false); + }); + + it('should spend exactly the stored change coin on a follow-up spend', async () => { + const first = await treasury._send(Z_RECIPIENT, COLOR, 150n); // 250 change stored + const storedChange = first.change.value; + + const snap = zswapSnapshot(treasury); + const second = await treasury._send(Z_RECIPIENT, COLOR, 250n); // spend the change + const { inputs, outputs } = zswapDelta(treasury, snap); + + // A node would reject this as a double spend if the 250 change coin had + // already been nullified by the first send. The single input must be + // exactly that stored change coin (same nonce/value/color). + expect(inputs).toHaveLength(1); + expect(inputs[0].value).toBe(250n); + expect(inputs[0].color).toStrictEqual(COLOR); + expect(inputs[0].nonce).toStrictEqual(storedChange.nonce); + + // Full spend of the change: one output to the recipient, no further change. + expect(second.change.is_some).toBe(false); + expect(outputs).toHaveLength(1); + expect(outputs[0].recipient.is_left).toBe(true); + expect(outputs[0].coinInfo.value).toBe(250n); + expect(await treasury.getTokenBalance(COLOR)).toBe(0n); + }); + + it('should spend the balance and produce only the payment when sending in full', async () => { + const snap = zswapSnapshot(treasury); + const result = await treasury._send(Z_RECIPIENT, COLOR, 400n); + const { inputs, outputs } = zswapDelta(treasury, snap); + + // No change: one input (the 400 balance), one output (the payment). + expect(result.change.is_some).toBe(false); + expect(inputs).toHaveLength(1); + expect(inputs[0].value).toBe(400n); + expect(outputs).toHaveLength(1); + expect(outputs[0].recipient.is_left).toBe(true); + expect(outputs[0].recipient.left.bytes).toStrictEqual( + Z_RECIPIENT.left.bytes, + ); + expect(outputs[0].coinInfo.value).toBe(400n); + expect(outputs[0].coinInfo).toStrictEqual(result.sent); + }); + }); + describe('accounting consistency', () => { it('should keep receivedMinusSent equal to balance', async () => { await treasury._deposit(makeCoin(COLOR, 500n)); From 7ca9a7e6fd79f865435f47391b4830dc1bcdc170 Mon Sep 17 00:00:00 2001 From: andrew Date: Mon, 6 Jul 2026 04:29:47 -0300 Subject: [PATCH 6/8] add shielded treasury stateless sim and tests --- .../test/ShieldedTreasuryStateless.test.ts | 202 ++++++++++++++++++ .../MockShieldedTreasuryStatelessSimulator.ts | 90 ++++++++ 2 files changed, 292 insertions(+) create mode 100644 contracts/src/multisig/test/ShieldedTreasuryStateless.test.ts create mode 100644 contracts/src/multisig/test/simulators/MockShieldedTreasuryStatelessSimulator.ts diff --git a/contracts/src/multisig/test/ShieldedTreasuryStateless.test.ts b/contracts/src/multisig/test/ShieldedTreasuryStateless.test.ts new file mode 100644 index 00000000..532b7e5e --- /dev/null +++ b/contracts/src/multisig/test/ShieldedTreasuryStateless.test.ts @@ -0,0 +1,202 @@ +import { beforeEach, describe, expect, it } from 'vitest'; +import * as utils from '#test-utils/address.js'; +import { + bytesToHex, + isNonceSpent, + zswapDelta, + zswapSnapshot, +} from '#test-utils/zswap.js'; +import { MockShieldedTreasuryStatelessSimulator } from './simulators/MockShieldedTreasuryStatelessSimulator.js'; + +const COLOR = new Uint8Array(32).fill(1); +const AMOUNT = 1000n; + +// A non-zero deploy address so the change output (routed to self) carries a +// recognizable address rather than the zero `dummyContractAddress()` default. +const TREASURY_ADDRESS = '5c'.repeat(32); + +const Z_RECIPIENT = utils.createEitherTestUser('RECIPIENT'); + +function makeCoin(color: Uint8Array, value: bigint, nonce?: Uint8Array) { + return { nonce: nonce ?? new Uint8Array(32).fill(0), color, value }; +} + +function makeQualifiedCoin( + color: Uint8Array, + value: bigint, + mtIndex: bigint, + nonce?: Uint8Array, +) { + return { + nonce: nonce ?? new Uint8Array(32).fill(0), + color, + value, + mt_index: mtIndex, + }; +} + +let treasury: MockShieldedTreasuryStatelessSimulator; + +describe('ShieldedTreasuryStateless', () => { + beforeEach(async () => { + treasury = await MockShieldedTreasuryStatelessSimulator.create({ + contractAddress: TREASURY_ADDRESS, + }); + await treasury._deposit(makeCoin(COLOR, AMOUNT)); + }); + + describe('_send', () => { + it('should send the requested amount and return change', async () => { + const result = await treasury._send( + makeQualifiedCoin(COLOR, AMOUNT, 0n), + Z_RECIPIENT, + 400n, + ); + expect(result.sent.value).toBe(400n); + expect(result.sent.color).toStrictEqual(COLOR); + expect(result.change.is_some).toBe(true); + expect(result.change.value.value).toBe(AMOUNT - 400n); + expect(result.change.value.color).toStrictEqual(COLOR); + }); + + it('should produce no change when sending the full value', async () => { + const result = await treasury._send( + makeQualifiedCoin(COLOR, AMOUNT, 0n), + Z_RECIPIENT, + AMOUNT, + ); + expect(result.change.is_some).toBe(false); + }); + + it('should fail when amount exceeds coin value', async () => { + await expect( + treasury._send( + makeQualifiedCoin(COLOR, AMOUNT, 0n), + Z_RECIPIENT, + AMOUNT + 1n, + ), + ).rejects.toThrow(); + }); + }); + + // Regression: `_send` returns `result.change` to the caller to persist and + // spend next. `sendShielded` already routes change back to the contract as a + // self-owned output; re-spending it here with `sendImmediateShielded` would + // reveal its nullifier in the same tx, so the returned change coin + // would be a double-spent coin a node rejects on the next spend. The dry + // simulator does not enforce nullifiers, so we assert on the recorded Zswap + // I/O: the returned change coin's nonce must not appear among the spent + // inputs. + describe('_send — change coin is spendable (no double spend)', () => { + it('should spend the supplied coin and route the change back to itself on a partial send', async () => { + const snap = zswapSnapshot(treasury); + const result = await treasury._send( + makeQualifiedCoin(COLOR, AMOUNT, 0n), + Z_RECIPIENT, + 400n, + ); + const { inputs, outputs } = zswapDelta(treasury, snap); + + // One coin consumed: the supplied coin (nonce 0, full value). + expect(inputs).toHaveLength(1); + expect(inputs[0].value).toBe(AMOUNT); + expect(inputs[0].color).toStrictEqual(COLOR); + expect(inputs[0].nonce).toStrictEqual(new Uint8Array(32).fill(0)); + + // Two coins produced: the payment (recipient key, `left` arm) and the + // change (back to this contract, `right`/self arm). + expect(outputs).toHaveLength(2); + const toRecipient = outputs.filter((o) => o.recipient.is_left); + const toSelf = outputs.filter((o) => !o.recipient.is_left); + expect(toRecipient).toHaveLength(1); + expect(toSelf).toHaveLength(1); + + // Payment: 400 of COLOR to the recipient key; equals result.sent. + expect(toRecipient[0].coinInfo.value).toBe(400n); + expect(toRecipient[0].coinInfo).toStrictEqual(result.sent); + expect(toRecipient[0].recipient.left.bytes).toStrictEqual( + Z_RECIPIENT.left.bytes, + ); + + // Change: the remainder back to THIS contract's address, identical to + // the returned change coin, and NOT spent in this same tx. + expect(result.change.is_some).toBe(true); + expect(toSelf[0].coinInfo.value).toBe(AMOUNT - 400n); + expect(toSelf[0].coinInfo).toStrictEqual(result.change.value); + expect(bytesToHex(toSelf[0].recipient.right.bytes)).toBe( + TREASURY_ADDRESS, + ); + expect(isNonceSpent(inputs, result.change.value.nonce)).toBe(false); + }); + + it('should spend the coin and produce only the payment when sending in full', async () => { + const snap = zswapSnapshot(treasury); + const result = await treasury._send( + makeQualifiedCoin(COLOR, AMOUNT, 0n), + Z_RECIPIENT, + AMOUNT, + ); + const { inputs, outputs } = zswapDelta(treasury, snap); + + // No change: one input (the supplied coin), one output (the payment). + expect(result.change.is_some).toBe(false); + expect(inputs).toHaveLength(1); + expect(inputs[0].value).toBe(AMOUNT); + expect(outputs).toHaveLength(1); + expect(outputs[0].recipient.is_left).toBe(true); + expect(outputs[0].recipient.left.bytes).toStrictEqual( + Z_RECIPIENT.left.bytes, + ); + expect(outputs[0].coinInfo.value).toBe(AMOUNT); + expect(outputs[0].coinInfo).toStrictEqual(result.sent); + }); + }); + + // The fix returns a live, unspent change coin, so an implementing contract can + // route it onward to a different recipient in the same tx (the burn()-style pattern). + // This would be impossible if `_send` handed back a coin it had already spent. + describe('_send — implementing contract routes the change onward', () => { + const CHANGE_DEST = utils.createEitherTestUser('CHANGE_DEST'); + + it('should let the caller send the change to a different recipient using the send result', async () => { + const snap = zswapSnapshot(treasury); + const routed = await treasury._sendAndRouteChange( + makeQualifiedCoin(COLOR, AMOUNT, 0n), + Z_RECIPIENT, + 400n, + CHANGE_DEST, + ); + const { inputs, outputs } = zswapDelta(treasury, snap); + + // The onward send delivered the whole change to `changeRecipient`. + expect(routed.change.is_some).toBe(false); + expect(routed.sent.value).toBe(AMOUNT - 400n); + + // Two coins consumed: the supplied coin, then the change coin (spent to + // route it onward — correct here, unlike the keep-change path). + expect(inputs).toHaveLength(2); + + // The payment still went to the original recipient for `amount`... + const toRecipient = outputs.filter( + (o) => + o.recipient.is_left && + bytesToHex(o.recipient.left.bytes) === + bytesToHex(Z_RECIPIENT.left.bytes), + ); + expect(toRecipient).toHaveLength(1); + expect(toRecipient[0].coinInfo.value).toBe(400n); + + // ...and the change was routed onward to `changeRecipient`, matching the + // returned coin. + const toChangeDest = outputs.filter( + (o) => + o.recipient.is_left && + bytesToHex(o.recipient.left.bytes) === + bytesToHex(CHANGE_DEST.left.bytes), + ); + expect(toChangeDest).toHaveLength(1); + expect(toChangeDest[0].coinInfo.value).toBe(AMOUNT - 400n); + expect(toChangeDest[0].coinInfo).toStrictEqual(routed.sent); + }); + }); +}); diff --git a/contracts/src/multisig/test/simulators/MockShieldedTreasuryStatelessSimulator.ts b/contracts/src/multisig/test/simulators/MockShieldedTreasuryStatelessSimulator.ts new file mode 100644 index 00000000..dfff1ad6 --- /dev/null +++ b/contracts/src/multisig/test/simulators/MockShieldedTreasuryStatelessSimulator.ts @@ -0,0 +1,90 @@ +import { + createSimulator, + type SimulatorOptions, +} from '@openzeppelin/compact-simulator'; +import { + ledger, + Contract as MockShieldedTreasuryStateless, +} from '../../../../artifacts/MockShieldedTreasuryStateless/contract/index.js'; +import { + ShieldedTreasuryPrivateState, + ShieldedTreasuryWitnesses, +} from '../witnesses/ShieldedTreasuryWitnesses.js'; + +type ShieldedCoinInfo = { nonce: Uint8Array; color: Uint8Array; value: bigint }; +type QualifiedShieldedCoinInfo = ShieldedCoinInfo & { mt_index: bigint }; +type ShieldedSendResult = { + change: { is_some: boolean; value: ShieldedCoinInfo }; + sent: ShieldedCoinInfo; +}; + +type ShieldedTreasuryStatelessArgs = readonly []; + +const MockShieldedTreasuryStatelessSimulatorBase = createSimulator< + ShieldedTreasuryPrivateState, + ReturnType, + ReturnType, + MockShieldedTreasuryStateless, + ShieldedTreasuryStatelessArgs +>({ + contractFactory: (witnesses) => + new MockShieldedTreasuryStateless(witnesses), + defaultPrivateState: () => ShieldedTreasuryPrivateState, + contractArgs: () => [], + ledgerExtractor: (state) => ledger(state), + witnessesFactory: () => ShieldedTreasuryWitnesses(), + artifactName: 'MockShieldedTreasuryStateless', +}); + +export class MockShieldedTreasuryStatelessSimulator extends MockShieldedTreasuryStatelessSimulatorBase { + static async create( + options: SimulatorOptions< + ShieldedTreasuryPrivateState, + ReturnType + > = {}, + ): Promise { + // biome-ignore lint/complexity/noThisInStatic: super.create must keep the subclass `this` + return super.create( + [], + options, + ) as Promise; + } + + public _deposit(coin: ShieldedCoinInfo): Promise<[]> { + return this.circuits.impure._deposit(coin); + } + + public _send( + coin: QualifiedShieldedCoinInfo, + recipient: { + is_left: boolean; + left: { bytes: Uint8Array }; + right: { bytes: Uint8Array }; + }, + amount: bigint, + ): Promise { + return this.circuits.impure._send(coin, recipient, amount); + } + + public _sendAndRouteChange( + coin: QualifiedShieldedCoinInfo, + recipient: { + is_left: boolean; + left: { bytes: Uint8Array }; + right: { bytes: Uint8Array }; + }, + amount: bigint, + changeRecipient: { + is_left: boolean; + left: { bytes: Uint8Array }; + right: { bytes: Uint8Array }; + }, + ): Promise { + return this.circuits.impure._sendAndRouteChange( + coin, + recipient, + amount, + changeRecipient, + ); + } +} From c0ea51792474d866e4c044915210c53c7066effd Mon Sep 17 00:00:00 2001 From: andrew Date: Mon, 6 Jul 2026 04:32:41 -0300 Subject: [PATCH 7/8] clean up comment --- .../multisig/test/mocks/MockShieldedTreasuryStateless.compact | 1 - 1 file changed, 1 deletion(-) diff --git a/contracts/src/multisig/test/mocks/MockShieldedTreasuryStateless.compact b/contracts/src/multisig/test/mocks/MockShieldedTreasuryStateless.compact index 9e6255f5..97c83e99 100644 --- a/contracts/src/multisig/test/mocks/MockShieldedTreasuryStateless.compact +++ b/contracts/src/multisig/test/mocks/MockShieldedTreasuryStateless.compact @@ -22,7 +22,6 @@ export circuit _send( return Treasury__send(coin, recipient, amount); } -// Demonstrates how an implementing contract drains to the parent, // Demonstrates how an implementing contract sends, then uses the // returned `result.change` to route the change onward to a *different* // recipient in the same transaction (the burn()-style pattern). Only From 5f1195ca9824b0cbb6601d20ed2140fe984cc3bc Mon Sep 17 00:00:00 2001 From: andrew Date: Tue, 7 Jul 2026 16:49:40 -0300 Subject: [PATCH 8/8] improve documentation --- .../src/multisig/ForwarderPrivate.compact | 6 +++-- .../src/multisig/ShieldedTreasury.compact | 7 ++++-- .../ShieldedTreasuryStateless.compact | 14 +++++++---- .../multisig/test/ForwarderPrivate.test.ts | 23 +++++++++--------- .../multisig/test/ShieldedTreasury.test.ts | 16 ++++++------- .../test/ShieldedTreasuryStateless.test.ts | 24 ++++++++++--------- .../test/mocks/MockForwarderPrivate.compact | 12 +++++----- .../MockShieldedTreasuryStateless.compact | 12 +++++----- contracts/test-utils/zswap.ts | 2 +- 9 files changed, 64 insertions(+), 52 deletions(-) diff --git a/contracts/src/multisig/ForwarderPrivate.compact b/contracts/src/multisig/ForwarderPrivate.compact index e1159ab0..9f2437ed 100644 --- a/contracts/src/multisig/ForwarderPrivate.compact +++ b/contracts/src/multisig/ForwarderPrivate.compact @@ -99,8 +99,10 @@ module ForwarderPrivate { * matching the stored commitment; the coin is sent as a plain shielded note, * so the parent stays hidden on-chain. If `coin.value` exceeds `value`, * `sendShielded` routes the change back to this contract as a self-owned - * output (dwelling at the contract address for future drains); it is - * returned untouched via `result.change` and must not be re-spent here. + * output; `_drain` returns it untouched via `result.change`. The + * implementing contract chooses what to do with it: leave it dwelling at + * the contract for a future drain, or spend it onward to a different + * recipient in the same transaction via `sendImmediateShielded`. * * The parent is a `ZswapCoinPublicKey`, never a `ContractAddress`: a shielded * send to a contract publishes that contract's address in cleartext on the diff --git a/contracts/src/multisig/ShieldedTreasury.compact b/contracts/src/multisig/ShieldedTreasury.compact index 6180cc56..511ac49f 100644 --- a/contracts/src/multisig/ShieldedTreasury.compact +++ b/contracts/src/multisig/ShieldedTreasury.compact @@ -86,8 +86,11 @@ module ShieldedTreasury { * * Looks up the stored coin by color, verifies sufficient value, * and executes the shielded send. `sendShielded` already routes any - * change back to this contract as a self-owned output; the change coin - * is simply recorded in the UTXO map for the next spend. + * change back to this contract as a self-owned output. Unlike the + * stateless variant, this treasury RETAINS the change: it is recorded in + * the UTXO map for the next spend. The returned `result.change` therefore + * belongs to the treasury and must not be re-spent by the caller (doing + * so would double-spend the recorded coin). * * @notice Access control is NOT enforced here. * The consuming contract must gate this behind its own diff --git a/contracts/src/multisig/ShieldedTreasuryStateless.compact b/contracts/src/multisig/ShieldedTreasuryStateless.compact index 4a058b13..5ca6da11 100644 --- a/contracts/src/multisig/ShieldedTreasuryStateless.compact +++ b/contracts/src/multisig/ShieldedTreasuryStateless.compact @@ -37,11 +37,15 @@ module ShieldedTreasuryStateless { * * The coin is supplied by the caller rather than looked up from * on-chain state; the token color is derived from `coin.color`. - * Executes the shielded send and returns the result. `sendShielded` - * already routes any change back to this contract as a self-owned - * output; the caller must persist `result.change` and pass it back as - * the `coin` for the next spend. No balance or received/sent - * accounting is tracked on the ledger. + * Executes the shielded send and returns the result untouched. + * `sendShielded` already routes any change back to this contract as a + * self-owned output, so `result.change` is a live, spendable coin. The + * implementing contract chooses what to do with it: persist it and pass + * it back as the `coin` for a later spend, or spend it onward to a + * different recipient in the same transaction via `sendImmediateShielded`. + * (Re-spending change is only meaningful when it goes somewhere other than + * self — `sendShielded` already routes it to self.) No balance or + * received/sent accounting is tracked on the ledger. * * @notice Access control is NOT enforced here. * The consuming contract must gate this behind its own diff --git a/contracts/src/multisig/test/ForwarderPrivate.test.ts b/contracts/src/multisig/test/ForwarderPrivate.test.ts index ff6d871c..23b2cacb 100644 --- a/contracts/src/multisig/test/ForwarderPrivate.test.ts +++ b/contracts/src/multisig/test/ForwarderPrivate.test.ts @@ -288,13 +288,13 @@ describe('ForwarderPrivate module', () => { }); }); - // Regression: a partial drain must return a change coin that is still - // spendable. `sendShielded` routes change back to the contract as a - // self-owned output; re-spending it here with `sendImmediateShielded` would - // reveal its nullifier in the same tx, so the `result.change` handed - // back would be a double-spent coin a node rejects on the next drain. The dry - // simulator does not enforce nullifiers, so we assert on the recorded Zswap - // I/O: the change coin's nonce must not appear among the spent inputs. + // `sendShielded` routes a partial drain's change back to the contract as a + // self-owned output, handed to the caller via `result.change`. That coin must + // stay spendable: if `_drain` also re-spent it with `sendImmediateShielded`, + // that would reveal its nullifier in the same transaction, making the returned + // coin a double spend the node rejects on the next drain. The dry simulator + // does not enforce nullifiers, so these tests read the recorded Zswap I/O: the + // change coin's nonce must not appear among the spent inputs. describe('drain — change coin is spendable (no double spend)', () => { // A non-zero deploy address so the change output (routed to self for future // drains) carries a recognizable address rather than the zero @@ -374,10 +374,11 @@ describe('ForwarderPrivate module', () => { }); }); - // Tests that `_drain` (inner call in the mock `drainAndRouteChange`) returns a live, - // unspent change coin, so an implementing contract can route it onward to a different - // recipient in the same tx (the burn()-style pattern). This would be impossible if `_drain` - // handed back a coin it had already spent. + // Tests that `_drain` (inner call in the mock `drainAndRouteChange`) returns a + // live, unspent change coin, so an implementing contract can spend it onward to + // a different recipient in the same tx (the only reason to re-spend change, + // since `sendShielded` already routes it to self). This would be impossible if + // `_drain` handed back a coin it had already spent. describe('drain — implementing contract routes the change onward', () => { const CHANGE_DEST = utils.createEitherTestUser('CHANGE_DEST'); let mock: MockForwarderPrivateSimulator; diff --git a/contracts/src/multisig/test/ShieldedTreasury.test.ts b/contracts/src/multisig/test/ShieldedTreasury.test.ts index e620c061..017723fd 100644 --- a/contracts/src/multisig/test/ShieldedTreasury.test.ts +++ b/contracts/src/multisig/test/ShieldedTreasury.test.ts @@ -138,14 +138,14 @@ describe('ShieldedTreasury', () => { }); }); - // Regression: a partial `_send` must not re-spend the change coin it stores. - // - // `sendShielded` already emits change as a self-owned output; re-spending it - // with `sendImmediateShielded` reveals the stored coin's nullifier in the - // same transaction, so the next spend of that stored coin is a double spend a - // node rejects with `Zswap(NullifierAlreadyPresent)`. The dry simulator does - // not enforce nullifiers, so we assert on the recorded Zswap I/O instead: the - // spend of the change coin shows up as an extra input carrying its nonce. + // `sendShielded` emits a partial spend's change as a fresh output owned by + // this contract, which `_send` records in `_coins` for the next spend. That + // recorded coin must stay spendable: if the circuit also re-spent it with + // `sendImmediateShielded`, that would reveal its nullifier in the same + // transaction, and the next spend of it would be a double spend the node + // rejects (`Zswap(NullifierAlreadyPresent)`). The dry simulator does not + // enforce nullifiers, so these tests read the recorded Zswap I/O: a re-spend + // of the change coin would show up as an extra input carrying its nonce. describe('_send — change coin is spendable (no double spend)', () => { beforeEach(async () => { await treasury._deposit(makeCoin(COLOR, 400n)); diff --git a/contracts/src/multisig/test/ShieldedTreasuryStateless.test.ts b/contracts/src/multisig/test/ShieldedTreasuryStateless.test.ts index 532b7e5e..c7edaa7f 100644 --- a/contracts/src/multisig/test/ShieldedTreasuryStateless.test.ts +++ b/contracts/src/multisig/test/ShieldedTreasuryStateless.test.ts @@ -79,14 +79,14 @@ describe('ShieldedTreasuryStateless', () => { }); }); - // Regression: `_send` returns `result.change` to the caller to persist and - // spend next. `sendShielded` already routes change back to the contract as a - // self-owned output; re-spending it here with `sendImmediateShielded` would - // reveal its nullifier in the same tx, so the returned change coin - // would be a double-spent coin a node rejects on the next spend. The dry - // simulator does not enforce nullifiers, so we assert on the recorded Zswap - // I/O: the returned change coin's nonce must not appear among the spent - // inputs. + // `_send` hands `result.change` back to the caller to persist and spend later. + // `sendShielded` routes that change back to the contract as a self-owned + // output, so it must stay spendable: if `_send` also re-spent it with + // `sendImmediateShielded`, that would reveal its nullifier in the same + // transaction, making the returned change a double spend the node rejects on + // the next spend. The dry simulator does not enforce nullifiers, so these + // tests read the recorded Zswap I/O: the returned change coin's nonce must not + // appear among the spent inputs. describe('_send — change coin is spendable (no double spend)', () => { it('should spend the supplied coin and route the change back to itself on a partial send', async () => { const snap = zswapSnapshot(treasury); @@ -152,9 +152,11 @@ describe('ShieldedTreasuryStateless', () => { }); }); - // The fix returns a live, unspent change coin, so an implementing contract can - // route it onward to a different recipient in the same tx (the burn()-style pattern). - // This would be impossible if `_send` handed back a coin it had already spent. + // `_send` hands back a live, unspent change coin, so an implementing contract + // can spend it onward to a different recipient in the same tx (the only reason + // to re-spend change, since `sendShielded` already routes it to self). If + // `_send` returned a coin it had already spent, this same-tx re-spend would be + // a double spend and fail. describe('_send — implementing contract routes the change onward', () => { const CHANGE_DEST = utils.createEitherTestUser('CHANGE_DEST'); diff --git a/contracts/src/multisig/test/mocks/MockForwarderPrivate.compact b/contracts/src/multisig/test/mocks/MockForwarderPrivate.compact index 887a354a..e4952922 100644 --- a/contracts/src/multisig/test/mocks/MockForwarderPrivate.compact +++ b/contracts/src/multisig/test/mocks/MockForwarderPrivate.compact @@ -40,12 +40,12 @@ export circuit drain( return ForwarderPrivate__drain(coin, parent, opSecret, value); } -// Demonstrates how an implementing contract drains to the parent, -// then uses the returned `result.change` to route the change onward -// to a *different* recipient in the same transaction (the burn()-style -// pattern). Only possible because `_drain` hands back a live, unspent change -// coin. Returns the onward send's result, whose `sent` is the coin delivered to -// `changeRecipient`. +// Demonstrates how an implementing contract drains to the parent, then uses the +// returned `result.change` to spend the change onward to a *different* recipient +// in the same transaction. (Re-spending change is only meaningful when it goes +// somewhere other than self — `sendShielded` already routes it to self.) Only +// possible because `_drain` hands back a live, unspent change coin. Returns the +// onward send's result, whose `sent` is the coin delivered to `changeRecipient`. export circuit drainAndRouteChange( coin: QualifiedShieldedCoinInfo, parent: ZswapCoinPublicKey, diff --git a/contracts/src/multisig/test/mocks/MockShieldedTreasuryStateless.compact b/contracts/src/multisig/test/mocks/MockShieldedTreasuryStateless.compact index 97c83e99..469dcb6d 100644 --- a/contracts/src/multisig/test/mocks/MockShieldedTreasuryStateless.compact +++ b/contracts/src/multisig/test/mocks/MockShieldedTreasuryStateless.compact @@ -22,12 +22,12 @@ export circuit _send( return Treasury__send(coin, recipient, amount); } -// Demonstrates how an implementing contract sends, then uses the -// returned `result.change` to route the change onward to a *different* -// recipient in the same transaction (the burn()-style pattern). Only -// possible because `_send` hands back a live, unspent change coin. Returns -// the onward send's result, whose `sent` is the coin delivered to -// `changeRecipient`. +// Demonstrates how an implementing contract sends, then uses the returned +// `result.change` to spend the change onward to a *different* recipient in the +// same transaction. (Re-spending change is only meaningful when it goes +// somewhere other than self — `sendShielded` already routes it to self.) Only +// possible because `_send` hands back a live, unspent change coin. Returns the +// onward send's result, whose `sent` is the coin delivered to `changeRecipient`. export circuit _sendAndRouteChange( coin: QualifiedShieldedCoinInfo, recipient: Either, diff --git a/contracts/test-utils/zswap.ts b/contracts/test-utils/zswap.ts index 8526594b..e315abd1 100644 --- a/contracts/test-utils/zswap.ts +++ b/contracts/test-utils/zswap.ts @@ -11,7 +11,7 @@ * in particular, a circuit that emits a change coin, immediately re-spends it * (revealing its nullifier), and then persists/returns that same coin as if it * were still spendable. See the `ShieldedTreasury` / `ForwarderPrivate` - * change-handling regression tests. + * "change coin is spendable" tests. */ type ShieldedCoinInfo = { nonce: Uint8Array; color: Uint8Array; value: bigint };