refactor(multisig): make forwarder parent type-safe - #566
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:
WalkthroughReplaces the generic ChangesForwarder module split and test infrastructure update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
Comment |
0xisk
force-pushed
the
refactor/forwarder-review-feedback
branch
2 times, most recently
from
June 9, 2026 15:08
2720d91 to
8d33f18
Compare
andrew-fleming
left a comment
andrew-fleming
left a comment
Contributor
There was a problem hiding this comment.
Changes look good, @0xisk! I just left two questions that I think we should address before merging
| */ | ||
| module ForwarderUnshielded { | ||
| import CompactStandardLibrary; | ||
| import "../security/Initializable" prefix Initializable_; |
Member
Author
There was a problem hiding this comment.
Done. Same inline _isInitialized flag here too, via the #610 rebase.
0xisk
added a commit
that referenced
this pull request
Jun 10, 2026
Drop the shared Initializable import from ForwarderShielded and ForwarderUnshielded; each now owns its _isInitialized ledger flag and inlines assertInitialized / assertNotInitialized with module-specific revert messages. These are new, unreleased modules, so this avoids shipping the shared transitive-dependency pattern that collapses two same-directory imports into one ledger slot (compiler#270), matching the per-module layout used elsewhere. Tests assert the new ForwarderShielded / ForwarderUnshielded init messages. Refs: #566
0xisk
added a commit
that referenced
this pull request
Jun 10, 2026
Add init coverage for a contract-address parent on both forwarder modules, including a zero-contract-address failure case, per review followup. Adds createEitherTestUnshieldedContract / ZERO_UNSHIELDED_CONTRACT test helpers for the unshielded contract arm (Either<ContractAddress, UserAddress>); the shielded arm reuses the existing contract-address helpers. Refs: #566
Member
Author
|
Thanks @andrew-fleming! Pushed the fixes:
Verified: |
0xisk
enabled auto-merge (squash)
June 10, 2026 12:22
0xisk
added a commit
that referenced
this pull request
Jun 17, 2026
Drop the shared Initializable import from ForwarderShielded and ForwarderUnshielded; each now owns its _isInitialized ledger flag and inlines assertInitialized / assertNotInitialized with module-specific revert messages. These are new, unreleased modules, so this avoids shipping the shared transitive-dependency pattern that collapses two same-directory imports into one ledger slot (compiler#270), matching the per-module layout used elsewhere. Tests assert the new ForwarderShielded / ForwarderUnshielded init messages. Refs: #566
0xisk
added a commit
that referenced
this pull request
Jun 17, 2026
Add init coverage for a contract-address parent on both forwarder modules, including a zero-contract-address failure case, per review followup. Adds createEitherTestUnshieldedContract / ZERO_UNSHIELDED_CONTRACT test helpers for the unshielded contract arm (Either<ContractAddress, UserAddress>); the shielded arm reuses the existing contract-address helpers. Refs: #566
0xisk
force-pushed
the
refactor/forwarder-review-feedback
branch
from
June 17, 2026 12:03
e520c26 to
ff118f9
Compare
![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 (1)
contracts/src/multisig/ForwarderUnshielded.compact (1)
62-70: 💤 Low valueConsider extracting zero-check to
Utilsfor consistency withForwarderShielded.
ForwarderShieldedusesUtils_isKeyOrAddressZero(parent)while this module inlines the zero check. Both are correct, but extracting aUtils_isContractOrUserAddressZerohelper would align the modules and reduce duplication if similar checks are needed elsewhere.🤖 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/multisig/ForwarderUnshielded.compact` around lines 62 - 70, The initialize function in ForwarderUnshielded contains an inlined zero-check for the parent parameter (the isZero variable assignment that checks either default ContractAddress or default UserAddress), while ForwarderShielded uses a utility function Utils_isKeyOrAddressZero for the same purpose. Extract this zero-check logic into a new utility function in the Utils module (name it Utils_isContractOrUserAddressZero to reflect that it handles Either<ContractAddress, UserAddress>) and replace the inline logic in the initialize function with a call to this new utility function, ensuring consistency between both ForwarderUnshielded and ForwarderShielded modules.
🤖 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/address.ts`:
- Around line 123-125: The encodeToUserAddress function does not enforce a fixed
32-byte width for UserAddress fixtures, allowing bytes to exceed 32 bytes when
inputs are longer than 32 ASCII bytes since toHexPadded pads but never
truncates. Modify the function to slice or truncate the resulting bytes array to
exactly 32 bytes after the Uint8Array.from conversion, ensuring all UserAddress
fixtures have a consistent fixed width regardless of input length.
---
Nitpick comments:
In `@contracts/src/multisig/ForwarderUnshielded.compact`:
- Around line 62-70: The initialize function in ForwarderUnshielded contains an
inlined zero-check for the parent parameter (the isZero variable assignment that
checks either default ContractAddress or default UserAddress), while
ForwarderShielded uses a utility function Utils_isKeyOrAddressZero for the same
purpose. Extract this zero-check logic into a new utility function in the Utils
module (name it Utils_isContractOrUserAddressZero to reflect that it handles
Either<ContractAddress, UserAddress>) and replace the inline logic in the
initialize function with a call to this new utility function, ensuring
consistency between both ForwarderUnshielded and ForwarderShielded modules.
🪄 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: e957fe25-669c-43f3-b1ba-c93c03f86b75
📒 Files selected for processing (26)
contracts/src/multisig/Forwarder.compactcontracts/src/multisig/ForwarderShielded.compactcontracts/src/multisig/ForwarderUnshielded.compactcontracts/src/multisig/presets/forwarder/ForwarderShielded.compactcontracts/src/multisig/presets/forwarder/ForwarderUnshielded.compactcontracts/src/multisig/test/EmptyWitnesses.tscontracts/src/multisig/test/Forwarder.test.tscontracts/src/multisig/test/ForwarderPrivate.test.tscontracts/src/multisig/test/mocks/MockForwarder.compactcontracts/src/multisig/test/mocks/MockForwarderShielded.compactcontracts/src/multisig/test/mocks/MockForwarderUnshielded.compactcontracts/src/multisig/test/presets/ForwarderShielded.test.tscontracts/src/multisig/test/presets/ForwarderUnshielded.test.tscontracts/src/multisig/test/simulators/MockForwarderPrivateSimulator.tscontracts/src/multisig/test/simulators/MockForwarderShieldedSimulator.tscontracts/src/multisig/test/simulators/MockForwarderSimulator.tscontracts/src/multisig/test/simulators/MockForwarderUnshieldedSimulator.tscontracts/src/multisig/test/simulators/presets/ForwarderPrivateSimulator.tscontracts/src/multisig/test/simulators/presets/ForwarderShieldedSimulator.tscontracts/src/multisig/test/simulators/presets/ForwarderUnshieldedSimulator.tscontracts/src/multisig/test/witnesses/MockForwarderPrivateWitnesses.tscontracts/src/multisig/test/witnesses/MockForwarderWitnesses.tscontracts/src/multisig/test/witnesses/presets/ForwarderPrivateWitnesses.tscontracts/src/multisig/test/witnesses/presets/ForwarderShieldedWitnesses.tscontracts/src/multisig/test/witnesses/presets/ForwarderUnshieldedWitnesses.tscontracts/test-utils/address.ts
💤 Files with no reviewable changes (8)
- contracts/src/multisig/test/witnesses/MockForwarderPrivateWitnesses.ts
- contracts/src/multisig/test/witnesses/presets/ForwarderPrivateWitnesses.ts
- contracts/src/multisig/test/witnesses/presets/ForwarderUnshieldedWitnesses.ts
- contracts/src/multisig/test/simulators/MockForwarderSimulator.ts
- contracts/src/multisig/Forwarder.compact
- contracts/src/multisig/test/mocks/MockForwarder.compact
- contracts/src/multisig/test/witnesses/presets/ForwarderShieldedWitnesses.ts
- contracts/src/multisig/test/witnesses/MockForwarderWitnesses.ts
Comment on lines
+123
to
+125
| export const encodeToUserAddress = (str: string): UserAddress => ({ | ||
| bytes: Uint8Array.from(Buffer.from(toHexPadded(str), 'hex')), | ||
| }); |
Contributor
There was a problem hiding this comment.
Enforce fixed 32-byte width for UserAddress fixtures.
Line 124 can produce bytes.length > 32 for inputs longer than 32 ASCII bytes because toHexPadded pads but never truncates. That can silently create invalid unshielded-recipient fixtures.
Suggested patch
export const encodeToUserAddress = (str: string): UserAddress => ({
- bytes: Uint8Array.from(Buffer.from(toHexPadded(str), 'hex')),
-});
+ bytes: (() => {
+ const bytes = Uint8Array.from(Buffer.from(toHexPadded(str), 'hex'));
+ if (bytes.length !== 32) {
+ throw new Error('Invalid Input: `UserAddress` must be exactly 32 bytes');
+ }
+ return bytes;
+ })(),
+});🤖 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/test-utils/address.ts` around lines 123 - 125, The
encodeToUserAddress function does not enforce a fixed 32-byte width for
UserAddress fixtures, allowing bytes to exceed 32 bytes when inputs are longer
than 32 ASCII bytes since toHexPadded pads but never truncates. Modify the
function to slice or truncate the resulting bytes array to exactly 32 bytes
after the Uint8Array.from conversion, ensuring all UserAddress fixtures have a
consistent fixed width regardless of input length.
andrew-fleming
left a comment
andrew-fleming
left a comment
Contributor
There was a problem hiding this comment.
I left a few comments but this looks about good to go. I think we should improve our tests by checking the zswap inputs and outputs to ensure the forwarder is forwarding correctly on the zswap level. Let's call it a followup. This is also true of the other modules that handle send and receive
Member
Author
I agree and the best way for doing this is by hacing the simulator runs those tests on the local node infra so we are more sure about that. |
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
Drop the shared Initializable import from ForwarderShielded and ForwarderUnshielded; each now owns its _isInitialized ledger flag and inlines assertInitialized / assertNotInitialized with module-specific revert messages. These are new, unreleased modules, so this avoids shipping the shared transitive-dependency pattern that collapses two same-directory imports into one ledger slot (compiler#270), matching the per-module layout used elsewhere. Tests assert the new ForwarderShielded / ForwarderUnshielded init messages. Refs: #566
Add init coverage for a contract-address parent on both forwarder modules, including a zero-contract-address failure case, per review followup. Adds createEitherTestUnshieldedContract / ZERO_UNSHIELDED_CONTRACT test helpers for the unshielded contract arm (Either<ContractAddress, UserAddress>); the shielded arm reuses the existing contract-address helpers. Refs: #566
Generalize ForwarderPrivate._drain's parent from Bytes<32> to Either<ZswapCoinPublicKey, ContractAddress> so the operator selects the recipient type (coin public key or contract address) at drain time. The commitment still binds only the 32 address bytes plus opSecret, so the deployer's off-chain computation is unchanged and the same commitment authorizes either arm. * canonicalize the parent first, zeroing the inactive arm, then derive the preimage bytes from the active arm so a dual-arm input cannot desync the committed bytes from the sent recipient * reject a zero parent before the commitment gate * disclose(p) sends to the operator-chosen recipient; both the address bytes and the arm selector stay encrypted in the Zswap output * drop the sealed modifier on _parentCommitment to match the public forwarders' _parent; write-once stays enforced by the Initializable init gate Tests drive both arms, zero-parent rejection, and dual-arm canonicalization; the commitment is read via a new mock getter since a prefix-imported module ledger field is not in the public reader. _drain recompiles at k=16, rows=48133.
Extend the ForwarderPrivate module header to reflect the generalized drain: the operator chooses the recipient arm (coin public key or contract address) via the Either parent, and the commitment binds the parent bytes, not the recipient type. Comment-only; no behavior change.
A shielded send to a contract publishes the recipient contract address in cleartext on the transaction (the protocol routes the coin to a named contract); a coin-public-key recipient stays hidden in the Zswap note. Confirmed end-to-end on preprod. The prior _drain accepted an Either<ZswapCoinPublicKey, ContractAddress> parent, so the contract arm leaked the parent at every drain — defeating the private-parent guarantee. Restrict the parent to a ZswapCoinPublicKey: * _drain / preset drain: parent Either<...> -> ZswapCoinPublicKey, always sent via the left arm. Drop the canonicalize + arm-selection (and the dual-arm-desync / operator-selected-type concerns they guarded). * zero-parent guard is now isKeyZero. * commitment scheme unchanged (over the parent key's 32 bytes); the deployer's off-chain computation is unchanged. * tests: drop the contract-arm / dual-arm cases; drain drives a coin-key parent. 24/24 pass. Breaking for callers (pre-release): drain's parent argument changes type. _drain recompiles at k=16, rows=41961 (was 48133).
A shielded or unshielded send to a contract recipient is valid only if that contract claims the output in the same transaction. An atomic forwarder runs only its own circuit, so a third-party contract parent never claims the output and every deposit is rejected. Confirmed on preprod for the shielded case (unclaimed output, node error 186). Narrow the ForwarderShielded and ForwarderUnshielded circuit parameter to the deliverable arm (ZswapCoinPublicKey and UserAddress) so a contract parent cannot be expressed. The _parent ledger field stays a generic Either, so a future CMA circuit upgrade can add contract support with no state-layout migration. initialize stores the supported arm via left()/right(); getParent returns the stored Either. Update mocks, simulators, and tests for the narrow constructor argument and drop the obsolete contract-address parent cases.
ForwarderPrivate now uses the inline _isInitialized flag (init-dep removal from #610, merged during rebase), which asserts "ForwarderPrivate: contract not initialized". Update the two init-guard assertions that still expected the old Initializable message.
0xisk
force-pushed
the
refactor/forwarder-review-feedback
branch
from
June 18, 2026 10:41
07e7aec to
79f8e79
Compare
Member
Author
Forwarder recipient finding + rebased on
|
Per review: a forwarder strands every deposit if the parent cannot spend the forwarded funds, and the zero key/address guard catches only the all-zero value. Document the deployer's responsibility in both the module header and `initialize`, for ForwarderShielded and ForwarderUnshielded.
Per review: `toHexPadded` pads but never truncates, so inputs longer than 32 ASCII bytes silently produced oversized UserAddress fixtures. Throw if the encoded value is not exactly 32 bytes, matching `encodeToAddress`.
andrew-fleming
approved these changes
Jun 19, 2026
andrew-fleming
left a comment
andrew-fleming
left a comment
Contributor
There was a problem hiding this comment.
Great work, @0xisk! Non-blocking final suggestions. We can apply the suggestions in a new PR to avoid an approve + merge lag. Your call
Comment on lines
+255
to
+265
| // INV-12 / INV-25: a drain performs no ledger write. `_parentCommitment` is | ||
| // written only at init; it is unchanged after a drain and no recipient field | ||
| // is added. (Read via the getter circuit — the module is imported with a | ||
| // prefix only, so it is not in the public ledger reader.) | ||
| // | ||
| // INV-17 (recipient privacy): the parent coin public key flows only into the | ||
| // `sendShielded` recipient, where it is encrypted inside the Zswap output and | ||
| // never appears on the public transcript. Confirmed end-to-end on preprod (a | ||
| // coin-public-key recipient occurs 0 times in the published tx); not | ||
| // simulator-observable, so it is asserted by the residual-surface check here. | ||
| describe('drain — residual public surface (INV-12 / INV-17 / INV-25)', () => { |
| @@ -0,0 +1,152 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/ForwarderUnshielded.compact) | |||
Contributor
There was a problem hiding this comment.
Suggested change
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/ForwarderUnshielded.compact) | |
| // OpenZeppelin Compact Contracts v0.2.0 (multisig/ForwarderUnshielded.compact) |
| @@ -0,0 +1,149 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/ForwarderShielded.compact) | |||
Contributor
There was a problem hiding this comment.
Suggested change
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/ForwarderShielded.compact) | |
| // OpenZeppelin Compact Contracts v0.2.0 (multisig/ForwarderShielded.compact) |
| @@ -0,0 +1,31 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/test/mocks/MockForwarderShielded.compact) | |||
Contributor
There was a problem hiding this comment.
Suggested change
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/test/mocks/MockForwarderShielded.compact) | |
| // OpenZeppelin Compact Contracts v0.2.0 (multisig/test/mocks/MockForwarderShielded.compact) |
Contributor
There was a problem hiding this comment.
followup: We should also apply this to all the mocks and remove the version
// WARNING: FOR TESTING PURPOSES ONLY.
// This contract exposes internal circuits and bypasses safety checks that the
// corresponding production contract relies on. DO NOT deploy or use this
// contract in any production application.
| @@ -0,0 +1,31 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/test/mocks/MockForwarderUnshielded.compact) | |||
Contributor
There was a problem hiding this comment.
Suggested change
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/test/mocks/MockForwarderUnshielded.compact) | |
| // OpenZeppelin Compact Contracts v0.2.0 (multisig/test/mocks/MockForwarderUnshielded.compact) |
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.
Important
Depends on #526 (
feat/forwarder). This branch is stacked on it.The diff will also show #526's commits until that PR merges into
post-release; review only therefactor(multisig): make forwarder parent type-safecommit here.Types of changes
What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Fixes #565
Addresses the three remaining review threads from @andrew-fleming on #526.
The forwarder is not yet released, so these refine unmerged code.
Type safety. The generic
Forwarder<T>rebuilt the recipient from_parent.bytesand wrapped it in a hard-codedEitherarm, soinstantiating it with the wrong address kind silently mis-encoded the
recipient. It is replaced by two concrete modules that store the real
recipient type and pass it straight to the send call:
ForwarderShielded—Either<ZswapCoinPublicKey, ContractAddress>ForwarderUnshielded—Either<ContractAddress, UserAddress>The deployer now chooses the recipient arm explicitly; there is no way
to mis-encode it.
Immutability. Dropped
sealedfrom the public forwarder parent andremoved the "immutable after init" claim. A preset that keeps the
parent fixed simply omits a setter; a consuming contract may add one.
ForwarderPrivatestays sealed (its commitment is the sole drain gate).Test cleanup. Empty private state + witnesses defined once in
multisig/test/EmptyWitnesses.tsand shared by the forwardersimulators; five redundant per-contract witness files removed. The
combined mock is split into
MockForwarderShielded/MockForwarderUnshielded(the two modules cannot share oneInitializableinstance).PR Checklist
Further comments
A single
Either<T1, T2>generic could not serve both deposit kinds(shielded and unshielded need different arm orderings), so the two-module
split is the type-safe path Andrew preferred. All forwarder module +
preset suites pass (
vitest run Forwarder, 40 tests);biomeandtscare clean.
Summary by CodeRabbit
New Features
Improvements