perf(contract): Optimizations and clean ups (#637)

Replaced internal `IERC165` imports with those from OpenZeppelin, and renamed a local interface for better clarity. Additionally, refactored `IntrospectionHelper` contract initialization to improve code readability and consistency. Refactor internal deploy functions to use memory-safe assembly and add explicit reverts on failure. Integrate `LibClone` for address prediction to improve clarity and reliability. Refactor loops to improve performance by reducing redundant evaluations and increasing readability. Added `unchecked` blocks for vote counting to minimize gas usage. Streamlined the fetching of rule data directly within return statements. Modified function parameters to use `calldata` over `memory` for better gas efficiency. This change impacts functions in both `Architect.sol` and `ArchitectBase.sol` enhancing performance by reducing the cost of memory allocation. Changed the method to use `membership.settings` directly, significantly reducing code redundancy. Also ensured `feeRecipient` defaults to `msg.sender` if unset, maintaining intended functionality while improving code readability.
river-build · Aug 7, 2024 · aa6719e · aa6719e
1 parent aa4997f
commit aa6719e
Show file tree

Hide file tree

Showing 11 changed files with 63 additions and 79 deletions.
diff --git a/.../diamond/facets/introspection/IERC165.sol → ...cets/introspection/IIntrospectionBase.sol b/.../diamond/facets/introspection/IERC165.sol → ...cets/introspection/IIntrospectionBase.sol
@@ -21,16 +21,3 @@ interface IIntrospectionBase {
    */
   event InterfaceRemoved(bytes4 indexed interfaceId);
 }
-
-/**
- * @title IERC165
- * @notice Interface of the ERC165 standard. See [EIP-165](https://eips.ethereum.org/EIPS/eip-165).
- */
-interface IERC165 is IIntrospectionBase {
-  /**
-   * @notice Returns true if this contract implements the interface
-   * @param interfaceId The 4 bytes interface identifier, as specified in ERC-165
-   * @dev Has to be manually set by a facet at initialization.
-   */
-  function supportsInterface(bytes4 interfaceId) external view returns (bool);
-}
diff --git a/contracts/src/diamond/facets/introspection/IntrospectionBase.sol b/contracts/src/diamond/facets/introspection/IntrospectionBase.sol
@@ -2,8 +2,8 @@
 pragma solidity ^0.8.23;
 
 // interfaces
-import {IIntrospectionBase} from "./IERC165.sol";
 import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol";
+import {IIntrospectionBase} from "./IIntrospectionBase.sol";
 
 // libraries
 import {IntrospectionStorage} from "./IntrospectionStorage.sol";
@@ -32,7 +32,6 @@ abstract contract IntrospectionBase is IIntrospectionBase {
   }
 
   function _supportsInterface(bytes4 interfaceId) internal view returns (bool) {
-    return
-      IntrospectionStorage.layout().supportedInterfaces[interfaceId] == true;
+    return IntrospectionStorage.layout().supportedInterfaces[interfaceId];
   }
 }
diff --git a/contracts/src/factory/facets/architect/Architect.sol b/contracts/src/factory/facets/architect/Architect.sol
@@ -40,7 +40,7 @@ contract Architect is
   //                            Space
   // =============================================================
   function createSpace(
-    SpaceInfo memory spaceInfo
+    SpaceInfo calldata spaceInfo
   ) external nonReentrant whenNotPaused returns (address) {
     return _createSpace(spaceInfo);
   }

