-
Notifications
You must be signed in to change notification settings - Fork 193
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: add AccessControl to managing ACL in WQ #487
Conversation
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, except for a few comments.
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 for testnets 👍
Though, left some notes on the fields for the future.
bytes32 internal constant REQUESTS_PLACEMENT_RESUMED_POSITION = | ||
keccak256("lido.WithdrawalQueue.requestsPlacementResumed"); | ||
/// Withdrawal queue resume/pause control storage slot | ||
bytes32 internal constant RESUMED_POSITION = keccak256("lido.WithdrawalQueue.resumed"); | ||
|
||
/// Lido stETH token address to be set upon construction | ||
address public immutable STETH; |
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.
Should be placed in unstructured storage eventually
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.
Should it? Not sure that it can be changed in a some way.
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 to merge 👍
Add role-based access control to WithdrawalQueue
There are three roles:
and pause now is for finalization also