-
Notifications
You must be signed in to change notification settings - Fork 0
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
Create basic governor #16
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent job @jferas, a few nits for you
contracts/CompoundGovernor.sol
Outdated
OwnableUpgradeable | ||
{ | ||
/// @notice The number of votes in support of a proposal required in order for a quorum to be reached and for a vote | ||
/// to succeed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line break issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: 3977e0b
contracts/CompoundGovernor.sol
Outdated
public | ||
view | ||
virtual | ||
override(GovernorTimelockControlUpgradeable, GovernorUpgradeable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit, @jferas
usually it's the rightmost contract whose functionality is relevant / "super". I lightly prefer putting the functionality we're inheriting from as the last contract in the override args, to be a little more idiomatic. this applies to all the overridden functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: c518484
contracts/CompoundGovernor.sol
Outdated
return address(this); | ||
} | ||
|
||
/// @dev Queues a proposal to be executed after it has succeeded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we @inheritdoc GovernorTimelockControlUpgradeable
this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: dbee29e
contracts/CompoundGovernor.sol
Outdated
} | ||
|
||
/// @dev returns executor address | ||
/// @return address of the executor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we @inheritdoc GovernorTimelockControlUpgradeable
this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: dbee29e
contracts/CompoundGovernor.sol
Outdated
/// @param _targets A list of target addresses for calls to be made in the proposal. | ||
/// @param _values A list of values (ETH) to be passed to the calls in the proposal. | ||
/// @param _calldatas A list of calldata for the calls in the proposal. | ||
/// @param _descriptionHash The hash of the description for the proposal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we @inheritdoc GovernorTimelockControlUpgradeable
this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: dbee29e
Pull Request Test Coverage Report for Build 11163701066Details
💛 - Coveralls |
contracts/CompoundGovernor.sol
Outdated
contract CompoundGovernor is | ||
Initializable, | ||
GovernorVotesUpgradeable, | ||
GovernorTimelockControlUpgradeable, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to use GovernorTimelockCompoundUpgradeable.sol
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay with doing it in a seperate PR as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ defer to you @jferas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a separate issue for it, so I'll add it in here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: 08411a0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now or later, we need to update this to use the HEAD commit instead of v5.0.2 because it doesn't support flexible-voting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.. I'll make that change in a future PR that addresses this issue: #11
contracts/CompoundGovernor.sol
Outdated
override(GovernorTimelockControlUpgradeable, GovernorUpgradeable) | ||
returns (address) | ||
{ | ||
return address(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think the executor is wrong here; we want it to be the timelock address, no? it should probably return GovernorTimelockCompoundUpgradeable._executor()
or something, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: 0f1b6c9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
* feat: proposal guardian * remove `.only` * create new `proposalGuardian` role * add tests * Update comments per recommendation * feat: proposal guardian expires * lint `GovernorBravoDelegate` * feat: expose guardian expiration (#17)
* forge install: openzeppelin-contracts-upgradeable v5.0.2 * Create initial basic governor * Line break fix * Inheritance order fix * Inheritdoc updates * Fix eslint issue in lib directory * Switch to using GovernorTimelockCompoundUpgradeable * Fixed _executor function
Resolves #4
Creates the initial governor, inheriting from a variety of OZ contracts.
Features like flexible voting and settable fixed quorum and others will be in future PRs.