Add a bool return to _grantRole and _revokeRole (#4241)

Co-authored-by: Ernesto García <ernestognw@gmail.com>
OpenZeppelin · Aug 1, 2023 · 5ae6306 · 5ae6306
1 parent 48cc8a9
commit 5ae6306
Show file tree

Hide file tree

Showing 5 changed files with 63 additions and 16 deletions.
diff --git a/.changeset/two-wasps-punch.md 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
@@ -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
@@ -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
@@ -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
@@ -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) {