feat: update hook interface with tests

Removes the need for an extra bool flag.
Still broken on some tests

src/helpers/Logger.sol

@@ -18,6 +18,12 @@ library Logger {
     }
   }
 
+  function logBytes4(bytes4 bytesToLog) internal view {
+    if (block.chainid == 260) {
+      console.logBytes4(bytesToLog);
+    }
+  }
+
   function logBytes32(bytes32 bytesToLog) internal view {
     if (block.chainid == 260) {
       console.logBytes32(bytesToLog);

src/interfaces/IHook.sol

@@ -1,4 +1,5 @@
 // SPDX-License-Identifier: GPL-3.0
+
 pragma solidity ^0.8.24;
 
 import { Transaction } from "@matterlabs/zksync-contracts/l2/system-contracts/libraries/TransactionHelper.sol";

src/interfaces/IHookManager.sol

@@ -22,28 +22,25 @@ interface IHookManager {
    * @notice Add a hook to the list of hooks and call it's init function
    * @dev Can only be called by self
    * @param hook - Address of the hook
-   * @param isValidation bool          - True if the hook is a validation hook, false otherwise
    * @param initData bytes calldata    - Data to pass to the hook's `onInstall` function
    */
-  function addHook(address hook, bool isValidation, bytes calldata initData) external;
+  function addHook(address hook, bytes calldata initData) external;
 
   /**
    * @notice Remove a hook from the list of hooks
    * @dev Can only be called by self
    * @param hook address      - Address of the hook to remove
-   * @param isValidation bool - True if the hook is a validation hook, false otherwise
    * @param deinitData bytes calldata - Data to pass to the hook's `onUninstall` function
    */
-  function removeHook(address hook, bool isValidation, bytes calldata deinitData) external;
+  function removeHook(address hook, bytes calldata deinitData) external;
 
   /**
    * @notice Remove a hook from the list of hooks while ignoring reverts from its `onUninstall` teardown function
    * @dev Can only be called by self
    * @param hook address      - Address of the hook to remove
-   * @param isValidation bool - True if the hook is a validation hook, false otherwise
    * @param deinitData bytes calldata - Data to pass to the hook's `onUninstall` function
    */
-  function unlinkHook(address hook, bool isValidation, bytes calldata deinitData) external;
+  function unlinkHook(address hook, bytes calldata deinitData) external;
 
   /**
    * @notice Check if an address is in the list of hooks

src/libraries/Errors.sol

@@ -36,7 +36,7 @@ library Errors {
     //////////////////////////////////////////////////////////////*/
 
   error EMPTY_HOOK_ADDRESS(uint256 hookAndDataLength);
-  error HOOK_ERC165_FAIL(address hookAddress, bool isValidation);
+  error HOOK_ERC165_FAIL(address hookAddress);
   error INVALID_KEY(bytes32 key);
 
   /*//////////////////////////////////////////////////////////////

src/managers/HookManager.sol

@@ -16,7 +16,6 @@ import { IModule } from "../interfaces/IModule.sol";
 /**
  * @title Manager contract for hooks
  * @notice Abstract contract for managing the enabled hooks of the account
- * @dev Hook addresses are stored in a linked list
  * @author https://getclave.io
  */
 abstract contract HookManager is IHookManager, Auth {
@@ -27,19 +26,20 @@ abstract contract HookManager is IHookManager, Auth {
   using ExcessivelySafeCall for address;
 
   /// @inheritdoc IHookManager
-  function addHook(address hook, bool isValidation, bytes calldata initData) external override onlySelf {
-    _addHook(hook, isValidation, initData);
+  function addHook(address hook, bytes calldata initData) external override onlySelf {
+    _addHook(hook);
+    IModule(hook).onInstall(initData);
   }
 
   /// @inheritdoc IHookManager
-  function removeHook(address hook, bool isValidation, bytes calldata deinitData) external override onlySelf {
-    _removeHook(hook, isValidation);
+  function removeHook(address hook, bytes calldata deinitData) external override onlySelf {
+    _removeHook(hook);
     IModule(hook).onUninstall(deinitData);
   }
 
   /// @inheritdoc IHookManager
-  function unlinkHook(address hook, bool isValidation, bytes calldata deinitData) external onlySelf {
-    _removeHook(hook, isValidation);
+  function unlinkHook(address hook, bytes calldata deinitData) external onlySelf {
+    _removeHook(hook);
     hook.excessivelySafeCall(gasleft(), 0, abi.encodeWithSelector(IModule.onUninstall.selector, deinitData));
   }
 
@@ -89,27 +89,29 @@ abstract contract HookManager is IHookManager, Auth {
     }
   }
 
-  function _addHook(address hook, bool isValidation, bytes calldata initData) internal {
-    if (!_supportsHook(hook, isValidation)) {
-      revert Errors.HOOK_ERC165_FAIL(hook, isValidation);
+  function _addHook(address hook) internal {
+    bool isExecutionHook = hook.supportsInterface(type(IExecutionHook).interfaceId);
+    bool isValidationHook = hook.supportsInterface(type(IValidationHook).interfaceId);
+    if (!isExecutionHook && !isValidationHook) {
+      revert Errors.HOOK_ERC165_FAIL(hook);
     }
-
-    if (isValidation) {
-      _validationHooks().add(hook);
-    } else {
-      _executionHooks().add(hook);
+    if (isValidationHook) {
+      require(_validationHooks().add(hook), "Validation hook already exists");
+    }
+    if (isExecutionHook) {
+      require(_executionHooks().add(hook), "Execution hook already exists");
     }
-
-    IModule(hook).onInstall(initData);
 
     emit HookAdded(hook);
   }
 
-  function _removeHook(address hook, bool isValidation) internal {
-    if (isValidation) {
-      _validationHooks().remove(hook);
-    } else {
-      _executionHooks().remove(hook);
+  function _removeHook(address hook) internal {
+    if (_validationHooks().contains(hook)) {
+      require(_validationHooks().remove(hook), "Validation hook not found");
+    }
+
+    if (_executionHooks().contains(hook)) {
+      require(_executionHooks().remove(hook), "Execution hook not found");
     }
 
     emit HookRemoved(hook);
@@ -132,14 +134,4 @@ abstract contract HookManager is IHookManager, Auth {
   function _executionHooks() private view returns (EnumerableSet.AddressSet storage executionHooks) {
     executionHooks = SsoStorage.layout().executionHooks;
   }
-
-  function _supportsHook(address hook, bool isValidation) private view returns (bool) {
-    return
-      hook.supportsInterface(type(IModule).interfaceId) &&
-      (
-        isValidation
-          ? hook.supportsInterface(type(IValidationHook).interfaceId)
-          : hook.supportsInterface(type(IExecutionHook).interfaceId)
-      );
-  }
 }

src/test/SampleHooks.sol

@@ -0,0 +1,137 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.24;
+
+import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol";
+import { Transaction } from "@matterlabs/zksync-contracts/l2/system-contracts/libraries/TransactionHelper.sol";
+
+import { IModule } from "../interfaces/IModule.sol";
+import { IExecutionHook, IValidationHook } from "../interfaces/IHook.sol";
+
+abstract contract BaseHookValidator is IValidationHook {
+  /// @notice Emitted during validation
+  event Validating(address indexed accountAddress);
+
+  function onInstall(bytes calldata data) external override {}
+  function onUninstall(bytes calldata data) external override {}
+  function supportsInterface(bytes4 interfaceId) external pure override returns (bool) {
+    return
+      interfaceId == type(IValidationHook).interfaceId ||
+      interfaceId == type(IModule).interfaceId ||
+      interfaceId == type(IERC165).interfaceId;
+  }
+}
+
+abstract contract BaseHookExecution is IExecutionHook {
+  /// @notice Emitted during execution
+  event Preexecute(address indexed accountAddress);
+  event Postexecute(address indexed accountAddress);
+
+  function onInstall(bytes calldata data) external override {}
+  function onUninstall(bytes calldata data) external override {}
+  function supportsInterface(bytes4 interfaceId) external pure override returns (bool) {
+    return
+      interfaceId == type(IExecutionHook).interfaceId ||
+      interfaceId == type(IModule).interfaceId ||
+      interfaceId == type(IERC165).interfaceId;
+  }
+}
+
+/// @title FailHookValidator
+/// @author Matter Labs
+/// @custom:security-contact [email protected]
+/// @dev Sample hook validator that always fails (BURNS THE ACCOUNT)
+contract FailHookValidator is BaseHookValidator {
+  function validationHook(bytes32, Transaction calldata) external override {
+    emit Validating(msg.sender);
+    require(false, "SampleHookValidator: validationHook failed");
+  }
+}
+
+/// @title SuccessHookValidator
+/// @author Matter Labs
+/// @custom:security-contact [email protected]
+/// @dev Sample hook validator that always passes
+contract SuccessHookValidator is BaseHookValidator {
+  function validationHook(bytes32, Transaction calldata) external override {
+    emit Validating(msg.sender);
+  }
+}
+
+/// @title PreFailHookExecutor
+/// @author Matter Labs
+/// @custom:security-contact [email protected]
+/// @dev Sample hook executor that always fails on pre-execute (BURNS THE ACCOUNT)
+contract PreFailHookExecutor is BaseHookExecution {
+  function preExecutionHook(Transaction calldata) external override returns (bytes memory _context) {
+    emit Preexecute(msg.sender);
+    require(false, "SampleHookExecutor: execution hook failed");
+  }
+
+  function postExecutionHook() external override {
+    emit Postexecute(msg.sender);
+  }
+}
+
+/// @title PostFailHookExecutor
+/// @author Matter Labs
+/// @custom:security-contact [email protected]
+/// @dev Sample hook executor that always fails on post-execute (BURNS THE ACCOUNT)
+contract PostFailHookExecutor is BaseHookExecution {
+  function preExecutionHook(Transaction calldata) external override returns (bytes memory _context) {
+    emit Preexecute(msg.sender);
+  }
+
+  function postExecutionHook() external override {
+    emit Postexecute(msg.sender);
+    require(false, "SampleHookExecutor: executionHook failed");
+  }
+}
+
+/// @title SuccessHookExecutor
+/// @author Matter Labs
+/// @custom:security-contact [email protected]
+/// @dev Sample hook validator that always passes
+contract SuccessHookExecutor is BaseHookExecution {
+  function preExecutionHook(Transaction calldata) external override returns (bytes memory _context) {
+    emit Preexecute(msg.sender);
+  }
+
+  function postExecutionHook() external override {
+    emit Postexecute(msg.sender);
+  }
+}
+
+/// @title SuccessBothHookExecutor
+/// @author Matter Labs
+/// @custom:security-contact [email protected]
+/// @dev Sample hook execution and validator that always passes
+contract SuccessBothHook is IExecutionHook, IValidationHook {
+  /// @notice Emitted during execution
+  event Preexecute(address indexed accountAddress);
+  event Postexecute(address indexed accountAddress);
+  /// @notice Emitted during validation
+  event Validating(address indexed accountAddress);
+
+  function onInstall(bytes calldata data) external {}
+  function onUninstall(bytes calldata data) external {}
+  function supportsInterface(bytes4 interfaceId) external pure returns (bool) {
+    return
+      interfaceId == type(IValidationHook).interfaceId ||
+      interfaceId == type(IExecutionHook).interfaceId ||
+      interfaceId == type(IModule).interfaceId ||
+      interfaceId == type(IERC165).interfaceId;
+  }
+
+  function preExecutionHook(Transaction calldata) external override returns (bytes memory _context) {
+    emit Preexecute(msg.sender);
+  }
+
+  function postExecutionHook() external override {
+    emit Postexecute(msg.sender);
+  }
+
+  function validationHook(bytes32, Transaction calldata) external override {
+    emit Validating(msg.sender);
+  }
+}