Skip to content

PULL_REQUEST_TEMPLATE.md

Latest commit

 

History

History
132 lines (98 loc) · 6.36 KB

PULL_REQUEST_TEMPLATE.md

File metadata and controls

132 lines (98 loc) · 6.36 KB
Raw

Pull Request

Description

Make GOVERNANCE_QUORUM relative to signer-set size and block remove_signer from dropping below quorum.

The governance contract previously hardcoded GOVERNANCE_QUORUM: u32 = 2 independent of how many signers were actually registered. This caused two critical issues: (1) remove_signer performed no check that the remaining signer count stays >= quorum, allowing the admin to shrink the signer set to a single signer while quorum still claimed to require 2 — permanently bricking all future proposals. (2) A fixed quorum of 2 does not scale; with 7 signers, 2-of-7 is far weaker than the multisig intends.

This PR replaces the fixed quorum with a configurable threshold stored at init, enforces the invariant 1 <= threshold <= signers.len() on every signer-set mutation, and snapshots the threshold at quorum time so in-flight proposals are immune to mid-flight threshold changes.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Test coverage improvement
  • Refactoring (no functional changes)

Related Issues

Closes #621

Changes Made

  • contracts/governance/src/lib.rs:

    • Removed hardcoded GOVERNANCE_QUORUM: u32 = 2 constant
    • Added QuorumInfo struct to snapshot (reached_at, threshold) when quorum is first reached
    • Added DataKey::Threshold for instance-stored configurable threshold
    • Added GovernanceError::InvalidThreshold (15) for init-time validation failures
    • Added GovernanceError::QuorumWouldBreak (16) for removal that would violate the invariant
    • Added get_threshold() storage helper
    • Updated init(admin, signers, threshold) — validates 1 <= threshold <= signers.len()
    • Updated remove_signer() — rejects removal if signers.len() - 1 < threshold
    • Updated approve() — uses stored threshold; snapshots threshold in QuorumInfo when quorum reached
    • Updated execute() — reads threshold from QuorumInfo snapshot (immune to threshold changes)
    • Updated quorum() view — returns stored threshold

  • contracts/stream/tests/governance_integration.rs:

    • Updated all existing tests to pass threshold parameter to init()
    • Added 5 new integration tests: zero threshold rejection, threshold above signers rejection, removal below threshold errors, execution with exactly threshold approvals, threshold unchanged after add_signer

  • docs/governance.md:

    • Replaced fixed GOVERNANCE_QUORUM docs with configurable threshold model
    • Documented init(admin, signers, threshold) entrypoint
    • Documented remove_signer rejection with QuorumWouldBreak
    • Added quorum() entrypoint doc returning stored threshold
    • Added 3 new security considerations (bricking prevention, threshold snapshotting, init validation)
    • Updated storage layout table to include Threshold and QuorumInfo types
    • Updated test coverage list

Snapshot Test Changes

Did this PR modify snapshot test files?

  • Yes - snapshot files were updated (explain below)
  • No - no snapshot changes

If yes, explain why snapshots changed:

N/A — no snapshot files were present in the repository for the governance contract.

Testing

Test Coverage

  • All governance unit tests pass locally: cargo test -p fluxora_governance
  • All tests pass locally: cargo test -p fluxora_stream
  • New tests added for new functionality
  • Existing tests updated for changed functionality
  • Test coverage remains above 95%

Manual Testing

  • Tested on local environment
  • Tested edge cases
  • Tested error conditions

Edge cases covered:

  • Init with threshold = 0 → InvalidThreshold
  • Init with threshold > signers → InvalidThreshold
  • Init with threshold = signers count → succeeds
  • Init with threshold = 1 → succeeds
  • Remove signer down to threshold boundary → succeeds
  • Remove signer below threshold → QuorumWouldBreak
  • Remove non-existent signer → no-op (ok)
  • Execute with exactly threshold approvals → succeeds
  • Threshold unchanged after adding more signers
  • Quorum snapshot protects against mid-flight threshold changes

Documentation

  • Code comments added/updated
  • Documentation updated (if behavior changed)
  • README updated (if needed)
  • Snapshot test documentation reviewed

Security Considerations

  • No new security concerns introduced
  • Authorization boundaries verified
  • Input validation added/verified
  • Error handling reviewed

Key security properties:

  1. Governance bricking prevented: remove_signer enforces signers.len() - 1 >= threshold, so the signer set can never shrink below quorum.
  2. In-flight proposal protection: When quorum is first reached, the current threshold is snapshotted in QuorumInfo. Execution uses this snapshot rather than the live threshold, so an admin cannot raise the threshold after quorum to block execution, nor lower it to let a proposal with fewer approvals through.
  3. Init bounds enforced: threshold must be >= 1 and <= signers.len(), preventing degenerate configurations at deployment.

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published

Additional Notes

The stream contract (fluxora_stream) has pre-existing compilation errors unrelated to this PR (duplicate type definitions, missing struct fields, etc.), which prevented running the integration tests in contracts/stream/tests/governance_integration.rs. The governance unit tests (27 tests, 10 new) all pass successfully, and cargo build --target wasm32-unknown-unknown -p fluxora_governance completes without errors.

Reviewer Checklist

  • Code quality and style
  • Test coverage adequate
  • Documentation complete
  • Snapshot changes justified and correct
  • Security implications reviewed
  • Breaking changes documented