From c38adddc3cb8cb47bd4f6de1f0b76b66566ef4cf Mon Sep 17 00:00:00 2001 From: 0xBA5ED <83727748+xBA5ED@users.noreply.github.com> Date: Thu, 21 Mar 2024 22:31:05 +0100 Subject: [PATCH 1/2] refactor: Optimize JBPermissions gas usage --- src/JBPermissions.sol | 36 +++++++++++------------------- test/TestPermissions.sol | 47 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 23 deletions(-) diff --git a/src/JBPermissions.sol b/src/JBPermissions.sol index de13f959..320db633 100644 --- 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,10 @@ 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 +132,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 index a9e429d6..09cdd361 100644 --- a/test/TestPermissions.sol +++ b/test/TestPermissions.sol @@ -132,6 +132,53 @@ 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); From a435fbae42942b1d171e83b4172fb51dba128d76 Mon Sep 17 00:00:00 2001 From: 0xBA5ED <83727748+xBA5ED@users.noreply.github.com> Date: Thu, 21 Mar 2024 22:55:36 +0100 Subject: [PATCH 2/2] style: forge fmt --- src/JBPermissions.sol | 3 ++- test/TestPermissions.sol | 26 ++++++++++++-------------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/JBPermissions.sol b/src/JBPermissions.sol index 320db633..a5b619bd 100644 --- a/src/JBPermissions.sol +++ b/src/JBPermissions.sol @@ -104,7 +104,8 @@ contract JBPermissions is IJBPermissions { function setPermissionsFor(address account, JBPermissionsData calldata permissionsData) external override { // Enforce permissions. if ( - msg.sender != account && !hasPermission(msg.sender, account, permissionsData.projectId, JBPermissionIds.ROOT) + msg.sender != account + && !hasPermission(msg.sender, account, permissionsData.projectId, JBPermissionIds.ROOT) && !hasPermission(msg.sender, account, 0, JBPermissionIds.ROOT) ) revert UNAUTHORIZED(); diff --git a/test/TestPermissions.sol b/test/TestPermissions.sol index 09cdd361..6975cb74 100644 --- a/test/TestPermissions.sol +++ b/test/TestPermissions.sol @@ -138,27 +138,29 @@ contract TestPermissions_Local is TestBaseWorkflow { uint256 _projectId, uint8[] memory _u8_check_permissions, uint8[] memory _u8_set_permissions - ) public { + ) + 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++) { + 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++) { + _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]){ + 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) { + if (_exists == false) { _shouldHavePermissions = false; break; } @@ -166,16 +168,12 @@ contract TestPermissions_Local is TestBaseWorkflow { // Set the permissions. vm.prank(_account); - _permissions.setPermissionsFor(_account, JBPermissionsData({ - operator: _operator, - projectId: _projectId, - permissionIds: _set_permissions - })); - + _permissions.setPermissionsFor( + _account, JBPermissionsData({operator: _operator, projectId: _projectId, permissionIds: _set_permissions}) + ); assertEq( - _permissions.hasPermissions(_operator, _account, _projectId, _check_permissions), - _shouldHavePermissions + _permissions.hasPermissions(_operator, _account, _projectId, _check_permissions), _shouldHavePermissions ); }