diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol index d84b6d20..ca4da1dd 100644 --- a/src/account/AccountLoupe.sol +++ b/src/account/AccountLoupe.sol @@ -4,10 +4,16 @@ pragma solidity ^0.8.25; import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; -import {IAccountLoupe} from "../interfaces/IAccountLoupe.sol"; +import {IAccountLoupe, ExecutionHook} from "../interfaces/IAccountLoupe.sol"; import {FunctionReference, IPluginManager} from "../interfaces/IPluginManager.sol"; import {IStandardExecutor} from "../interfaces/IStandardExecutor.sol"; -import {AccountStorage, getAccountStorage, SelectorData, toFunctionReferenceArray} from "./AccountStorage.sol"; +import { + AccountStorage, + getAccountStorage, + SelectorData, + toFunctionReferenceArray, + toExecutionHook +} from "./AccountStorage.sol"; abstract contract AccountLoupe is IAccountLoupe { using EnumerableSet for EnumerableSet.Bytes32Set; @@ -36,25 +42,16 @@ abstract contract AccountLoupe is IAccountLoupe { } /// @inheritdoc IAccountLoupe - function getExecutionHooks(bytes4 selector) external view returns (ExecutionHooks[] memory execHooks) { + function getExecutionHooks(bytes4 selector) external view returns (ExecutionHook[] memory execHooks) { SelectorData storage selectorData = getAccountStorage().selectorData[selector]; - uint256 preExecHooksLength = selectorData.preHooks.length(); - uint256 postOnlyExecHooksLength = selectorData.postOnlyHooks.length(); + uint256 executionHooksLength = selectorData.executionHooks.length(); - execHooks = new ExecutionHooks[](preExecHooksLength + postOnlyExecHooksLength); + execHooks = new ExecutionHook[](executionHooksLength); - for (uint256 i = 0; i < preExecHooksLength; ++i) { - bytes32 key = selectorData.preHooks.at(i); - FunctionReference preExecHook = FunctionReference.wrap(bytes21(key)); - FunctionReference associatedPostExecHook = selectorData.associatedPostHooks[preExecHook]; - - execHooks[i].preExecHook = preExecHook; - execHooks[i].postExecHook = associatedPostExecHook; - } - - for (uint256 i = 0; i < postOnlyExecHooksLength; ++i) { - bytes32 key = selectorData.postOnlyHooks.at(i); - execHooks[preExecHooksLength + i].postExecHook = FunctionReference.wrap(bytes21(key)); + for (uint256 i = 0; i < executionHooksLength; ++i) { + bytes32 key = selectorData.executionHooks.at(i); + ExecutionHook memory execHook = execHooks[i]; + (execHook.hookFunction, execHook.isPreHook, execHook.isPostHook) = toExecutionHook(key); } } diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 728f7f5c..5562fdcd 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.25; import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import {IPlugin} from "../interfaces/IPlugin.sol"; +import {ExecutionHook} from "../interfaces/IAccountLoupe.sol"; import {FunctionReference} from "../interfaces/IPluginManager.sol"; // bytes = keccak256("ERC6900.UpgradeableModularAccount.Storage") @@ -44,10 +45,7 @@ struct SelectorData { // The pre validation hooks for this function selector. EnumerableSet.Bytes32Set preValidationHooks; // The execution hooks for this function selector. - EnumerableSet.Bytes32Set preHooks; - // bytes21 key = pre hook function reference - mapping(FunctionReference => FunctionReference) associatedPostHooks; - EnumerableSet.Bytes32Set postOnlyHooks; + EnumerableSet.Bytes32Set executionHooks; } struct AccountStorage { @@ -79,6 +77,34 @@ function getPermittedCallKey(address addr, bytes4 selector) pure returns (bytes2 using EnumerableSet for EnumerableSet.Bytes32Set; +function toSetValue(FunctionReference functionReference) pure returns (bytes32) { + return bytes32(FunctionReference.unwrap(functionReference)); +} + +function toFunctionReference(bytes32 setValue) pure returns (FunctionReference) { + return FunctionReference.wrap(bytes21(setValue)); +} + +// ExecutionHook layout: +// 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF______________________ Hook Function Reference +// 0x__________________________________________AA____________________ is pre hook +// 0x____________________________________________BB__________________ is post hook + +function toSetValue(ExecutionHook memory executionHook) pure returns (bytes32) { + return bytes32(FunctionReference.unwrap(executionHook.hookFunction)) + | bytes32(executionHook.isPreHook ? uint256(1) << 80 : 0) + | bytes32(executionHook.isPostHook ? uint256(1) << 72 : 0); +} + +function toExecutionHook(bytes32 setValue) + pure + returns (FunctionReference hookFunction, bool isPreHook, bool isPostHook) +{ + hookFunction = FunctionReference.wrap(bytes21(setValue)); + isPreHook = (uint256(setValue) >> 80) & 0xFF == 1; + isPostHook = (uint256(setValue) >> 72) & 0xFF == 1; +} + /// @dev Helper function to get all elements of a set into memory. function toFunctionReferenceArray(EnumerableSet.Bytes32Set storage set) view diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 1d2ee5b8..8f228932 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -14,11 +14,13 @@ import { ManifestExternalCallPermission, PluginManifest } from "../interfaces/IPlugin.sol"; +import {ExecutionHook} from "../interfaces/IAccountLoupe.sol"; import {FunctionReference, IPluginManager} from "../interfaces/IPluginManager.sol"; import { AccountStorage, getAccountStorage, SelectorData, + toSetValue, getPermittedCallKey, PermittedExternalCallData } from "./AccountStorage.sol"; @@ -96,49 +98,30 @@ abstract contract PluginManagerInternals is IPluginManager { _selectorData.validation = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; } - function _addExecHooks(bytes4 selector, FunctionReference preExecHook, FunctionReference postExecHook) - internal - { - SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - - if (preExecHook.notEmpty()) { - // Don't need to check for duplicates, as the hook can be run at most once. - _selectorData.preHooks.add(_toSetValue(preExecHook)); - - if (postExecHook.notEmpty()) { - _selectorData.associatedPostHooks[preExecHook] = postExecHook; - } - - return; - } - - if (postExecHook.isEmpty()) { - // both pre and post hooks cannot be null - revert NullFunctionReference(); - } - - _selectorData.postOnlyHooks.add(_toSetValue(postExecHook)); + function _addExecHooks( + bytes4 selector, + FunctionReference hookFunction, + bool isPreExecHook, + bool isPostExecHook + ) internal { + getAccountStorage().selectorData[selector].executionHooks.add( + toSetValue( + ExecutionHook({hookFunction: hookFunction, isPreHook: isPreExecHook, isPostHook: isPostExecHook}) + ) + ); } - function _removeExecHooks(bytes4 selector, FunctionReference preExecHook, FunctionReference postExecHook) - internal - { - SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - - if (preExecHook.notEmpty()) { - _selectorData.preHooks.remove(_toSetValue(preExecHook)); - - if (postExecHook.notEmpty()) { - _selectorData.associatedPostHooks[preExecHook] = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; - } - - return; - } - - // The case where both pre and post hooks are null was checked during installation. - - // May ignore return value, as the manifest hash is validated to ensure that the hook exists. - _selectorData.postOnlyHooks.remove(_toSetValue(postExecHook)); + function _removeExecHooks( + bytes4 selector, + FunctionReference hookFunction, + bool isPreExecHook, + bool isPostExecHook + ) internal { + getAccountStorage().selectorData[selector].executionHooks.remove( + toSetValue( + ExecutionHook({hookFunction: hookFunction, isPreHook: isPreExecHook, isPostHook: isPostExecHook}) + ) + ); } function _addPreValidationHook(bytes4 selector, FunctionReference preValidationHook) @@ -151,7 +134,7 @@ abstract contract PluginManagerInternals is IPluginManager { _selectorData.denyExecutionCount += 1; return; } - _selectorData.preValidationHooks.add(_toSetValue(preValidationHook)); + _selectorData.preValidationHooks.add(toSetValue(preValidationHook)); } function _removePreValidationHook(bytes4 selector, FunctionReference preValidationHook) @@ -165,7 +148,7 @@ abstract contract PluginManagerInternals is IPluginManager { return; } // May ignore return value, as the manifest hash is validated to ensure that the hook exists. - _selectorData.preValidationHooks.remove(_toSetValue(preValidationHook)); + _selectorData.preValidationHooks.remove(toSetValue(preValidationHook)); } function _installPlugin( @@ -299,15 +282,8 @@ abstract contract PluginManagerInternals is IPluginManager { length = manifest.executionHooks.length; for (uint256 i = 0; i < length; ++i) { ManifestExecutionHook memory mh = manifest.executionHooks[i]; - _addExecHooks( - mh.executionSelector, - _resolveManifestFunction( - mh.preExecHook, plugin, emptyDependencies, ManifestAssociatedFunctionType.NONE - ), - _resolveManifestFunction( - mh.postExecHook, plugin, emptyDependencies, ManifestAssociatedFunctionType.NONE - ) - ); + FunctionReference hookFunction = FunctionReferenceLib.pack(plugin, mh.functionId); + _addExecHooks(mh.executionSelector, hookFunction, mh.isPreHook, mh.isPostHook); } length = manifest.interfaceIds.length; @@ -365,15 +341,8 @@ abstract contract PluginManagerInternals is IPluginManager { length = manifest.executionHooks.length; for (uint256 i = 0; i < length; ++i) { ManifestExecutionHook memory mh = manifest.executionHooks[i]; - _removeExecHooks( - mh.executionSelector, - _resolveManifestFunction( - mh.preExecHook, plugin, emptyDependencies, ManifestAssociatedFunctionType.NONE - ), - _resolveManifestFunction( - mh.postExecHook, plugin, emptyDependencies, ManifestAssociatedFunctionType.NONE - ) - ); + FunctionReference hookFunction = FunctionReferenceLib.pack(plugin, mh.functionId); + _removeExecHooks(mh.executionSelector, hookFunction, mh.isPreHook, mh.isPostHook); } length = manifest.preValidationHooks.length; @@ -461,14 +430,6 @@ abstract contract PluginManagerInternals is IPluginManager { emit PluginUninstalled(plugin, onUninstallSuccess); } - function _toSetValue(FunctionReference functionReference) internal pure returns (bytes32) { - return bytes32(FunctionReference.unwrap(functionReference)); - } - - function _toFunctionReference(bytes32 setValue) internal pure returns (FunctionReference) { - return FunctionReference.wrap(bytes21(setValue)); - } - function _isValidPluginManifest(PluginManifest memory manifest, bytes32 manifestHash) internal pure diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 3161eb45..47192abd 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -16,7 +16,14 @@ import {FunctionReference, IPluginManager} from "../interfaces/IPluginManager.so import {IStandardExecutor, Call} from "../interfaces/IStandardExecutor.sol"; import {AccountExecutor} from "./AccountExecutor.sol"; import {AccountLoupe} from "./AccountLoupe.sol"; -import {AccountStorage, getAccountStorage, getPermittedCallKey, SelectorData} from "./AccountStorage.sol"; +import { + AccountStorage, + getAccountStorage, + getPermittedCallKey, + SelectorData, + toFunctionReference, + toExecutionHook +} from "./AccountStorage.sol"; import {AccountStorageInitializable} from "./AccountStorageInitializable.sol"; import {PluginManagerInternals} from "./PluginManagerInternals.sol"; @@ -51,7 +58,6 @@ contract UpgradeableModularAccount is error AuthorizeUpgradeReverted(bytes revertReason); error ExecFromPluginNotPermitted(address plugin, bytes4 selector); error ExecFromPluginExternalNotPermitted(address plugin, address target, uint256 value, bytes data); - error InvalidConfiguration(); error NativeTokenSpendingNotPermitted(address plugin); error PostExecHookReverted(address plugin, uint8 functionId, bytes revertReason); error PreExecHookReverted(address plugin, uint8 functionId, bytes revertReason); @@ -355,7 +361,7 @@ contract UpgradeableModularAccount is uint256 preUserOpValidationHooksLength = preUserOpValidationHooks.length(); for (uint256 i = 0; i < preUserOpValidationHooksLength; ++i) { bytes32 key = preUserOpValidationHooks.at(i); - FunctionReference preUserOpValidationHook = _toFunctionReference(key); + FunctionReference preUserOpValidationHook = toFunctionReference(key); (address plugin, uint8 functionId) = preUserOpValidationHook.unpack(); currentValidationData = IPlugin(plugin).preUserOpValidationHook(functionId, userOp, userOpHash); @@ -398,7 +404,7 @@ contract UpgradeableModularAccount is uint256 preRuntimeValidationHooksLength = preRuntimeValidationHooks.length(); for (uint256 i = 0; i < preRuntimeValidationHooksLength; ++i) { bytes32 key = preRuntimeValidationHooks.at(i); - FunctionReference preRuntimeValidationHook = _toFunctionReference(key); + FunctionReference preRuntimeValidationHook = toFunctionReference(key); (address plugin, uint8 functionId) = preRuntimeValidationHook.unpack(); // solhint-disable-next-line no-empty-blocks @@ -432,44 +438,34 @@ contract UpgradeableModularAccount is { SelectorData storage selectorData = getAccountStorage().selectorData[selector]; - uint256 preExecHooksLength = selectorData.preHooks.length(); - uint256 postOnlyHooksLength = selectorData.postOnlyHooks.length(); + uint256 hooksLength = selectorData.executionHooks.length(); // Overallocate on length - not all of this may get filled up. We set the correct length later. - postHooksToRun = new PostExecToRun[](preExecHooksLength + postOnlyHooksLength); + postHooksToRun = new PostExecToRun[](hooksLength); // Copy all post hooks to the array. This happens before any pre hooks are run, so we can // be sure that the set of hooks to run will not be affected by state changes mid-execution. - - // Copy post-only hooks. - for (uint256 i = 0; i < postOnlyHooksLength; ++i) { - bytes32 key = selectorData.postOnlyHooks.at(i); - postHooksToRun[i].postExecHook = _toFunctionReference(key); - } - - // Copy associated post hooks to the array. - for (uint256 i = 0; i < preExecHooksLength; ++i) { - FunctionReference preExecHook = _toFunctionReference(selectorData.preHooks.at(i)); - - FunctionReference associatedPostExecHook = selectorData.associatedPostHooks[preExecHook]; - - if (associatedPostExecHook.notEmpty()) { - postHooksToRun[i + postOnlyHooksLength].postExecHook = associatedPostExecHook; + for (uint256 i = 0; i < hooksLength; ++i) { + bytes32 key = selectorData.executionHooks.at(i); + (FunctionReference hookFunction,, bool isPostHook) = toExecutionHook(key); + if (isPostHook) { + postHooksToRun[i].postExecHook = hookFunction; } } // Run the pre hooks and copy their return data to the post hooks array, if an associated post-exec hook // exists. - for (uint256 i = 0; i < preExecHooksLength; ++i) { - bytes32 key = selectorData.preHooks.at(i); - FunctionReference preExecHook = _toFunctionReference(key); + for (uint256 i = 0; i < hooksLength; ++i) { + bytes32 key = selectorData.executionHooks.at(i); + (FunctionReference hookFunction, bool isPreHook, bool isPostHook) = toExecutionHook(key); - bytes memory preExecHookReturnData = _runPreExecHook(preExecHook, data); + if (isPreHook) { + bytes memory preExecHookReturnData = _runPreExecHook(hookFunction, data); - // If there is an associated post-exec hook, save the return data. - PostExecToRun memory postExecToRun = postHooksToRun[i + postOnlyHooksLength]; - if (postExecToRun.postExecHook.notEmpty()) { - postExecToRun.preExecHookReturnData = preExecHookReturnData; + // If there is an associated post-exec hook, save the return data. + if (isPostHook) { + postHooksToRun[i].preExecHookReturnData = preExecHookReturnData; + } } } } diff --git a/src/interfaces/IAccountLoupe.sol b/src/interfaces/IAccountLoupe.sol index 5b8a7516..a1b3c15f 100644 --- a/src/interfaces/IAccountLoupe.sol +++ b/src/interfaces/IAccountLoupe.sol @@ -3,6 +3,14 @@ pragma solidity ^0.8.25; import {FunctionReference} from "../interfaces/IPluginManager.sol"; +/// @notice Pre and post hooks for a given selector. +/// @dev It's possible for one of either `preExecHook` or `postExecHook` to be empty. +struct ExecutionHook { + FunctionReference hookFunction; + bool isPreHook; + bool isPostHook; +} + interface IAccountLoupe { /// @notice Config for an execution function, given a selector. struct ExecutionFunctionConfig { @@ -10,13 +18,6 @@ interface IAccountLoupe { FunctionReference validationFunction; } - /// @notice Pre and post hooks for a given selector. - /// @dev It's possible for one of either `preExecHook` or `postExecHook` to be empty. - struct ExecutionHooks { - FunctionReference preExecHook; - FunctionReference postExecHook; - } - /// @notice Get the validation functions and plugin address for a selector. /// @dev If the selector is a native function, the plugin address will be the address of the account. /// @param selector The selector to get the configuration for. @@ -26,7 +27,7 @@ interface IAccountLoupe { /// @notice Get the pre and post execution hooks for a selector. /// @param selector The selector to get the hooks for. /// @return The pre and post execution hooks for this selector. - function getExecutionHooks(bytes4 selector) external view returns (ExecutionHooks[] memory); + function getExecutionHooks(bytes4 selector) external view returns (ExecutionHook[] memory); /// @notice Get the pre user op and runtime validation hooks associated with a selector. /// @param selector The selector to get the hooks for. diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index ca5c7ff7..d3163730 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -40,9 +40,11 @@ struct ManifestAssociatedFunction { } struct ManifestExecutionHook { + // TODO(erc6900 spec): These fields can be packed into a single word bytes4 executionSelector; - ManifestFunction preExecHook; - ManifestFunction postExecHook; + uint8 functionId; + bool isPreHook; + bool isPostHook; } struct ManifestExternalCallPermission { diff --git a/standard/ERCs/erc-6900.md b/standard/ERCs/erc-6900.md index f4be1e0d..06b51e4d 100644 --- a/standard/ERCs/erc-6900.md +++ b/standard/ERCs/erc-6900.md @@ -221,8 +221,9 @@ interface IAccountLoupe { /// @notice Pre and post hooks for a given selector. /// @dev It's possible for one of either `preExecHook` or `postExecHook` to be empty. struct ExecutionHooks { - FunctionReference preExecHook; - FunctionReference postExecHook; + FunctionReference hookFunction; + bool isPreHook; + bool isPostHook; } /// @notice Get the validation functions and plugin address for a selector. @@ -368,9 +369,11 @@ struct ManifestAssociatedFunction { } struct ManifestExecutionHook { - bytes4 selector; - ManifestFunction preExecHook; - ManifestFunction postExecHook; + // TODO(erc6900 spec): These fields can be packed into a single word + bytes4 executionSelector; + uint8 functionId; + bool isPreHook; + bool isPostHook; } struct ManifestExternalCallPermission { diff --git a/test/account/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol index 8e1fef94..9b7cde45 100644 --- a/test/account/AccountExecHooks.t.sol +++ b/test/account/AccountExecHooks.t.sol @@ -9,8 +9,7 @@ import { ManifestFunction, PluginManifest } from "../../src/interfaces/IPlugin.sol"; -import {PluginManagerInternals} from "../../src/account/PluginManagerInternals.sol"; -import {FunctionReference, FunctionReferenceLib} from "../../src/helpers/FunctionReferenceLib.sol"; +import {FunctionReference} from "../../src/helpers/FunctionReferenceLib.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {MockPlugin} from "../mocks/MockPlugin.sol"; @@ -25,8 +24,7 @@ contract AccountExecHooksTest is AccountTestBase { bytes4 internal constant _EXEC_SELECTOR = bytes4(uint32(1)); uint8 internal constant _PRE_HOOK_FUNCTION_ID_1 = 1; uint8 internal constant _POST_HOOK_FUNCTION_ID_2 = 2; - uint8 internal constant _PRE_HOOK_FUNCTION_ID_3 = 3; - uint8 internal constant _POST_HOOK_FUNCTION_ID_4 = 4; + uint8 internal constant _BOTH_HOOKS_FUNCTION_ID_3 = 3; PluginManifest public m1; PluginManifest public m2; @@ -55,13 +53,12 @@ contract AccountExecHooksTest is AccountTestBase { function test_preExecHook_install() public { _installPlugin1WithHooks( - _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, + ManifestExecutionHook({ + executionSelector: _EXEC_SELECTOR, functionId: _PRE_HOOK_FUNCTION_ID_1, - dependencyIndex: 0 - }), - ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}) + isPreHook: true, + isPostHook: false + }) ); } @@ -94,16 +91,11 @@ contract AccountExecHooksTest is AccountTestBase { function test_execHookPair_install() public { _installPlugin1WithHooks( - _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _PRE_HOOK_FUNCTION_ID_1, - dependencyIndex: 0 - }), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _POST_HOOK_FUNCTION_ID_2, - dependencyIndex: 0 + ManifestExecutionHook({ + executionSelector: _EXEC_SELECTOR, + functionId: _BOTH_HOOKS_FUNCTION_ID_3, + isPreHook: true, + isPostHook: true }) ); } @@ -118,7 +110,7 @@ contract AccountExecHooksTest is AccountTestBase { emit ReceivedCall( abi.encodeWithSelector( IPlugin.preExecutionHook.selector, - _PRE_HOOK_FUNCTION_ID_1, + _BOTH_HOOKS_FUNCTION_ID_3, address(this), // caller 0, // msg.value in call to account abi.encodeWithSelector(_EXEC_SELECTOR) @@ -131,7 +123,7 @@ contract AccountExecHooksTest is AccountTestBase { vm.expectEmit(true, true, true, true); // post hook call emit ReceivedCall( - abi.encodeCall(IPlugin.postExecutionHook, (_POST_HOOK_FUNCTION_ID_2, "")), + abi.encodeCall(IPlugin.postExecutionHook, (_BOTH_HOOKS_FUNCTION_ID_3, "")), 0 // msg value in call to plugin ); @@ -147,12 +139,11 @@ contract AccountExecHooksTest is AccountTestBase { function test_postOnlyExecHook_install() public { _installPlugin1WithHooks( - _EXEC_SELECTOR, - ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, + ManifestExecutionHook({ + executionSelector: _EXEC_SELECTOR, functionId: _POST_HOOK_FUNCTION_ID_2, - dependencyIndex: 0 + isPreHook: false, + isPostHook: true }) ); } @@ -236,53 +227,8 @@ contract AccountExecHooksTest is AccountTestBase { ); } - function test_execHookDependencies_failInstall() public { - // Install the first plugin. - _installPlugin1WithHooks( - _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _PRE_HOOK_FUNCTION_ID_1, - dependencyIndex: 0 - }), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _POST_HOOK_FUNCTION_ID_2, - dependencyIndex: 0 - }) - ); - - // Attempt to install a second plugin that applies the first plugin's hook pair (as dependencies) to the - // same selector. This should revert. - FunctionReference[] memory dependencies = new FunctionReference[](2); - dependencies[0] = FunctionReferenceLib.pack(address(mockPlugin1), _PRE_HOOK_FUNCTION_ID_1); - dependencies[1] = FunctionReferenceLib.pack(address(mockPlugin1), _POST_HOOK_FUNCTION_ID_2); - - _installPlugin2WithHooksExpectFail( - _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.DEPENDENCY, - functionId: 0, - dependencyIndex: 0 - }), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.DEPENDENCY, - functionId: 0, - dependencyIndex: 1 - }), - dependencies, - abi.encodePacked(PluginManagerInternals.InvalidPluginManifest.selector) - ); - - vm.stopPrank(); - } - - function _installPlugin1WithHooks( - bytes4 selector, - ManifestFunction memory preHook, - ManifestFunction memory postHook - ) internal { - m1.executionHooks.push(ManifestExecutionHook(selector, preHook, postHook)); + function _installPlugin1WithHooks(ManifestExecutionHook memory execHooks) internal { + m1.executionHooks.push(execHooks); mockPlugin1 = new MockPlugin(m1); manifestHash1 = keccak256(abi.encode(mockPlugin1.pluginManifest())); @@ -322,65 +268,6 @@ contract AccountExecHooksTest is AccountTestBase { }); } - function _installPlugin2WithHooksExpectSuccess( - bytes4 selector, - ManifestFunction memory preHook, - ManifestFunction memory postHook, - FunctionReference[] memory dependencies - ) internal { - if (preHook.functionType == ManifestAssociatedFunctionType.DEPENDENCY) { - m2.dependencyInterfaceIds.push(type(IPlugin).interfaceId); - } - if (postHook.functionType == ManifestAssociatedFunctionType.DEPENDENCY) { - m2.dependencyInterfaceIds.push(type(IPlugin).interfaceId); - } - - m2.executionHooks.push(ManifestExecutionHook(selector, preHook, postHook)); - - mockPlugin2 = new MockPlugin(m2); - manifestHash2 = keccak256(abi.encode(mockPlugin2.pluginManifest())); - - vm.expectEmit(true, true, true, true); - emit ReceivedCall(abi.encodeCall(IPlugin.onInstall, (bytes(""))), 0); - vm.expectEmit(true, true, true, true); - emit PluginInstalled(address(mockPlugin2), manifestHash2, dependencies); - - account1.installPlugin({ - plugin: address(mockPlugin2), - manifestHash: manifestHash2, - pluginInstallData: bytes(""), - dependencies: dependencies - }); - } - - function _installPlugin2WithHooksExpectFail( - bytes4 selector, - ManifestFunction memory preHook, - ManifestFunction memory postHook, - FunctionReference[] memory dependencies, - bytes memory revertData - ) internal { - if (preHook.functionType == ManifestAssociatedFunctionType.DEPENDENCY) { - m2.dependencyInterfaceIds.push(type(IPlugin).interfaceId); - } - if (postHook.functionType == ManifestAssociatedFunctionType.DEPENDENCY) { - m2.dependencyInterfaceIds.push(type(IPlugin).interfaceId); - } - - m2.executionHooks.push(ManifestExecutionHook(selector, preHook, postHook)); - - mockPlugin2 = new MockPlugin(m2); - manifestHash2 = keccak256(abi.encode(mockPlugin2.pluginManifest())); - - vm.expectRevert(revertData); - account1.installPlugin({ - plugin: address(mockPlugin2), - manifestHash: manifestHash2, - pluginInstallData: bytes(""), - dependencies: dependencies - }); - } - function _installPlugin2WithPreValidationHook(bytes4 selector, ManifestFunction memory preValidationHook) internal { diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol index d56a8ff9..9fbc1e21 100644 --- a/test/account/AccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -4,19 +4,12 @@ pragma solidity ^0.8.19; import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; import {FunctionReference, FunctionReferenceLib} from "../../src/helpers/FunctionReferenceLib.sol"; -import { - ManifestAssociatedFunctionType, - ManifestExecutionHook, - ManifestFunction, - PluginManifest -} from "../../src/interfaces/IPlugin.sol"; -import {IAccountLoupe} from "../../src/interfaces/IAccountLoupe.sol"; +import {IAccountLoupe, ExecutionHook} from "../../src/interfaces/IAccountLoupe.sol"; import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; import {IStandardExecutor} from "../../src/interfaces/IStandardExecutor.sol"; import {ISingleOwnerPlugin} from "../../src/plugins/owner/ISingleOwnerPlugin.sol"; import {ComprehensivePlugin} from "../mocks/plugins/ComprehensivePlugin.sol"; -import {MockPlugin} from "../mocks/MockPlugin.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; contract AccountLoupeTest is AccountTestBase { @@ -107,82 +100,40 @@ contract AccountLoupeTest is AccountTestBase { } function test_pluginLoupe_getExecutionHooks() public { - IAccountLoupe.ExecutionHooks[] memory hooks = account1.getExecutionHooks(comprehensivePlugin.foo.selector); - - assertEq(hooks.length, 1); - assertEq( - FunctionReference.unwrap(hooks[0].preExecHook), - FunctionReference.unwrap( - FunctionReferenceLib.pack( + ExecutionHook[] memory hooks = account1.getExecutionHooks(comprehensivePlugin.foo.selector); + ExecutionHook[3] memory expectedHooks = [ + ExecutionHook({ + hookFunction: FunctionReferenceLib.pack( + address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.BOTH_EXECUTION_HOOKS) + ), + isPreHook: true, + isPostHook: true + }), + ExecutionHook({ + hookFunction: FunctionReferenceLib.pack( address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.PRE_EXECUTION_HOOK) - ) - ) - ); - assertEq( - FunctionReference.unwrap(hooks[0].postExecHook), - FunctionReference.unwrap( - FunctionReferenceLib.pack( - address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.POST_EXECUTION_HOOK) - ) - ) - ); - } - - function test_pluginLoupe_getHooks_multiple() public { - // Add a second set of execution hooks to the account, and validate that it can return all hooks applied - // over the function. - - PluginManifest memory mockPluginManifest; - - mockPluginManifest.executionHooks = new ManifestExecutionHook[](1); - mockPluginManifest.executionHooks[0] = ManifestExecutionHook({ - executionSelector: ComprehensivePlugin.foo.selector, - preExecHook: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: 0, - dependencyIndex: 0 + ), + isPreHook: true, + isPostHook: false }), - postExecHook: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: 0, - dependencyIndex: 0 + ExecutionHook({ + hookFunction: FunctionReferenceLib.pack( + address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.POST_EXECUTION_HOOK) + ), + isPreHook: false, + isPostHook: true }) - }); - - MockPlugin mockPlugin = new MockPlugin(mockPluginManifest); - bytes32 manifestHash = keccak256(abi.encode(mockPlugin.pluginManifest())); - - account1.installPlugin(address(mockPlugin), manifestHash, "", new FunctionReference[](0)); - - // Assert that the returned execution hooks are what is expected + ]; - IAccountLoupe.ExecutionHooks[] memory hooks = account1.getExecutionHooks(comprehensivePlugin.foo.selector); - - assertEq(hooks.length, 2); - assertEq( - FunctionReference.unwrap(hooks[0].preExecHook), - FunctionReference.unwrap( - FunctionReferenceLib.pack( - address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.PRE_EXECUTION_HOOK) - ) - ) - ); - assertEq( - FunctionReference.unwrap(hooks[0].postExecHook), - FunctionReference.unwrap( - FunctionReferenceLib.pack( - address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.POST_EXECUTION_HOOK) - ) - ) - ); - assertEq( - FunctionReference.unwrap(hooks[1].preExecHook), - FunctionReference.unwrap(FunctionReferenceLib.pack(address(mockPlugin), uint8(0))) - ); - assertEq( - FunctionReference.unwrap(hooks[1].postExecHook), - FunctionReference.unwrap(FunctionReferenceLib.pack(address(mockPlugin), uint8(0))) - ); + assertEq(hooks.length, 3); + for (uint256 i = 0; i < hooks.length; i++) { + assertEq( + FunctionReference.unwrap(hooks[i].hookFunction), + FunctionReference.unwrap(expectedHooks[i].hookFunction) + ); + assertEq(hooks[i].isPreHook, expectedHooks[i].isPreHook); + assertEq(hooks[i].isPostHook, expectedHooks[i].isPostHook); + } } function test_pluginLoupe_getValidationHooks() public { diff --git a/test/account/ManifestValidity.t.sol b/test/account/ManifestValidity.t.sol index 2631cf1c..21023ef3 100644 --- a/test/account/ManifestValidity.t.sol +++ b/test/account/ManifestValidity.t.sol @@ -6,11 +6,8 @@ import {FunctionReference} from "../../src/helpers/FunctionReferenceLib.sol"; import { BadValidationMagicValue_PreValidationHook_Plugin, - BadValidationMagicValue_PreExecHook_Plugin, - BadValidationMagicValue_PostExecHook_Plugin, BadHookMagicValue_UserOpValidationFunction_Plugin, - BadHookMagicValue_RuntimeValidationFunction_Plugin, - BadHookMagicValue_PostExecHook_Plugin + BadHookMagicValue_RuntimeValidationFunction_Plugin } from "../mocks/plugins/ManifestValidityMocks.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; @@ -36,36 +33,6 @@ contract ManifestValidityTest is AccountTestBase { }); } - // Tests that the plugin manager rejects a plugin with a pre-execution hook set to "validation always allow" - function test_ManifestValidity_invalid_ValidationAlwaysAllow_PreExecHook() public { - BadValidationMagicValue_PreExecHook_Plugin plugin = new BadValidationMagicValue_PreExecHook_Plugin(); - - bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - - vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); - account1.installPlugin({ - plugin: address(plugin), - manifestHash: manifestHash, - pluginInstallData: "", - dependencies: new FunctionReference[](0) - }); - } - - // Tests that the plugin manager rejects a plugin with a post-execution hook set to "validation always allow" - function test_ManifestValidity_invalid_ValidationAlwaysAllow_PostExecHook() public { - BadValidationMagicValue_PostExecHook_Plugin plugin = new BadValidationMagicValue_PostExecHook_Plugin(); - - bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - - vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); - account1.installPlugin({ - plugin: address(plugin), - manifestHash: manifestHash, - pluginInstallData: "", - dependencies: new FunctionReference[](0) - }); - } - // Tests that the plugin manager rejects a plugin with a user op validationFunction set to "hook always deny" function test_ManifestValidity_invalid_HookAlwaysDeny_UserOpValidation() public { BadHookMagicValue_UserOpValidationFunction_Plugin plugin = @@ -97,19 +64,4 @@ contract ManifestValidityTest is AccountTestBase { dependencies: new FunctionReference[](0) }); } - - // Tests that the plugin manager rejects a plugin with a post-execution hook set to "hook always deny" - function test_ManifestValidity_invalid_HookAlwaysDeny_PostExecHook() public { - BadHookMagicValue_PostExecHook_Plugin plugin = new BadHookMagicValue_PostExecHook_Plugin(); - - bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - - vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); - account1.installPlugin({ - plugin: address(plugin), - manifestHash: manifestHash, - pluginInstallData: "", - dependencies: new FunctionReference[](0) - }); - } } diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index 2e2f2441..87f85210 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -19,10 +19,9 @@ contract ComprehensivePlugin is BasePlugin { PRE_VALIDATION_HOOK_1, PRE_VALIDATION_HOOK_2, VALIDATION, + BOTH_EXECUTION_HOOKS, PRE_EXECUTION_HOOK, - PRE_PERMITTED_CALL_EXECUTION_HOOK, - POST_EXECUTION_HOOK, - POST_PERMITTED_CALL_EXECUTION_HOOK + POST_EXECUTION_HOOK } string public constant NAME = "Comprehensive Plugin"; @@ -97,7 +96,7 @@ contract ComprehensivePlugin is BasePlugin { { if (functionId == uint8(FunctionId.PRE_EXECUTION_HOOK)) { return ""; - } else if (functionId == uint8(FunctionId.PRE_PERMITTED_CALL_EXECUTION_HOOK)) { + } else if (functionId == uint8(FunctionId.BOTH_EXECUTION_HOOKS)) { return ""; } revert NotImplemented(); @@ -106,7 +105,7 @@ contract ComprehensivePlugin is BasePlugin { function postExecutionHook(uint8 functionId, bytes calldata) external pure override { if (functionId == uint8(FunctionId.POST_EXECUTION_HOOK)) { return; - } else if (functionId == uint8(FunctionId.POST_PERMITTED_CALL_EXECUTION_HOOK)) { + } else if (functionId == uint8(FunctionId.BOTH_EXECUTION_HOOKS)) { return; } revert NotImplemented(); @@ -163,19 +162,24 @@ contract ComprehensivePlugin is BasePlugin { }) }); - manifest.executionHooks = new ManifestExecutionHook[](1); + manifest.executionHooks = new ManifestExecutionHook[](3); manifest.executionHooks[0] = ManifestExecutionHook({ executionSelector: this.foo.selector, - preExecHook: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_EXECUTION_HOOK), - dependencyIndex: 0 // Unused. - }), - postExecHook: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.POST_EXECUTION_HOOK), - dependencyIndex: 0 // Unused. - }) + functionId: uint8(FunctionId.BOTH_EXECUTION_HOOKS), + isPreHook: true, + isPostHook: true + }); + manifest.executionHooks[1] = ManifestExecutionHook({ + executionSelector: this.foo.selector, + functionId: uint8(FunctionId.PRE_EXECUTION_HOOK), + isPreHook: true, + isPostHook: false + }); + manifest.executionHooks[2] = ManifestExecutionHook({ + executionSelector: this.foo.selector, + functionId: uint8(FunctionId.POST_EXECUTION_HOOK), + isPreHook: false, + isPostHook: true }); return manifest; diff --git a/test/mocks/plugins/ManifestValidityMocks.sol b/test/mocks/plugins/ManifestValidityMocks.sol index c467f2ef..ed5370fb 100644 --- a/test/mocks/plugins/ManifestValidityMocks.sol +++ b/test/mocks/plugins/ManifestValidityMocks.sol @@ -5,7 +5,6 @@ import { ManifestFunction, ManifestAssociatedFunctionType, ManifestAssociatedFunction, - ManifestExecutionHook, PluginManifest } from "../../../src/interfaces/IPlugin.sol"; @@ -52,79 +51,6 @@ contract BadValidationMagicValue_PreValidationHook_Plugin is BaseTestPlugin { } } -// solhint-disable-next-line contract-name-camelcase -contract BadValidationMagicValue_PreExecHook_Plugin is BaseTestPlugin { - function onInstall(bytes calldata) external override {} - - function onUninstall(bytes calldata) external override {} - - function foo() external pure returns (bytes32) { - return keccak256("bar"); - } - - function pluginManifest() external pure override returns (PluginManifest memory) { - PluginManifest memory manifest; - - manifest.executionFunctions = new bytes4[](1); - manifest.executionFunctions[0] = this.foo.selector; - - manifest.executionHooks = new ManifestExecutionHook[](1); - - // Illegal assignment: validation always allow only usable on runtime validation functions - manifest.executionHooks[0] = ManifestExecutionHook({ - executionSelector: this.foo.selector, - preExecHook: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - functionId: 0, - dependencyIndex: 0 - }), - postExecHook: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: 0, // Dummy unimplemented function id, but can be added correctly - dependencyIndex: 0 - }) - }); - - return manifest; - } -} - -// solhint-disable-next-line contract-name-camelcase -contract BadValidationMagicValue_PostExecHook_Plugin is BaseTestPlugin { - function onInstall(bytes calldata) external override {} - - function onUninstall(bytes calldata) external override {} - - function foo() external pure returns (bytes32) { - return keccak256("bar"); - } - - function pluginManifest() external pure override returns (PluginManifest memory) { - PluginManifest memory manifest; - - manifest.executionFunctions = new bytes4[](1); - manifest.executionFunctions[0] = this.foo.selector; - - manifest.executionHooks = new ManifestExecutionHook[](1); - // Illegal assignment: validation always allow only usable on runtime validation functions - manifest.executionHooks[0] = ManifestExecutionHook({ - executionSelector: this.foo.selector, - preExecHook: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: 0, // Dummy unimplemented function id, but can be added correctly - dependencyIndex: 0 - }), - postExecHook: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - functionId: 0, - dependencyIndex: 0 - }) - }); - - return manifest; - } -} - // solhint-disable-next-line contract-name-camelcase contract BadHookMagicValue_UserOpValidationFunction_Plugin is BaseTestPlugin { function onInstall(bytes calldata) external override {} @@ -184,39 +110,3 @@ contract BadHookMagicValue_RuntimeValidationFunction_Plugin is BaseTestPlugin { return manifest; } } - -// solhint-disable-next-line contract-name-camelcase -contract BadHookMagicValue_PostExecHook_Plugin is BaseTestPlugin { - function onInstall(bytes calldata) external override {} - - function onUninstall(bytes calldata) external override {} - - function foo() external pure returns (bytes32) { - return keccak256("bar"); - } - - function pluginManifest() external pure override returns (PluginManifest memory) { - PluginManifest memory manifest; - - manifest.executionFunctions = new bytes4[](1); - manifest.executionFunctions[0] = this.foo.selector; - - manifest.executionHooks = new ManifestExecutionHook[](1); - // Illegal assignment: hook always deny only usable on runtime validation functions - manifest.executionHooks[0] = ManifestExecutionHook({ - executionSelector: this.foo.selector, - preExecHook: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: 0, // Dummy unimplemented function id, but can be added correctly - dependencyIndex: 0 - }), - postExecHook: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, - functionId: 0, - dependencyIndex: 0 - }) - }); - - return manifest; - } -}