-
Notifications
You must be signed in to change notification settings - Fork 358
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
Clean up and small refactors to the access module #1221
Conversation
Co-authored-by: Eric Nordelo <[email protected]>
Co-authored-by: Eric Nordelo <[email protected]>
Co-authored-by: Eric Nordelo <[email protected]>
Co-authored-by: Eric Nordelo <[email protected]>
Should we update the Changelog to reflect any of these changes? I think ony the change in the initializer is worth calling out maybe |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1221 +/- ##
==========================================
+ Coverage 92.06% 92.12% +0.06%
==========================================
Files 49 49
Lines 1399 1397 -2
==========================================
- Hits 1288 1287 -1
+ Misses 111 110 -1
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
Looking good! I left a few minor doc suggestions
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.
Looking good! The only missing things I see:
- Updating the doc site accordingly
- In line 69:
{RoleAdminChanged}
should use backticks instead.
All comments have been addressed and changelog has been updated. |
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.
Looking good, left a couple of 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.
I left a final suggestion. Otherwise, LGTM!
Co-authored-by: Andrew Fleming <[email protected]>
Add comments where missing
Small refactors
Remove a check in Ownable that verifies that sender is the zero address
Add a check to Ownable initializer that the owner is not being set to the zero address
Tests
Added entry to CHANGELOG.md