Multisig contracts draft feedback by pepebndc · Pull Request #378 · OpenZeppelin/compact-contracts

pepebndc · 2026-03-10T11:47:21Z

No description provided.

coderabbitai · 2026-03-10T11:47:40Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 82eb1aab-ab0f-4a76-8771-f36a9a2af001

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch add-multisig

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

contracts/src/multisig/presets/BasicMultisig.compact

pepebndc · 2026-03-10T11:49:01Z

contracts/src/multisig/presets/ShieldedMultiSig.compact

+
+constructor(
+  initialOwner: Either<ZswapCoinPublicKey, ContractAddress>,
+  signers: Vector<3, Either<ZswapCoinPublicKey, ContractAddress>>,


Why are we forcing the size of this vector to be 3?

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

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

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); }

contracts/src/multisig/presets/BasicMultisig.compact

pepebndc · 2026-03-10T11:53:18Z

The basic multisig proposed is just for shielded tokens, maybe worth changing the name/clearly document this

contracts/src/multisig/presets/ShieldedMultiSig.compact

contracts/src/multisig/test/mocks/MockShieldedTreasury.compact

contracts/src/multisig/test/mocks/MockSignerManager.compact

contracts/src/multisig/presets/BasicMultisig.compact

andrew-fleming · 2026-03-11T07:03:55Z

The basic multisig proposed is just for shielded tokens, maybe worth changing the name/clearly document this

Fixed 1b979e6

0xisk

Looking good @andrew-fleming Thank you! Left comments, questions, and suggestions. Also we need to add @circuitInfo for all.

0xisk · 2026-03-18T12:25:05Z

contracts/src/multisig/ProposalManager.compact

@@ -0,0 +1,310 @@
+pragma language_version >= 0.21.0;


Suggested change

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;

0xisk · 2026-03-18T12:25:30Z

contracts/src/multisig/ShieldedTreasury.compact

@@ -0,0 +1,207 @@
+pragma language_version >= 0.21.0;


Suggested change

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;

0xisk · 2026-03-18T12:26:03Z

contracts/src/multisig/SignerManager.compact

@@ -0,0 +1,195 @@
+pragma language_version >= 0.21.0;


Suggested change

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;

0xisk · 2026-03-18T12:26:27Z

contracts/src/multisig/UnshieldedTreasury.compact

@@ -0,0 +1,113 @@
+pragma language_version >= 0.21.0;


Suggested change

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;

0xisk · 2026-03-18T12:28:34Z

contracts/src/multisig/ProposalManager.compact

+  // ─── State ──────────────────────────────────────────────────────
+
+  ledger _nextProposalId: Counter;
+  ledger _proposals: Map<Uint<64>, Proposal>;


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

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

0xisk · 2026-03-18T14:43:16Z

contracts/src/multisig/UnshieldedTreasury.compact

+
+  // ─── Constant ───────────────────────────────────────────────────
+
+  export circuit UINT128_MAX(): Uint<128> {


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.

Agreed 👍

0xisk · 2026-03-18T14:54:25Z

contracts/src/multisig/UnshieldedTreasury.compact

+    receiveUnshielded(disclose(color), disclose(amount));
+
+    const bal = getTokenBalance(color);
+    _balances.insert(disclose(color), disclose(bal + amount as Uint<128>));


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.

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

0xisk · 2026-03-18T14:58:14Z

contracts/src/multisig/ShieldedTreasury.compact

+  ledger _shieldedReceived: Map<Bytes<32>, Uint<128>>;
+  ledger _shieldedSent: Map<Bytes<32>, Uint<128>>;


Suggested change

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

0xisk · 2026-03-18T15:01:42Z

contracts/src/multisig/ShieldedTreasury.compact

+    const currentSent = getSentTotal(color);
+    assert(currentSent <= UINT128_MAX() - amount, "ShieldedTreasury: overflow");


I think it is better to check this overflow from the beginning before the send operation.

0xisk · 2026-03-18T15:08:16Z

contracts/src/multisig/SignerManager.compact

+    const newCount = _signerCount - 1 as Uint<8>;
+    assert(newCount >= _threshold, "SignerManager: removal would breach threshold");
+
+    _signers.remove(disclose(signer));


If we mean to use ledger _signers: Map<T, Boolean>; then shouldn't it be just an update with false here.

andrew-fleming added 4 commits March 9, 2026 00:00

add multisig compile as turbo task

78196e0

add init multisig modules

3f13582

add helpers

48330f1

add basic multisig contract

1252803

pepebndc commented Mar 10, 2026

View reviewed changes

pepebndc assigned andrew-fleming Mar 10, 2026

pepebndc commented Mar 10, 2026

View reviewed changes

contracts/src/multisig/presets/ShieldedMultiSig.compact Show resolved Hide resolved

pepebndc changed the title ~~Multisig contracts draft~~ Multisig contracts draft feedback Mar 10, 2026

pepebndc commented Mar 10, 2026

View reviewed changes

contracts/src/multisig/test/mocks/MockShieldedTreasury.compact Show resolved Hide resolved

contracts/src/multisig/test/mocks/MockSignerManager.compact Show resolved Hide resolved

pepebndc commented Mar 10, 2026

View reviewed changes

contracts/src/multisig/presets/BasicMultisig.compact Outdated Show resolved Hide resolved

andrew-fleming added 14 commits March 10, 2026 11:28

remove unused param

7abe151

fix newCount var in _approveProposal

5def030

remove setup, remove ownable

e15782e

fix isProposalApprovedBySigner

8348bc9

add member check to _approveProposal

7db320b

replace cancelApproval with revokeApproval

41d9888

fix isProposalApprovedBySigner

f13f3a0

add empty witnesses

66cd730

export compact types from mock

22ca211

add SignerManager sim

b4e36f9

add SignerManager tests

700bdb2

fix param order

0fccc11

move deposit

da3365d

move file/improve contract name

1b979e6

andrew-fleming added 3 commits March 11, 2026 04:15

add comma to param

fdbb640

fix fmt

e5b4e06

add multisig script to contracts manifest

f521b3d

andrew-fleming and others added 8 commits March 11, 2026 04:59

normalize file names

c780647

update import path

2452867

Merge branch 'main' into add-multisig

cd23631

validate threshold does not exceed signer count after initialization

75d36ea

Add multisig tests (#380)

e26647e

Merge branch 'main' into add-multisig

fd2d72b

Merge branch 'main' into add-multisig

c4c0f77

fix fmt

01ed666

0xisk reviewed Mar 18, 2026

View reviewed changes

andrew-fleming added 7 commits March 24, 2026 01:29

add stateless treasury and multisig v2

0d64f87

fix mod name, add stateless mocj

5aefaaf

export circuit

71b8a6e

remove export

d1552a5

clean up comment

75ec231

fix domain

0938974

fix title

eafb62a


		// ─── Constant ───────────────────────────────────────────────────

		export circuit UINT128_MAX(): Uint<128> {

		ledger _shieldedReceived: Map<Bytes<32>, Uint<128>>;
		ledger _shieldedSent: Map<Bytes<32>, Uint<128>>;

		const currentSent = getSentTotal(color);
		assert(currentSent <= UINT128_MAX() - amount, "ShieldedTreasury: overflow");

Conversation

pepebndc commented Mar 10, 2026

Uh oh!

coderabbitai bot commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pepebndc commented Mar 10, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrew-fleming commented Mar 11, 2026

Uh oh!

0xisk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrew-fleming Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai bot commented Mar 10, 2026 •

edited

Loading

andrew-fleming Mar 24, 2026 •

edited

Loading