Enable timestamp-based governor with choice of timer type

[ericnordelo]

Fixes #3081

PR Checklist

• [x] Tests
• [ ] Documentation
• [ ] Changelog entry

[ericnordelo]

@Amxx

I'm using your Governance implementation, and I want to check proposal deadlines with Timestamps instead of Block Numbers.

Currently, you have a hardcoded Timer.BlockNumber in the ProposalCore struct inside the Governor contract, and I made some research through your implementation checking how to change this behavior. In this search I found that you have already a Timer for timestamps with the same interface as block numbers in the Timers library, so changing the ProposalCore struct to Timers.Timestamp achieves what I want.

The issue is that in order to change this struct, right now I would need to define another Governor contract and fix the inheritance tree (to redefine the struct), or override a lot of functions to fix the usage of ProposalCore with a new one (struct), I think that is too complicated for the purpose of the library.

So I propose to use a Generic Timer inside the Timer library that allows us to define the behavior depending on some configurations, specifically in a virtual method in Governor called timerType (added to the interface to get the current Timer used on the Governance), which can be easily overridden to select what Timer we want to use, and we could add also a config in the GovernorSettings module to allow swapping between this types in production. I made a PR for this and is linked to this issue.

[Amxx]

Hello @ericnordelo, and thank you for this very interesting proposal.

Block number is something we inherited from governor bravo compatibility, and IMO timestamp would be better. Your approach is really interesting, and I'd definitely want to explore it deeper.

One thing however, it that we realized that using Timers structure was a bad idea because it forces memory alignment. In this case, even though

Timers.Generic voteStart;
Timers.Generic voteEnd;
bool executed;
bool canceled;

use a total of 8 + 8 + 1 + 1 = 18 bytes ... they are not packed into a single storage slot (of 32 bytes) because the structure forces a new slot to be used. I'd like to fix that using UDVT at some point.

On one hand, it would make sense to do both changes at once, but since the transition to UDVT would be breaking, we would want to wait for 5.0. Maybe your proposal should make its way into the code before we do this breaking change.

@frangio, any thought ?

[ericnordelo]

Got it, makes total sense, just would be nice to get this Idea of Generic as soon as possible because now the workaround that we have is copying the Governor contract to modify the ProposalCore struct, and then copying the rest of the modules to modify the Governor inheritance (or add a lot of overrides in the Governance contract for each one of the module's methods using the Governor).

[frangio]

I think this is an elegant solution that makes sense, and doesn't add significant overhead.

I don't think we should offer a timestamp-based version out of the box until it's clearer how it interacts with the Governor ecosystem. But we can include this mechanism for switching to timestamps, and inform people of these potential compatibility issues, but give them the choice.

[frangio]

@Amxx I think this is unrelated to moving to UDVT. The storage layout in this PR is compatible, right?

[Amxx]

It is compatible with the current storage layout ... that we may break when we move to UDVT (which I should optimize that whenever we get to 5.0).

Really this PR is mostly good. I don't like the fact that it is not backward compatible from an ERC165 point of view, but if we fix that, then it should be good to merge!

[tarrencev]

Very nice! This would enable govornor to work on blockchains with irregular blocktimes, like optimism. This current implementation will break ERC20Votes though. We will still need to store a block number for the snapshot block.

[TripleD1337]

Hey guys, I would like to contribute and wanted things to clarify. Next steps would be to find a fitting solution for defining deadlines and generating a decent snapshot?

[Amxx]

AFAIK, next steps are to figure out how we want the token <> governor interaction to include info about the timestamps vs blocknumbers ... and standardize that interface.

I started working on an ERC for that ... then moved on to other priorities. I should resume working on that soon.

[z0r0z]

I completely support this initiative and hope to see an OZ governor templates that can support tokens that use timestamp checkpoints for better multichain compatibility. This would allow OZ to better integrate with Kali and Baal DAO voting token implementations.

[frangio]

@z0r0z I see that Baal has implemented this by defining getPriorVotes(address account, uint256 timeStamp). In our case we have concerns with this simple approach because it leads to a possible error where a timestamp-driven ERC20Votes is connected to a block number-driven Governor contract, or vice versa. So the key challenge for us is to make sure that the Governor, ERC20Votes, and any other off- or on-chain infrastructure is properly coordinated in terms of what measure of time they're using.

In any case, this issue is among our priorities for Q4 so we are going to find a solution for that concern and will be sharing updates here.

[TtheBC01]

Hey everyone, just wondering if there was currently any timeline regarding this improvement making it to the OZ main repository? Would really like to use it in an upcoming project.

[frangio]

Closing in favor of https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3934. This is our approach to this problem while maintaining backwards compatibility and minimizing the chance of setting up a system with mixed timer types.