Contract Clock for Votes

[immrsd]

Fixes #1242

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 93.11%. Comparing base (cbfda3c) to head (8d575cb). Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1417      +/-   ##
==========================================
+ Coverage   93.00%   93.11%   +0.10%     
==========================================
  Files          80       82       +2     
  Lines        2230     2251      +21     
==========================================
+ Hits         2074     2096      +22     
+ Misses        156      155       -1     

Files with missing lines	Coverage Δ
...nance/src/governor/extensions/governor_votes.cairo	88.23% <100.00%> (+6.41%)	:arrow_up:
...or/extensions/governor_votes_quorum_fraction.cairo	100.00% <100.00%> (ø)
packages/governance/src/governor/governor.cairo	92.54% <ø> (ø)
packages/governance/src/governor/interface.cairo	100.00% <ø> (ø)
.../governance/src/timelock/timelock_controller.cairo	81.37% <100.00%> (+0.12%)	:arrow_up:
packages/governance/src/votes/interface.cairo	100.00% <ø> (ø)
packages/governance/src/votes/votes.cairo	91.93% <100.00%> (+2.28%)	:arrow_up:
...ckages/utils/src/contract_clock/block_number.cairo	100.00% <100.00%> (ø)
packages/utils/src/contract_clock/timestamp.cairo	100.00% <100.00%> (ø)

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cbfda3c...8d575cb. Read the comment docs.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ericnordelo]

Hey @immrsd. Can you please format the files and go through the pending previous review comments before I give this a second pass? Thanks!

[immrsd]

@ericnordelo Would be great to hear your thoughts on that!

1. Had to support ERC-6372 in the TImelock component as well, since it's used in GovernorTimelockExecution

• Decided to keep the storage var's name TimelockController_timestamps as it is so that the storage layout stays consistent
• I'm leaning towards also keeping get_timestamp function name (rather than renaming it to get_timepoint) to avoid breaking already integrated contracts. The changes in behaviour will be clarified in doc

2. It made me think of another issue, what if the Governor component wants to use a ERC20/ERC721 token implementing the old version of Votes component? It won't be possible because of clock() calls to the token will fail due to entrypoint-not-found error. Same problem with old version of the Timelock controller being integrated with Governor. Should we use fallback approach and assume that if a token/timelock doesn't implement the clock function, then it must be using timestamp under the hood? I believe it might be the right approach in this case

[ericnordelo]

Great job @immrsd. The PR looks great, the only thing as I mention below is that adding clock_mode to Timelock as well is not a requirement for this issue, so let's roll it back and potentially address it in the future.

Had to support ERC-6372 in the TImelock component as well, since it's used in GovernorTimelockExecution

For the sake of simplicity and keeping things organized, let's follow the same approach we are following in the Solidity library, being not allowing to choose block numbers in the timelock controller (as seen here). We needed to update VotesComponent to allow block numbers in Governor which implements ClockMode by default, but TimelockController doesn't implement it, and if we were to add it, let's do it in another issue.

It made me think of another issue, what if the Governor component wants to use a ERC20/ERC721 token implementing the old version of Votes component? It won't be possible because of clock() calls to the token will fail due to entrypoint-not-found error. Same problem with old version of the Timelock controller being integrated with Governor. Should we use fallback approach and assume that if a token/timelock doesn't implement the clock function, then it must be using timestamp under the hood? I believe it might be the right approach in this case

Since this will be released as a breaking version, users should be aware that they shouldn't upgrade from the previous one, unless they use an appropriate migration path. With this said, let's be loud about the incompatibility by adding both an entry in the component comment and the docsite, explaining why this version is not compatible with 2.0.0 (it will be 3.0.0)