feat(multisig): add Forwarder + ForwarderPrivate modules#526
Open
0xisk wants to merge 10 commits into
Open
Conversation
|
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 |
✅ Actions performedReview triggered.
|
|
Actionable comments posted: 0 |
Member
Author
|
@coderabbitai review this pr. |
|
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/src/multisig/witnesses/presets/ForwarderPrivateWitnesses.ts`:
- Line 2: Update the header comment on the top of ForwarderPrivateWitnesses.ts
so the file path in the comment matches the actual location by adding the
missing "presets/" segment; replace the existing "// OpenZeppelin Compact
Contracts v0.0.1-alpha.1 (multisig/witnesses/ForwarderPrivateWitnesses.ts)" with
the correct path including "presets" (i.e.,
"(multisig/witnesses/presets/ForwarderPrivateWitnesses.ts)") so the file comment
reflects the true location.
In `@contracts/src/multisig/witnesses/presets/ForwarderShieldedWitnesses.ts`:
- Line 2: Update the review comment's file attribute to include the missing
presets subdirectory: change the comment metadata file value to
"multisig/witnesses/presets/ForwarderShieldedWitnesses.ts" so it matches the
actual file location (referencing ForwarderShieldedWitnesses.ts) and ensure any
other review entries for this file use the same corrected path.
In `@contracts/src/multisig/witnesses/presets/ForwarderUnshieldedWitnesses.ts`:
- Line 2: Update the header comment path to match the actual file location by
adding the missing "presets/" segment; in the file
ForwarderUnshieldedWitnesses.ts (symbol: ForwarderUnshieldedWitnesses) replace
the current top-line comment that omits "presets/" so it reads the correct path
"multisig/witnesses/presets/ForwarderUnshieldedWitnesses.ts".
🪄 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: fb8c2a1a-166f-4b43-90d6-c865c64e2903
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (25)
.gitignorecontracts/package.jsoncontracts/src/multisig/Forwarder.compactcontracts/src/multisig/ForwarderPrivate.compactcontracts/src/multisig/presets/forwarder/ForwarderPrivate.compactcontracts/src/multisig/presets/forwarder/ForwarderShielded.compactcontracts/src/multisig/presets/forwarder/ForwarderUnshielded.compactcontracts/src/multisig/test/Forwarder.test.tscontracts/src/multisig/test/ForwarderPrivate.test.tscontracts/src/multisig/test/mocks/MockForwarder.compactcontracts/src/multisig/test/mocks/MockForwarderPrivate.compactcontracts/src/multisig/test/presets/ForwarderPrivate.test.tscontracts/src/multisig/test/presets/ForwarderShielded.test.tscontracts/src/multisig/test/presets/ForwarderUnshielded.test.tscontracts/src/multisig/test/simulators/MockForwarderPrivateSimulator.tscontracts/src/multisig/test/simulators/MockForwarderSimulator.tscontracts/src/multisig/test/simulators/presets/ForwarderPrivateSimulator.tscontracts/src/multisig/test/simulators/presets/ForwarderShieldedSimulator.tscontracts/src/multisig/test/simulators/presets/ForwarderUnshieldedSimulator.tscontracts/src/multisig/witnesses/MockForwarderPrivateWitnesses.tscontracts/src/multisig/witnesses/MockForwarderWitnesses.tscontracts/src/multisig/witnesses/presets/ForwarderPrivateWitnesses.tscontracts/src/multisig/witnesses/presets/ForwarderShieldedWitnesses.tscontracts/src/multisig/witnesses/presets/ForwarderUnshieldedWitnesses.tscontracts/vitest.config.ts
Header comment on the preset witness files still referenced the pre-rename location (multisig/witnesses/...) after the move to presets/. Add the missing presets/ segment so the comment matches the actual path. Raised by CodeRabbit on #526.
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
2 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