Skip to content

msig cleanup#410

Merged
czareko merged 5 commits intomainfrom
msig_review
Mar 10, 2026
Merged

msig cleanup#410
czareko merged 5 commits intomainfrom
msig_review

Conversation

@n13
Copy link
Collaborator

@n13 n13 commented Mar 10, 2026

From #409

Note: Not doing 4, 5 from #409

# Severity Finding Fix Effort
1 HIGH Weight undercharge: iter_prefix().count() not benchmarked; active_proposals unused Easy -- use existing counter
2 MEDIUM Redundant call size check (DRY) Trivial -- delete 1 line
3 MEDIUM Silent try_insert error on per-signer counter Easy
6 LOW Extra write for threshold=1 proposals Easy
7 LOW Triple access to Multisigs storage in propose Moderate refactor
8 LOW Misleading auto-cleanup docstring Trivial
9 LOW claim_deposits no signer check Trivial

n13 added 3 commits March 10, 2026 14:17
that was not a good change, would allow filibuster, and not really bring benefits
@n13
Copy link
Collaborator Author

n13 commented Mar 10, 2026

GPT Codex PR Review: #410 msig cleanup

PR: Quantus-Network/chain#410

Scope Reviewed

  • pallets/multisig/src/lib.rs
  • Commits:
    • c58fd4019696f1e036fab2dd8ba122f931707e8d
    • 4047064ed91afeb63171b8319b5f17ba7949a591
    • 907e8853beb3323bb939dac0481bb6de19c644a8

Findings

High

  • None.

Medium

  • None.

Low

  • Added missing negative test coverage for claim_deposits signer-gating:
    • claim_deposits_fails_for_non_signer in pallets/multisig/src/tests.rs
    • Verifies non-signer gets NotASigner and proposal/deposit state is unchanged.

Behavioral Review Notes

  • propose now uses active_proposals instead of iterating storage for total proposal limit checks.
  • Redundant call-size guard was removed while preserving early size-check behavior.
  • Per-signer proposal counter insertion now propagates errors instead of ignoring them.
  • Threshold-1 proposals are created as Approved directly and emit ProposalReadyToExecute.
  • approve continues to allow approvals after Approved status (no regression to previously fixed behavior).
  • claim_deposits now enforces signer access, matching the intended permission model.

Validation

  • cargo test -p pallet-multisig
  • Result: 39 passed, 0 failed.

Recommendation

  • Approve.

@czareko czareko self-requested a review March 10, 2026 08:40
@czareko czareko merged commit bd6932b into main Mar 10, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants