From c41f431adee6312f61b87ca0784b0db0dcfc7a74 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 17 May 2023 09:33:15 +0200 Subject: [PATCH 1/5] Add a bool return to _grantRole and _revokeRole --- contracts/access/AccessControl.sol | 10 ++++++++-- .../access/AccessControlDefaultAdminRules.sol | 8 ++++---- contracts/access/AccessControlEnumerable.sol | 18 ++++++++++++------ 3 files changed, 24 insertions(+), 12 deletions(-) diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol index 3a73de78b55..6ed3dc5752b 100644 --- a/contracts/access/AccessControl.sol +++ b/contracts/access/AccessControl.sol @@ -225,10 +225,13 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { * * May emit a {RoleGranted} event. */ - function _grantRole(bytes32 role, address account) internal virtual { + function _grantRole(bytes32 role, address account) internal virtual returns (bool) { if (!hasRole(role, account)) { _roles[role].members[account] = true; emit RoleGranted(role, account, _msgSender()); + return true; + } else { + return false; } } @@ -239,10 +242,13 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { * * May emit a {RoleRevoked} event. */ - function _revokeRole(bytes32 role, address account) internal virtual { + function _revokeRole(bytes32 role, address account) internal virtual returns (bool) { if (hasRole(role, account)) { _roles[role].members[account] = false; emit RoleRevoked(role, account, _msgSender()); + return true; + } else { + return false; } } } diff --git a/contracts/access/AccessControlDefaultAdminRules.sol b/contracts/access/AccessControlDefaultAdminRules.sol index 07a5b4f7095..b8a438ce64a 100644 --- a/contracts/access/AccessControlDefaultAdminRules.sol +++ b/contracts/access/AccessControlDefaultAdminRules.sol @@ -126,22 +126,22 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu * NOTE: Exposing this function through another mechanism may make the `DEFAULT_ADMIN_ROLE` * assignable again. Make sure to guarantee this is the expected behavior in your implementation. */ - function _grantRole(bytes32 role, address account) internal virtual override { + function _grantRole(bytes32 role, address account) internal virtual override returns (bool) { if (role == DEFAULT_ADMIN_ROLE) { require(defaultAdmin() == address(0), "AccessControl: default admin already granted"); _currentDefaultAdmin = account; } - super._grantRole(role, account); + return super._grantRole(role, account); } /** * @dev See {AccessControl-_revokeRole}. */ - function _revokeRole(bytes32 role, address account) internal virtual override { + function _revokeRole(bytes32 role, address account) internal virtual override returns (bool) { if (role == DEFAULT_ADMIN_ROLE && account == defaultAdmin()) { delete _currentDefaultAdmin; } - super._revokeRole(role, account); + return super._revokeRole(role, account); } /** diff --git a/contracts/access/AccessControlEnumerable.sol b/contracts/access/AccessControlEnumerable.sol index 354e1bed298..924c9d5148b 100644 --- a/contracts/access/AccessControlEnumerable.sol +++ b/contracts/access/AccessControlEnumerable.sol @@ -49,16 +49,22 @@ abstract contract AccessControlEnumerable is IAccessControlEnumerable, AccessCon /** * @dev Overload {_grantRole} to track enumerable memberships */ - function _grantRole(bytes32 role, address account) internal virtual override { - super._grantRole(role, account); - _roleMembers[role].add(account); + function _grantRole(bytes32 role, address account) internal virtual override returns (bool) { + bool granted = super._grantRole(role, account); + if (granted) { + _roleMembers[role].add(account); + } + return granted; } /** * @dev Overload {_revokeRole} to track enumerable memberships */ - function _revokeRole(bytes32 role, address account) internal virtual override { - super._revokeRole(role, account); - _roleMembers[role].remove(account); + function _revokeRole(bytes32 role, address account) internal virtual override returns (bool) { + bool revoked = super._revokeRole(role, account); + if (revoked) { + _roleMembers[role].remove(account); + } + return revoked; } } From 0efefd878f3cdac7a69b4dd16f0cc0da5da66970 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 6 Jul 2023 13:36:44 -0600 Subject: [PATCH 2/5] Add changeset --- .changeset/two-wasps-punch.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/two-wasps-punch.md diff --git a/.changeset/two-wasps-punch.md b/.changeset/two-wasps-punch.md new file mode 100644 index 00000000000..d382ab6e968 --- /dev/null +++ b/.changeset/two-wasps-punch.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`AccessControl`: Add a boolean return value to the internal `_grantRole` and `_revokeRole` functions indicating whether the role was granted or revoked. From 47d03450111b97f8c7fea634b3d65ff9d171a385 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 7 Jul 2023 17:18:48 +0200 Subject: [PATCH 3/5] add tests --- test/access/AccessControl.behavior.js | 30 +++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 20804f04990..cc3e8d63f1b 100644 --- a/test/access/AccessControl.behavior.js +++ b/test/access/AccessControl.behavior.js @@ -191,6 +191,36 @@ function shouldBehaveLikeAccessControl(admin, authorized, other, otherAdmin) { ); }); }); + + describe('internal functions', function () { + describe('_grantRole', function () { + it('return true if the account does not have the role', async function () { + const receipt = await this.accessControl.$_grantRole(ROLE, authorized); + expectEvent(receipt, 'return$_grantRole', { ret0: true }); + }); + + it('return false if the account has the role', async function () { + await this.accessControl.$_grantRole(ROLE, authorized); + + const receipt = await this.accessControl.$_grantRole(ROLE, authorized); + expectEvent(receipt, 'return$_grantRole', { ret0: false }); + }); + }); + + describe('_revokeRole', function () { + it('return true if the account has the role', async function () { + await this.accessControl.$_grantRole(ROLE, authorized); + + const receipt = await this.accessControl.$_revokeRole(ROLE, authorized); + expectEvent(receipt, 'return$_revokeRole', { ret0: true }); + }); + + it('return false if the account does not have the role', async function () { + const receipt = await this.accessControl.$_revokeRole(ROLE, authorized); + expectEvent(receipt, 'return$_revokeRole', { ret0: false }); + }); + }); + }); } function shouldBehaveLikeAccessControlEnumerable(admin, authorized, other, otherAdmin, otherAuthorized) { From 917d743ce48f7c3b7916bfd3f2c412a94e744022 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 7 Jul 2023 17:21:20 +0200 Subject: [PATCH 4/5] update natspec comments --- contracts/access/AccessControl.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol index b038ec74f68..6fd122ce35f 100644 --- a/contracts/access/AccessControl.sol +++ b/contracts/access/AccessControl.sol @@ -184,7 +184,7 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { } /** - * @dev Grants `role` to `account`. + * @dev Attempts to grant `role` to `account` and returns a boolean indicating if `role` was granted. * * Internal function without access restriction. * @@ -201,7 +201,7 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 { } /** - * @dev Revokes `role` from `account`. + * @dev Attempts to revoke `role` to `account` and returns a boolean indicating if `role` was revoked. * * Internal function without access restriction. * From 3939cb4064c54c417b6e9ab7d3fb54ea74fe1166 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 7 Jul 2023 17:21:41 +0200 Subject: [PATCH 5/5] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ernesto García --- contracts/access/AccessControlEnumerable.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/access/AccessControlEnumerable.sol b/contracts/access/AccessControlEnumerable.sol index 0633ecb92e5..4c0dae492bb 100644 --- a/contracts/access/AccessControlEnumerable.sol +++ b/contracts/access/AccessControlEnumerable.sol @@ -47,7 +47,7 @@ abstract contract AccessControlEnumerable is IAccessControlEnumerable, AccessCon } /** - * @dev Overload {_grantRole} to track enumerable memberships + * @dev Overload {AccessControl-_grantRole} to track enumerable memberships */ function _grantRole(bytes32 role, address account) internal virtual override returns (bool) { bool granted = super._grantRole(role, account); @@ -58,7 +58,7 @@ abstract contract AccessControlEnumerable is IAccessControlEnumerable, AccessCon } /** - * @dev Overload {_revokeRole} to track enumerable memberships + * @dev Overload {AccessControl-_revokeRole} to track enumerable memberships */ function _revokeRole(bytes32 role, address account) internal virtual override returns (bool) { bool revoked = super._revokeRole(role, account);