Conversation
WalkthroughIntroduces a comprehensive multisig governance framework comprising core modules for proposal lifecycle management, signer registry, shielded/unshielded treasury operations, complete test coverage, simulators, and two preset contract implementations supporting multi-signature approval workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Caller
participant Contract as ShieldedMultiSig
participant Proposal as ProposalManager
participant Signer as SignerManager
participant Treasury as ShieldedTreasury
User->>Contract: createShieldedProposal(recipient, color, amount)
Contract->>Signer: assertSigner(caller)
Signer-->>Contract: ✓
Contract->>Proposal: _createProposal(recipient, color, amount)
Proposal-->>Contract: proposal_id
User->>Contract: approveProposal(proposal_id)
Contract->>Signer: assertSigner(caller)
Signer-->>Contract: ✓
Contract->>Proposal: assertProposalActive(proposal_id)
Proposal-->>Contract: ✓
Contract->>Contract: update approval ledgers
User->>Contract: executeShieldedProposal(proposal_id)
Contract->>Signer: assertThresholdMet(approval_count)
Signer-->>Contract: ✓
Contract->>Proposal: getProposal(proposal_id)
Proposal-->>Contract: proposal_data
Contract->>Treasury: _send(recipient, color, amount)
Treasury-->>Contract: send_result
Contract->>Proposal: _markExecuted(proposal_id)
Proposal-->>Contract: ✓
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
|
||
| constructor( | ||
| initialOwner: Either<ZswapCoinPublicKey, ContractAddress>, | ||
| signers: Vector<3, Either<ZswapCoinPublicKey, ContractAddress>>, |
There was a problem hiding this comment.
Why are we forcing the size of this vector to be 3?
There was a problem hiding this comment.
The initializer is generic in the module so it can support n signers in a for loop. The compiler needs fully determined types and circuit shapes so it can define the constraints. On the contract layer, we can't leave it as generic
There was a problem hiding this comment.
any workaround to be flexible and not have to create different contracts for 3,4,5... signers?
Maybe creating the size to be relatively big (ej 32 signers), and any extra signers not required to be initialized to a null address of some sort?
The initialize circuit in the module can then keep control of this null address and stop iterating to save on gas
There was a problem hiding this comment.
Soooo yes and no to your question. We can’t use a null address bc this assumes that all Signers will bottom out to a Bytes<32> type. The workaround for this workaround is to wrap the signers as Maybe<T> and hardcode 32 or w/e max amount in the module's initialize. This is what it looks like
export circuit initializePad32(
signers: Vector<32, Maybe<T>>,
thresh: Uint<8>
): [] {
assert(thresh > 0, "SignerManager: threshold must be > 0");
_threshold = disclose(thresh);
for (const signer of signers) {
// No early exit capability in compact
if (signer.is_some) {
_addSigner(signer.value);
}
}
assert(_signerCount >= thresh, "SignerManager: threshold exceeds signer count");
}
Compact does not support early exiting a loop ATM so there are no savings to be had (the break keyword is reserved though). I benchmarked both approaches:
circuit "initialize" (k=11, rows=1817)
circuit "initializePad32" (k=15, rows=23229) // 16x domain size, 12.8x constraints
If we wanted to consider this flexibility, it should not be enforced. The options as I see it:
- Have two initialize circuits that users can choose from
- And perhaps support a smaller amount of potential signers to bring down the cost
- Have the contract not use the initialize circuit and instead insert this logic directly in the preset contract's constructor. This undermines the module design, but we're not bloating the module code
- Accept the current tech stack limitation with generics. If users c/p the contract code, they can insert the appropriate number of signers in their contract. It's not genius engineering, it's just simple and efficient:
constructor(
signers: Vector<5, Either<ZswapCoinPublicKey, ContractAddress>>,
thresh: Uint<8>
) {
Signer_initialize<5>(signers, thresh);
}
|
The basic multisig proposed is just for shielded tokens, maybe worth changing the name/clearly document this |
Fixed 1b979e6 |
0xisk
left a comment
0xisk
left a comment
There was a problem hiding this comment.
Looking good @andrew-fleming Thank you! Left comments, questions, and suggestions. Also we need to add @circuitInfo for all.
| @@ -0,0 +1,310 @@ | |||
| pragma language_version >= 0.21.0; | |||
There was a problem hiding this comment.
| pragma language_version >= 0.21.0; | |
| // SPDX-License-Identifier: MIT | |
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (access/ProposalManager.compact) | |
| pragma language_version >= 0.21.0; |
| @@ -0,0 +1,207 @@ | |||
| pragma language_version >= 0.21.0; | |||
There was a problem hiding this comment.
| pragma language_version >= 0.21.0; | |
| // SPDX-License-Identifier: MIT | |
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (access/ShieldedTreasury.compact) | |
| pragma language_version >= 0.21.0; |
| @@ -0,0 +1,195 @@ | |||
| pragma language_version >= 0.21.0; | |||
There was a problem hiding this comment.
| pragma language_version >= 0.21.0; | |
| // SPDX-License-Identifier: MIT | |
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (access/SignerManager.compact) | |
| pragma language_version >= 0.21.0; |
| @@ -0,0 +1,113 @@ | |||
| pragma language_version >= 0.21.0; | |||
There was a problem hiding this comment.
| pragma language_version >= 0.21.0; | |
| // SPDX-License-Identifier: MIT | |
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (access/UnshieldedTreasury.compact) | |
| pragma language_version >= 0.21.0; |
| // ─── State ────────────────────────────────────────────────────── | ||
|
|
||
| ledger _nextProposalId: Counter; | ||
| ledger _proposals: Map<Uint<64>, Proposal>; |
There was a problem hiding this comment.
Why is it private? I think that should be exported so that the developer could easily fetch the whole state without only needing to use the getters. Because with the getters, the user would actually need to do txs with paying gas to call them and also wait for the generated proof, so the dev would simply fetch the public state and then be able to do all the getters but from the client-side, which will be for free and instant without any proofing time. Similar to this example from Lunarswap frontend to fetch all the pool data from the ledger: https://github.com/OpenZeppelin/midnight-apps/blob/7063bc0fb81b6d93d54c7953deb3f363f94dbcf5/apps/lunarswap-ui/lib/lunarswap-integration.ts#L188
There was a problem hiding this comment.
If I recall correctly, wasn't there discussion about having the getters be free (if they're not already somehow)? Either way, agreed for now 👍 will update
|
|
||
| // ─── Constant ─────────────────────────────────────────────────── | ||
|
|
||
| export circuit UINT128_MAX(): Uint<128> { |
There was a problem hiding this comment.
That is repeated in the ShieldedTreasury, that should be replaced by just one, maybe for now that can be added in the Utils.compact with a TODO to be removed once math lib is available.
| receiveUnshielded(disclose(color), disclose(amount)); | ||
|
|
||
| const bal = getTokenBalance(color); | ||
| _balances.insert(disclose(color), disclose(bal + amount as Uint<128>)); |
There was a problem hiding this comment.
Maybe that is a dump question, but is it possible that the user sends tokens to the contract directly without using _deposit(), which has the receiveUnshielded(). Just need to double check this, my understanding is that it is not possible, but I might be wrong. As this might cause out-sync of the balances map.
There was a problem hiding this comment.
My understanding is that this is not possible. From the docs (ref):
Sending tokens to contracts is only possible if contract claims the tokens, which means an explicit contract interaction needs to be involved.
This was something we tried and confirmed a long time ago, didn't we? It wouldn't hurt to reconfirm
| ledger _shieldedReceived: Map<Bytes<32>, Uint<128>>; | ||
| ledger _shieldedSent: Map<Bytes<32>, Uint<128>>; |
There was a problem hiding this comment.
| ledger _shieldedReceived: Map<Bytes<32>, Uint<128>>; | |
| ledger _shieldedSent: Map<Bytes<32>, Uint<128>>; | |
| ledger _received: Map<Bytes<32>, Uint<128>>; | |
| ledger _sent: Map<Bytes<32>, Uint<128>>; |
nit: I think that's better
| const currentSent = getSentTotal(color); | ||
| assert(currentSent <= UINT128_MAX() - amount, "ShieldedTreasury: overflow"); |
There was a problem hiding this comment.
I think it is better to check this overflow from the beginning before the send operation.
| const newCount = _signerCount - 1 as Uint<8>; | ||
| assert(newCount >= _threshold, "SignerManager: removal would breach threshold"); | ||
|
|
||
| _signers.remove(disclose(signer)); |
There was a problem hiding this comment.
If we mean to use ledger _signers: Map<T, Boolean>; then shouldn't it be just an update with false here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
contracts/src/multisig/ShieldedTreasury.compact (1)
130-136:⚠️ Potential issue | 🟠 MajorOverflow check should occur before the send operation.
The overflow check at lines 130-131 happens after
sendShieldedexecutes. If the assertion fails, the send has already occurred, potentially causing state inconsistency. This was noted in a past review comment.Consider moving the overflow check before the send:
export circuit _send( recipient: Either<ZswapCoinPublicKey, ContractAddress>, color: Bytes<32>, amount: Uint<128> ): ShieldedSendResult { assert(_coins.member(disclose(color)), "ShieldedTreasury: no balance"); const coin = _coins.lookup(disclose(color)); assert(coin.value >= amount, "ShieldedTreasury: coin value insufficient"); + const currentSent = getSentTotal(color); + assert(currentSent <= UINT128_MAX() - amount, "ShieldedTreasury: overflow"); + const result = sendShielded(disclose(coin), disclose(recipient), disclose(amount)); ... - const currentSent = getSentTotal(color); - assert(currentSent <= UINT128_MAX() - amount, "ShieldedTreasury: overflow");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/multisig/ShieldedTreasury.compact` around lines 130 - 136, Move the overflow guard to run before performing the send: compute currentSent via getSentTotal(color), assert currentSent <= UINT128_MAX() - amount to prevent overflow, then proceed to call sendShielded (or the send operation) and finally update storage with _shieldedSent.insert(disclose(color), disclose((currentSent + amount) as Uint<128>)); ensure the same identifiers are used (getSentTotal, UINT128_MAX, _shieldedSent.insert, disclose) and that the type cast for the sum remains Uint<128>.
🧹 Nitpick comments (3)
contracts/src/multisig/SignerManager.compact (1)
146-148: Consider adding overflow protection for signer count.When
_signerCountreaches 255 (maxUint<8>), adding another signer will overflow. While 255 signers is unlikely in practice, consider adding a defensive check:export circuit _addSigner(signer: T): [] { assert( !isSigner(signer), "SignerManager: signer already active" ); + assert(_signerCount < 255, "SignerManager: max signers reached"); _signers.insert(disclose(signer), true); _signerCount = _signerCount + 1 as Uint<8>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/multisig/SignerManager.compact` around lines 146 - 148, Add a defensive overflow check before incrementing _signerCount to prevent wrapping when it is already at the Uint<8> max (255); in the function that calls _signers.insert(disclose(signer), true) and then sets _signerCount = _signerCount + 1 as Uint<8>, verify _signerCount < Uint<8>(255) (or use a named MAX_SIGNERS constant) and revert/throw if the limit is reached so the insert and increment cannot cause an overflow.contracts/src/multisig/ShieldedTreasury.compact (1)
193-195: Potential underflow if accounting invariant is violated.If
getSentTotalever exceedsgetReceivedTotal(due to bugs), the subtraction could underflow. Consider adding a defensive check or documenting that callers should verify the invariant.export circuit getReceivedMinusSent(color: Bytes<32>): Uint<128> { + const received = getReceivedTotal(color); + const sent = getSentTotal(color); + assert(received >= sent, "ShieldedTreasury: accounting invariant violated"); - return getReceivedTotal(color) - getSentTotal(color) as Uint<128>; + return received - sent as Uint<128>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/multisig/ShieldedTreasury.compact` around lines 193 - 195, The subtraction in getReceivedMinusSent can underflow if getSentTotal(color) > getReceivedTotal(color); add a defensive check in getReceivedMinusSent that verifies getReceivedTotal(color) >= getSentTotal(color) (using the contract/circuit assert/require mechanism available) and either revert/assert with a clear message or handle the case (e.g., return 0) before performing the subtraction; reference functions getReceivedMinusSent, getReceivedTotal, and getSentTotal when locating and fixing the code.contracts/src/multisig/test/ProposalManager.test.ts (1)
14-14: Unused variable_RECIPIENT.The destructured
_RECIPIENTis never used in the test file. Consider prefixing with underscore convention or removing if not needed.-const [_RECIPIENT, Z_RECIPIENT] = utils.generatePubKeyPair('RECIPIENT'); +const [, Z_RECIPIENT] = utils.generatePubKeyPair('RECIPIENT');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/multisig/test/ProposalManager.test.ts` at line 14, The variable _RECIPIENT from the destructuring of utils.generatePubKeyPair('RECIPIENT') is unused; fix by removing the unused binding and skip the first element in the destructuring (e.g., change const [_RECIPIENT, Z_RECIPIENT] to const [, Z_RECIPIENT]) so only Z_RECIPIENT is declared, referencing the generatePubKeyPair call and the symbols _RECIPIENT and Z_RECIPIENT to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/src/multisig/presets/ShieldedMultiSig.compact`:
- Around line 31-39: The createShieldedProposal circuit currently accepts any
Proposal_Recipient and persists unshielded recipients that later fail in
executeShieldedProposal; update createShieldedProposal to validate the `to:
Proposal_Recipient` parameter immediately and reject
RecipientKind.UnshieldedUser (and any other non-shielded kinds you consider
invalid) before calling Proposal__createProposal. In practice, inspect `to.kind`
(or equivalent discriminator) inside createShieldedProposal and throw/assert
when it equals RecipientKind.UnshieldedUser so only shielded recipients progress
to Proposal__createProposal and avoid permanently unexecutable proposals.
- Line 7: The preset currently allows Either<ZswapCoinPublicKey,
ContractAddress> signers but getCaller() always returns left(ownPublicKey()), so
contract-address signers will never authenticate; update the preset to only
accept ZswapCoinPublicKey signers by replacing Either<ZswapCoinPublicKey,
ContractAddress> with ZswapCoinPublicKey for the Signer_ prefix (or
alternatively implement contract-call authentication by adding a
kernel.claimContractCall(...) flow and altering getCaller()/authentication
checks to accept right(ContractAddress) callers); make the change in the Signer_
type declaration and adjust getCaller(), the checks that reference
ownPublicKey(), and any places handling right(ContractAddress) accordingly so
only supported signer types are accepted.
In `@contracts/src/multisig/presets/ShieldedMultiSigV2.compact`:
- Around line 79-86: The constructor allows thresh up to 3 via Signer_initialize
but execute only verifies two signatures (Vector<2, Bytes<64>>), so
Signer_assertThresholdMet(finalState.validCount) can never succeed for
thresh==3; update the constructor to validate thresh ≤ 2 (reject or revert when
thresh > 2) to match the two-signature verification in execute (refer to
constructor, Signer_initialize, execute, Signer_assertThresholdMet, and
finalState.validCount), or alternatively update the contract title/docs to state
this is a 2-of-3 contract — prefer adding the runtime check in the constructor
to enforce the constraint.
- Around line 246-252: The stub stubVerifySignature currently returns true and
must never be deployable to production; modify stubVerifySignature to guard
against production by either (1) adding a runtime revert with message
"STUB_NOT_IMPLEMENTED" when on a production/mainnet network, or (2) adding a
build-time assertion that the Compact ECDSA primitive (e.g., ecdsaVerify or the
commented proper implementation) is available before compilation and fail the
build if not present; implement one of these checks inside stubVerifySignature
(or adjacent init/compile-time logic) and reference the commented proper
implementation/ecdsaVerify so the guard prevents accidental mainnet deployment
until the real ecdsaVerify implementation replaces the stub.
In `@contracts/src/multisig/ShieldedTreasuryStateless.compact`:
- Around line 43-58: The docstring for the circuit function _send is out of
sync: it documents a `color` parameter but the signature takes `coin:
QualifiedShieldedCoinInfo`; update the comment block to reflect the actual
parameters and behavior (remove `color` param and document `coin:
QualifiedShieldedCoinInfo` and that the color is derived from coin), and adjust
the `@param` entries to include `coin` and the existing `recipient` and `amount`,
keeping the `@returns` as-is; locate the function named _send and its surrounding
docstring to make this change.
In `@contracts/src/multisig/test/simulators/SignerManagerSimulator.ts`:
- Around line 20-23: SignerManagerArgs currently permits an unbounded Either[]
but MockSignerManager expects exactly three signers (Vector<3>), so change
SignerManagerArgs to a fixed-length tuple of three signer entries (e.g.,
readonly [Either<ZswapCoinPublicKey, ContractAddress>,
Either<ZswapCoinPublicKey, ContractAddress>, Either<ZswapCoinPublicKey,
ContractAddress>, thresh: bigint]) to enforce the size at the type level;
alternatively, if you prefer not to change the type, add a clear JSDoc on
SignerManagerArgs and MockSignerManager stating that the first tuple element
must contain exactly three Either<ZswapCoinPublicKey, ContractAddress> entries
to avoid runtime mismatches.
In `@contracts/src/multisig/witnesses/ProposalManagerWitnesses.ts`:
- Line 2: The file header comment references "ProposalManagerWitness.ts" but the
filename is "ProposalManagerWitnesses.ts"; update the top-of-file header string
to match the actual filename (change "ProposalManagerWitness.ts" to
"ProposalManagerWitnesses.ts") so the header accurately reflects the file name
in ProposalManagerWitnesses.ts and any internal comments referencing
ProposalManagerWitness should be corrected to ProposalManagerWitnesses where
applicable.
In `@contracts/src/multisig/witnesses/ShieldedTreasuryWitnesses.ts`:
- Line 2: The file header comment references the wrong filename
("ShieldedTreasuryWitness.ts" singular) while the actual file is named
"ShieldedTreasuryWitnesses.ts" (plural); update the header comment at the top of
ShieldedTreasuryWitnesses.ts to reference the correct plural filename (e.g.,
change "ShieldedTreasuryWitness.ts" to "ShieldedTreasuryWitnesses.ts") so the
header matches the file name and project conventions.
In `@contracts/src/multisig/witnesses/SignerManagerWitnesses.ts`:
- Line 2: The file header comment references SignerManagerWitness.ts (singular)
but the file is named SignerManagerWitnesses.ts (plural); update the top-of-file
header string to reference SignerManagerWitnesses.ts so the header and filename
match (modify the header line that currently reads
"multisig/witnesses/SignerManagerWitness.ts" to
"multisig/witnesses/SignerManagerWitnesses.ts"), ensuring consistency for tools
that rely on the header.
In `@contracts/src/multisig/witnesses/UnshieldedTreasuryWitnesses.ts`:
- Line 2: The file header comment at the top of UnshieldedTreasuryWitnesses.ts
references the wrong filename ("UnshieldedTreasuryWitness.ts"); update the
header comment to match the actual file name "UnshieldedTreasuryWitnesses.ts" so
the module/file comment is consistent with the exported symbols and file name
(verify any top-of-file comment or banner string in
UnshieldedTreasuryWitnesses.ts and replace the singular form with the plural
form).
In `@turbo.json`:
- Around line 32-38: The compact:multisig Turbo task has the wrong inputs glob
(currently "src/access/**/*.compact"); update the inputs array for the
compact:multisig task to reference the multisig sources instead — replace
"src/access/**/*.compact" with "src/multisig/**/*.compact" in the
compact:multisig task definition so multisig changes correctly invalidate the
Turbo cache and access changes no longer trigger multisig recompilation.
---
Duplicate comments:
In `@contracts/src/multisig/ShieldedTreasury.compact`:
- Around line 130-136: Move the overflow guard to run before performing the
send: compute currentSent via getSentTotal(color), assert currentSent <=
UINT128_MAX() - amount to prevent overflow, then proceed to call sendShielded
(or the send operation) and finally update storage with
_shieldedSent.insert(disclose(color), disclose((currentSent + amount) as
Uint<128>)); ensure the same identifiers are used (getSentTotal, UINT128_MAX,
_shieldedSent.insert, disclose) and that the type cast for the sum remains
Uint<128>.
---
Nitpick comments:
In `@contracts/src/multisig/ShieldedTreasury.compact`:
- Around line 193-195: The subtraction in getReceivedMinusSent can underflow if
getSentTotal(color) > getReceivedTotal(color); add a defensive check in
getReceivedMinusSent that verifies getReceivedTotal(color) >=
getSentTotal(color) (using the contract/circuit assert/require mechanism
available) and either revert/assert with a clear message or handle the case
(e.g., return 0) before performing the subtraction; reference functions
getReceivedMinusSent, getReceivedTotal, and getSentTotal when locating and
fixing the code.
In `@contracts/src/multisig/SignerManager.compact`:
- Around line 146-148: Add a defensive overflow check before incrementing
_signerCount to prevent wrapping when it is already at the Uint<8> max (255); in
the function that calls _signers.insert(disclose(signer), true) and then sets
_signerCount = _signerCount + 1 as Uint<8>, verify _signerCount < Uint<8>(255)
(or use a named MAX_SIGNERS constant) and revert/throw if the limit is reached
so the insert and increment cannot cause an overflow.
In `@contracts/src/multisig/test/ProposalManager.test.ts`:
- Line 14: The variable _RECIPIENT from the destructuring of
utils.generatePubKeyPair('RECIPIENT') is unused; fix by removing the unused
binding and skip the first element in the destructuring (e.g., change const
[_RECIPIENT, Z_RECIPIENT] to const [, Z_RECIPIENT]) so only Z_RECIPIENT is
declared, referencing the generatePubKeyPair call and the symbols _RECIPIENT and
Z_RECIPIENT to locate the change.
🪄 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: 1ddf1d1f-02b9-449e-aed6-57a607c191c0
📒 Files selected for processing (27)
contracts/package.jsoncontracts/src/multisig/ProposalManager.compactcontracts/src/multisig/ShieldedTreasury.compactcontracts/src/multisig/ShieldedTreasuryStateless.compactcontracts/src/multisig/SignerManager.compactcontracts/src/multisig/UnshieldedTreasury.compactcontracts/src/multisig/presets/ShieldedMultiSig.compactcontracts/src/multisig/presets/ShieldedMultiSigV2.compactcontracts/src/multisig/test/ProposalManager.test.tscontracts/src/multisig/test/ShieldedMultiSig.test.tscontracts/src/multisig/test/ShieldedTreasury.test.tscontracts/src/multisig/test/SignerManager.test.tscontracts/src/multisig/test/mocks/MockProposalManager.compactcontracts/src/multisig/test/mocks/MockShieldedTreasury.compactcontracts/src/multisig/test/mocks/MockShieldedTreasuryStateless.compactcontracts/src/multisig/test/mocks/MockSignerManager.compactcontracts/src/multisig/test/mocks/MockUnshieldedTreasury.compactcontracts/src/multisig/test/simulators/ProposalManagerSimulator.tscontracts/src/multisig/test/simulators/ShieldedMultiSigSimulator.tscontracts/src/multisig/test/simulators/ShieldedTreasurySimulator.tscontracts/src/multisig/test/simulators/SignerManagerSimulator.tscontracts/src/multisig/witnesses/ProposalManagerWitnesses.tscontracts/src/multisig/witnesses/ShieldedMultiSigWitnesses.tscontracts/src/multisig/witnesses/ShieldedTreasuryWitnesses.tscontracts/src/multisig/witnesses/SignerManagerWitnesses.tscontracts/src/multisig/witnesses/UnshieldedTreasuryWitnesses.tsturbo.json
|
|
||
| import "../ProposalManager" prefix Proposal_; | ||
| import "../ShieldedTreasury" prefix Treasury_; | ||
| import "../SignerManager"<Either<ZswapCoinPublicKey, ContractAddress>> prefix Signer_; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Compact language >= 0.21.0, what API exposes the immediate caller for contract-to-contract calls? Can ownPublicKey()ever resolve aContractAddress, or should a different primitive be used when signers are typed as Either<ZswapCoinPublicKey, ContractAddress>?
💡 Result:
-
Immediate caller API (contract-to-contract): In the current Midnight docs for Compact (language 0.22.0), there is no
kernel.caller()/msg.sender-style primitive documented. The Kernel ADT exposes things likekernel.self()(the current contract’s address) and call-claiming primitives likekernel.claimContractCall(...), but nothing that returns the immediate caller. (docs.midnight.network) -
Can
ownPublicKey()ever be aContractAddress? No.ownPublicKey()is defined to returnZswapCoinPublicKeyof the end-user creating the transaction, and its type isZswapCoinPublicKey(not anEither), so it cannot resolve to aContractAddress. (docs.midnight.network) -
What to do with
Either<ZswapCoinPublicKey, ContractAddress>“signers”:ownPublicKey()should only be wrapped asleft<ZswapCoinPublicKey, ContractAddress>(ownPublicKey())for user-initiated calls. If you need to represent/authorize a contract caller, you’ll need a different mechanism thanownPublicKey()(e.g., pass the contract identity explicitly and/or use cross-contract call–verification viakernel.claimContractCall(...)), becauseownPublicKey()cannot produce theright(ContractAddress)case. (docs.midnight.network)
Citations:
- 1: https://docs.midnight.network/compact/reference/ledger-adt
- 2: https://docs.midnight.network/compact/standard-library/exports
- 3: https://docs.midnight.network/compact/standard-library/exports
🏁 Script executed:
find . -name "ShieldedMultiSig.compact" -type fRepository: OpenZeppelin/compact-contracts
Length of output: 131
🏁 Script executed:
wc -l ./contracts/src/multisig/presets/ShieldedMultiSig.compactRepository: OpenZeppelin/compact-contracts
Length of output: 135
🏁 Script executed:
cat -n ./contracts/src/multisig/presets/ShieldedMultiSig.compactRepository: OpenZeppelin/compact-contracts
Length of output: 6693
Preset accepts contract signers that cannot authenticate.
Lines 16-20 accept Either<ZswapCoinPublicKey, ContractAddress> signers, but getCaller() on lines 115-117 always returns left(ownPublicKey()). Since ownPublicKey() returns only ZswapCoinPublicKey and cannot produce a ContractAddress, any signer initialized as right<ContractAddress> will fail the checks on lines 37, 48, and 63. Additionally, Compact >= 0.21.0 provides no documented caller API (like kernel.caller()) for contract-to-contract calls.
Either narrow this preset to accept only public-key signers, or implement a separate mechanism to authenticate contract-originated calls (e.g., via kernel.claimContractCall(...)).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/multisig/presets/ShieldedMultiSig.compact` at line 7, The
preset currently allows Either<ZswapCoinPublicKey, ContractAddress> signers but
getCaller() always returns left(ownPublicKey()), so contract-address signers
will never authenticate; update the preset to only accept ZswapCoinPublicKey
signers by replacing Either<ZswapCoinPublicKey, ContractAddress> with
ZswapCoinPublicKey for the Signer_ prefix (or alternatively implement
contract-call authentication by adding a kernel.claimContractCall(...) flow and
altering getCaller()/authentication checks to accept right(ContractAddress)
callers); make the change in the Signer_ type declaration and adjust
getCaller(), the checks that reference ownPublicKey(), and any places handling
right(ContractAddress) accordingly so only supported signer types are accepted.
| export circuit createShieldedProposal( | ||
| to: Proposal_Recipient, | ||
| color: Bytes<32>, | ||
| amount: Uint<128> | ||
| ): Uint<64> { | ||
| const callerPK = getCaller(); | ||
| Signer_assertSigner(callerPK); | ||
|
|
||
| return Proposal__createProposal(to, color, amount); |
There was a problem hiding this comment.
Reject unshielded recipients when creating a shielded proposal.
Line 39 persists any Proposal_Recipient, including RecipientKind.UnshieldedUser. The failure is deferred to Line 85 during executeShieldedProposal, and because this preset does not expose a cancel path, that leaves a permanently unexecutable active proposal. Validate to up front.
Suggested fix
export circuit createShieldedProposal(
to: Proposal_Recipient,
color: Bytes<32>,
amount: Uint<128>
): Uint<64> {
const callerPK = getCaller();
Signer_assertSigner(callerPK);
+ Proposal_toShieldedRecipient(to);
return Proposal__createProposal(to, color, amount);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/multisig/presets/ShieldedMultiSig.compact` around lines 31 -
39, The createShieldedProposal circuit currently accepts any Proposal_Recipient
and persists unshielded recipients that later fail in executeShieldedProposal;
update createShieldedProposal to validate the `to: Proposal_Recipient` parameter
immediately and reject RecipientKind.UnshieldedUser (and any other non-shielded
kinds you consider invalid) before calling Proposal__createProposal. In
practice, inspect `to.kind` (or equivalent discriminator) inside
createShieldedProposal and throw/assert when it equals
RecipientKind.UnshieldedUser so only shielded recipients progress to
Proposal__createProposal and avoid permanently unexecutable proposals.
| constructor( | ||
| instanceSalt: Bytes<32>, | ||
| signerCommitments: Vector<3, Bytes<32>>, | ||
| thresh: Uint<8>, | ||
| ) { | ||
| _instanceSalt = disclose(instanceSalt); | ||
| Signer_initialize<3>(signerCommitments, thresh); | ||
| } |
There was a problem hiding this comment.
Critical: Threshold can exceed verifiable signature count.
The constructor accepts thresh up to 3 (validated by Signer_initialize), but execute only processes 2 signatures (Vector<2, Bytes<64>>). If thresh=3, the assertion at line 171 (Signer_assertThresholdMet(finalState.validCount)) will always fail since validCount can never exceed 2.
Add a constructor check to enforce this constraint:
constructor(
instanceSalt: Bytes<32>,
signerCommitments: Vector<3, Bytes<32>>,
thresh: Uint<8>,
) {
+ assert(thresh <= 2, "ShieldedMultiSigV2: threshold must be <= 2 for 2-of-3 execution");
_instanceSalt = disclose(instanceSalt);
Signer_initialize<3>(signerCommitments, thresh);
}Alternatively, update the title/docs to clarify this is specifically a "2-of-3" contract where threshold must be ≤2.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor( | |
| instanceSalt: Bytes<32>, | |
| signerCommitments: Vector<3, Bytes<32>>, | |
| thresh: Uint<8>, | |
| ) { | |
| _instanceSalt = disclose(instanceSalt); | |
| Signer_initialize<3>(signerCommitments, thresh); | |
| } | |
| constructor( | |
| instanceSalt: Bytes<32>, | |
| signerCommitments: Vector<3, Bytes<32>>, | |
| thresh: Uint<8>, | |
| ) { | |
| assert(thresh <= 2, "ShieldedMultiSigV2: threshold must be <= 2 for 2-of-3 execution"); | |
| _instanceSalt = disclose(instanceSalt); | |
| Signer_initialize<3>(signerCommitments, thresh); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/multisig/presets/ShieldedMultiSigV2.compact` around lines 79 -
86, The constructor allows thresh up to 3 via Signer_initialize but execute only
verifies two signatures (Vector<2, Bytes<64>>), so
Signer_assertThresholdMet(finalState.validCount) can never succeed for
thresh==3; update the constructor to validate thresh ≤ 2 (reject or revert when
thresh > 2) to match the two-signature verification in execute (refer to
constructor, Signer_initialize, execute, Signer_assertThresholdMet, and
finalState.validCount), or alternatively update the contract title/docs to state
this is a 2-of-3 contract — prefer adding the runtime check in the constructor
to enforce the constraint.
| circuit stubVerifySignature( | ||
| pubkey: Bytes<64>, | ||
| msgHash: Bytes<32>, | ||
| signature: Bytes<64> | ||
| ): Boolean { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for any deployment or CI configurations that might reference this contract
rg -l "ShieldedMultiSigV2" --type=json --type=yaml --type=toml
# Check if there are any TODO tracking mechanisms for this stub
rg -n "stubVerifySignature|ecdsaVerify" contracts/Repository: OpenZeppelin/compact-contracts
Length of output: 734
Add a programmatic safeguard to prevent production deployment of this stub.
stubVerifySignature always returns true, bypassing signature validation entirely. While documented as a temporary stub pending Compact ECDSA primitive support (lines 123-124), add a runtime or build-time check to ensure it cannot reach production. Consider:
- A revert with "STUB_NOT_IMPLEMENTED" if deployed on a production network
- A build-time assertion that ecdsaVerify is available before compilation
- An explicit network detection to prevent accidental use on mainnet
The commented proper implementation (line 206) is ready to replace the stub when Compact ECDSA support is available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/multisig/presets/ShieldedMultiSigV2.compact` around lines 246 -
252, The stub stubVerifySignature currently returns true and must never be
deployable to production; modify stubVerifySignature to guard against production
by either (1) adding a runtime revert with message "STUB_NOT_IMPLEMENTED" when
on a production/mainnet network, or (2) adding a build-time assertion that the
Compact ECDSA primitive (e.g., ecdsaVerify or the commented proper
implementation) is available before compilation and fail the build if not
present; implement one of these checks inside stubVerifySignature (or adjacent
init/compile-time logic) and reference the commented proper
implementation/ecdsaVerify so the guard prevents accidental mainnet deployment
until the real ecdsaVerify implementation replaces the stub.
| * Requirements: | ||
| * | ||
| * - A coin of the given `color` must exist in the treasury. | ||
| * - The coin's value must be >= `amount`. | ||
| * - Send must not cause sent total overflow. | ||
| * | ||
| * @param {Either<ZswapCoinPublicKey, ContractAddress>} recipient - The recipient. | ||
| * @param {Bytes<32>} color - The token color. | ||
| * @param {Uint<128>} amount - The amount to send. | ||
| * @returns {ShieldedSendResult} The result containing sent coin and any change. | ||
| */ | ||
| export circuit _send( | ||
| coin: QualifiedShieldedCoinInfo, | ||
| recipient: Either<ZswapCoinPublicKey, ContractAddress>, | ||
| amount: Uint<128> | ||
| ): ShieldedSendResult { |
There was a problem hiding this comment.
Docstring does not match the actual function signature.
The @param documentation lists color as a parameter, but the actual signature takes coin: QualifiedShieldedCoinInfo as the first parameter, not a separate color. The color is presumably derived from the coin.
📝 Suggested fix
* Requirements:
*
* - A coin of the given `color` must exist in the treasury.
* - The coin's value must be >= `amount`.
* - Send must not cause sent total overflow.
*
+ * `@param` {QualifiedShieldedCoinInfo} coin - The coin to send from.
* `@param` {Either<ZswapCoinPublicKey, ContractAddress>} recipient - The recipient.
- * `@param` {Bytes<32>} color - The token color.
* `@param` {Uint<128>} amount - The amount to send.
* `@returns` {ShieldedSendResult} The result containing sent coin and any change.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * Requirements: | |
| * | |
| * - A coin of the given `color` must exist in the treasury. | |
| * - The coin's value must be >= `amount`. | |
| * - Send must not cause sent total overflow. | |
| * | |
| * @param {Either<ZswapCoinPublicKey, ContractAddress>} recipient - The recipient. | |
| * @param {Bytes<32>} color - The token color. | |
| * @param {Uint<128>} amount - The amount to send. | |
| * @returns {ShieldedSendResult} The result containing sent coin and any change. | |
| */ | |
| export circuit _send( | |
| coin: QualifiedShieldedCoinInfo, | |
| recipient: Either<ZswapCoinPublicKey, ContractAddress>, | |
| amount: Uint<128> | |
| ): ShieldedSendResult { | |
| * Requirements: | |
| * | |
| * - A coin of the given `color` must exist in the treasury. | |
| * - The coin's value must be >= `amount`. | |
| * - Send must not cause sent total overflow. | |
| * | |
| * `@param` {QualifiedShieldedCoinInfo} coin - The coin to send from. | |
| * `@param` {Either<ZswapCoinPublicKey, ContractAddress>} recipient - The recipient. | |
| * `@param` {Uint<128>} amount - The amount to send. | |
| * `@returns` {ShieldedSendResult} The result containing sent coin and any change. | |
| */ | |
| export circuit _send( | |
| coin: QualifiedShieldedCoinInfo, | |
| recipient: Either<ZswapCoinPublicKey, ContractAddress>, | |
| amount: Uint<128> | |
| ): ShieldedSendResult { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/multisig/ShieldedTreasuryStateless.compact` around lines 43 -
58, The docstring for the circuit function _send is out of sync: it documents a
`color` parameter but the signature takes `coin: QualifiedShieldedCoinInfo`;
update the comment block to reflect the actual parameters and behavior (remove
`color` param and document `coin: QualifiedShieldedCoinInfo` and that the color
is derived from coin), and adjust the `@param` entries to include `coin` and the
existing `recipient` and `amount`, keeping the `@returns` as-is; locate the
function named _send and its surrounding docstring to make this change.
| @@ -0,0 +1,6 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/ProposalManagerWitness.ts) | |||
There was a problem hiding this comment.
Header filename mismatch.
The header references ProposalManagerWitness.ts (singular) but the actual filename is ProposalManagerWitnesses.ts (plural).
Proposed fix
-// OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/ProposalManagerWitness.ts)
+// OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/ProposalManagerWitnesses.ts)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/ProposalManagerWitness.ts) | |
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/ProposalManagerWitnesses.ts) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/multisig/witnesses/ProposalManagerWitnesses.ts` at line 2, The
file header comment references "ProposalManagerWitness.ts" but the filename is
"ProposalManagerWitnesses.ts"; update the top-of-file header string to match the
actual filename (change "ProposalManagerWitness.ts" to
"ProposalManagerWitnesses.ts") so the header accurately reflects the file name
in ProposalManagerWitnesses.ts and any internal comments referencing
ProposalManagerWitness should be corrected to ProposalManagerWitnesses where
applicable.
| @@ -0,0 +1,6 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/ShieldedTreasuryWitness.ts) | |||
There was a problem hiding this comment.
Header filename mismatch.
The header references ShieldedTreasuryWitness.ts (singular) but the actual filename is ShieldedTreasuryWitnesses.ts (plural).
Proposed fix
-// OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/ShieldedTreasuryWitness.ts)
+// OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/ShieldedTreasuryWitnesses.ts)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/ShieldedTreasuryWitness.ts) | |
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/ShieldedTreasuryWitnesses.ts) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/multisig/witnesses/ShieldedTreasuryWitnesses.ts` at line 2, The
file header comment references the wrong filename ("ShieldedTreasuryWitness.ts"
singular) while the actual file is named "ShieldedTreasuryWitnesses.ts"
(plural); update the header comment at the top of ShieldedTreasuryWitnesses.ts
to reference the correct plural filename (e.g., change
"ShieldedTreasuryWitness.ts" to "ShieldedTreasuryWitnesses.ts") so the header
matches the file name and project conventions.
| @@ -0,0 +1,6 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/SignerManagerWitness.ts) | |||
There was a problem hiding this comment.
Header filename mismatch.
The header references SignerManagerWitness.ts (singular) but the actual filename is SignerManagerWitnesses.ts (plural).
Proposed fix
-// OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/SignerManagerWitness.ts)
+// OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/SignerManagerWitnesses.ts)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/SignerManagerWitness.ts) | |
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/SignerManagerWitnesses.ts) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/multisig/witnesses/SignerManagerWitnesses.ts` at line 2, The
file header comment references SignerManagerWitness.ts (singular) but the file
is named SignerManagerWitnesses.ts (plural); update the top-of-file header
string to reference SignerManagerWitnesses.ts so the header and filename match
(modify the header line that currently reads
"multisig/witnesses/SignerManagerWitness.ts" to
"multisig/witnesses/SignerManagerWitnesses.ts"), ensuring consistency for tools
that rely on the header.
| @@ -0,0 +1,7 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/UnshieldedTreasuryWitness.ts) | |||
There was a problem hiding this comment.
Header filename mismatch.
The header references UnshieldedTreasuryWitness.ts (singular) but the actual filename is UnshieldedTreasuryWitnesses.ts (plural).
Proposed fix
-// OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/UnshieldedTreasuryWitness.ts)
+// OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/UnshieldedTreasuryWitnesses.ts)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/UnshieldedTreasuryWitness.ts) | |
| // OpenZeppelin Compact Contracts v0.0.1-alpha.1 (multisig/witnesses/UnshieldedTreasuryWitnesses.ts) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/multisig/witnesses/UnshieldedTreasuryWitnesses.ts` at line 2,
The file header comment at the top of UnshieldedTreasuryWitnesses.ts references
the wrong filename ("UnshieldedTreasuryWitness.ts"); update the header comment
to match the actual file name "UnshieldedTreasuryWitnesses.ts" so the
module/file comment is consistent with the exported symbols and file name
(verify any top-of-file comment or banner string in
UnshieldedTreasuryWitnesses.ts and replace the singular form with the plural
form).
| "compact:multisig": { | ||
| "dependsOn": ["^build", "compact:security", "compact:utils"], | ||
| "env": ["COMPACT_HOME", "SKIP_ZK"], | ||
| "inputs": ["src/access/**/*.compact"], | ||
| "outputLogs": "new-only", | ||
| "outputs": ["artifacts/**/"] | ||
| }, |
There was a problem hiding this comment.
Incorrect inputs path for compact:multisig task.
The inputs array references src/access/**/*.compact instead of src/multisig/**/*.compact. This appears to be a copy-paste error from the compact:access task configuration.
This will cause:
- Multisig source changes to not invalidate the Turbo cache
- Access source changes to incorrectly trigger multisig recompilation
Proposed fix
"compact:multisig": {
"dependsOn": ["^build", "compact:security", "compact:utils"],
"env": ["COMPACT_HOME", "SKIP_ZK"],
- "inputs": ["src/access/**/*.compact"],
+ "inputs": ["src/multisig/**/*.compact"],
"outputLogs": "new-only",
"outputs": ["artifacts/**/"]
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "compact:multisig": { | |
| "dependsOn": ["^build", "compact:security", "compact:utils"], | |
| "env": ["COMPACT_HOME", "SKIP_ZK"], | |
| "inputs": ["src/access/**/*.compact"], | |
| "outputLogs": "new-only", | |
| "outputs": ["artifacts/**/"] | |
| }, | |
| "compact:multisig": { | |
| "dependsOn": ["^build", "compact:security", "compact:utils"], | |
| "env": ["COMPACT_HOME", "SKIP_ZK"], | |
| "inputs": ["src/multisig/**/*.compact"], | |
| "outputLogs": "new-only", | |
| "outputs": ["artifacts/**/"] | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@turbo.json` around lines 32 - 38, The compact:multisig Turbo task has the
wrong inputs glob (currently "src/access/**/*.compact"); update the inputs array
for the compact:multisig task to reference the multisig sources instead —
replace "src/access/**/*.compact" with "src/multisig/**/*.compact" in the
compact:multisig task definition so multisig changes correctly invalidate the
Turbo cache and access changes no longer trigger multisig recompilation.
3 participants
Summary by CodeRabbit
New Features
Tests