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

Create basic governor #16

Merged
merged 8 commits into from
Oct 3, 2024
Merged

Create basic governor #16

merged 8 commits into from
Oct 3, 2024

Conversation

jferas
Copy link
Collaborator

@jferas jferas commented Oct 2, 2024

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.

Copy link
Collaborator

@wildmolasses wildmolasses left a 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

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

line break issues

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done: 3977e0b

public
view
virtual
override(GovernorTimelockControlUpgradeable, GovernorUpgradeable)
Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done: c518484

return address(this);
}

/// @dev Queues a proposal to be executed after it has succeeded.
Copy link
Collaborator

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done: dbee29e

}

/// @dev returns executor address
/// @return address of the executor.
Copy link
Collaborator

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done: dbee29e

/// @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.
Copy link
Collaborator

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done: dbee29e

@coveralls
Copy link

coveralls commented Oct 2, 2024

Pull Request Test Coverage Report for Build 11163701066

Details

  • 0 of 17 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-3.5%) to 95.327%

Changes Missing Coverage Covered Lines Changed/Added Lines %
contracts/CompoundGovernor.sol 0 17 0.0%
Totals Coverage Status
Change from base Build 11128743978: -3.5%
Covered Lines: 318
Relevant Lines: 335

💛 - Coveralls

contract CompoundGovernor is
Initializable,
GovernorVotesUpgradeable,
GovernorTimelockControlUpgradeable,
Copy link
Member

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

Copy link
Member

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

^ defer to you @jferas

Copy link
Collaborator Author

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done: 08411a0

Copy link
Member

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.

Copy link
Collaborator Author

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

override(GovernorTimelockControlUpgradeable, GovernorUpgradeable)
returns (address)
{
return address(this);
Copy link
Collaborator

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done: 0f1b6c9

Copy link
Collaborator

@wildmolasses wildmolasses left a comment

Choose a reason for hiding this comment

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

lgtm

@wildmolasses wildmolasses merged commit 25c4034 into main Oct 3, 2024
3 checks passed
wildmolasses pushed a commit that referenced this pull request Nov 1, 2024
* 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)
wildmolasses pushed a commit that referenced this pull request Nov 1, 2024
* 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
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.

Create upgradeable governor that inherits the appropriate base contracts
4 participants