Update documentation in IAccessControl #4924
Conversation
… with latest logic on AccessControl
|
|
I removed the changeset because this is not something we'd like to document in the changelog. |
ernestognw
changed the title
|
Understood, and thanks for reviewing! |
contracts/access/IAccessControl.sol
Outdated
| @@ -30,8 +30,7 @@ interface IAccessControl { | |||
| /** | |||
| * @dev Emitted when `account` is granted `role`. | |||
| * | |||
| * `sender` is the account that originated the contract call, an admin role | |||
| * bearer except when using {AccessControl-_setupRole}. | |||
| * `sender` is the account that originated the contract call i.e. the admin role bearer. | |||
There was a problem hiding this comment.
When the role is granted using the internal _grantRole (for example during construction, or through any other process), then the msg.sender may not bear the admin rights for the role that is granted.
There was a problem hiding this comment.
Oh right, my bad, so it should be
`sender` is the account that originated the contract call, an admin role
bearer except when using {AccessControl-_grantRole}.
There was a problem hiding this comment.
* `sender` is the account that originated the contract call, an admin role
* bearer except when using {AccessControl-_grantRole}.
works.
| * `sender` is the account that originated the contract call i.e. the admin role bearer. | |
| * `sender` is the account that originated the contract call. This account bears the admin role (for the granted | |
| * role), expect in cases where the role was granted using the internal {AccessControl-_grantRole}. |
works as well.
There was a problem hiding this comment.
If that's the case, should we update the description for the event RoleRevoked as well then?
/**
* @dev Emitted when `account` is revoked `role`.
*
* `sender` is the account that originated the contract call:
* - if using `revokeRole`, it bears the admin role (for the granted role)
* - if using `renounceRole`, it is the role bearer (i.e. `account`)
*/
event RoleRevoked(bytes32 indexed role, address indexed account, address indexed sender);
There was a problem hiding this comment.
I think the current description of RoleRevoked is fine. Given there are already contracts out there that may grant roles without emitting RoleGranted, it justifies clarifying only for that case.
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Yes, more precise. Thanks! |
Amxx
changed the title
| @@ -30,8 +30,8 @@ interface IAccessControl { | |||
| /** | |||
| * @dev Emitted when `account` is granted `role`. | |||
| * | |||
| * `sender` is the account that originated the contract call. This account bears the admin role (for the granted | |||
| * role), expect in cases where the role was granted using the internal {AccessControl-_grantRole}. | |||
| * `sender` is the account that originated the contract call. This account bears the admin role (for the granted role). | |||
There was a problem hiding this comment.
that line is 124 chars, should not pass linting.
There was a problem hiding this comment.
Interesting, I ran npm lint:fix before pushing.
There was a problem hiding this comment.
could be another issue to look into...
Fixes #4923
As mentioned, this is to fix the natspec comment of IAccessControl on "RoleGranted" event to confirm with latest logic on AccessControl.
PR Checklist
npx changeset add)