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.

Sign up for GitHub

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

Jump to bottom

Add timestamp based governor with EIP-6372 and EIP-5805 #3934

Merged
merged 70 commits into from Feb 9, 2023
Merged

Add timestamp based governor with EIP-6372 and EIP-5805 #3934

merged 70 commits into from Feb 9, 2023

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Jan 6, 2023 •
edited

Fixes #3081

WIP

This includes a lot of retypes that should be safe, but that must be double-checked, and that the plugin needs to understand.

PR Checklist

  • Update IVotes to include clock()
  • Refactor Votes & ERC20Votes to use the value returned by clock()
    • Check upgradeability
  • Refactor Governor to use a clock() function
    • Extract the clock() value from the token contract
    • Include a fallback for pre-EIP-5805 tokens
    • Check upgradeability
  • Refactor the governance helper to support the two mode (blockNumber/timestamp)
  • Test the new governor in both modes
    • Including the comp compatibility layer
    • Including the NFT voting
  • Test the new governor with legacy tokens
  • Documentation
  • Changelog entry

@Amxx Amxx marked this pull request as draft January 6, 2023 14:30
@Amxx Amxx added this to the 4.9 milestone Jan 6, 2023
@frangio frangio changed the title Draft: timestamp based governor with EIP-5805 Timestamp based governor with EIP-5805 Jan 9, 2023
@Amxx Amxx force-pushed the feature/timestamp-governor branch from f0ccbfb to bcedb7e Compare January 12, 2023 09:41
@Amxx Amxx marked this pull request as ready for review January 12, 2023 10:55
frangio
frangio reviewed Jan 18, 2023
View reviewed changes
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Some initial comments.

test/helpers/time.js Outdated Show resolved Hide resolved
test/governance/Governor.test.js Outdated
Comment on lines 106 to 108
startBlock: web3.utils.toBN(await clockFromReceipt[mode](txPropose.receipt)).add(votingDelay),
endBlock: web3.utils
.toBN(await clockFromReceipt[mode](txPropose.receipt))
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we always convert the result of clockFromReceipt[mode] and clock[mode] into BN. Can those helpers return a BN directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are many places (25 occurences) where we do:

const timepoint = await clockFromReceipt[mode](receipt);
// [...]
expect(await this.token.getPastVotes(holder, timepoint - 1)).to.be.bignumber.equal('0');

we would need to to timepoint.subn(1) which affects readability.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is to compare to 9 occurences of

.toBN(await clockFromReceipt[mode](...))

contracts/governance/Governor.sol Outdated Show resolved Hide resolved
contracts/governance/extensions/GovernorPreventLateQuorum.sol Outdated Show resolved Hide resolved
contracts/governance/utils/Votes.sol Outdated Show resolved Hide resolved
contracts/governance/utils/Votes.sol Show resolved Hide resolved
@frangio frangio removed this from the 4.9 milestone Jan 19, 2023
@frangio
Copy link
Contributor

frangio commented Jan 20, 2023

FYI I removed this PR from the milestone because the corresponding issue is already in it.

@changeset-bot
Copy link

changeset-bot bot commented Jan 20, 2023 •
edited

🦋 Changeset detected

Latest commit: 8397ce7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Amxx Amxx force-pushed the feature/timestamp-governor branch from 6d243e8 to 21588f0 Compare January 25, 2023 15:30
@Amxx Amxx requested a review from ernestognw January 25, 2023 15:42
@frangio
Copy link
Contributor

frangio commented Feb 4, 2023

The storage layout errors we are seeing, which say "Layout could have changed...", are because the compiler is not producing storage layout information so the Upgrades plugin conservatively reports that there could be some error. I've added the layout in the config in 3b591a4. After that finishes producing the new layout artifact, we should merge and run it in this PR.

@frangio
Copy link
Contributor

frangio commented Feb 4, 2023

The diff test coverage is not looking super good. See on Codecov. It seems to be in large parte because CLOCK_MODE is never tested. What do you think about adding an EIP-6372 behavior test file?

@Amxx
Copy link
Collaborator Author

Amxx commented Feb 6, 2023

What do you think about adding an EIP-6372 behavior test file?

done

frangio
frangio previously approved these changes Feb 9, 2023
View reviewed changes
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

This is looking good to me.

Documentation still needs work but as discussed elsewhere we can do this in a follow up PR before release.

@Amxx
Merge branch 'master'
cc0f45f 
Conflicts:
	test/governance/Governor.test.js
	test/governance/extensions/GovernorTimelockCompound.test.js
	test/governance/extensions/GovernorTimelockControl.test.js
	test/governance/extensions/GovernorWithParams.test.js
	test/governance/utils/Votes.behavior.js
	test/token/ERC20/extensions/ERC20Votes.test.js
	test/token/ERC20/extensions/ERC20VotesComp.test.js
@Amxx
Update governance.js
e2eb048
@Amxx
Update ERC721Votes.test.js
5e54e88
@Amxx
improve coverage
60992b6
@Amxx
remove full IGovernor from ERC165
8397ce7
frangio
frangio approved these changes Feb 9, 2023
View reviewed changes
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

LGTM!

@frangio frangio changed the title Timestamp based governor with EIP-5805 Add timestamp based governor with EIP-6372 and EIP-5805 Feb 9, 2023
@Amxx Amxx merged commit 790cc5b into OpenZeppelin:master Feb 9, 2023
@Amxx Amxx deleted the feature/timestamp-governor branch February 9, 2023 21:34
@r0qs r0qs mentioned this pull request Feb 15, 2023
Closed
@frangio frangio mentioned this pull request Feb 24, 2023
Closed
@jonwalch
Copy link

jonwalch commented Mar 6, 2023

When is this getting released?

@Amxx
Copy link
Collaborator Author

Amxx commented Mar 7, 2023

@jonwalch in the next minor (4.9) in a few weeks.

@jonwalch
Copy link

jonwalch commented May 1, 2023

@jonwalch in the next minor (4.9) in a few weeks.

Hey @Amxx, any update on when 4.9 comes out? Thanks!

@frangio
Copy link
Contributor

frangio commented May 3, 2023

Next week. Sorry for the delays, we've taken a lot of time to test and audit.

@jonwalch
Copy link

jonwalch commented May 3, 2023

All good. Thanks for the update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ernestognw ernestognw ernestognw left review comments

@frangio frangio frangio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Timestamp-based Governor
4 participants
@Amxx @frangio @jonwalch @ernestognw