Add multisig preset v3 targeting main#604
Open
andrew-fleming wants to merge 24 commits into
Open
Conversation
Co-authored-by: 0xisk <iskander.andrews@openzeppelin.com> Co-authored-by: 0xisk <0xisk@proton.me>
Signed-off-by: 0xisk <0xisk@proton.me> Co-authored-by: andrew <fleming-andrew@protonmail.com> Co-authored-by: 0xisk <iskander.andrews@openzeppelin.com>
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 ChangesShieldedMultiSigV3 Contract, Simulator, and Tests
Placeholder Witness Module Scaffolding
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ShieldedMultiSigV3
participant SignerRegistry
participant ZswapRuntime
rect rgba(100, 149, 237, 0.5)
Note over Caller, ZswapRuntime: mint flow
Caller->>ShieldedMultiSigV3: mint(amount, recipient, pubkeys[2], signatures[2])
ShieldedMultiSigV3->>ShieldedMultiSigV3: _counter++, build multisig:mint: message hash
loop fold over 2 signers
ShieldedMultiSigV3->>SignerRegistry: derive commitment(pubkey, instanceSalt), assert member, no duplicate
end
ShieldedMultiSigV3->>ShieldedMultiSigV3: evolve _coinNonce
ShieldedMultiSigV3->>ZswapRuntime: mintShieldedToken(recipient, amount, nonce)
end
rect rgba(200, 100, 100, 0.5)
Note over Caller, ZswapRuntime: burn flow
Caller->>ShieldedMultiSigV3: burn(coin, amount, pubkeys[2], signatures[2])
ShieldedMultiSigV3->>ShieldedMultiSigV3: _counter++, build multisig:burn: message hash
loop fold over 2 signers
ShieldedMultiSigV3->>SignerRegistry: derive commitment(pubkey, instanceSalt), assert member, no duplicate
end
ShieldedMultiSigV3->>ShieldedMultiSigV3: assert coin.color == tokenType, coin.value >= amount
ShieldedMultiSigV3->>ZswapRuntime: receiveShieldedCoin(coin), send amount to burnAddress
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 3
🧹 Nitpick comments (1)
contracts/src/multisig/test/simulators/ShieldedMultiSigV3Simulator.ts (1)
16-22: ⚡ Quick winUse tuple types for fixed-size multisig vectors.
The contract expects fixed lengths, but simulator APIs accept unbounded arrays. Type this as tuples to catch invalid call shapes at compile time.
Suggested typing refinement
type ShieldedMultiSigV3Args = readonly [ instanceSalt: Uint8Array, initCoinNonce: Uint8Array, tokenDomain: Uint8Array, - signerCommitments: Uint8Array[], + signerCommitments: [Uint8Array, Uint8Array, Uint8Array], thresh: bigint, ]; @@ public mint( amount: bigint, recipient: Either<ZswapCoinPublicKey, ContractAddress>, - pubkeys: Uint8Array[], - signatures: Uint8Array[], + pubkeys: [Uint8Array, Uint8Array], + signatures: [Uint8Array, Uint8Array], ) { @@ amount: bigint, - pubkeys: Uint8Array[], - signatures: Uint8Array[], + pubkeys: [Uint8Array, Uint8Array], + signatures: [Uint8Array, Uint8Array], ) {Also applies to: 67-72, 84-85
🤖 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/test/simulators/ShieldedMultiSigV3Simulator.ts` around lines 16 - 22, The ShieldedMultiSigV3Args type definition at lines 16-22 (and similar type definitions at lines 67-72 and 84-85) uses unbounded array types like Uint8Array[] for signerCommitments, but the contract expects fixed-length vectors. Replace the unbounded array types with fixed-length tuple types that match the contract's requirements. For signerCommitments in ShieldedMultiSigV3Args and any similar vector fields in the other affected type definitions, define them as tuples with a specific length (for example, as readonly [Uint8Array, Uint8Array, ...] or similar) instead of Uint8Array[], so that TypeScript enforces the correct number of elements at compile time. Ensure all three type definition sites (lines 16-22, 67-72, and 84-85) are updated consistently with the same fixed-length tuple patterns that the contract expects.
🤖 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/presets/ShieldedMultiSigV3.compact`:
- Around line 86-87: The signer-domain literal used in documentation has
inconsistent casing compared to the actual implementation in _calculateSignerId.
Update the documentation comments at the affected locations to use
"Multisig:signer:" (with capital M) instead of "multisig:signer:" (lowercase m)
to match what the implementation actually hashes, ensuring developers following
the documentation produce correct commitments. This inconsistency exists at
multiple locations including lines 86-87 and line 337.
- Around line 115-118: The assertion at ShieldedMultiSigV3.compact line 115-118
allows thresh <= 2 (including thresh = 1), but the runtime execution path always
requires and validates exactly 2 signatures, making thresh = 1 unenforceable.
Change the assertion to enforce thresh = 2 (or thresh >= 2 if higher values are
possible) and update the error message accordingly. Apply the same fix to all
sibling locations at lines 162-167, 237-242, and 305-310 to ensure consistency
across all threshold validation blocks in the contract.
- Around line 305-307: The stubVerifySignature function currently always returns
true, completely bypassing signature verification in the multisig contract and
making the authorization policy unenforceable. Replace the stub implementation
to fail-closed by returning false or throwing an error instead, ensuring that
signature verification fails until real ECDSA verification with ecdsaVerify and
keccak256 primitives can be properly implemented. This prevents authentication
bypass in both the signature assertion at line 305-307 and any other locations
where stubVerifySignature is called within the 345-351 range.
---
Nitpick comments:
In `@contracts/src/multisig/test/simulators/ShieldedMultiSigV3Simulator.ts`:
- Around line 16-22: The ShieldedMultiSigV3Args type definition at lines 16-22
(and similar type definitions at lines 67-72 and 84-85) uses unbounded array
types like Uint8Array[] for signerCommitments, but the contract expects
fixed-length vectors. Replace the unbounded array types with fixed-length tuple
types that match the contract's requirements. For signerCommitments in
ShieldedMultiSigV3Args and any similar vector fields in the other affected type
definitions, define them as tuples with a specific length (for example, as
readonly [Uint8Array, Uint8Array, ...] or similar) instead of Uint8Array[], so
that TypeScript enforces the correct number of elements at compile time. Ensure
all three type definition sites (lines 16-22, 67-72, and 84-85) are updated
consistently with the same fixed-length tuple patterns that the contract
expects.
🪄 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: 52caec6e-96c8-4565-81a5-b47ca638f541
📒 Files selected for processing (11)
contracts/src/multisig/presets/ShieldedMultiSigV3.compactcontracts/src/multisig/test/ShieldedMultiSigV3.test.tscontracts/src/multisig/test/simulators/ShieldedMultiSigV3Simulator.tscontracts/src/multisig/witnesses/ProposalManagerWitnesses.tscontracts/src/multisig/witnesses/ShieldedMultiSigV2Witnesses.tscontracts/src/multisig/witnesses/ShieldedMultiSigV3Witnesses.tscontracts/src/multisig/witnesses/ShieldedMultiSigWitnesses.tscontracts/src/multisig/witnesses/ShieldedTreasuryWitnesses.tscontracts/src/multisig/witnesses/SignerManagerWitnesses.tscontracts/src/multisig/witnesses/SignerWitnesses.tscontracts/src/multisig/witnesses/UnshieldedTreasuryWitnesses.ts
Comment on lines
+86
to
+87
| * `persistentHash(SignerCommitmentInput { pk, instanceSalt, domain })` | ||
| * where domain is `pad(32, "multisig:signer:")`. |
Contributor
There was a problem hiding this comment.
Signer-domain literal is inconsistent between docs and implementation.
Docs use multisig:signer: while _calculateSignerId hashes Multisig:signer:. Following the constructor docs can produce wrong commitments.
Suggested doc alignment (non-breaking)
- * where domain is `pad(32, "multisig:signer:")`.
+ * where domain is `pad(32, "Multisig:signer:")`.Also applies to: 337-337
🤖 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/presets/ShieldedMultiSigV3.compact` around lines 86 -
87, The signer-domain literal used in documentation has inconsistent casing
compared to the actual implementation in _calculateSignerId. Update the
documentation comments at the affected locations to use "Multisig:signer:" (with
capital M) instead of "multisig:signer:" (lowercase m) to match what the
implementation actually hashes, ensuring developers following the documentation
produce correct commitments. This inconsistency exists at multiple locations
including lines 86-87 and line 337.
Comment on lines
+305
to
+307
| // TODO: Replace with ecdsaVerify + keccak256 when primitives are available | ||
| assert(stubVerifySignature(pubkey, state.msgHash, signature), "Multisig: invalid signature"); | ||
|
|
Contributor
There was a problem hiding this comment.
Authentication is effectively disabled by the stub verifier.
Line 350 always returns true, so signature ownership is never proven in mint/burn. This makes authorization policy unenforceable.
Suggested fail-closed mitigation until real ECDSA verification is available
circuit stubVerifySignature(
pubkey: Bytes<64>,
msgHash: Bytes<32>,
signature: Bytes<64>
): Boolean {
- return true;
+ assert(false, "Multisig: signature verification unavailable in this preset");
+ return false;
}Also applies to: 345-351
🤖 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/presets/ShieldedMultiSigV3.compact` around lines 305 -
307, The stubVerifySignature function currently always returns true, completely
bypassing signature verification in the multisig contract and making the
authorization policy unenforceable. Replace the stub implementation to
fail-closed by returning false or throwing an error instead, ensuring that
signature verification fails until real ECDSA verification with ecdsaVerify and
keccak256 primitives can be properly implemented. This prevents authentication
bypass in both the signature assertion at line 305-307 and any other locations
where stubVerifySignature is called within the 345-351 range.
0xisk
requested changes
Jun 18, 2026
0xisk
left a comment
0xisk
left a comment
Member
There was a problem hiding this comment.
Thank you @andrew-fleming! Looking good just some comments left.
Comment on lines
+265
to
+271
| const _coin = ShieldedCoinInfo { | ||
| nonce: coin.nonce, | ||
| color: coin.color, | ||
| value: coin.value | ||
| }; | ||
| receiveShielded(disclose(_coin)); | ||
| sendShielded(disclose(coin), shieldedBurnAddress(), disclose(amount)); |
Member
There was a problem hiding this comment.
Suggested change
| const _coin = ShieldedCoinInfo { | |
| nonce: coin.nonce, | |
| color: coin.color, | |
| value: coin.value | |
| }; | |
| receiveShielded(disclose(_coin)); | |
| sendShielded(disclose(coin), shieldedBurnAddress(), disclose(amount)); | |
| sendShielded(disclose(coin), shieldedBurnAddress(), disclose(amount)); |
🔴 blocking: no need to receiveShielded as this is already a QualifiedCoinInfo.
Member
There was a problem hiding this comment.
🔴 blocking: Needs to rebase to remove all those repeated witnesses.
| // SPDX-License-Identifier: MIT | ||
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/presets/ShieldedMultiSigV3.compact) | ||
|
|
||
| pragma language_version >= 0.21.0; |
The coin passed to ShieldedMultiSigV3.burn is already a QualifiedShieldedCoinInfo, so sendShielded can spend it directly. The receiveShielded call and the intermediate ShieldedCoinInfo reconstruction were redundant; remove them and align the doc comments. Refs: OpenZeppelin#604
Align ShieldedMultiSigV3 with the repo-wide pragma raised in OpenZeppelin#598 and with its own deps (Signer, Utils), which already declare >= 0.23.0. Refs: OpenZeppelin#604
The PR introduced a second witnesses dir at src/multisig/witnesses/ holding byte-identical copies of files already in test/witnesses/. Only ShieldedMultiSigV3Witnesses.ts was actually consumed. Drop the 7 duplicates, move the V3 witness under test/witnesses/ to match the rest of the suite, and repoint the V3 simulator import to ../witnesses/ like the V2 simulator. Refs: OpenZeppelin#604
8 tasks
0xisk
reviewed
Jun 18, 2026
Comment on lines
+115
to
+118
| assert( | ||
| thresh <= 2, | ||
| "Multisig: threshold cannot exceed 2 (circuits verify at most 2 signatures)" | ||
| ); |
Member
There was a problem hiding this comment.
❔︎ question: shouldn't we assert also > 0?
Contributor
Author
There was a problem hiding this comment.
Signer disallows 0 already
export circuit initialize<#n>(
signers: Vector<n, T>,
thresh: Uint<8>
): [] {
(...)
_changeThreshold(thresh);
}
export circuit _changeThreshold(newThreshold: Uint<8>): [] {
assert(newThreshold <= _signerCount, "Signer: threshold exceeds signer count");
_setThreshold(newThreshold);
}
export circuit _setThreshold(newThreshold: Uint<8>): [] {
assert(newThreshold != 0, "Signer: threshold must not be zero");
_threshold = disclose(newThreshold);
}
Contributor
Author
There was a problem hiding this comment.
Contributor
Author
There was a problem hiding this comment.
Removed the thresh param since it can only be 2
…kers fix(multisig): resolve v3 preset review blockers
…g/compact-contracts into re-add-multisig-v3
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.
Summary by CodeRabbit
New Features
Tests