feat(token): add confidential note token core - #743
Open
0xisk wants to merge 16 commits into
Open
Conversation
Extract the note-based token core from the exploration draft (#679) onto a clean branch, with a full unit suite. Value lives as notes: `(value, nonce)` owned by `pk = Hf(sk)`, published only as a hiding commitment `cm = H(domain, value, nonce, pk)` in a `HistoricMerkleTree`. Spending publishes `nf = H(domain, nonce)` and proves membership in-circuit, so amounts, sender, and recipient all stay off the public ledger. The ledger carries the tree and the nullifier set and nothing else: no balances, no accounts, no supply. * `token/ConfidentialNoteFungibleToken.compact` — self-gated `transfer` and `burn`, plus the ungated `_mint` / `_mintNote` / `_transfer` / `_burn` / `_consumeNote` building blocks. No roles and no initialization; the composing contract supplies policy. * Domain tags read `OZ:note:*`; the draft's `OZ:cnt:*` is gone. * Mock, simulator, and witness harness. The witnesses close over a mutable wallet rather than simulator private state, so one spec file runs on the dry and live backends alike (`setPrivateState` throws on live). * 55 specs covering value conservation, single-spend, ownership, historical roots, path/commitment binding, nonce hygiene, and the unauthorized-consume primitive behind escrow-free clawback. Refs: #679 Closes #723
0xisk
added
enhancement
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a confidential note fungible token with commitment/nullifier ledgers, private mint, transfer, and burn circuits. It also adds simulators, witnesses, compatibility, privacy, invariant, concurrency, compiler metadata, rejection, and published-transaction test utilities. ChangesConfidential note token
Estimated code review effort: 5 (Critical) | ~90+ minutes Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Bring `ConfidentialNoteFungibleToken` up to the documentation convention the other token modules follow. No code change. * `@param` / `@return` on every circuit, `@type` on both ledger fields and all three structs, `@witness` + `@returns` on all four witnesses. * `@circuitInfo` moved ahead of the Requirements list, matching `FungibleToken`. * Requirements lists spell out the ownership and path preconditions the asserts enforce, not just the value ones. * `@warning` on each ungated block, stating what re-exporting it from a deployed contract would mean. Refs: #723
7 tasks
Asserts, per contract, which concurrent calls commute and which conflict
by design. A Compact transaction carries a fixed transcript built against
the state the wallet saw, so a conflict is a divergence between that
state and the state the transcript is applied to, not a wall-clock race.
Building two calls on one snapshot and applying them in order reproduces
it deterministically, with no node and no flake.
* test-utils/concurrency
- `types.ts` holds the vocabulary a spec writes against, so the same
cases can run on a second backend later.
- `DryReplayHarness.ts` applies a transcript through
`QueryContext.runTranscript`, the verifying-mode entry point the node
itself runs. It replays against current state rather than reusing the
state the build produced; reusing it would skip the pinned-read
checks and score every case as landed.
- A failed replay is classified by definition, not by matching error
text: re-replay against the snapshot the transcript was built on. If
it passes there, only the state changed, so it is a conflict; if it
fails there too, the transcript was never valid and the error is
rethrown. The runtime's messages are static data inside the wasm
binary with no exported type, so any pattern would be a copy of a
string we do not own.
- `parties.ts` is contract-agnostic: each party gets its own wallet and
its own contract instance bound to it, while the ledger they share
lives in the harness. Secrets derive from the party name so a failing
case reproduces.
* scope
`run_transcript` forwards only the program and the gas budget to
`query` and ignores the declared effects; the effects check runs in the
ledger after the transcript does. So this covers pinned reads only, and
an effects-divergence claim is inherently live-only.
* ConfidentialNoteFungibleToken
Seven cases, four commuting and three conflicting. The load-bearing one
is a spend landing after a concurrent mint: it guards the choice of
`HistoricMerkleTree` over `MerkleTree`, since plain `checkRoot` pins the
current root and every mint would then invalidate every in-flight
spend. Verified non-vacuous by compiling the mutated variant, which
flips that case to rejected.
Dry for now. The live backend needs the raw unproven-transaction path and
per-party wallet providers, neither of which the simulator exposes.
Refs: #749
Turns the module's privacy claims into executable assertions. The
functional suite asks what a circuit did; this asks what the chain gets
to see.
* four layers, weakest to strongest
- no secret's byte encoding appears in the public transcript,
- the transcript's shape does not vary with the secrets,
- two runs differing only in a secret differ only in hash digests,
- the serialized transaction the indexer stored carries no secret.
Layer 3 is the one that catches a value-dependent branch, the classic
leak in a Compact circuit: the branch bit shows up as a different
operation sequence even when no value is ever disclosed.
* how it reaches the evidence
A `Probe` drives the contract directly rather than through the
simulator, whose proxy discards `proofData` — the per-call record a real
transaction carries. `test-utils/harness/publishedTx.ts` is the other
end of the same claim: the serialized transaction as the indexer stored
it. Kept isolated the way `ledgerEvents.ts` is, one query and `fetch`
only, so an indexer schema change is a one-file fix.
* why the layers split across backends
Presence claims are verifiable live against `Transaction.raw`. The
indistinguishability claims are structurally dry-only: two real
transactions always differ in proof bytes, fees, and wallet nonces, so
there is no live analogue of "these differ only in digests".
* two findings that shaped the assertions
- The tree publishes `H(commitment)`, not the commitment, so the claim
is "one opaque digest, not the commitment itself".
- The runtime zero-trims byte encodings, so searching a transcript for a
32-byte secret's raw hex passes vacuously. Hence `encoded()`, and the
positive control asserting the nullifier IS published — without it the
four `not.toContain` assertions prove nothing.
* what is deliberately public
`ContractCall.entryPoint` names the circuit, so the operation type is
observable. Asserted rather than hidden.
Mutation-checked: a planted `_leakedValue = disclose(value)` was caught by
three tests but by no differential one, because both probes minted the
same amount. Two mint-differential cases close that gap, verified to fail
under the leak. The live layer passed against the local stack; it uses
high-entropy secrets, since short encodings turn up in proof blobs by
coincidence.
Refs: #723
Reads top-down the way Clean Code's stepdown rule prescribes: the entry
points come first in the order a token standard presents them, and each
callee sits directly beneath the circuit that calls it, depth-first.
_mint freshNonce, _mintNote, commitOf
burn _spenderPk, derivePk, _inputNote, _burn, _consumeNote,
nullifierOf
transfer _transfer
`mint / burn / transfer` at the top level mirrors ERC-20 and
`ConfidentialFungibleToken`, so a reader arriving from either finds the
same shape. Depth-first rather than breadth-first keeps a helper next to
its only caller: `derivePk` follows `_spenderPk` because that is where it
is used, not grouped with the other pure circuits.
Pure code motion: 148 insertions and 148 deletions, and the file's lines
compare identical to HEAD once sorted. No line content changed, so every
`@circuitInfo` k and row count still describes the circuit it annotates.
Refs: #723
* order
The mock, the simulator, and the spec's describes now follow the core's
own circuit order, so all four files can be read side by side. The mock
loses its "building blocks" divider, which only made sense while the
blocks were grouped apart from their callers. Every `it` reads as a
claim (`should …` / `should not …`).
* witness-thrown reasons, invisible live
Five negative tests assert `wit_Path: commitment not found in tree`, a
message our witness raises, not the contract. `rejects.toThrow` reads
`error.message` only, and the live backend wraps a circuit failure
twice:
[0] Unexpected error executing scoped transaction '…': …
[1] Error executing circuit '_consumeNote'
[2] wit_Path: commitment not found in tree
so the reason sits at depth 2 and the assertion failed for no
behavioural reason. Both wrappers do keep `cause`
(`midnight-js-contracts` passes `{ cause: err }`, and `compact-js`'s
`ContractRuntimeError.make(message, cause)` retains it), so
`test-utils/assertions/rejection.ts` walks the whole chain and the five
assertion strings stay exactly as they were. It uses `String(err)`
rather than `.message` because an effect `FiberFailure` only renders
what it wraps via `toString`, and it follows `AggregateError.errors`
alongside `cause`. On a miss it prints every layer, so a backend that
wraps differently reports what it produced instead of just failing.
* circuits that cannot be transactions
`_spenderPk` and `_inputNote` read a witness but touch no ledger state,
so their public transcript is empty: the generated artifact lists them
under `ImpureCircuits` (9) and NOT under `ProvableCircuits` (7), the
constructor registers no operation for them, and no verifier key is
emitted. They are callable in-circuit only, which is how `burn` and
`transfer` use them. Their describes are gated with
`skipIf(isLiveBackend())`; the behaviour still runs live inside every
`burn` and `transfer` case.
Verified against the first live run of the suite, which passed 46 of 55.
Two of the remaining failures were a three-hour laptop suspend mid-run,
which no code change addresses.
Refs: #723
Compatibility pins need the contract's published surface: which ledger slots exist at which index, how each is stored, and which circuits a deployed instance will accept. All of that is in the compiler's `contract-info.json`, emitted on every build including `--skip-zk`. No Midnight package describes that file, so the shapes are declared here. The near misses were checked: `CompactType<A>` is a runtime codec with no static shape, and `SparseCompactADT` is tagged with a partial `'cell' | 'set' | 'list' | 'map'` vocabulary for locating contract references. Every variant is instead derived from the 472 compiled artifacts in this monorepo, which also settled three shapes a guess would have got wrong: * `Counter` slots carry no element type and `Map` slots carry `key`/`value` instead of `type`, so `LedgerSlot` is a union discriminated on `storage` rather than one interface with optional members. Reading `.type` off a map slot is now a type error. * `ledger` is absent, not empty, for a contract with no ledger state (64 of the 472). `ledgerSlots()` normalizes that away. * Witnesses key their result as `'result type'` with a space where circuits use `'result-type'`. That is the compiler's inconsistency. `NameOf` and `Exhaustive` bind a pin to the generated `contract/index.d.ts`, the compiler's other output, so a renamed or added circuit is a compile error at the pin site rather than a runtime surprise. The harness test reads no real artifact, since `test:harness` has no `compile` dependency. It writes a throwaway one instead, which still covers the likeliest breakage: the path resolved against this module.
Every other suite compares the module against itself, so all of them stay green when the wire format moves. Rename a domain tag or reorder a hash preimage and every digest moves together, leaving each relative assertion consistent. Verified by mutation: renaming `OZ:note:commit` to `OZ:note:commitment` keeps the functional, privacy, and concurrency suites passing while making every note in a deployed instance unspendable. So this suite pins absolutes instead. A holder rebuilds a commitment and derives a nullifier to spend, and a client reads the ledger by slot and calls circuits by name, so both the digests and the published shape are load-bearing outside this repo. * Digests for `derivePk`, `commitOf`, and `nullifierOf`, plus the mint and change nonces under a fixed seed. The nonce pair deploys, and is worth its cost on live: it proves the deployed bytecode derives what the local artifact does. * The ledger layout whole, so an added or removed slot fails too. This is the only place `HistoricMerkleTree` and `depth: 32` are asserted statically. * The circuit surface, keyed on the generated `Circuits` type so an added, removed, or renamed circuit is a compile error, cross-checked against `ProvableCircuits` and `Ledger` so the compiler's two outputs cannot disagree. Both new pins were verified non-vacuous by mutating the generated metadata: `HistoricMerkleTree` to `MerkleTree` and `_spenderPk.proof` to true each fail on both backends, while the digest tests correctly stay green. Toolchain versions are recorded rather than asserted. Compilers 0.31.0 and 0.31.1 produce byte-identical digests, layout, and surface here, so a pinned `compiler-version` would fail with nothing broken. Refs #752.
The functional suite states each claim at hand-picked points, so `nullifierOf` ignoring the value is shown for 100 and 999. These state the same claims over generated inputs, so they hold where nobody chose and a failure shrinks to the smallest counterexample. Scope is single calls on the three pure circuits: no ledger, no deployment. Sequence claims are a different kind of statement and live in the invariant suite. Generators are pinned to each circuit's declared type rather than a convenient range. Values and nonces span `Uint<128>`, and owner keys are derived through `derivePk` from generated secrets so every one is a valid `Field`. Runs on either backend, since pure circuits evaluate locally even on live and cost no block.
Neither the functional nor the property suite says anything about a sequence: that after any interleaving of mint, transfer, and burn, the ledger still agrees with what a wallet believes. These carry a shadow model of an honest wallet and re-check it after every step, asserting that nullifiers only ever accumulate and that a note the wallet thinks it holds really is committed and unspent. Conservation is checked per operation rather than globally, since the core keeps no supply figure on the ledger: `transfer` splits its input exactly and `burn` destroys exactly what it was asked to. Spends name a percentage of the held note rather than an absolute amount. Absolutes collapse the run onto the insufficient-value guard as the change note shrinks, measured at 106 of 223 steps. Weighting spends above mints and prefixing every sequence with one mint removes a second dead path, spending against an empty wallet, measured at 154 of 306. The fuzzer found two bugs in the model itself while this was written, both now covered: the value guard runs before the witness, and a replay has to re-present the exact spent note so the nullifier check is what fires rather than a missing commitment. Dry only. Each operation is a transaction on live, so these runs would be roughly 120 transactions and 90 minutes.
Both privacy helpers assumed a 32-byte digest is always 64 hex characters. The runtime trims leading zero bytes, so roughly 1 in 256 digests arrives 31 bytes wide, and the suite was already flaky because of it: a run failed and then passed, and replaying the reported seed reproduced a moved value at `len=62`. Each assumption failed differently. `digestsIn` filtered on an exact width, so it silently dropped a trimmed digest and shrank the set a leak would have to hide in. `expectOpaque` asserted the exact width, so it failed outright on a legitimate value. Both now bound rather than equate: at least 56 hex to be a digest at all, at most 64. Verified over six consecutive privacy runs and three full note-core runs.
The shape and differential layers compared hand-picked pairs, which is exactly the arrangement those layers are weakest at. A planted `disclose(value)` survived them once because both probes happened to mint the same amount, so the comparison had nothing to detect. Both sides of every comparison are now generated. Amounts span the full `Uint<128>` where the call allows it, which matters because a leaked amount most often surfaces as a change in byte length rather than a changed byte. Recipients derive through `derivePk` from generated secrets so each is a valid `Field`. Run counts stay low, since every case drives two full circuit executions. Verified non-vacuous by mutation. A leak planted at a threshold of 10^31 passes under the old fixed pair of 1 and 10^30 and fails here, shrinking to `[0, 10^31 + 1]`. An earlier claim that the fixed values missed a plain `disclose(value)` was wrong: re-running that mutation showed the committed version caught it, which is why the threshold case is the one recorded.
The suite covered a chosen handful of operation pairs. The interesting input here is the pair itself, and with five callable operations the space is only 25 ordered pairs, doubled for spend-vs-spend where it matters whether both calls spend the same note. Enumerating a space that small is both complete and deterministic in a way sampling it cannot be, so all 34 cases now run. Each case asserts against a stated conflict model rather than a memorised verdict: two calls built on one snapshot collide only where they pin the same key, and the only pinned key here is a nullifier. A newly added circuit whose pinning differs then fails here instead of quietly widening the gap between the model and the module. Verified non-vacuous by negating the model, which fails 9 cases. The `_mint` by `transfer` row is the load-bearing one: swapping `HistoricMerkleTree` for `MerkleTree` fails it, because plain `checkRoot` pins the current root so every mint invalidates every in-flight spend. Confirmed by compiling that variant. No functional or privacy test notices the one-word change.
Member
Author
Recent additions
This batch added Why New shared module. Fixed a flake ( Filed: #752 (compiler version declared in three places; CI pins 0.31.0, local gives 0.31.1) · #749 · #750 · #751 Verification. Note-token unit 127 passed / 5 skipped · harness 146 / 4 skipped · biome clean on 153 files. Each addition checked non-vacuous by mutation. Two caveats: the other 37 unit files fail to load in this worktree (uncompiled mocks, zero failed tests), and |
0xisk
marked this pull request as ready for review
July 29, 2026 12:36
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
contracts/src/token/test/ConfidentialNoteFungibleToken.compatibility.test.ts (1)
234-250: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComparator mismatch between the two sorted lists.
circuitSurfacesorts withlocaleCompare(contracts/test-utils/compiler/contractInfo.ts:364-368), while Line 249 compares againstObject.keys(declared).sort(), which sorts by UTF-16 code unit. The two orderings agree for the current underscore-prefixed names, but they can diverge for future names (ICU collation weights_differently from raw code units), producing a confusing failure that isn't a real compatibility break. Using the same comparator on both sides removes the ambiguity.♻️ Align the comparator
- expect(provable).toStrictEqual(Object.keys(declared).sort()); + expect(provable).toStrictEqual( + Object.keys(declared).sort((left, right) => left.localeCompare(right)), + );Line 269 has the same shape; both sides there use the default sort, so it is already self-consistent — but consider
localeComparethere too for uniformity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/src/token/test/ConfidentialNoteFungibleToken.compatibility.test.ts` around lines 234 - 250, Align the expected-name ordering in the compatibility tests with circuitSurface by sorting declared keys using localeCompare instead of the default sort, especially in the test around the declared ProvableCircuits names. Apply the same comparator to the corresponding comparison near the other compatibility assertion for consistent ordering.contracts/src/token/test/ConfidentialNoteFungibleToken.privacy.test.ts (1)
604-610: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe disclose-site scan is comment-sensitive and duplicate-blind.
Two soft spots in this otherwise valuable pin:
- The filter only excludes lines starting with
*, so a//or/*comment in the core that merely mentionsdisclose(fails the test with nothing actually changed.- Comparing
Sets means an added duplicate of an existing disclose line (e.g. a second_nullifiers.insert(disclose(nf));) passes silently, which is exactly the kind of new disclosure this test exists to catch.♻️ Suggested tightening
it('should disclose only at the reviewed sites', () => { const sites = CORE_SOURCE.split('\n') .map((line) => line.trim()) - .filter((line) => line.includes('disclose(') && !line.startsWith('*')); + .filter( + (line) => + line.includes('disclose(') && + !line.startsWith('*') && + !line.startsWith('//') && + !line.startsWith('/*'), + ) + .sort(); - expect(new Set(sites)).toStrictEqual(new Set(EXPECTED_DISCLOSURES)); + expect(sites).toStrictEqual([...EXPECTED_DISCLOSURES].sort()); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/src/token/test/ConfidentialNoteFungibleToken.privacy.test.ts` around lines 604 - 610, Harden the disclose-site scan in the `should disclose only at the reviewed sites` test: ignore lines that are inside or start with `//` and `/*` comments before matching `disclose(`, and compare the collected sites as ordered or count-preserving arrays rather than `Set`s so duplicate disclosure lines fail the test while retaining the existing expected-site validation.contracts/src/token/test/ConfidentialNoteFungibleToken.invariant.test.ts (1)
30-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIdentity/ledger helpers duplicated with
ConfidentialNoteFungibleToken.test.ts.
secretKey,ALICE/BOBderivation,publicState,commitmentCount,nullifierCount,isSpent,isCommitted, andspendAsare re-implemented identically here and in the functional suite. See the consolidated comment for a proposed shared-helper extraction.Also applies to: 118-139
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/src/token/test/ConfidentialNoteFungibleToken.invariant.test.ts` around lines 30 - 38, Extract the duplicated identity and ledger helpers from ConfidentialNoteFungibleToken.invariant.test.ts and ConfidentialNoteFungibleToken.test.ts into a shared test helper module. Reuse that module for secretKey, ALICE/BOB derivation, publicState, commitmentCount, nullifierCount, isSpent, isCommitted, and spendAs, preserving their current behavior and exports.contracts/src/token/test/ConfidentialNoteFungibleToken.test.ts (1)
13-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared test identities/ledger helpers to avoid drift across the note-token suite.
Both files independently re-implement the same deterministic
secretKey,ALICE/BOBderivation, and ledger-reading helpers (spendAs,isCommitted,isSpent,commitmentCount,nullifierCount,publicState). A single shared module (e.g. ahelpers.tsalongside the simulator/witnesses) would remove the duplication and keep future edits (e.g. to how ownership or spend status is read) in one place as more suites (compatibility, privacy, concurrency) are added on top of this cohort.
contracts/src/token/test/ConfidentialNoteFungibleToken.test.ts#L13-L58: movesecretKey,ALICE/BOB/CAROL,spendAs,isCommitted,isSpent,commitmentCount,nullifierCount,publicStateinto a shared helper module and import from there.contracts/src/token/test/ConfidentialNoteFungibleToken.invariant.test.ts#L30-L139: replace the local re-implementations ofsecretKey,ALICE/BOB,publicState,commitmentCount,nullifierCount,isSpent,isCommitted,spendAswith imports from the same shared helper module.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/src/token/test/ConfidentialNoteFungibleToken.test.ts` around lines 13 - 58, Create a shared helper module for the token tests and move the deterministic identities and ledger helpers (including secretKey, ALICE/BOB/CAROL, spendAs, publicState, isCommitted, isSpent, commitmentCount, and nullifierCount) there; update contracts/src/token/test/ConfidentialNoteFungibleToken.test.ts#L13-L58 and contracts/src/token/test/ConfidentialNoteFungibleToken.invariant.test.ts#L30-L139 to import them and remove their local duplicates, preserving existing behavior.contracts/src/token/test/ConfidentialNoteFungibleToken.concurrency.test.ts (1)
238-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead branch: third
armcondition is unreachable.
secondParty === aliceonly happens whentestCase.sameNote === true(line 242), andsharings(line 159) only ever producessameNote === truewhen bothSPENDS[first]andSPENDS[second]are true. So the third condition's!SPENDS[testCase.first](line 253) can never hold alongsidesecondParty === alice— this branch never executes. Every reachable combination is already armed by the first twoifblocks.Given the file's stated goal of being an exhaustive, precisely-reasoned matrix ("keep that row if this matrix is trimmed"), leaving unreachable arming logic here risks misleading a future editor into thinking a scenario is covered when it isn't.
🧹 Suggested cleanup
- if ( - SPENDS[testCase.second] && - secondParty === alice && - !SPENDS[testCase.first] - ) { - await arm(alice, ALICE); - } -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/src/token/test/ConfidentialNoteFungibleToken.concurrency.test.ts` around lines 238 - 266, Remove the unreachable third arm(alice, ALICE) conditional from the test matrix loop. Keep the first two arming conditions and the existing race invocation and verdict assertion unchanged, since all reachable sameNote cases are already covered by those conditions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/test-utils/harness/publishedTx.ts`:
- Around line 69-89: Update gql() and its callers, including awaitPublishedTxs,
to issue each fetch with an AbortSignal-based per-request timeout bounded by the
remaining timeoutMs deadline. Ensure an aborted request propagates as the
existing timeout failure rather than hanging indefinitely, while preserving
normal GraphQL response and error handling.
---
Nitpick comments:
In
`@contracts/src/token/test/ConfidentialNoteFungibleToken.compatibility.test.ts`:
- Around line 234-250: Align the expected-name ordering in the compatibility
tests with circuitSurface by sorting declared keys using localeCompare instead
of the default sort, especially in the test around the declared ProvableCircuits
names. Apply the same comparator to the corresponding comparison near the other
compatibility assertion for consistent ordering.
In `@contracts/src/token/test/ConfidentialNoteFungibleToken.concurrency.test.ts`:
- Around line 238-266: Remove the unreachable third arm(alice, ALICE)
conditional from the test matrix loop. Keep the first two arming conditions and
the existing race invocation and verdict assertion unchanged, since all
reachable sameNote cases are already covered by those conditions.
In `@contracts/src/token/test/ConfidentialNoteFungibleToken.invariant.test.ts`:
- Around line 30-38: Extract the duplicated identity and ledger helpers from
ConfidentialNoteFungibleToken.invariant.test.ts and
ConfidentialNoteFungibleToken.test.ts into a shared test helper module. Reuse
that module for secretKey, ALICE/BOB derivation, publicState, commitmentCount,
nullifierCount, isSpent, isCommitted, and spendAs, preserving their current
behavior and exports.
In `@contracts/src/token/test/ConfidentialNoteFungibleToken.privacy.test.ts`:
- Around line 604-610: Harden the disclose-site scan in the `should disclose
only at the reviewed sites` test: ignore lines that are inside or start with
`//` and `/*` comments before matching `disclose(`, and compare the collected
sites as ordered or count-preserving arrays rather than `Set`s so duplicate
disclosure lines fail the test while retaining the existing expected-site
validation.
In `@contracts/src/token/test/ConfidentialNoteFungibleToken.test.ts`:
- Around line 13-58: Create a shared helper module for the token tests and move
the deterministic identities and ledger helpers (including secretKey,
ALICE/BOB/CAROL, spendAs, publicState, isCommitted, isSpent, commitmentCount,
and nullifierCount) there; update
contracts/src/token/test/ConfidentialNoteFungibleToken.test.ts#L13-L58 and
contracts/src/token/test/ConfidentialNoteFungibleToken.invariant.test.ts#L30-L139
to import them and remove their local duplicates, preserving existing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cdeec834-99af-475e-a580-29d50e16b99a
📒 Files selected for processing (23)
CHANGELOG.mdcontracts/src/token/ConfidentialNoteFungibleToken.compactcontracts/src/token/test/ConfidentialNoteFungibleToken.compatibility.test.tscontracts/src/token/test/ConfidentialNoteFungibleToken.concurrency.test.tscontracts/src/token/test/ConfidentialNoteFungibleToken.invariant.test.tscontracts/src/token/test/ConfidentialNoteFungibleToken.privacy.test.tscontracts/src/token/test/ConfidentialNoteFungibleToken.property.test.tscontracts/src/token/test/ConfidentialNoteFungibleToken.test.tscontracts/src/token/test/mocks/MockConfidentialNoteFungibleToken.compactcontracts/src/token/test/simulators/ConfidentialNoteFungibleTokenSimulator.tscontracts/src/token/test/witnesses/ConfidentialNoteFungibleTokenWitnesses.tscontracts/test-utils/assertions/rejection.tscontracts/test-utils/assertions/test/rejection.test.tscontracts/test-utils/compiler/contractInfo.tscontracts/test-utils/compiler/test/contractInfo.test.tscontracts/test-utils/concurrency/DryReplayHarness.tscontracts/test-utils/concurrency/backend.tscontracts/test-utils/concurrency/parties.tscontracts/test-utils/concurrency/race.tscontracts/test-utils/concurrency/test/DryReplayHarness.test.tscontracts/test-utils/concurrency/test/parties.test.tscontracts/test-utils/concurrency/types.tscontracts/test-utils/harness/publishedTx.ts
`awaitPublishedTxs` documents "give up after this long" but only checked the deadline between polls, so a stuck indexer outlived it. `fetch` has no total-response timeout, and undici's body timeout is far longer than any budget a caller passes. Bounding each request at a fixed ceiling is not enough on its own: `publishedTxsSince` issues one request per block, so a 10s ceiling still allows an arbitrarily long total. Each request is instead bounded by whatever is left of the caller's deadline, and the deadline is threaded through `indexerHead` and `publishedTxsSince` so the per-block loop stops with it. Timeouts and protocol failures are now distinguished. Slowness is what this function exists to absorb, so it keeps polling while time remains; an HTTP status or a GraphQL error is a real defect and surfaces at once with its original message, rather than being flattened into the poll-exhausted error. Adds the module's first test, `fetch` stubbed, which also closes the one gap in harness test coverage. Verified non-vacuous: removing the abort signal leaves both timeout cases hanging until the runner kills them, which is the reported failure. Raised by CodeRabbit on #743.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Types of changes
What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an `` in the boxes that apply
Fixes #723
The
ConfidentialNoteFungibleTokencore, extracted from the exploration draft #679 onto a clean branch offmain. Core only — no extensions, no presets, no crypto primitives, no concurrency utils. Those are the sibling sub-issues of #722.Value lives as notes:
(value, nonce)owned bypk = Hf(sk), published only as a hiding commitmentcm = H(domain, value, nonce, pk)in aHistoricMerkleTree. Spending publishesnf = H(domain, nonce)and proves membership in-circuit without revealing which leaf, so amounts, sender, and recipient all stay off the public ledger. The ledger carries the tree and the nullifier set and nothing else: no balances, no accounts, no supply.Surface
transfer/burn: spending requires the owner's secret viawit_SecretKey, and output nonces come from the caller's own randomness witness. Created notes return to the caller as a local private result and move out of band._mint/_mintNote/_transfer/_burn/_consumeNotebuilding blocks. No roles, no initialization — the composing contract supplies policy.derivePk/commitOf/nullifierOfso wallets and auditors recompute off-circuit exactly what the circuits do.Costs (compiler's own numbers, matching the
@circuitInfotags)transfer_transferburn_burn_consumeNote_mint_mintNote**Changes from the draft in **#679
OZ:note:*instead ofOZ:cnt:*. These are permanent on-chain constants, so the rename had to land before any deployment; costs are unchanged.main.PR Checklist
Summary by CodeRabbit