Access control facet and lib#20
Conversation
|
Thank you, I will be reviewing this. We probably want to change "assert" to require because "assert" means something else in Solidity. @0xlynett please take a look at this too. |
|
There's a lot of Also, the storage struct is called LibERC165Storage and the storage position is Thanks! (I just remembered that the Ownable library lacks a requireOwner function as well, rip. I'll fix that later) |
|
Also the RoleData struct feels like it could just be split into two mappings such as _roleAdmin and _hasRole |
|
Oh yeah, I headed to a diff direction at the start. Noted, will do these changes. |
|
gm @mudgen @0xlynett, I have refactored the implementation as recommended
|
|
@Rushikesh0125, thanks I am looking at it |
There was a problem hiding this comment.
@Rushikesh0125 Looks good. Just some styling issues to match the rest of the code base.
|
@Rushikesh0125 How compatible is this with OpenZeppelin's version? I am unsure if compatibility matters much with this functionality, but maybe, do you have an idea? Are there any ERC standards that cover this functionality? |
|
@mudgen I've followed the same approach as Oz's implementation, with the addition of the recommended |
…low standard compose practices
|
@mudgen, I have made the required changes. |
panditdhamdhere
left a comment
There was a problem hiding this comment.
great work @Rushikesh0125
Coverage Report
Last updated: Sun, 26 Oct 2025 15:11:29 GMT for commit |
|
Good job! |
Access control facet and lib
Implemented access control facet and library