Fix/roles revamp #1772 #1889
Fix/roles revamp #1772 #1889
Conversation
|
Hi all! |
|
Hello @alanarvelo! Sorry for taking so long to review this, I try to give feedback as early as possible and really dropped the ball here. This is quite a big feature that we want to get right, so progress may not be super fast :)
We should definitely keep the contracts and the current interface (see our API stability guideline. It feels like it should be possible to do a light rewrite using this new paradigm while keeping the external interface intact, but I guess we won't know until we try :)
No, I simply tried to convey the fact that they are 256-bit ids. If |
There was a problem hiding this comment.
Thanks for your work @alanarvelo! I left a couple high-level comments, no point in nit-picking or writing tests until we get the generalities right :)
|
|
||
| function roleExists(uint256 _roleId) internal view returns (bool) { return roles[_roleId].exists; } | ||
|
|
||
| function newRole() internal returns (uint256) { |
There was a problem hiding this comment.
We may want to add a string here, to help with debugging
| _; | ||
| } | ||
|
|
||
| function roleExists(uint256 _roleId) internal view returns (bool) { return roles[_roleId].exists; } |
There was a problem hiding this comment.
Interesting, I had not considered this! Can you think of use cases where someone might want to check if a role exists?
| @@ -31,6 +32,7 @@ library Roles { | |||
| */ | |||
| function has(Role storage role, address account) internal view returns (bool) { | |||
| require(account != address(0), "Roles: account is the zero address"); | |||
| // require(role.exists, "Roles: role does not exists");git | |||
There was a problem hiding this comment.
The fact that this check is not really necessary makes me think that perhaps having .exists is not the best way to go: it is possible to misuse the Roles struct by forgetting to set this field.
|
Hi all! |
|
Hi folks! |
|
We're definitely interested in getting this merged! If @alanarvelo is unable to continue the PR, is there anyone else that might take up development? |
|
Hi all! |
|
Putting this on hold until we have time to dedicate to it. |
frangio
added
the
on hold
|
This PR was superceded by #2112 - sorry to have kept it open for so long. |
Hi all,
This is a first pass at the Issue Proposal to revamp Roles #1772. Merely an implementation of the
RoleChecker.solcontract, whose intellectual author is @nventuro. Actual repo functionality was not altered. Made a PR to facilitate the discussion over the implementation details.A few questions:
RoleChecker.solaccurate and viable?/access/rolese.g. CapperRole, MinterRole, PauserRole, etc.? Should they be refactored to abide by the new paradigm as depicted by @nventuro in issue #1772, or should those contracts be deleted and all roles & functionality be defined where they are needed/used?roleIds— hashes of address + counter — to be typeuint256instead of the return type of keccak256bytes32?TO-DO:
Looking forward to continue working on this!