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

Cleanup timelockId on execution for gas refund #4118

Merged

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Mar 14, 2023

When calling execute, the state is checked. We expect to be in Queued. That means the state call done by execute is going to read _timelockIds[proposalId] and check the operation status. This sload makes the slot "hot".

As a consequence, cleaning it will cost 2900 and refund 4800, resulting in a theoretical 1900 gas savings. Because of the hash necessary to compute storage slot, this saving is reduced to 1825, which is still "nice to have".

  • This does not change the behavior of the state(uint256) function
    • execute() sets the executed flag, which means the state will be returns without even looking at _timelockIds[proposalId]
  • This does not change the behavior of the proposalEta(uint256) function
    • before this change, calling proposalEta on an executed proposal would lookup the deadline on the timelock. That returns _DONE_TIMESTAMP (1) which is rewritten to 0 by the governor
    • after this change, calling proposalEta on an executed proposal would lookup the deadline of 0x0 on the timelock, which should be 0.

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@changeset-bot
Copy link

changeset-bot bot commented Mar 14, 2023

🦋 Changeset detected

Latest commit: 830920a

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 marked this pull request as ready for review March 14, 2023 09:32
@Amxx Amxx requested a review from frangio March 14, 2023 09:32
@Amxx
Copy link
Collaborator Author

Amxx commented Mar 14, 2023

Capture d’écran du 2023-03-14 11-41-50

@Amxx
Copy link
Collaborator Author

Amxx commented Mar 14, 2023

With the reordering
Capture d’écran du 2023-03-14 12-13-27

@frangio
Copy link
Contributor

frangio commented Jun 15, 2023

This looks good, but there is a conflict that needs to be resolved.

@frangio frangio added this to the 5.0 milestone Jun 15, 2023
This was referenced Sep 10, 2024
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.

2 participants