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

Conversation

jferas
Copy link
Contributor

@jferas jferas commented Nov 16, 2023

Added GovernorVotesCompQuorumFraction for settable quorum

Also added test for setting quorum numerator

@jferas jferas changed the title Added GovernorVotesCompQuorumFraction for settable quorum Implement settable quorum Nov 21, 2023
@jferas jferas changed the title Implement settable quorum Implement settable quorum via GovernorVotesCompQuorumFraction Nov 21, 2023
@jferas jferas changed the title Implement settable quorum via GovernorVotesCompQuorumFraction Added GovernorVotesCompQuorumFraction for settable quorum Nov 21, 2023
@jferas jferas changed the base branch from update-readme to main December 7, 2023 16:39
@@ -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

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

* 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

Copy link

Coverage after merging settable-quorum into main will be

80.00%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   RadworksGovernor.sol73.91%75%69.23%76%124–125, 133, 136, 150, 206, 82, 99
src/lib
   GovernorVotesCompQuorumFraction.sol75%50%80%86.67%106, 115–116, 56, 60, 63

@jferas
Copy link
Contributor Author

jferas commented Dec 15, 2023

Radworks has chosen to go with a fixed, constant quorum.

@jferas jferas closed this Dec 15, 2023
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