diff --git a/contracts/src/factory/facets/architect/ArchitectBase.sol b/contracts/src/factory/facets/architect/ArchitectBase.sol
@@ -52,7 +52,7 @@ abstract contract ArchitectBase is Factory, IArchitectBase {
   }
 
   function _createSpace(
-    SpaceInfo memory spaceInfo
+    SpaceInfo calldata spaceInfo
   ) internal returns (address spaceAddress) {
     ArchitectStorage.Layout storage ds = ArchitectStorage.layout();
     ImplementationStorage.Layout storage ims = ImplementationStorage.layout();
@@ -168,7 +168,7 @@ abstract contract ArchitectBase is Factory, IArchitectBase {
   function _createDefaultChannel(
     address space,
     uint256 roleId,
-    ChannelInfo memory channelInfo
+    ChannelInfo calldata channelInfo
   ) internal {
     uint256[] memory roleIds = new uint256[](1);
     roleIds[0] = roleId;
@@ -192,7 +192,7 @@ abstract contract ArchitectBase is Factory, IArchitectBase {
     address spaceAddress,
     IUserEntitlement userEntitlement,
     IRuleEntitlement ruleEntitlement,
-    MembershipRequirements memory requirements
+    MembershipRequirements calldata requirements
   ) internal returns (uint256 roleId) {
     string[] memory joinPermissions = new string[](1);
     joinPermissions[0] = Permissions.JoinSpace;
@@ -248,8 +248,8 @@ abstract contract ArchitectBase is Factory, IArchitectBase {
 
   function _createMemberEntitlement(
     address spaceAddress,
-    string memory memberName,
-    string[] memory memberPermissions,
+    string calldata memberName,
+    string[] calldata memberPermissions,
     IUserEntitlement userEntitlement
   ) internal returns (uint256 roleId) {
     address[] memory users = new address[](1);
@@ -273,7 +273,7 @@ abstract contract ArchitectBase is Factory, IArchitectBase {
 
   function _deploySpace(
     uint256 spaceTokenId,
-    Membership memory membership
+    Membership calldata membership
   ) internal returns (address space) {
     // get deployment info
     (bytes memory initCode, bytes32 salt) = _getSpaceDeploymentInfo(
@@ -301,13 +301,18 @@ abstract contract ArchitectBase is Factory, IArchitectBase {
 
   function _getSpaceDeploymentInfo(
     uint256 spaceTokenId,
-    Membership memory membership
+    Membership calldata membership
   ) internal view returns (bytes memory initCode, bytes32 salt) {
-    ImplementationStorage.Layout storage ds = ImplementationStorage.layout();
+    address spaceToken = address(ImplementationStorage.layout().spaceToken);
 
     // calculate salt
     salt = keccak256(abi.encode(msg.sender, spaceTokenId, block.timestamp));
 
+    IMembershipBase.Membership memory membershipSettings = membership.settings;
+    if (membershipSettings.feeRecipient == address(0)) {
+      membershipSettings.feeRecipient = msg.sender;
+    }
+
     // calculate init code
     initCode = abi.encodePacked(
       type(SpaceProxy).creationCode,
@@ -318,22 +323,10 @@ abstract contract ArchitectBase is Factory, IArchitectBase {
           manager: address(this)
         }),
         ITokenOwnableBase.TokenOwnable({
-          collection: address(ds.spaceToken),
+          collection: spaceToken,
           tokenId: spaceTokenId
         }),
-        IMembershipBase.Membership({
-          name: membership.settings.name,
-          symbol: membership.settings.symbol,
-          price: membership.settings.price,
-          maxSupply: membership.settings.maxSupply,
-          duration: membership.settings.duration,
-          currency: membership.settings.currency,
-          feeRecipient: membership.settings.feeRecipient == address(0)
-            ? msg.sender
-            : membership.settings.feeRecipient,
-          freeAllocation: membership.settings.freeAllocation,
-          pricingModule: membership.settings.pricingModule
-        })
+        membershipSettings
       )
     );
   }

diff --git a/contracts/src/factory/facets/architect/pricing/PricingModulesBase.sol b/contracts/src/factory/facets/architect/pricing/PricingModulesBase.sol
@@ -2,9 +2,9 @@
 pragma solidity ^0.8.23;
 
 // interfaces
+import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol";
 import {IPricingModulesBase} from "./IPricingModules.sol";
 import {IMembershipPricing} from "contracts/src/spaces/facets/membership/pricing/IMembershipPricing.sol";
-import {IERC165} from "contracts/src/diamond/facets/introspection/IERC165.sol";
 
 // libraries
 import {PricingModulesStorage} from "./PricingModulesStorage.sol";

diff --git a/contracts/src/spaces/facets/entitlements/EntitlementsManagerService.sol b/contracts/src/spaces/facets/entitlements/EntitlementsManagerService.sol
@@ -2,8 +2,8 @@
 pragma solidity ^0.8.23;
 
 // interfaces
+import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol";
 import {IEntitlement} from "contracts/src/spaces/entitlements/IEntitlement.sol";
-import {IERC165} from "contracts/src/diamond/facets/introspection/IERC165.sol";
 
 // libraries
 import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";

diff --git a/contracts/src/spaces/facets/gated/EntitlementGatedBase.sol b/contracts/src/spaces/facets/gated/EntitlementGatedBase.sol
@@ -46,8 +46,9 @@ abstract contract EntitlementGatedBase is IEntitlementGatedBase {
 
     Transaction storage transaction = ds.transactions[transactionId];
 
-    if (transaction.hasBenSet == true) {
-      for (uint256 i = 0; i < transaction.roleIds.length; i++) {
+    if (transaction.hasBenSet) {
+      uint256 _length = transaction.roleIds.length;
+      for (uint256 i; i < _length; ++i) {
         if (transaction.roleIds[i] == roleId) {
           revert EntitlementGated_TransactionCheckAlreadyRegistered();
         }
@@ -69,7 +70,8 @@ abstract contract EntitlementGatedBase is IEntitlementGatedBase {
 
     transaction.roleIds.push(roleId);
 
-    for (uint256 i = 0; i < selectedNodes.length; i++) {
+    uint256 length = selectedNodes.length;
+    for (uint256 i; i < length; ++i) {
       transaction.nodeVotesArray[roleId].push(
         NodeVote({node: selectedNodes[i], vote: NodeVoteStatus.NOT_VOTED})
       );
@@ -111,7 +113,7 @@ abstract contract EntitlementGatedBase is IEntitlementGatedBase {
 
     uint256 transactionNodesLength = transaction.nodeVotesArray[roleId].length;
 
-    for (uint256 i = 0; i < transactionNodesLength; i++) {
+    for (uint256 i; i < transactionNodesLength; ++i) {
       NodeVote storage tempVote = transaction.nodeVotesArray[roleId][i];
 
       // Update vote if not yet voted
@@ -123,11 +125,14 @@ abstract contract EntitlementGatedBase is IEntitlementGatedBase {
         found = true;
       }
 
-      // Count votes
-      if (tempVote.vote == NodeVoteStatus.PASSED) {
-        passed++;
-      } else if (tempVote.vote == NodeVoteStatus.FAILED) {
-        failed++;
+      unchecked {
+        NodeVoteStatus currentStatus = tempVote.vote;
+        // Count votes
+        if (currentStatus == NodeVoteStatus.PASSED) {
+          ++passed;
+        } else if (currentStatus == NodeVoteStatus.FAILED) {
+          ++failed;
+        }
       }
     }
 
@@ -153,7 +158,8 @@ abstract contract EntitlementGatedBase is IEntitlementGatedBase {
       .layout();
 
     Transaction storage transaction = ds.transactions[transactionId];
-    for (uint256 i = 0; i < transaction.roleIds.length; i++) {
+    uint256 length = transaction.roleIds.length;
+    for (uint256 i; i < length; ++i) {
       delete transaction.nodeVotesArray[transaction.roleIds[i]];
     }
     delete transaction.roleIds;
@@ -174,9 +180,7 @@ abstract contract EntitlementGatedBase is IEntitlementGatedBase {
     }
 
     IRuleEntitlement re = IRuleEntitlement(address(transaction.entitlement));
-    IRuleEntitlement.RuleData memory ruleData = re.getRuleData(roleId);
-
-    return ruleData;
+    return re.getRuleData(roleId);
   }
 
   function _onEntitlementCheckResultPosted(

diff --git a/contracts/src/utils/Factory.sol b/contracts/src/utils/Factory.sol
@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.23;
 
+import {LibClone} from "solady/utils/LibClone.sol";
+
 /**
  * @title Factory for arbitrary code deployment using the "CREATE" and "CREATE2" opcodes
  */
@@ -15,13 +17,15 @@ abstract contract Factory {
   function _deploy(
     bytes memory initCode
   ) internal returns (address deployment) {
-    assembly {
+    assembly ("memory-safe") {
       let encoded_data := add(0x20, initCode)
       let encoded_size := mload(initCode)
       deployment := create(0, encoded_data, encoded_size)
+      if iszero(deployment) {
+        mstore(0, 0xef35ca19) // revert Factory__FailedDeployment()
+        revert(0x1c, 0x04)
+      }
     }
-
-    if (deployment == address(0)) revert Factory__FailedDeployment();
   }
 
   /**
@@ -35,34 +39,35 @@ abstract contract Factory {
     bytes memory initCode,
     bytes32 salt
   ) internal returns (address deployment) {
-    assembly {
+    assembly ("memory-safe") {
       let encoded_data := add(0x20, initCode)
       let encoded_size := mload(initCode)
       deployment := create2(0, encoded_data, encoded_size, salt)
+      if iszero(deployment) {
+        mstore(0, 0xef35ca19) // revert Factory__FailedDeployment()
+        revert(0x1c, 0x04)
+      }
     }
-
-    if (deployment == address(0)) revert Factory__FailedDeployment();
   }
 
   /**
    * @notice calculate the _deployMetamorphicContract deployment address for a given salt
    * @param initCodeHash hash of contract initialization code
    * @param salt input for deterministic address calculation
-   * @return deployment address
+   * @return deployment deployment address
    */
   function _calculateDeploymentAddress(
     bytes32 initCodeHash,
     bytes32 salt
-  ) internal view returns (address) {
-    return
-      address(
-        uint160(
-          uint256(
-            keccak256(
-              abi.encodePacked(hex"ff", address(this), salt, initCodeHash)
-            )
-          )
-        )
-      );
+  ) internal view returns (address deployment) {
+    deployment = LibClone.predictDeterministicAddress(
+      initCodeHash,
+      salt,
+      address(this)
+    );
+    assembly {
+      // clean the upper 96 bits
+      deployment := and(deployment, 0xffffffffffffffffffffffffffffffffffffffff)
+    }
   }
 }
diff --git a/contracts/test/diamond/erc721a/ERC721A.t.sol b/contracts/test/diamond/erc721a/ERC721A.t.sol
@@ -2,7 +2,7 @@
 pragma solidity ^0.8.23;
 
 //interfaces
-import {IERC165} from "contracts/src/diamond/facets/introspection/IERC165.sol";
+import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol";
 import {IERC721} from "@openzeppelin/contracts/token/ERC721/IERC721.sol";
 
 //libraries

diff --git a/contracts/test/diamond/introspection/IntrospectionSetup.sol b/contracts/test/diamond/introspection/IntrospectionSetup.sol
@@ -38,11 +38,7 @@ contract IntrospectionSetup is FacetTest {
 }
 
 contract IntrospectionHelper is FacetHelper {
-  IntrospectionFacet internal introspection;
-
-  constructor() {
-    introspection = new IntrospectionFacet();
-  }
+  IntrospectionFacet internal introspection = new IntrospectionFacet();
 
   function facet() public view override returns (address) {
     return address(introspection);

diff --git a/contracts/test/diamond/loupe/DiamondLoupe.t.sol b/contracts/test/diamond/loupe/DiamondLoupe.t.sol
@@ -2,9 +2,9 @@
 pragma solidity ^0.8.23;
 
 //interfaces
+import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol";
 import {IDiamond} from "contracts/src/diamond/IDiamond.sol";
 import {IDiamondLoupe} from "contracts/src/diamond/facets/loupe/IDiamondLoupe.sol";
-import {IERC165} from "contracts/src/diamond/facets/introspection/IERC165.sol";
 
 //libraries