Add multi-sig governance guard for contract upgrades and storage migration (#16) - #26
Conversation
New library crate, not a deployed contract itself. Provides propose, approve, cancel for a versioned upgrade proposal gated by an M-of-N signer threshold, plus storage-version tracking for a follow-up migrate step. Deliberately does not touch each contract's existing single-admin auth for its normal operations, this only gates the ability to swap a contract's own wasm and run its migration, not the day to day admin powers each contract already has. Signer set and threshold are immutable after init in this version, there is no signer-rotation flow yet, that is its own governance problem and shipping it here would roughly double the attack surface for a capability this issue did not ask for. propose_upgrade and approve_upgrade both report when their approval is the one that crosses the threshold, since a 1-of-N governance setup reaches threshold on the proposer's own approval and has no second signer left to place a distinct approve call. This crate cannot call update_current_contract_wasm itself, since that always replaces whichever contract is currently executing, so it returns readiness and leaves the actual swap to the host contract that embeds it. 20 tests cover threshold math, duplicate and non-signer rejection, approval replay across a changed proposal, proposal expiry, and migration version gating including the backwards case. Part of workman-labs#16.
initialize now takes signers and threshold, wired straight into guildworkman-governance-guard. Adds propose_upgrade, approve_upgrade, cancel_upgrade, and migrate as new entrypoints, alongside read-only get_signers, get_upgrade_threshold, get_pending_upgrade, and get_storage_version. The existing admin field and every action gated on it (update_config, set_stake, recalculate_score) are untouched, governance only reaches the ability to swap this contract's code and run its migration. migrate is version gated against CURRENT_STORAGE_VERSION and currently a no-op body since storage hasn't changed shape yet, it exists so the version-checked path is real and a future migration has a place to land its actual field transformations. 10 new tests cover the governance wiring through the real contract entrypoints, stopping one approval short of the configured threshold everywhere. Crossing it calls update_current_contract_wasm, which needs Wasm actually uploaded on the ledger, not available inside a plain cargo test run, guildworkman-governance-guard's own 20 tests already cover the threshold/replay/expiry logic this builds on in isolation. Part of workman-labs#16.
Same wiring as the reputation contract, initialize takes signers and threshold, propose_upgrade/approve_upgrade/cancel_upgrade/migrate added alongside their read-only getters. The dispute-arbiter admin is untouched. 6 new tests cover the wiring through the real contract entrypoints, including one that gets the proposal to exactly one approval and stops there rather than crossing the threshold, for the same reason documented in reputation's test file. Part of workman-labs#16.
…truct Adding two loose params to loyalty-token's initialize took it to 8 arguments and clippy's too_many_arguments lint failed the build (limit is 7). Rather than suppress the lint, bundled signers and threshold into one GovernanceInit struct and applied it everywhere for consistency, not just where it was strictly required, so all four contracts take the same shape. Also applies cargo fmt across every file this touched, the repo had formatting drift that predates this work and fmt --check was never actually run in CI. Part of workman-labs#16.
Same wiring as the other three contracts, initialize now takes a GovernanceInit alongside its existing admin/minter/metadata params. The admin's existing power to rotate the minter is untouched. 6 new tests cover the wiring through the real contract entrypoints. Part of workman-labs#16.
…guard Same wiring as the other three, plus this contract's own test suite deploys a real loyalty-token internally, so its fixture needed the same GovernanceInit argument added to that inner initialize call, on top of emissions' own. 6 new tests cover the wiring through the real contract entrypoints. Part of workman-labs#16.
fmt --check was listed as a task on the issue but never actually wired up, added it as its own fast-failing step before clippy. Also switched clippy to --all-targets so it covers #[cfg(test)] code, not just the lib target, it was silently skipping every test.rs file before this. Part of workman-labs#16.
New Upgrade governance section covers the shared pattern once: the propose/approve/cancel/migrate flow, why cancel only needs one signer while approving needs the full threshold, and why signer rotation is explicitly out of scope for this pass. Each contract's initialize signature, method list, error table, and CLI example are updated to match, except reputation's section, which already described a different, older interface than what's in src/lib.rs before this PR — flagged that drift inline rather than folding an unrelated rewrite into this change. Also updated the two security-considerations bullets that this work made outdated: the 'no pause/upgrade mechanism' line and the single-key-admin line, plus added a note on what cargo test does and doesn't actually exercise for the upgrade path (the real Wasm swap needs an uploaded artifact no unit test run has). Part of workman-labs#16.
This is a security-critical change (it gates code upgrades across all four contracts), so I read through it carefully. Overall the scoping discipline here is good — you've been explicit about what's in and out, which makes this much easier to review than a PR that tried to solve governance end-to-end. On the core design Multi-sig gating only on code swaps, leaving existing admin auth untouched, is the right cut for this PR — upgrade authority and operational authority are genuinely different concerns and conflating them would've made this harder to reason about. On signer rotation being out of scope Agreed this is the right call to defer rather than fold in here. Fixed-forever signers is a real limitation worth flagging loudly though — can we get an issue opened now (even if it's not prioritized) so this doesn't get lost, and maybe a note in the README/module docs saying rotation is explicitly not supported yet, so it's not a surprise to whoever deploys this? On migrate Makes sense to land the version-gated scaffolding now with no-op migrations, rather than waiting until there's an actual migration to design the interface around. Just confirming: is migrate itself also gated behind the same multi-sig flow, or only the code swap? Want to make sure there's no path where migrate can be called post-upgrade without the same approval bar. On the testing gap The update_current_contract_wasm limitation is a real constraint of the test environment, not a shortcut you're taking — makes sense that governance-guard's own 20 tests carry the threshold/replay logic coverage while the through-contract tests stop one approval short. Is there a plan (even informal/manual) for verifying the full path against a real uploaded wasm before this goes to a live network, or does that happen naturally in a testnet deployment step later? On CI Adding cargo fmt --check and fixing clippy silently skipping test files are both good catches independent of the main change — glad these didn't slip through. 107 passing tests and clean fmt/clippy is a good signal. Nothing here blocks approval for me, but I'd like answers on the expiry/hash-change and migrate-gating questions before merge, and a tracking issue for signer rotation opened alongside this. |
…on hash change Answers a review question on workman-labs#26. The doc comment implied replacement only happened for a different or expired proposal, the actual behavior is unconditional: even re-proposing the identical hash resets the approval count, so every signer has to approve again regardless of whether they'd already approved the same hash under the prior proposal. Part of workman-labs#16.
|
answers, checked against the actual code rather than from memory: expiry is a hardcoded constant in governance-guard (PROPOSAL_TTL_LEDGERS, ~7 days), same for every contract right now, not part of GovernanceInit. making it tunable per contract would mean adding it to that struct and threading it through, happy to do that here or as a follow-up if you want it configurable. hash change resets to zero, and it's actually broader than that. propose_upgrade unconditionally wipes whatever was pending and starts a fresh approvals list with just the new proposer, even if you call it again with the identical hash. so yes, everyone has to approve again, including someone who'd already approved the exact same hash under the proposal that got replaced. pushed a doc comment making that explicit since you were right that it wasn't stated clearly enough before. migrate only requires one signer, not the full threshold. that's deliberate, not an oversight, reasoning is in the code comment: the risky decision (which code to trust) already went through the full multi-sig when the upgrade itself was approved, migrate just runs whatever that already-approved code ships with. but I get why you'd want to double check this one specifically, if you'd rather it require the full threshold too, that's a small change, let me know. no automated plan for the real wasm swap, would be a manual testnet deploy-propose-approve-confirm dry run before this goes near a live network with real funds. can write that up as a checklist if useful. opened #27 for signer rotation like you asked. |
meshackyaro
left a comment
meshackyaro
left a comment
There was a problem hiding this comment.
Well done! This is clear, specific, and each answer is anchored to something checkable (constant name, code comment, issue number) rather than vague reassurance. I like that.
Thanks for contributing to Guildworkman
The governance-guard multi-sig previously froze its signer set and threshold at initialize, with no way to change them short of a full contract upgrade. This adds a timelocked, threshold-gated signer rotation flow (propose -> approve -> execute) to the shared guard and wires it into every host contract that consumes it. Security model: - Authorized by the current signer set at the current (upgrade) threshold -- rotation grants the set no power it lacked via upgrade. - Mandatory timelock between reaching threshold and execution gives a minority about to be removed a visible on-chain window: no silent, instant lockout (minority protection). - No unilateral veto; only one live rotation at a time, so a lone signer can neither reset a gathering rotation's approvals nor displace a scheduled one -- a compromised signer cannot block its own removal (majority protection). - Executing a rotation clears any pending upgrade, whose approvals came from the old signer set. Adds PendingRotation storage, RotationProposed/Approved/Scheduled/ Executed events, five rotation error variants, and 37 guard tests (now covering the full rotation lifecycle and both protection properties). Host contracts (escrow, reputation, loyalty-token, loyalty-emissions) gain propose/approve/execute_signer_rotation and get_pending_rotation entrypoints plus the mapped error variants. Follow-up from #26 / #16. Co-authored-by: jayteemoney <mac@Macs-MacBook-Pro.local>
2 participants
Closes #16
Adds a shared governance-guard crate (a library, not its own deployed contract) that gates upgrading a contract's code behind a multi-sig proposal/approval flow, plus a version-gated migrate step for after. All four contracts take signers and a threshold at initialize now and get propose_upgrade/approve_upgrade/cancel_upgrade/migrate. Doesn't touch existing admin auth anywhere, it only ever gates code swaps.
Caught a real bug of my own while writing tests: with a single signer, their own proposal already meets the threshold, so propose_upgrade has to report readiness itself instead of waiting on a second approval that can never come. Also handled replay/expiry (can't approve twice, approvals don't carry over if the hash changes, proposals expire after about a week, any one signer can cancel).
Signer rotation is deliberately not in this PR, the set is fixed at initialize forever, that's its own governance problem and I didn't want to double the trust surface in one pass.
migrate is real and version-gated but there's no actual migration to run yet, every contract is still on version 1.
One honest limitation: crossing the threshold calls update_current_contract_wasm, which needs the target wasm already uploaded on-chain, nothing cargo test can do. Governance tests through real contracts stop one approval short on purpose, the threshold/replay logic itself is fully covered in governance-guard's own 20 tests.
Also added the missing cargo fmt --check to CI and fixed clippy silently skipping test files.
107 tests passing, fmt/clippy clean, release wasm build succeeds.