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

Feat: locking l2 upgrade (DON'T MERGE) #542

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

baroooo
Copy link
Contributor

@baroooo baroooo commented Nov 18, 2024

Description

Updates Locking Contracts to handle upcoming block.time change

Mainly changes the roundTimestamp() to have a forked implementation that uses different values to calculate week no for the blocks before and after l2 transition. Week numbers that return from roundTimestamp before and after should be continues and consistent.

Other changes

No

Tested

Unit and fork tests

Related issues

@baroooo
Copy link
Contributor Author

baroooo commented Nov 20, 2024

I think the ci is failing because they removed regex parsing for version pragma in a recent foundry upgrade which is causing similar problems for other people. It also fails for dev branch at the moment.

It should be fixed once this change is released:
foundry-rs/compilers#223

edit: they are fast :D the fix is released

Copy link
Contributor

@philbow61 philbow61 left a comment

Choose a reason for hiding this comment

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

LGTM nice work on testing this.
1 thing can we add a test in the fork tests verifying pausing the locking contract results in
in Governance operations are being blocked:

  • propose
  • voting

@nvtaveras nvtaveras requested review from bowd and removed request for nvtaveras November 22, 2024 16:15
Copy link
Contributor

@bowd bowd left a comment

Choose a reason for hiding this comment

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

Great work 🙏 😍 - have only cosmetic comments, and more meant as a discussion, so let me know your thoughts.

contracts/governance/locking/LockingBase.sol Outdated Show resolved Hide resolved
contracts/governance/locking/LockingBase.sol Outdated Show resolved Hide resolved
contracts/governance/locking/LockingBase.sol Outdated Show resolved Hide resolved
test/fork/upgrades/LockingUpgradeForkTest.sol Show resolved Hide resolved
@baroooo baroooo requested a review from bowd November 27, 2024 12:48
@baroooo baroooo changed the title Feat: locking l2 upgrade Feat: locking l2 upgrade (DON'T MERGE) Nov 27, 2024
Copy link
Contributor

@bowd bowd left a comment

Choose a reason for hiding this comment

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

Love it, way more readable now!

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.

Update Locking to handle upcoming block.time change
4 participants