refactor(multisig): rename presets by capability#682
Draft
0xisk wants to merge 51 commits into
Draft
Conversation
Wire @openzeppelin/compact-simulator's live backend to the local stack (make env-up) so the multisig unit specs run unchanged on both MIDNIGHT_BACKEND=dry and =live. testkit-js drives the wallet pool (deployer + SIGNER1-3 from the dev-preset seeds) and provider wiring; each await Sim.create() deploys and attaches a real contract. * test-utils/live: registerSimulatorLive (deploy-per-create, per-alias providers), live.setup, and a shielded coin tracker (ledgerEvents + shieldedCoinTracker) that recovers contract-owned coin indices from the indexer's zswapLedgerEvents stream * test-utils: backend-aware shielded fixtures (liveShielded) and address helpers, decoupled from the harness via isLiveBackend() * config: vitest.live, local-env, turbo task, and resolution pins the live wallet stack needs * tests: backend-aware edits to the forwarder and treasury specs so they exercise both backends
The V1/V2/V3 suffixes implied versions of one contract, but they are three distinct designs. Rename each by what it does: * ShieldedMultiSig -> ShieldedProposalMultisig (on-chain proposal lifecycle) * ShieldedMultiSigV2 -> ShieldedStatelessMultisig (off-chain signatures, stateless treasury) * ShieldedMultiSigV3 -> NativeShieldedTokenMultisig (threshold mint/burn native token) Renames the .compact sources in presets/ along with their simulators, witnesses, and test files, and updates every identifier, artifact path, and the CHANGELOG entry. No behaviour change.
Reorganize test-utils into fixtures/ (backend-aware coin/key/address builders) and harness/ (live wallet pool, simulator backend, coin tracker), and collapse the separate live/integration vitest configs into per-project flavours in one config. Fund the fourth pooled signer from the deployer at live setup. The local dev preset genesis-funds only three of the four seeds, so SIGNER3 starts with zero NIGHT and cannot pay tx fees. FundedWallet now reports both NIGHT and generated-dust balances (registering NIGHT for dust zeroes the plain NIGHT balance, so the readiness gate must accept either), and funding.ts transfers NIGHT deployer->signer and waits for dust to generate before the gate. Verified live: SIGNER3 funds and the deployer-path spec tests pass. Skip the coin-tracker dry-surface tests on the live backend, where getQualifiedShieldedCoinInfo resolves a real commitment rather than the dry passthrough those assertions cover. Known-remaining: multi-signer .as() calls still fail on live because the simulator backend opens the private-state LevelDB per alias and scopes it by each wallet's coin public key. Fixed in a follow-up.
Split the live simulator backend's provider wiring into one factory per
provider (public-data, zk-config, proof, private-state) with buildContext
orchestrating them, dropping the opaque initializeMidnightProviders
bundler.
Use a single in-memory private-state store shared across every caller
alias. The previous per-alias LevelDB store both serialized on one
on-disk handle AND scoped state by the wallet's coin public key, so a
non-deployer signer fought over the lock and never saw the deployed
private state — every `.as('SIGNERn')` call failed with "Database failed
to open" / "No private state found". In-memory sidesteps both and keeps
the run hermetic (no disk, no stale state across runs).
Declare the three midnight-js provider packages as direct devDeps rather
than relying on transitive resolution through testkit.
Verified live: no private-state errors across the multisig spec; the
multi-signer `.as()` flows that previously failed now run.
Consecutive submissions from the same signer on the live backend could balance a tx against wallet state that did not yet reflect the prior tx, reusing a dust/coin UTXO the prior call already spent. The node then rejected the tx with a bare "Transaction submission error". Wrap the wallet provider's balanceTx to await syncWallet first, so each build sees state caught up to the latest block. Guarded on a real balanceTx (the unit-test mock omits it). Add a test asserting the pre-balance sync runs before the underlying balance.
The full-lifecycle live specs run ~10 ZK-proven txs each (~220s on the local stack) and exceeded the 180s per-test cap. Raise the live testTimeout to 600s. Flag the full-lifecycle describe with a TODO to relocate to the integration project (tracked as a follow-up); these end-to-end flows do not belong in the unit-live suite.
The planning note was scratch left in test-utils/harness/ and is referenced nowhere in code. Remove it from the shipped test infra.
The harness reorg renamed the root scripts (compact -> compile, fmt-and-lint -> check), but the CI workflows and the bug-report template still called the old names, so Run Checks and Run Test Suite failed at 'yarn compact' / 'yarn fmt-and-lint:ci'. Point them at the current script names.
The per-contract witness modules under multisig/test/witnesses/ were all identical empties (Record<string, never> private state + a no-op witnesses factory). Delete the directory and point every simulator at the existing shared EmptyWitnesses (EmptyPrivateState / emptyWitnesses), matching what the forwarder simulators already do.
The `seed(lastByte)` helper built a 32-byte wallet seed as 63 zeros plus the decimal `lastByte`, which only yields a valid 64-hex string for bytes 1-9. Per-worker wallet partitions use two-digit hex bytes (e.g. 0x11), so pad the byte to two hex digits. Output is unchanged for the existing 0x01 -0x04 seeds.
The `unit-live` project forced `fileParallelism: false` because every worker shared one deployer wallet, so concurrent files would race on its UTXOs and nonces. The whole multisig live suite ran sequentially (~5.4h), bottlenecked entirely on the ~18s per-tx indexer finality lag. Partition the wallets per vitest worker so files run concurrently against the shared node: * `walletSeedsFor(worker)` gives worker w a disjoint set — genesis deployer `0x..0w` plus derived signers `0x..(0x10*w + slot)`, disjoint from the genesis seeds and across workers. Signers only pay fees, so they need no genesis grant and are topped up from the worker's deployer by the existing `fundFromDeployer` path. * `live.setup.ts` resolves the worker from `VITEST_POOL_ID`, fails fast when it exceeds the 3-deployer cap, and logs to `live-harness-w<N>.log`. * `vitest.config.ts` runs `unit-live` with `maxWorkers` from `MIDNIGHT_LIVE_WORKERS` (default 3, clamp 1..3, forced to 1 when `MIDNIGHT_WALLET_SEED` pins a custom deployer). `groupOrder` isolates its scheduler group so a mixed-project run doesn't reject the differing `maxWorkers`. `harness-live` stays fully sequential. Aliases and `MIDNIGHT_<ALIAS>_COIN_PK` env names are unchanged, so specs need no edits.
With parallel workers several deploys can contend for one block and the node may bounce a submission. Retry the deploy once with a jittered backoff so that race (and the occasional transient submit flake) doesn't fail a spec. A deterministic failure just fails again on the retry, so nothing is masked.
On the live backend every test deploys a fresh contract in `beforeEach`, and each deploy is a full ~18s on-chain tx. Read-only and pure describe groups don't need per-test isolation, so deploy once per group in a `beforeAll` and let the group's tests share it; mutating groups keep the per-test `beforeEach`. This removes ~55 deploy/setup txs from the live multisig suite (concentrated in the view/getter/pure groups) with no change to what any test asserts. * Each file gets a small `freshX()` helper; the outer per-test deploy is removed and each group declares its own hook (beforeAll for read-only, beforeEach for mutating). * ProposalManager's pure `recipient helpers` (9) and `view circuits` (6), the `assertSigner`/`assertThresholdMet`/`isSigner` groups in Signer and SignerManager, the `view` groups, ShieldedTreasury's `initial state`, and ShieldedProposalMultisig's four view groups now share one deploy. * ShieldedTreasury's two mutually-exclusive Zswap blocks build a fresh treasury inline per test (a shared hook would trip biome's noDuplicateTestHooks, which doesn't model describe.skipIf/runIf scopes). Verified: dry `unit` 285 pass / 9 skip; live `unit-live` on ProposalManager + ShieldedTreasury + ShieldedStatelessMultisig 71 pass / 3 skip, shared beforeAll deploys confirmed working on the local node.
Interleaved output from parallel unit-live workers was not attributable to a worker. Print a per-worker "ready" banner after wallet funding, and a `[wN] > <file>` tag before each spec file, so a dev (or a captured log) can tell which worker ran which file. vitest.config publishes the resolved MIDNIGHT_LIVE_WORKERS so the banner can show w<n>/<total>; workers inherit it at fork time. Uses console.log because biome noConsole allows only log.
Add a "Running Tests" section covering the unit and live suites: the env:up + test:live flow, the fresh-genesis guarantee, the two reliability rules (always env:up first, one live run at a time), and capturing a colored log to a .ansi file (FORCE_COLOR + tee, viewed via the VS Code ANSI extension or less -R).
The live deploy path retries once on a submission error to absorb block-contention races. But an RPC 1010 "Invalid Transaction" (e.g. a shielded spend the ledger rejects on stale node state, Custom error 103) is deterministic: the retry just re-proves and fails again, doubling proving cost and log noise. Detect 1010 by walking the error graph (String(e) catches an effect FiberFailure whose cause hides behind a Symbol, plus Error message / cause / AggregateError.errors) and rethrow immediately instead of retrying. Adds four deploy-retry tests: transient error retries once; flat, nested-cause, and toString-only 1010 do not.
Live tests share one node, so state left by an earlier run makes later shielded spends fail (node Custom error 103), and two concurrent runs corrupt each other's coin state. Both failure modes now abort in ~1s, before any wallet build, via a vitest globalSetup on the unit-live and harness-live projects: * Freshness: genesis records its shielded coins in block 0 (measured: 28 events; the harness smoke adds none), so any coin event in block 1+ means a previous run left state behind. The guard counts events via the indexer (chunked, early-exit) and aborts with an env:up hint. * Single run: a pid-stamped lock file (logs/.live-run.lock) with stale-pid reclaim and same-pid reentrancy. Knobs: MIDNIGHT_LIVE_ALLOW_DIRTY=1 skips the freshness check; MIDNIGHT_LIVE_MAX_COIN_EVENTS / MIDNIGHT_LIVE_MAX_SCAN_BLOCKS tune the thresholds.
test:live:verify separates real failures from environment flakes: round 1 resets the stack and runs the live suite; any files that failed are re-run in round 2 on a fresh node with one worker. Fails round 1 + passes round 2 = FLAKY (exit 0, reported loudly); fails both = REAL (exit 1). Plain TypeScript run directly by Node's type stripping; only node: builtins.
Recommend test:live:verify for full runs, note that the freshness and single-run rules are now guard-enforced, and document the MIDNIGHT_LIVE_* knobs.
Per-worker tagging so far only marked file starts (`[wN] ❯ file`). Add a custom vitest reporter that prints one worker-tagged, globally counted line per test result, running alongside the built-in reporter that still owns the file lines, failure details, and final summary. The worker id is stamped onto each test's meta by a setup `beforeEach` so the reporter (main process) can read it — the worker is the only place that knows its `VITEST_POOL_ID`.
Replace the two-round `test:live:verify` wrapper with a single `test:live` entrypoint. It compiles, resets the stack, runs a harness smoke, then each live-ready category sequentially on a freshly reset node, and re-runs any failing files once in isolation to separate a real failure from an environment flake (FLAKY vs REAL verdict). * Self-heal truncated ZK keys. A killed compile or a cache race (#675) can leave a turbo cache entry that re-extracts 0-byte keys on every hit, which silently skips a whole suite. Detect them after compile, drain the cache, recompile serially, and only abort if they are still truncated. * Add `test:live:multisig`. A category joins the run as its specs are refactored for the live backend.
Describe the single `test:live` / `test:live:<category>` entrypoint and the FLAKY/REAL two-round verdict. Drop the manual `env:up` step and the truncated-key remedy now handled by the runner itself.
Z_RECIPIENT is assigned in beforeAll, not beforeEach — align the comment with the hook that actually resolves it.
On live each impure getter submits a tx. getReceivedMinusSent ran the two totals via Promise.all, so both balanced against the same wallet snapshot and could hit a stale-UTXO rejection. Await them in sequence.
start() succeeds before waitForFunds/sync can throw, leaving the provider started but unreturned — WalletPool.reset only stops wallets it recorded, so it leaked. Wrap init and stop the provider on any failure.
Bare Number() turned '', fractions, and out-of-range values into bad 0/NaN ports that only failed later as a malformed URL. Validate at load time and name the responsible env var.
The default-port assertion read module-load PORTS, so a real MIDNIGHT_*_PORT set in CI/dev broke it. Clear the overrides and re-import network.js before asserting the defaults.
The live builder funds every empty signer from the shared deployer. Promise.all balanced those funding txs against the same deployer snapshot, reintroducing the stale-UTXO race. Build serially instead.
The compact->compile split fanned the aggregate out to per-dir tasks but dropped compile:archive, so 'yarn test' (dependsOn compile) no longer rebuilt stale src/archive artifacts.
Make the single-file live run explicit: pass a spec name after -- to run just that file on the live backend, the fast loop while iterating on one feature. Note the two-round flake check still applies.
A dropdown beats free text in the Actions UI: no typos, and the live-ready set is discoverable. `all` (the default) maps to no category arg, since the runner would treat a bare unknown token as a file filter. The options list mirrors LIVE_READY in test-live.ts; a stale entry fails loudly because the runner rejects non-live-ready names.
`test:live --list` prints the live-ready categories as a JSON array and exits. CI derives its per-category job matrix from it, keeping LIVE_READY the single source of truth instead of a second hardcoded list in the workflow.
The 6-hour hosted-runner cap is per job, so running every category in a single job risks hitting it as more categories become live-ready. A small plan job derives the category list from the runner (test:live --list) and a matrix fans out one job per category, all in parallel: each gets its own runner, docker stack, compile, and 6-hour budget, so total wall clock is the slowest category rather than the sum. fail-fast is off so one category failing does not cancel the rest, and log artifacts are suffixed per category to avoid name collisions. A scoped workflow_dispatch (category != all) plans just that one category.
The CI section now reflects the matrix fan-out: a plan job reads test:live --list and each category runs in parallel with its own runner, stack, and 6-hour budget.
A flaky-only run exits 0, so on CI the flake report used to vanish into a green job log. When running under GitHub Actions the runner now writes the verdict (with flaky and real file lists) to the job summary and emits a ::warning:: annotation per flaky file; both helpers no-op locally. GITHUB_ACTIONS / GITHUB_STEP_SUMMARY join globalPassThroughEnv so the biome turborepo rule knows they are declared, and they stay out of task cache hashes.
Two operational gaps in the same workflow, batched because the hunks share live.yml: * Artifact split: the JSON verdict reports are tiny and useful on green runs too (flaky trends), so they now upload on every run as live-reports-<category>. The service/worker logs, which can reach hundreds of MB after a multi-hour run, stay failure-only as live-logs-<category>. * Nightly visibility: a failed scheduled run only emails the workflow author. A report-nightly job now mirrors the state into a live-nightly tracking issue: opened on first failure, commented on repeats, closed automatically on the next green run. A suite skipped because the plan job died counts as a failure; a cancelled run reports nothing.
Document the per-job verdict summaries, the always-uploaded live-reports-<category> artifact, and the live-nightly tracking issue.
Nothing has invoked `turbo run test:live` (or the contracts test:live script it wrapped) since the unified runner replaced the root script in f751932. Keeping them left a second, guard-less entry point that skipped the stack reset, lock, and flake verdict. The runner is now the single way to run live tests.
Record the design decision in the runner header: turbo models a DAG of stateless, cacheable tasks, while a live run needs stateful orchestration (two-round flake verdict, docker lifecycle against one shared node, ZK-key self-heal, infra-vs-test exit codes). Turbo still handles the DAG-shaped steps (compile, harness smoke) inside the run.
The harness PR bundled a `compact`->`compile` and `fmt-and-lint`->`check` script rename that is unrelated to the test harness. Per review, back it out so this PR stays scoped to the harness. The rename will land as its own PR with a changelog entry. * package.json / contracts/package.json / turbo.json: restore the `compact:*` and `fmt-and-lint*` names, keeping every harness script, turbo task, and dependency * bug_report.yml / checks.yml / test.yml: reverted to main * scripts/test-live.ts, scripts/keyIntegrity.ts: invoke `yarn compact` again
Co-authored-by: Andrew Fleming <fleming-andrew@protonmail.com> Signed-off-by: 0xisk <0xisk@proton.me>
The live CI workflow (`live.yml`) and its "Live tests in CI" CONTRIBUTING subsection are too large and complex to review alongside the harness. Per review, remove them here and land them in a separate PR stacked on this branch. Refs: #673
Companion to the previous commit: the "Live tests in CI" subsection documented and linked `live.yml`, which moves to the follow-up PR. Refs: #673
Compiling the archive module as part of the full `compact` task fails locally (per review). Archive is excluded from the full compile on `main` too. Keep the `compact:archive` task for targeted runs, but drop it from the aggregate `dependsOn`. Refs: #673
`docker compose up -d --wait` waits on every service, including proof-server, which has no healthcheck. That is not portable across Docker Compose versions and aborts `yarn test:live` on some setups (reported in review). Start all services, then wait only on node and indexer, which expose healthchecks. Refs: #673
The shielded preset rename (`ShieldedMultiSig*` to capability-based names) is a user-facing contract change unrelated to the test harness. Per review, revert it here so this PR keeps the presets under their existing names; the rename lands as its own stacked PR with a changelog entry. Harness-only content changes are preserved (e.g. the block-limit circuit trim in ShieldedMultiSig.compact). Refs: #673
Rename the shielded multisig presets to describe their capability: * ShieldedMultiSig -> ShieldedProposalMultisig (on-chain proposal lifecycle) * ShieldedMultiSigV2 -> ShieldedStatelessMultisig (off-chain signatures, stateless treasury) * ShieldedMultiSigV3 -> NativeShieldedTokenMultisig (threshold mint/burn native token) Update every identifier, artifact path, and test/simulator reference. Split out of the live-harness PR (#673) so this user-facing rename lands as a clean, reviewable change; stacked on that branch and retargets to main once it merges. Refs: #673
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
8 tasks
0xisk
force-pushed
the
test/multisig-live-harness
branch
from
July 17, 2026 05:37
cdd0b90 to
a45bda7
Compare
0xisk
force-pushed
the
test/multisig-live-harness
branch
from
July 17, 2026 16:14
a45bda7 to
7f6e09e
Compare
0xisk
marked this pull request as draft
July 20, 2026 09:29
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?
Split out of #673 per review (comments on the preset
.compactrename and thegit mvdiff): the multisig preset rename is a user-facing contract change, soit lands as its own clean, reviewable PR with a changelog entry instead of being
buried in the harness diff.
Renames the shielded multisig presets to describe their capability:
ShieldedMultiSigShieldedProposalMultisig(on-chain proposal lifecycle)ShieldedMultiSigV2ShieldedStatelessMultisig(off-chain signatures, stateless treasury)ShieldedMultiSigV3NativeShieldedTokenMultisig(threshold mint/burn native token)Presets, tests, and simulators are renamed together, so every file shows as a
clean git rename (95-99% for the
.compactpresets and their tests). Everyidentifier, artifact path, and reference is updated.
Stacked PR. Based on
test/multisig-live-harness(#673), so the diff here isonly the rename. Once #673 merges to
main, this retargets tomainautomatically.
PR Checklist