-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
Make AccessManager.execute/schedule more conservative when delay is 0 #4644
Conversation
(cherry picked from commit 42e8a1a)
🦋 Changeset detectedLatest commit: 1872a4e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM + minor comment
@@ -0,0 +1,5 @@ | |||
--- | |||
'openzeppelin-solidity': major |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's major, isn't a patch although we're in pre-release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, wasn't sure what to choose. In theory this is a breaking change, that's why I chose major. I don't think it will make any difference to the release process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok make sense, also make sure it'll be picked up by the changeset versioning, which I think it will
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
The skip wasn't correctly written. |
This PR makes the
schedule
function revert when the caller does not have an enforced delay. Note that this doesn't prevent a situation where a caller is able to schedule a call but later has their delay removed. To make that case more consistent with the normal schedule/execute case, this PR also makesschedule
consume an available schedule even when the caller doesn't have a currently enforced delay, and correctly return the corresponding nonce which is important forGovernorTimelockAccess
.Cherry picked from #4613, where tests were adjusted.