diff --git a/.changeset/two-wasps-punch.md b/.changeset/two-wasps-punch.md new file mode 100644 index 000000000..d382ab6e9 --- /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. diff --git a/contracts/access/AccessControl.sol b/contracts/access/AccessControl.sol index cb7650806..a2be30de3 100644 --- a/contracts/access/AccessControl.sol +++ b/contracts/access/AccessControl.sol @@ -174,30 +174,36 @@ 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. * * 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; } } /** - * @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. * * 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/extensions/AccessControlDefaultAdminRules.sol b/contracts/access/extensions/AccessControlDefaultAdminRules.sol index dc4dbfbcf..454608407 100644 --- a/contracts/access/extensions/AccessControlDefaultAdminRules.sol +++ b/contracts/access/extensions/AccessControlDefaultAdminRules.sol @@ -130,24 +130,24 @@ 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) { if (defaultAdmin() != address(0)) { revert AccessControlEnforcedDefaultAdminRules(); } _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/extensions/AccessControlEnumerable.sol b/contracts/access/extensions/AccessControlEnumerable.sol index f07aca637..707c5759b 100644 --- a/contracts/access/extensions/AccessControlEnumerable.sol +++ b/contracts/access/extensions/AccessControlEnumerable.sol @@ -47,18 +47,24 @@ 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 { - 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 + * @dev Overload {AccessControl-_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; } } diff --git a/test/access/AccessControl.behavior.js b/test/access/AccessControl.behavior.js index 20804f049..cc3e8d63f 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) {