Multisig

[immrsd]

Fixed #21

• Tests
• Documentation
• Added entry to CHANGELOG.md
• Tried the feature on a public network

[codecov]

Codecov Report

Attention: Patch coverage is 95.70552% with 7 lines in your changes missing coverage. Please review.

Project coverage is 92.02%. Comparing base (02c9ce6) to head (a7c569e).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
packages/governance/src/multisig/multisig.cairo	96.47%	5 Missing ⚠️
...ckages/governance/src/multisig/storage_utils.cairo	85.71%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1193      +/-   ##
==========================================
+ Coverage   91.56%   92.02%   +0.46%     
==========================================
  Files          47       49       +2     
  Lines        1233     1392     +159     
==========================================
+ Hits         1129     1281     +152     
- Misses        104      111       +7     

Files with missing lines	Coverage Δ
packages/account/src/account.cairo	98.11% <ø> (ø)
.../governance/src/timelock/timelock_controller.cairo	88.70% <100.00%> (ø)
packages/governance/src/utils/call_impls.cairo	78.57% <100.00%> (ø)
packages/testing/src/constants.cairo	100.00% <100.00%> (ø)
...ckages/governance/src/multisig/storage_utils.cairo	85.71% <85.71%> (ø)
packages/governance/src/multisig/multisig.cairo	96.47% <96.47%> (ø)

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02c9ce6...a7c569e. Read the comment docs.

[ericnordelo]

Hey @immrsd, I quickly went over the PR and the design is looking good, my main suggestion is to follow the off-chain approach for handling proposals similar to what we do in timelock and governor. Also, I know is a first iteration and there's no a lot of comments, but let's prioritize adding comment to functions so is also easier to review and keep track of everything.

[ericnordelo]

Looking good @immrsd! Left some comments.

[ericnordelo]

Also, if a signer is removed, should pending transactions be canceled since the signers that may have confirmed may have been the removed ones, and the confirmations of the transaction are not decreased while the quorum may have been? cc @andrew-fleming.

[immrsd]

Also, if a signer is removed, should pending transactions be canceled since the signers that may have confirmed may have been the removed ones, and the confirmations of the transaction are not decreased while the quorum may have been? cc @andrew-fleming.

That may be an issue indeed, good call. To my mind, introducing a new tx Canceled state and keeping track of all pending txs would overcomplicate the implementation and make it more error-prone. I addressed this issue by making confirmations a computable property that's not stored in the storage. It's calculated by iterating through all current signers and checking if a signer has confirmed the tx.

Only execute function becomes a bit more expensive in terms of computation, but it also removes storage variable for tracking number of confirmations and the necessity to update it in the confirm_transaction and revoke_confirmation functions

[andrew-fleming]

The improvements look good! I left a few comments—really just one that needs to be addressed

[andrew-fleming]

LGTM!

[ericnordelo]

Left a few comments on the comments :). Besides that I think is ready to merge.

[ericnordelo]

LGTM!