Optimzed callback execution (#135)

* gas benchmark

* optimize execute callback function

* fix typo

* optimize execute callback view function

* optimize callback mode loop

Author: PranavGarg01

.gas-snapshot

@@ -1,10 +1,10 @@
-CoreBenchmark:test_core_callCallbackFunction_required() (gas: 46411)
-CoreBenchmark:test_core_callFunction_callback_callbackFunctionRequired() (gas: 44067)
-CoreBenchmark:test_core_callFunction_notPermissionedDelegate() (gas: 18029)
-CoreBenchmark:test_core_callFunction_notPermissionedExternal() (gas: 17985)
-CoreBenchmark:test_deployCore() (gas: 1920924)
-CoreBenchmark:test_deployExtension() (gas: 258057)
-CoreBenchmark:test_extension_callFunction_notPermissionedDelegate() (gas: 8294)
-CoreBenchmark:test_extension_callFunction_notPermissionedExternal() (gas: 8314)
-CoreBenchmark:test_installExtension() (gas: 330369)
-CoreBenchmark:test_uninstallExtension() (gas: 18385)
+CoreBenchmark:test_core_callCallbackFunction_required() (gas: 67409)
+CoreBenchmark:test_core_callFunction_callback_callbackFunctionRequired() (gas: 65131)
+CoreBenchmark:test_core_callFunction_notPermissionedDelegate() (gas: 39115)
+CoreBenchmark:test_core_callFunction_notPermissionedExternal() (gas: 39093)
+CoreBenchmark:test_deployCore() (gas: 2139892)
+CoreBenchmark:test_deployModule() (gas: 350173)
+CoreBenchmark:test_installModule() (gas: 352153)
+CoreBenchmark:test_module_callFunction_notPermissionedDelegate() (gas: 29381)
+CoreBenchmark:test_module_callFunction_notPermissionedExternal() (gas: 29379)
+CoreBenchmark:test_uninstallModule() (gas: 91332)

src/Core.sol

@@ -38,10 +38,10 @@ abstract contract Core is ICore, OwnableRoles, ReentrancyGuard {
                                 EVENTS
     //////////////////////////////////////////////////////////////*/
 
-    /// @notice Emitted when an module is installed.
+    /// @notice Emitted when a module is installed.
     event ModuleInstalled(address caller, address implementation, address installedModule);
 
-    /// @notice Emitted when an module is uninstalled.
+    /// @notice Emitted when a module is uninstalled.
     event ModuleUninstalled(address caller, address implementation, address installedModule);
 
     /*//////////////////////////////////////////////////////////////
@@ -125,7 +125,7 @@ abstract contract Core is ICore, OwnableRoles, ReentrancyGuard {
                             EXTERNAL FUNCTIONS
     //////////////////////////////////////////////////////////////*/
 
-    /// @notice Installs an module contract.
+    /// @notice Installs a module contract.
     function installModule(address _module, bytes calldata _data)
         external
         payable
@@ -135,7 +135,7 @@ abstract contract Core is ICore, OwnableRoles, ReentrancyGuard {
         _installModule(_module, _data);
     }
 
-    /// @notice Uninstalls an module contract.
+    /// @notice Uninstalls a module contract.
     function uninstallModule(address _module, bytes calldata _data)
         external
         payable
@@ -171,7 +171,7 @@ abstract contract Core is ICore, OwnableRoles, ReentrancyGuard {
         return false;
     }
 
-    /// @dev Installs an module contract.
+    /// @dev Installs a module contract.
     function _installModule(address _module, bytes memory _data) internal {
         if (!modules.add(_module)) {
             revert ModuleAlreadyInstalled();
@@ -253,7 +253,7 @@ abstract contract Core is ICore, OwnableRoles, ReentrancyGuard {
         emit ModuleInstalled(msg.sender, _module, _module);
     }
 
-    /// @notice Uninstalls an module contract.
+    /// @notice Uninstalls a module contract.
     function _uninstallModule(address _module, bytes memory _data) internal {
         // Check: remove and check if the module is installed
         if (!modules.remove(_module)) {
@@ -288,7 +288,7 @@ abstract contract Core is ICore, OwnableRoles, ReentrancyGuard {
         emit ModuleUninstalled(msg.sender, _module, _module);
     }
 
-    /// @dev Calls an module callback function and checks whether it is optional or required.
+    /// @dev Calls a module callback function and checks whether it is optional or required.
     function _executeCallbackFunction(bytes4 _selector, bytes memory _abiEncodedCalldata)
         internal
         nonReentrant
@@ -301,29 +301,28 @@ abstract contract Core is ICore, OwnableRoles, ReentrancyGuard {
             revert CallbackFunctionNotSupported();
         }
 
-        // Get callback mode -- required or not required.
-        SupportedCallbackFunction[] memory functions = getSupportedCallbackFunctions();
-        uint256 len = functions.length;
-
-        CallbackMode callbackMode;
-        for (uint256 i = 0; i < len; i++) {
-            if (functions[i].selector == _selector) {
-                callbackMode = functions[i].mode;
-                break;
-            }
-        }
-
         if (callbackFunction.implementation != address(0)) {
             (success, returndata) = callbackFunction.implementation.delegatecall(_abiEncodedCalldata);
             if (!success) {
                 _revert(returndata, CallbackExecutionReverted.selector);
             }
-        } else if (callbackMode == CallbackMode.REQUIRED) {
-            revert CallbackFunctionRequired();
+        } else {
+            // Get callback mode -- required or not required.
+            SupportedCallbackFunction[] memory functions = getSupportedCallbackFunctions();
+            uint256 len = functions.length;
+
+            for (uint256 i = 0; i < len; i++) {
+                if (functions[i].selector == _selector) {
+                    if (functions[i].mode == CallbackMode.REQUIRED) {
+                        revert CallbackFunctionRequired();
+                    }
+                    break;
+                }
+            }
         }
     }
 
-    /// @dev Calls an module callback function and checks whether it is optional or required.
+    /// @dev Calls a module callback function and checks whether it is optional or required.
     function _executeCallbackFunctionView(bytes4 _selector, bytes memory _abiEncodedCalldata)
         internal
         view
@@ -336,25 +335,24 @@ abstract contract Core is ICore, OwnableRoles, ReentrancyGuard {
             revert CallbackFunctionNotSupported();
         }
 
-        // Get callback mode -- required or not required.
-        SupportedCallbackFunction[] memory functions = getSupportedCallbackFunctions();
-        uint256 len = functions.length;
-
-        CallbackMode callbackMode;
-        for (uint256 i = 0; i < len; i++) {
-            if (functions[i].selector == _selector) {
-                callbackMode = functions[i].mode;
-                break;
-            }
-        }
-
         if (callbackFunction.implementation != address(0)) {
             (success, returndata) = address(this).staticcall(_abiEncodedCalldata);
             if (!success) {
                 _revert(returndata, CallbackExecutionReverted.selector);
             }
-        } else if (callbackMode == CallbackMode.REQUIRED) {
-            revert CallbackFunctionRequired();
+        } else {
+            // Get callback mode -- required or not required.
+            SupportedCallbackFunction[] memory functions = getSupportedCallbackFunctions();
+            uint256 len = functions.length;
+
+            for (uint256 i = 0; i < len; i++) {
+                if (functions[i].selector == _selector) {
+                    if (functions[i].mode == CallbackMode.REQUIRED) {
+                        revert CallbackFunctionRequired();
+                    }
+                    break;
+                }
+            }
         }
     }