Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughA new Signer module is introduced for multisig governance, providing state ledgers, initialization, assertion guards, and signer reconfiguration circuits. Comprehensive test coverage and simulator infrastructure are added alongside supporting witness and mock contract implementations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
contracts/src/multisig/test/simulators/SignerSimulator.ts (1)
20-23: Align simulator input type with fixedVector<3>mock constructor.The simulator API currently accepts arbitrary signer-array lengths, but
MockSigneris compiled for 3 signers. Tightening the type avoids invalid calls.Proposed fix
type SignerArgs = readonly [ - signers: Either<ZswapCoinPublicKey, ContractAddress>[], + signers: readonly [ + Either<ZswapCoinPublicKey, ContractAddress>, + Either<ZswapCoinPublicKey, ContractAddress>, + Either<ZswapCoinPublicKey, ContractAddress>, + ], thresh: bigint, isInit: boolean, ]; ... constructor( - signers: Either<ZswapCoinPublicKey, ContractAddress>[], + signers: readonly [ + Either<ZswapCoinPublicKey, ContractAddress>, + Either<ZswapCoinPublicKey, ContractAddress>, + Either<ZswapCoinPublicKey, ContractAddress>, + ], thresh: bigint, isInit: boolean,Also applies to: 45-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/multisig/test/simulators/SignerSimulator.ts` around lines 20 - 23, The SignerArgs tuple currently allows an arbitrary-length signer array but the MockSigner/mock constructor expects exactly 3 signers; change the signers element type in SignerArgs to a fixed-length Vector of 3 (e.g., Vector<3, Either<ZswapCoinPublicKey, ContractAddress>>) so the tuple enforces 3 signers, and make the same change for the other similar tuple usage around the SignerSimulator (lines referencing the same tuple at 45-47). Update the type alias named SignerArgs (and the second tuple occurrence) to use Vector<3,...> instead of Either<...>[] to prevent invalid calls to MockSigner.
🤖 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/Signer.compact`:
- Around line 63-72: Add a one-time initialization guard to the initialize
circuit: check an "initialized" boolean/storage flag at the start of initialize
and abort (or revert) if it's already true, then proceed to call _addSigner for
each signer and _changeThreshold, and finally set the "initialized" flag to
true; use the existing initialize function plus the new flag (e.g., initialized,
isInitialized) so subsequent calls detect prior initialization and refuse to
run.
In `@contracts/src/multisig/test/Signer.test.ts`:
- Around line 48-61: Replace the two assertions that use .to.be.ok on function
callbacks with .not.toThrow(): change the initialization check wrapping the
SignerSimulator constructor (new SignerSimulator(SIGNERS, THRESHOLD, IS_INIT))
to use expect(() => { ... }).not.toThrow(), and change the loop that calls
contract.assertSigner(...) for each entry in SIGNERS to use expect(() => { for
(...) contract.assertSigner(SIGNERS[i]); }).not.toThrow(); this ensures the
SignerSimulator constructor and the contract.assertSigner checks are validated
to run without throwing.
---
Nitpick comments:
In `@contracts/src/multisig/test/simulators/SignerSimulator.ts`:
- Around line 20-23: The SignerArgs tuple currently allows an arbitrary-length
signer array but the MockSigner/mock constructor expects exactly 3 signers;
change the signers element type in SignerArgs to a fixed-length Vector of 3
(e.g., Vector<3, Either<ZswapCoinPublicKey, ContractAddress>>) so the tuple
enforces 3 signers, and make the same change for the other similar tuple usage
around the SignerSimulator (lines referencing the same tuple at 45-47). Update
the type alias named SignerArgs (and the second tuple occurrence) to use
Vector<3,...> instead of Either<...>[] to prevent invalid calls to MockSigner.
🪄 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: b0430dc6-60da-42b5-a8e0-21ab7f963145
📒 Files selected for processing (6)
CHANGELOG.mdcontracts/src/multisig/Signer.compactcontracts/src/multisig/test/Signer.test.tscontracts/src/multisig/test/mocks/MockSigner.compactcontracts/src/multisig/test/simulators/SignerSimulator.tscontracts/src/multisig/witnesses/SignerWitnesses.ts
1 participant
This PR proposes to add a generic signer module for use in constructing multisig contracts
Summary by CodeRabbit