-
Notifications
You must be signed in to change notification settings - Fork 355
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
Implement restriction to allow limiting chain admin in power #699
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.
Very nice! I think we can also add roles
to the contracts so instead of requiring onlyOwner
to be the caller, we can pass msg.sender
to restriction module contract and it will check whether such a role can do what it wants to do.
Changes to gas cost
🧾 Summary (100% most significant diffs)
Full diff report 👇
|
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 a final review
@StanislavBreadless Perhaps you should change the base branch to main as this is a change we can quickly roll out without need to do the proper upgrade. |
Co-authored-by: Vlad Bochok <41153528+vladbochok@users.noreply.github.com>
Co-authored-by: Vlad Bochok <41153528+vladbochok@users.noreply.github.com>
Co-authored-by: Vlad Bochok <41153528+vladbochok@users.noreply.github.com>
Co-authored-by: Vlad Bochok <41153528+vladbochok@users.noreply.github.com>
Co-authored-by: Vlad Bochok <41153528+vladbochok@users.noreply.github.com>
3e00834
to
d39081f
Compare
Co-authored-by: Vlad Bochok <41153528+vladbochok@users.noreply.github.com>
Co-authored-by: Vlad Bochok <41153528+vladbochok@users.noreply.github.com>
…limited-chain-amdin
What ❔
Why ❔
Checklist