Initial code for Radworks upgraded Governor #2
Conversation
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
There was a problem hiding this comment.
looks good @jferas! i gave you a bunch of nits, for you to do as you will with. Cheers.
src/RadworksGovernor.sol
Outdated
| @@ -0,0 +1,218 @@ | |||
| // SPDX-License-Identifier: AGPL-3.0-only | |||
| pragma solidity ^0.8.18; | |||
There was a problem hiding this comment.
I think this can bumped to ^0.8.20 as that is what we have as the version in the Counters.sol
| /// @inheritdoc IGovernor | ||
| // solhint-disable-next-line func-name-mixedcase | ||
| function COUNTING_MODE() public pure override returns (string memory) { | ||
| return "support=bravo&quorum=bravo"; |
There was a problem hiding this comment.
I think this needs to conform to https://eips.ethereum.org/EIPS/eip-6372. Do we need to override the inherited method.
There was a problem hiding this comment.
Need to override COUNTING_MODE here as it is required by Governor who RadworksGovernor inherits from
src/RadworksGovernor.sol
Outdated
| contract RadworksGovernor is | ||
| Governor, | ||
| GovernorVotesComp, | ||
| RadworksGovernorTimelockCompound, |
There was a problem hiding this comment.
I think we can use the stock GovernorTimelockCompound. We used a custom Timelock contract for PoolTogether because their GRACE_PERIOD method was called gracePeriod. But, for Radicle it seems to be compatible.
There was a problem hiding this comment.
Looks like Radicle also calls it gracePeriod .. https://etherscan.io/address/0x8dA8f82d2BbDd896822de723F55D6EdF416130ba#code
There was a problem hiding this comment.
Thanks! Indeed.. I guess I missed that both were present: c7f59d4
src/RadworksGovernor.sol
Outdated
| require( | ||
| _proposalVotersWeightCast[proposalId][account] == 0, | ||
| "Radworks Governor: vote would exceed weight" | ||
| ); |
There was a problem hiding this comment.
Nit: I think using an error type is more gas efficient
| } | ||
|
|
||
| /// @inheritdoc Governor | ||
| function _quorumReached(uint256 proposalId) internal view virtual override returns (bool) { |
There was a problem hiding this comment.
Why does this and the _voteSucceeded need to be overrided?
There was a problem hiding this comment.
I believe it needs to be as RadworksGovernor inherits from Governor
| // This RadworksGovernorTimelockCompound has been modified from the OZ GovernorTimelockCompound to | ||
| // support a Radworks Timelock which does not conform to Governor Alpha Timelock. If Radworks | ||
| // decides to change the Timelock in the future then it must conform to the Radworks Timelock | ||
| // interface. |
There was a problem hiding this comment.
Is the existing timelock acceptable enough?
There was a problem hiding this comment.
ah, ok! so radworks timelock is simply the default? this simplifies things 👍
There was a problem hiding this comment.
Yes, it is different, but it doesn't conflict in the same way as the poolTogether timelock did.
|
Coverage after merging feature/add-governor into 11-02-chore_install_OZ_contracts_library will be
|
jferas
changed the base branch from
11-02-chore_install_OZ_contracts_library
to
main
jferas
changed the base branch from
main
to
11-02-chore_install_OZ_contracts_library
jferas
merged commit ba760da
into
11-02-chore_install_OZ_contracts_library
Added Bravo/OZ style governor with needed timelock interface changes