feat(multisig): add Forwarder + ForwarderPrivate modules - #526
Merged
Conversation
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:
WalkthroughThis PR adds two forwarder contracts (public-parent Forwarder and commitment-based ForwarderPrivate), preset wrappers for shielded/unshielded/private deployments, Compact test mocks, TypeScript simulator presets, witness placeholders, comprehensive Vitest tests (including property tests), and Vitest coverage tooling plus minor .gitignore and package.json updates. ChangesForwarder Contracts and Testing
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
Local node-state directory holding wallet seeds and chain snapshots during development. Should not be tracked.
Two new modules providing forwarder primitives for inbound coin routing to a designated parent address. Forwarder (public): - _depositShielded: atomic receive + forward via Zswap, accumulates per-color total - _depositUnshielded: same flow for unshielded transfers - _recordReceived: shared overflow-guarded accounting helper - _parent + _received ledger fields, immutable parent after init ForwarderPrivate (private): - _deposit: receiveShielded only; coins dwell at the contract - _drain: preimage-gated send with change re-emission to self - _calculateParentCommitment: persistentHash([parentAddr, salt]) — pure - _parentCommitment ledger field hides parent under operational salt Both modules gated by Initializable. No witnesses in v1; all sensitive data flows through circuit parameters.
Three single-purpose presets composed from the Forwarder / ForwarderPrivate modules via named imports. Each preset is its own deployable contract with its own verifier key; a bank picks the right preset at deploy time based on the coin kind it accepts. - ForwarderShielded: exposes deposit(coin) for shielded receipts - ForwarderUnshielded: exposes depositUnshielded(color, amount) - ForwarderPrivate: exposes deposit + drain + _calculateParentCommitment for the private-parent flow No combined preset — banks that need both shielded and unshielded deploy two contracts. No ForwarderPrivateUnshielded — unshielded sends publish the parent on-chain, which defeats the private variant.
- @vitest/coverage-v8 for native v8 coverage with source-map back-mapping. The coverage include glob covers TS witnesses / simulators and the compactc-generated artifacts/Forwarder*/contract/ index.js files; v8 follows the .js.map to render coverage pages under .compact filenames. - fast-check for property-based tests (unlinkability across salts, per-color accumulation, partial-drain change arithmetic). - test:coverage script: compactc --skip-zk && vitest run --coverage. - 95% per-file threshold (lines/branches/functions/statements) as the closing gate. Subset runs override with --coverage.thresholds.lines=0 etc.
Test infrastructure for the three forwarder presets.
- Witness stubs (Record<string, never>) — v1 has no witnesses.
- Simulators wrap the compactc-emitted artifacts via createSimulator,
expose the public preset surface, and (for ForwarderPrivate) a static
calculateParentCommitment that delegates to pureCircuits.
- Three test suites covering 31 cases total:
* constructor + initial state
* shielded / unshielded deposit accumulation + overflow guard
* drain auth (correct, wrong parent, wrong salt, both wrong)
* drain change-coin handling (full vs partial)
* regression: no ledger mutation across drain failures
* property tests (fast-check) for accumulation, unlinkability across
salts, and partial-drain change arithmetic
pepebndc
requested changes
May 27, 2026
Contributor
Non-blocking suggestionsThree additional things worth considering before this ships as a stable library API. Overflow test missing in ForwarderShielded
Double-initialization revert tests None of the three preset test suites assert that calling the constructor (or Domain separation in
|
Move the three forwarder preset test files, simulators, and witnesses into matching `presets/` subdirectories so module-level fixtures stay flat alongside the rest of multisig/ while preset wiring lives in a clearly-scoped folder. The preset test files are slimmed to wiring-only checks (constructor arg storage, exposed circuit forwarding, zero-guard propagation, public-state accessor). Behavioural coverage moves out to the new module-level test files in the follow-up commit. Preset simulator import paths are bumped one level up to reach witnesses/ and artifacts/ from their new depth.
…der modules Address PR #526 review feedback: - `Forwarder._init` rejects `parent == default<Bytes<32>>`; a zero recipient would forward every deposit to an unspendable address with no recovery path. - `ForwarderPrivate._init` rejects a zero parentCommitment; since the commitment is the sole drain gate, a zero value would lock every accumulated coin permanently. - `ForwarderPrivate._calculateParentCommitment` now hashes `[pad(32, "ForwarderPrivate:commitment"), parentAddr, salt]` instead of `[parentAddr, salt]`. Domain tag prevents preimage collisions with other `persistentHash` users in the system. - `_drain` `@param salt` carries a prominent `@warning` block: salt loss is permanent fund loss (no rotation, revocation, or recovery path). Same warning mirrored on the preset's `drain` wrapper. - Preset `_calculateParentCommitment` re-export renamed to `calculateParentCommitment` (no leading underscore); the `_` prefix is reserved for module-internal helpers in this codebase. - `@circuitInfo` for `_drain` bumped 47778 → 47811 rows after the Vector<2> → Vector<3> commitment-input shape change.
Address PR #526 review feedback on missing double-init / init-guard coverage. The preset constructors are the only init entry on the shipping API, so this layer cannot drive a second `_init` call without re-exposing the underscore-prefixed circuit. Following the Signer/MockSigner convention, two mock contracts wrap the modules directly and expose `initialize`, `deposit*`, `drain`, etc. as test-only circuits: - MockForwarder.compact exposes the public Forwarder module - MockForwarderPrivate.compact exposes the private one The mocks take an `isInit` constructor flag (false to test the not-initialized path; true then `initialize(...)` for double-init). Empty witness stubs and `createSimulator`-based wrappers are added alongside. New module-level test files exercise behaviour previously covered in the preset tests: - test/Forwarder.test.ts — init guards (zero-parent, double-init, late-init), assertInitialized firing on every state-touching circuit, `_recordReceived` accumulation via both deposit paths, Uint<128> overflow on the unshielded path, Uint<64> Zswap cap on the shielded path, property-based accumulation. - test/ForwarderPrivate.test.ts — init guards (zero-commitment, double-init), assertInitialized, `calculateParentCommitment` purity + unlinkability, drain happy/failure paths, change arithmetic on partial drains, property-based change arithmetic. 971 / 971 tests pass.
Simplify the coverage `include` to glob patterns that cover every compiled artifact instead of listing each Forwarder file by hand, and collapse the duplicated `**/` patterns in `witnesses/` and `simulators/`. Drop `.compact` files from the report after source-map remap (`excludeAfterRemap: true`). The compactc-emitted source map is function-entry granularity only (every statement inside a circuit collapses onto the circuit header line), so back-projecting v8 branch/line coverage to `.compact` produces misleading partial coverage on circuits that are fully exercised in tests — e.g. `ForwarderPrivate._drain`'s `if (disclose(result.change.is_some))` reports 50 % branches even though both legs run via the partial-drain and full-drain test cases plus the property suite. Coverage now tracks: every TS witness, every test simulator, every artifacts/*/contract/index.js shim. With this scope all Forwarder TS files hit 100 % lines / branches / functions / statements; the JS shim retains real v8 instrumentation for circuit execution. Upstream tracker for the source-map fidelity work: LFDT-Minokawa/compact#465
Member
Author
|
Member
Author
|
@coderabbitai review |
The ForwarderPrivate simulators hand-inlined `ShieldedCoinInfo`, `QualifiedShieldedCoinInfo`, and `ShieldedSendResult` as structural types, which drift from the generated artifact. * Re-export the three STD types from the ForwarderPrivate preset and the MockForwarderPrivate fixture so compactc emits named aliases in the generated `index.d.ts`. * Import the named types in both simulators instead of inlining them. Mirrors the parent-typing change in the preceding commit. No behavioral change; the 37 forwarder tests still pass. Refs: #526
Comment 3 (6641069) replaced the brace-list imports in the forwarder presets and mocks with the bare `prefix` form, but the two module files were missed and still imported named symbols. * `Forwarder.compact` now uses `import "../security/Initializable" prefix Initializable_;`. * `ForwarderPrivate.compact` does the same, plus `import "../utils/Utils" prefix Utils_;`. The bare form imports every exported member under the prefix, so the call sites (`Initializable_initialize`, `Initializable_assertInitialized`, `Utils_selfAsRecipient`) are unchanged. Matches the other multisig presets; with no go-to-def in the compact-lsp the brace list bought nothing. 37 forwarder tests still pass. Refs: #526
Comment 6: `salt` reads like a throwaway randomizer, but in ForwarderPrivate it is the sole drain authorization. Losing it loses every accumulated coin. Rename it to `opSecret` (operational secret) so the name signals that criticality at every call site. * Rename the `salt` parameter to `opSecret` in `_drain` and `_calculateParentCommitment`, plus the preset and mock wrappers. * Rename the test constants `SALT` / `WRONG_SALT` to `OP_SECRET` / `WRONG_OP_SECRET`, and the simulator and helper params. * Reword the doc comments to drop the "salt" term and describe the value as the operational secret. Pure rename; no behavioral change. 37 forwarder tests pass. Refs: #526
Member
Author
|
@andrew-fleming Thanks for the good points! 👌 all fixed:
|
pepebndc
approved these changes
Jun 8, 2026
0xisk
enabled auto-merge (squash)
June 8, 2026 13:12
8 tasks
andrew-fleming
left a comment
andrew-fleming
left a comment
Contributor
There was a problem hiding this comment.
Looking good @0xisk! I left some comments
Comment on lines
+38
to
+39
| * Called once from the preset constructor. The parent is the | ||
| * recipient of every forwarded coin and is immutable after init. |
Contributor
There was a problem hiding this comment.
I actually think it'd be better if we don't enforce immutability. For contracts that will not change the parent, it's as simple as don't create/expose a circuit for it. For contracts that want to change the parent, this can be easily added...either in the module itself or it can be deferred for the contract to handle. Otherwise, users are forced to do something like create their own workaround or build their own module
Contributor
There was a problem hiding this comment.
Instead of having a bajillion empty witnesses/PS, can we just define it once in multisig/test/ or even in test-utils/ and import it into the simulator? I only ask now in this specific context because it's getting unnecessarily bloated and messy
| receiveShielded(disclose(coin)); | ||
| sendImmediateShielded( | ||
| disclose(coin), | ||
| left<ZswapCoinPublicKey, ContractAddress>(ZswapCoinPublicKey { bytes: _parent.bytes }), |
Contributor
There was a problem hiding this comment.
I'd be really careful here. What happens if a user instantiates the generic with ContractAddress? The encoding is different so that'd be a big problem and there's nothing preventing a user from doing this. I think the generic needs to be Either<T1, T2>
Contributor
There was a problem hiding this comment.
Or better yet, to avoid ALL confusion, we could just split this into two modules and define the concrete types (shielded or unshielded Either<>). There's really not a whole lot of duplication in the module...just a single state variable and initializer. WDYT?
5 tasks
Member
Author
|
Thanks @andrew-fleming for your followup review. I opened an issue having all those comments will solve them on a following PR. |
github-project-automation
Bot
moved this from Needs Review
to Complete/Stable Release
in OZ Development for Midnight
8 tasks
0xisk
added a commit
that referenced
this pull request
Jun 9, 2026
Address the remaining review feedback from #526, tracked in #565. * Type safety: replace the generic `Forwarder<T>` module, which rebuilt the recipient from `_parent.bytes` and could be mis-encoded if instantiated with the wrong address kind, with two concrete modules. `ForwarderShielded` stores `Either<ZswapCoinPublicKey, ContractAddress>` and `ForwarderUnshielded` stores `Either<ContractAddress, UserAddress>`. The parent is passed straight to the send call, so the deployer must choose the recipient arm explicitly and no encoding ambiguity remains. * Immutability: drop `sealed` from the public forwarder parent and stop claiming it is immutable. A preset that keeps the parent fixed simply omits a setter; a consuming contract may add one. ForwarderPrivate is left sealed (its commitment is the sole drain gate). * Tests: define the empty private state and witnesses once in `multisig/test/EmptyWitnesses.ts` and import it into the forwarder simulators, removing the five redundant per-contract witness files. Split the combined mock into `MockForwarderShielded` / `MockForwarderUnshielded` (the two modules cannot share one `Initializable` instance) and add the matching `Either` test helpers. Refs: #565
0xisk
added a commit
that referenced
this pull request
Jun 9, 2026
Address the remaining review feedback from #526, tracked in #565. * Type safety: replace the generic `Forwarder<T>` module, which rebuilt the recipient from `_parent.bytes` and could be mis-encoded if instantiated with the wrong address kind, with two concrete modules. `ForwarderShielded` stores `Either<ZswapCoinPublicKey, ContractAddress>` and `ForwarderUnshielded` stores `Either<ContractAddress, UserAddress>`. The parent is passed straight to the send call, so the deployer must choose the recipient arm explicitly and no encoding ambiguity remains. * Immutability: drop `sealed` from the public forwarder parent and stop claiming it is immutable. A preset that keeps the parent fixed simply omits a setter; a consuming contract may add one. ForwarderPrivate is left sealed (its commitment is the sole drain gate). * Tests: define the empty private state and witnesses once in `multisig/test/EmptyWitnesses.ts` and import it into the forwarder simulators, removing the five redundant per-contract witness files. Split the combined mock into `MockForwarderShielded` / `MockForwarderUnshielded` (the two modules cannot share one `Initializable` instance) and add the matching `Either` test helpers. Refs: #565
This was referenced Jun 15, 2026
This was referenced Jun 15, 2026
0xisk
added a commit
that referenced
this pull request
Jun 17, 2026
Address the remaining review feedback from #526, tracked in #565. * Type safety: replace the generic `Forwarder<T>` module, which rebuilt the recipient from `_parent.bytes` and could be mis-encoded if instantiated with the wrong address kind, with two concrete modules. `ForwarderShielded` stores `Either<ZswapCoinPublicKey, ContractAddress>` and `ForwarderUnshielded` stores `Either<ContractAddress, UserAddress>`. The parent is passed straight to the send call, so the deployer must choose the recipient arm explicitly and no encoding ambiguity remains. * Immutability: drop `sealed` from the public forwarder parent and stop claiming it is immutable. A preset that keeps the parent fixed simply omits a setter; a consuming contract may add one. ForwarderPrivate is left sealed (its commitment is the sole drain gate). * Tests: define the empty private state and witnesses once in `multisig/test/EmptyWitnesses.ts` and import it into the forwarder simulators, removing the five redundant per-contract witness files. Split the combined mock into `MockForwarderShielded` / `MockForwarderUnshielded` (the two modules cannot share one `Initializable` instance) and add the matching `Either` test helpers. Refs: #565
0xisk
added a commit
that referenced
this pull request
Jun 18, 2026
Address the remaining review feedback from #526, tracked in #565. * Type safety: replace the generic `Forwarder<T>` module, which rebuilt the recipient from `_parent.bytes` and could be mis-encoded if instantiated with the wrong address kind, with two concrete modules. `ForwarderShielded` stores `Either<ZswapCoinPublicKey, ContractAddress>` and `ForwarderUnshielded` stores `Either<ContractAddress, UserAddress>`. The parent is passed straight to the send call, so the deployer must choose the recipient arm explicitly and no encoding ambiguity remains. * Immutability: drop `sealed` from the public forwarder parent and stop claiming it is immutable. A preset that keeps the parent fixed simply omits a setter; a consuming contract may add one. ForwarderPrivate is left sealed (its commitment is the sole drain gate). * Tests: define the empty private state and witnesses once in `multisig/test/EmptyWitnesses.ts` and import it into the forwarder simulators, removing the five redundant per-contract witness files. Split the combined mock into `MockForwarderShielded` / `MockForwarderUnshielded` (the two modules cannot share one `Initializable` instance) and add the matching `Either` test helpers. Refs: #565
8 tasks
8 tasks
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
3 participants
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 #474
PR Checklist
Summary by CodeRabbit
New Features
Tests