Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added GovernorVotesCompQuorumFraction for settable quorum #13

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions script/DeployInput.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ contract DeployInput {
uint256 constant INITIAL_VOTING_DELAY = 7200; // 24 hours
uint256 constant INITIAL_VOTING_PERIOD = 17_280; // matches existing config
uint256 constant INITIAL_PROPOSAL_THRESHOLD = 1_000_000e18; // matches existing config
uint256 constant INITIAL_QUORUM_PCT = 4;
}
17 changes: 10 additions & 7 deletions src/RadworksGovernor.sol
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {IGovernor} from "@openzeppelin/contracts/governance/IGovernor.sol";
import {ICompoundTimelock} from
"@openzeppelin/contracts/governance/extensions/GovernorTimelockCompound.sol";
import {GovernorSettings} from "@openzeppelin/contracts/governance/extensions/GovernorSettings.sol";

import {GovernorVotesCompQuorumFraction} from "./lib/GovernorVotesCompQuorumFraction.sol";
import {
GovernorTimelockCompound,
ICompoundTimelock
Expand All @@ -22,7 +22,7 @@ import {GovernorCompatibilityBravo} from
/// @notice The upgraded Radworks Governor: Bravo compatible and extended from OpenZeppelin.
contract RadworksGovernor is
Governor,
GovernorVotesComp,
GovernorVotesCompQuorumFraction,
GovernorTimelockCompound,
GovernorSettings
{
Expand All @@ -47,9 +47,6 @@ contract RadworksGovernor is
/// @notice Human readable name of this Governor.
string private constant GOVERNOR_NAME = "Radworks Governor Bravo";

/// @notice The number of RAD (in "wei") that must participate in a vote to meet quorum threshold.
uint256 private constant QUORUM = 4_000_000e18; // 4,000,000 RAD

/// @param _initialVotingDelay The initial voting delay this Governor will enforce.
/// @param _initialVotingPeriod The initial voting period this Governor will enforce.
/// @param _initialProposalThreshold The initial number of RAD required to submit
Expand All @@ -59,6 +56,7 @@ contract RadworksGovernor is
uint256 _initialVotingPeriod,
uint256 _initialProposalThreshold
)
GovernorVotesCompQuorumFraction(4)
GovernorVotesComp(RAD_TOKEN)
GovernorSettings(_initialVotingDelay, _initialVotingPeriod, _initialProposalThreshold)
GovernorTimelockCompound(TIMELOCK)
Expand Down Expand Up @@ -177,8 +175,13 @@ contract RadworksGovernor is
/// @notice The amount of RAD required to meet the quorum threshold for a proposal
/// as of a given block.
/// @dev Our implementation ignores the block number parameter and returns a constant.
function quorum(uint256) public pure override returns (uint256) {
return QUORUM;
function quorum(uint256 timepoint)
alexkeating marked this conversation as resolved.
Show resolved Hide resolved
public
view
override(IGovernor, GovernorVotesCompQuorumFraction)
returns (uint256)
{
return GovernorVotesCompQuorumFraction.quorum(timepoint);
}

/// @inheritdoc Governor
Expand Down
125 changes: 125 additions & 0 deletions src/lib/GovernorVotesCompQuorumFraction.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
// SPDX-License-Identifier: MIT
// (Based on OpenZeppelin governance/extensions/GovernorVotesQuorumFraction.sol)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be better to use a permalink to the OpenZeppelin contract

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: 34b7f01


pragma solidity ^0.8.0;

import {
ERC20VotesComp,
GovernorVotesComp
} from "@openzeppelin/contracts/governance/extensions/GovernorVotesComp.sol";
import "@openzeppelin/contracts/utils/Checkpoints.sol";
import "@openzeppelin/contracts/utils/math/SafeCast.sol";

/**
* @dev Extension of {Governor} for voting weight extraction from an {GovernorVotesComp} token and a
* quorum
* expressed as a fraction of the total supply.
*/
abstract contract GovernorVotesCompQuorumFraction is GovernorVotesComp {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although obvious it may be good to explicitly say this line has changed from the original.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: 34b7f01

using Checkpoints for Checkpoints.Trace224;

uint256 private _quorumNumerator; // DEPRECATED in favor of _quorumNumeratorHistory

/// @custom:oz-retyped-from Checkpoints.History
Checkpoints.Trace224 private _quorumNumeratorHistory;

event QuorumNumeratorUpdated(uint256 oldQuorumNumerator, uint256 newQuorumNumerator);

/**
* @dev Initialize quorum as a fraction of the token's total supply.
*
* The fraction is specified as `numerator / denominator`. By default the denominator is 100, so
* quorum is pecified as a percent: a numerator of 10 corresponds to quorum being 10% of total
* supply.
* The denominator can be customized by overriding {quorumDenominator}.
*/
constructor(uint256 quorumNumeratorValue) {
_updateQuorumNumerator(quorumNumeratorValue);
}

/**
* @dev Returns the current quorum numerator. See {quorumDenominator}.
*/
function quorumNumerator() public view virtual returns (uint256) {
return _quorumNumeratorHistory._checkpoints.length == 0
? _quorumNumerator
: _quorumNumeratorHistory.latest();
}

/**
* @dev Returns the quorum numerator at a specific timepoint. See {quorumDenominator}.
*/
function quorumNumerator(uint256 timepoint) public view virtual returns (uint256) {
// If history is empty, fallback to old storage
uint256 length = _quorumNumeratorHistory._checkpoints.length;
if (length == 0) return _quorumNumerator;

// Optimistic search, check the latest checkpoint
Checkpoints.Checkpoint224 memory latest = _quorumNumeratorHistory._checkpoints[length - 1];
if (latest._key <= timepoint) return latest._value;

// Otherwise, do the binary search
return _quorumNumeratorHistory.upperLookupRecent(SafeCast.toUint32(timepoint));
}

/**
* @dev Returns the quorum denominator. Defaults to 100, but may be overridden.
*/
function quorumDenominator() public view virtual returns (uint256) {
return 100;
}

/**
* @dev Returns the quorum for a timepoint, in terms of number of votes:
* `supply * numerator / denominator`.
*/
function quorum(uint256 timepoint) public view virtual override returns (uint256) {
// @dev: 'token.totalSupply()` ignores timepoint, different from 'GovernorVotesQuorumFraction'
return (token.totalSupply() * quorumNumerator(timepoint)) / quorumDenominator();
}

/**
* @dev Changes the quorum numerator.
*
* Emits a {QuorumNumeratorUpdated} event.
*
* Requirements:
*
* - Must be called through a governance proposal.
* - New numerator must be smaller or equal to the denominator.
*/
function updateQuorumNumerator(uint256 newQuorumNumerator) external virtual onlyGovernance {
_updateQuorumNumerator(newQuorumNumerator);
}

/**
* @dev Changes the quorum numerator.
*
* Emits a {QuorumNumeratorUpdated} event.
*
* Requirements:
*
* - New numerator must be smaller or equal to the denominator.
*/
function _updateQuorumNumerator(uint256 newQuorumNumerator) internal virtual {
require(
newQuorumNumerator <= quorumDenominator(),
"GovernorVotesQuorumFraction: quorumNumerator over quorumDenominator"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to say "GovernorVotesQuorumCompFraction"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: 34b7f01

);

uint256 oldQuorumNumerator = quorumNumerator();

// Make sure we keep track of the original numerator in contracts upgraded from a version
// without checkpoints.
if (oldQuorumNumerator != 0 && _quorumNumeratorHistory._checkpoints.length == 0) {
_quorumNumeratorHistory._checkpoints.push(
Checkpoints.Checkpoint224({_key: 0, _value: SafeCast.toUint224(oldQuorumNumerator)})
);
}

// Set new quorum for future proposals
_quorumNumeratorHistory.push(SafeCast.toUint32(clock()), SafeCast.toUint224(newQuorumNumerator));

emit QuorumNumeratorUpdated(oldQuorumNumerator, newQuorumNumerator);
}
}
2 changes: 0 additions & 2 deletions test/Constants.sol
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,4 @@ contract Constants {

// we have not yet deployed the Radworks Bravo Governor
address constant DEPLOYED_BRAVO_GOVERNOR = 0x1111111111111111111111111111111111111111;

uint256 constant QUORUM = 4_000_000e18;
}
28 changes: 22 additions & 6 deletions test/RadworksGovernor.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ import {IGovernor} from "@openzeppelin/contracts/governance/IGovernor.sol";
import {IGovernorAlpha} from "src/interfaces/IGovernorAlpha.sol";
import {RadworksGovernorTest} from "test/helpers/RadworksGovernorTest.sol";
import {ProposalTest} from "test/helpers/ProposalTest.sol";
import {ProposalBuilder} from "test/helpers/Proposal.sol";
import "../script/DeployInput.sol";

abstract contract Constructor is RadworksGovernorTest {
function testFuzz_CorrectlySetsAllConstructorArgs(uint256 _blockNumber) public {
function test_CorrectlySetsAllConstructorArgs() public {
assertEq(governorBravo.name(), "Radworks Governor Bravo");
assertEq(address(governorBravo.token()), RAD_TOKEN);

Expand All @@ -21,7 +23,10 @@ abstract contract Constructor is RadworksGovernorTest {

assertEq(governorBravo.proposalThreshold(), INITIAL_PROPOSAL_THRESHOLD);

assertEq(governorBravo.quorum(_blockNumber), QUORUM);
assertEq(
governorBravo.quorum(block.number),
(governorBravo.token().totalSupply() * INITIAL_QUORUM_PCT) / 100
);
assertEq(governorBravo.timelock(), TIMELOCK);
assertEq(governorBravo.COUNTING_MODE(), "support=bravo&quorum=bravo");
}
Expand Down Expand Up @@ -206,7 +211,8 @@ abstract contract Propose is ProposalTest {
function testFuzz_NewGovernorCanUpdateSettingsViaSuccessfulProposal(
uint256 _newDelay,
uint256 _newVotingPeriod,
uint256 _newProposalThreshold
uint256 _newProposalThreshold,
uint256 _newQuorumNumerator
) public {
vm.assume(_newDelay != governorBravo.votingDelay());
vm.assume(_newVotingPeriod != governorBravo.votingPeriod());
Expand All @@ -216,12 +222,13 @@ abstract contract Propose is ProposalTest {
_newDelay = bound(_newDelay, 0, 50_000); // about a week at 1 block per 12s
_newVotingPeriod = bound(_newVotingPeriod, 1, 200_000); // about a month
_newProposalThreshold = bound(_newProposalThreshold, 0, 42 ether);
_newQuorumNumerator = bound(_newQuorumNumerator, 0, 99);

_upgradeToBravoGovernor();

address[] memory _targets = new address[](3);
uint256[] memory _values = new uint256[](3);
bytes[] memory _calldatas = new bytes[](3);
address[] memory _targets = new address[](4);
uint256[] memory _values = new uint256[](4);
bytes[] memory _calldatas = new bytes[](4);
string memory _description = "Update governance settings";

_targets[0] = address(governorBravo);
Expand All @@ -234,6 +241,10 @@ abstract contract Propose is ProposalTest {
_calldatas[2] =
_buildProposalData("setProposalThreshold(uint256)", abi.encode(_newProposalThreshold));

_targets[3] = address(governorBravo);
_calldatas[3] =
_buildProposalData("updateQuorumNumerator(uint256)", abi.encode(_newQuorumNumerator));

// Submit the new proposal
vm.prank(PROPOSER);
uint256 _newProposalId = governorBravo.propose(_targets, _values, _calldatas, _description);
Expand Down Expand Up @@ -271,6 +282,11 @@ abstract contract Propose is ProposalTest {
assertEq(governorBravo.votingDelay(), _newDelay);
assertEq(governorBravo.votingPeriod(), _newVotingPeriod);
assertEq(governorBravo.proposalThreshold(), _newProposalThreshold);
assertEq(governorBravo.quorumNumerator(block.number), _newQuorumNumerator);
assertEq(
governorBravo.quorum(block.number),
(governorBravo.token().totalSupply() * _newQuorumNumerator) / 100
);
}

function testFuzz_NewGovernorCanPassMixedProposal(
Expand Down
Loading