Merge pull request #117 from Bananapus/refactor/JBPermissions

refactor: optimize `JBPermissions` gas usage
Bananapus · Mar 23, 2024 · 1cb3d02 · 1cb3d02
2 parents 04cbd09 + a435fba
commit 1cb3d02
Show file tree

Hide file tree

Showing 2 changed files with 59 additions and 23 deletions.
diff --git a/src/JBPermissions.sol b/src/JBPermissions.sol
@@ -2,17 +2,17 @@
 pragma solidity 0.8.23;
 
 import {JBPermissionIds} from "@bananapus/permission-ids/src/JBPermissionIds.sol";
-import {JBPermissioned} from "./abstract/JBPermissioned.sol";
 import {IJBPermissions} from "./interfaces/IJBPermissions.sol";
 import {JBPermissionsData} from "./structs/JBPermissionsData.sol";
 
 /// @notice Stores permissions for all addresses and operators. Addresses can give permissions to any other address
 /// (i.e. an *operator*) to execute specific operations on their behalf.
-contract JBPermissions is JBPermissioned, IJBPermissions {
+contract JBPermissions is IJBPermissions {
     //*********************************************************************//
     // --------------------------- custom errors ------------------------- //
     //*********************************************************************//
     error PERMISSION_ID_OUT_OF_BOUNDS();
+    error UNAUTHORIZED();
 
     //*********************************************************************//
     // --------------------- public stored properties -------------------- //
@@ -47,7 +47,7 @@ contract JBPermissions is JBPermissioned, IJBPermissions {
         uint256 projectId,
         uint256 permissionId
     )
-        external
+        public
         view
         override
         returns (bool)
@@ -74,31 +74,25 @@ contract JBPermissions is JBPermissioned, IJBPermissions {
         override
         returns (bool)
     {
-        // Keep a reference to the number of permissions being iterated on.
-        uint256 numberOfPermissions = permissionIds.length;
-
         // Keep a reference to the permission being iterated on.
         uint256 permissionId;
 
-        for (uint256 i; i < numberOfPermissions; i++) {
+        // Keep a reference to the permission item being checked.
+        uint256 operatorAccountProjectPermissions = permissionsOf[operator][account][projectId];
+
+        for (uint256 i; i < permissionIds.length; i++) {
             // Set the permission being iterated on.
             permissionId = permissionIds[i];
 
             if (permissionId > 255) revert PERMISSION_ID_OUT_OF_BOUNDS();
 
-            if (((permissionsOf[operator][account][projectId] >> permissionId) & 1) == 0) {
+            if (((operatorAccountProjectPermissions >> permissionId) & 1) == 0) {
                 return false;
             }
         }
         return true;
     }
 
-    //*********************************************************************//
-    // -------------------------- constructor ---------------------------- //
-    //*********************************************************************//
-
-    constructor() JBPermissioned(this) {}
-
     //*********************************************************************//
     // ---------------------- external transactions ---------------------- //
     //*********************************************************************//
@@ -109,11 +103,11 @@ contract JBPermissions is JBPermissioned, IJBPermissions {
     /// @param permissionsData The data which specifies the permissions the operator is being given.
     function setPermissionsFor(address account, JBPermissionsData calldata permissionsData) external override {
         // Enforce permissions.
-        _requirePermissionFrom({
-            account: account,
-            projectId: permissionsData.projectId,
-            permissionId: JBPermissionIds.ROOT
-        });
+        if (
+            msg.sender != account
+                && !hasPermission(msg.sender, account, permissionsData.projectId, JBPermissionIds.ROOT)
+                && !hasPermission(msg.sender, account, 0, JBPermissionIds.ROOT)
+        ) revert UNAUTHORIZED();
 
         // Pack the permission IDs into a uint256.
         uint256 packed = _packedPermissions(permissionsData.permissionIds);
@@ -139,13 +133,10 @@ contract JBPermissions is JBPermissioned, IJBPermissions {
     /// @param permissionIds The IDs of the permissions to pack.
     /// @return packed The packed value.
     function _packedPermissions(uint256[] calldata permissionIds) internal pure returns (uint256 packed) {
-        // Keep a reference to the number of IDs being iterated on.
-        uint256 numberOfIds = permissionIds.length;
-
         // Keep a reference to the permission being iterated on.
         uint256 permissionId;
 
-        for (uint256 i; i < numberOfIds; i++) {
+        for (uint256 i; i < permissionIds.length; i++) {
             // Set the permission being iterated on.
             permissionId = permissionIds[i];
 

diff --git a/test/TestPermissions.sol b/test/TestPermissions.sol
@@ -132,6 +132,51 @@ contract TestPermissions_Local is TestBaseWorkflow {
         }
     }
 
+    function testHasPermissions(
+        address _account,
+        address _operator,
+        uint256 _projectId,
+        uint8[] memory _u8_check_permissions,
+        uint8[] memory _u8_set_permissions
+    )
+        public
+    {
+        uint256[] memory _check_permissions = new uint256[](_u8_check_permissions.length);
+        uint256[] memory _set_permissions = new uint256[](_u8_set_permissions.length);
+
+        // Check if all the items in `check_permissions` also exist in `set_permissions`.
+        bool _shouldHavePermissions = true;
+        for (uint256 _i; _i < _u8_check_permissions.length; _i++) {
+            bool _exists;
+            _check_permissions[_i] = _u8_check_permissions[_i];
+            for (uint256 _j; _j < _u8_set_permissions.length; _j++) {
+                // We update this lots of times unnecesarily but no need to optimize this.
+                _set_permissions[_j] = _u8_set_permissions[_j];
+                // If we find this item we break and mark the flag.
+                if (_u8_check_permissions[_i] == _u8_set_permissions[_j]) {
+                    _exists = true;
+                    break;
+                }
+            }
+
+            // If any item does not exist we should not have permission.
+            if (_exists == false) {
+                _shouldHavePermissions = false;
+                break;
+            }
+        }
+
+        // Set the permissions.
+        vm.prank(_account);
+        _permissions.setPermissionsFor(
+            _account, JBPermissionsData({operator: _operator, projectId: _projectId, permissionIds: _set_permissions})
+        );
+
+        assertEq(
+            _permissions.hasPermissions(_operator, _account, _projectId, _check_permissions), _shouldHavePermissions
+        );
+    }
+
     /* function testBasicAccessSetup() public {
         vm.prank(address(_projectOwner));
         bool _check = _permissions.hasPermission(address(_projectOwner), address(_projectOwner), 0, 2);