From db9960fd428cb5066e414d53aa59dbc9fe8e4086 Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Fri, 7 Jun 2024 11:54:49 -0400 Subject: [PATCH 1/7] chore: remove existing permissions system --- src/account/AccountStorage.sol | 3 +-- src/account/PluginManagerInternals.sol | 13 ------------- src/account/UpgradeableModularAccount.sol | 8 +++----- src/interfaces/IPlugin.sol | 2 -- test/account/UpgradeableModularAccount.t.sol | 4 +--- test/mocks/plugins/PermittedCallMocks.sol | 4 ---- test/mocks/plugins/ReturnDataPluginMocks.sol | 3 --- 7 files changed, 5 insertions(+), 32 deletions(-) diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 0a992a33..5a3b8c12 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -51,8 +51,7 @@ struct AccountStorage { mapping(address => PluginData) pluginData; // Execution functions and their associated functions mapping(bytes4 => SelectorData) selectorData; - mapping(FunctionReference validationFunction => ValidationData) validationData; - mapping(address caller => mapping(bytes4 selector => bool)) callPermitted; + mapping(FunctionReference => ValidationData) validationData; // For ERC165 introspection mapping(bytes4 => uint256) supportedIfaces; } diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 2ec06f81..05943e09 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -212,14 +212,6 @@ abstract contract PluginManagerInternals is IPluginManager { _setExecutionFunction(selector, isPublic, allowDefaultValidation, plugin); } - // Add installed plugin and selectors this plugin can call - length = manifest.permittedExecutionSelectors.length; - for (uint256 i = 0; i < length; ++i) { - // If there are duplicates, this will just enable the flag again. This is not a problem, since the - // boolean will be set to false twice during uninstall, which is fine. - _storage.callPermitted[plugin][manifest.permittedExecutionSelectors[i]] = true; - } - length = manifest.validationFunctions.length; for (uint256 i = 0; i < length; ++i) { ManifestAssociatedFunction memory mv = manifest.validationFunctions[i]; @@ -313,11 +305,6 @@ abstract contract PluginManagerInternals is IPluginManager { ); } - length = manifest.permittedExecutionSelectors.length; - for (uint256 i = 0; i < length; ++i) { - _storage.callPermitted[plugin][manifest.permittedExecutionSelectors[i]] = false; - } - length = manifest.executionFunctions.length; for (uint256 i = 0; i < length; ++i) { bytes4 selector = manifest.executionFunctions[i].executionSelector; diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 169b17a3..0c67ac81 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -555,11 +555,9 @@ contract UpgradeableModularAccount is AccountStorage storage _storage = getAccountStorage(); if ( - msg.sender == address(_ENTRY_POINT) || msg.sender == address(this) - || _storage.selectorData[msg.sig].isPublic - ) return; - - if (!_storage.callPermitted[msg.sender][msg.sig]) { + msg.sender != address(_ENTRY_POINT) && msg.sender != address(this) + && !_storage.selectorData[msg.sig].isPublic + ) { revert ExecFromPluginNotPermitted(msg.sender, msg.sig); } } diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index 3241e497..22d74546 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -73,8 +73,6 @@ struct PluginManifest { ManifestAssociatedFunction[] validationFunctions; ManifestExecutionHook[] executionHooks; uint8[] signatureValidationFunctions; - // Plugin execution functions already installed on the MSCA that this plugin will be able to call. - bytes4[] permittedExecutionSelectors; // List of ERC-165 interface IDs to add to account to support introspection checks. This MUST NOT include // IPlugin's interface ID. bytes4[] interfaceIds; diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index e3d09e7f..46ea1d5f 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -11,7 +11,7 @@ import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; import {PluginManagerInternals} from "../../src/account/PluginManagerInternals.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {FunctionReference, FunctionReferenceLib} from "../../src/helpers/FunctionReferenceLib.sol"; -import {IPlugin, PluginManifest} from "../../src/interfaces/IPlugin.sol"; +import {PluginManifest} from "../../src/interfaces/IPlugin.sol"; import {IAccountLoupe} from "../../src/interfaces/IAccountLoupe.sol"; import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; import {Call} from "../../src/interfaces/IStandardExecutor.sol"; @@ -262,8 +262,6 @@ contract UpgradeableModularAccountTest is AccountTestBase { vm.startPrank(address(entryPoint)); PluginManifest memory m; - m.permittedExecutionSelectors = new bytes4[](1); - m.permittedExecutionSelectors[0] = IPlugin.onInstall.selector; MockPlugin mockPluginWithBadPermittedExec = new MockPlugin(m); bytes32 manifestHash = keccak256(abi.encode(mockPluginWithBadPermittedExec.pluginManifest())); diff --git a/test/mocks/plugins/PermittedCallMocks.sol b/test/mocks/plugins/PermittedCallMocks.sol index 450a70fb..77548225 100644 --- a/test/mocks/plugins/PermittedCallMocks.sol +++ b/test/mocks/plugins/PermittedCallMocks.sol @@ -22,10 +22,6 @@ contract PermittedCallerPlugin is BasePlugin { manifest.executionFunctions[i].isPublic = true; } - // Request permission only for "foo", but not "bar", from ResultCreatorPlugin - manifest.permittedExecutionSelectors = new bytes4[](1); - manifest.permittedExecutionSelectors[0] = ResultCreatorPlugin.foo.selector; - return manifest; } diff --git a/test/mocks/plugins/ReturnDataPluginMocks.sol b/test/mocks/plugins/ReturnDataPluginMocks.sol index dae2c8e4..485b8456 100644 --- a/test/mocks/plugins/ReturnDataPluginMocks.sol +++ b/test/mocks/plugins/ReturnDataPluginMocks.sol @@ -138,9 +138,6 @@ contract ResultConsumerPlugin is BasePlugin, IValidation { allowDefaultValidation: false }); - manifest.permittedExecutionSelectors = new bytes4[](1); - manifest.permittedExecutionSelectors[0] = ResultCreatorPlugin.foo.selector; - return manifest; } From 538375b5bb78d75a87a7b8f2b39a897f7890240a Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Wed, 26 Jun 2024 17:05:19 -0400 Subject: [PATCH 2/7] fix: revert change --- src/account/AccountStorage.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 5a3b8c12..5d08ab6d 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -51,7 +51,7 @@ struct AccountStorage { mapping(address => PluginData) pluginData; // Execution functions and their associated functions mapping(bytes4 => SelectorData) selectorData; - mapping(FunctionReference => ValidationData) validationData; + mapping(FunctionReference validationFunction => ValidationData) validationData; // For ERC165 introspection mapping(bytes4 => uint256) supportedIfaces; } From 420e485d83502e1abb8a0eda42e7083fcba8e138 Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Fri, 7 Jun 2024 11:54:49 -0400 Subject: [PATCH 3/7] feat: add permission hooks to manifest and install flow and cache required context count --- src/account/AccountLoupe.sol | 30 ++++++++++++--- src/account/AccountStorage.sol | 10 ++++- src/account/PluginManagerInternals.sol | 35 +++++++++++------ src/account/UpgradeableModularAccount.sol | 43 ++++++++++++++------- src/interfaces/IAccountLoupe.sol | 9 +++++ src/interfaces/IExecutionHook.sol | 9 ++--- src/interfaces/IPlugin.sol | 3 ++ test/account/AccountExecHooks.t.sol | 45 ++++++++++++++++------ test/account/AccountLoupe.t.sol | 9 +++-- test/mocks/plugins/ComprehensivePlugin.sol | 16 ++++---- 10 files changed, 148 insertions(+), 61 deletions(-) diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol index 9cde18b4..99b5bd14 100644 --- a/src/account/AccountLoupe.sol +++ b/src/account/AccountLoupe.sol @@ -7,7 +7,7 @@ import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet import {IAccountLoupe, ExecutionHook} from "../interfaces/IAccountLoupe.sol"; import {FunctionReference, IPluginManager} from "../interfaces/IPluginManager.sol"; import {IStandardExecutor} from "../interfaces/IStandardExecutor.sol"; -import {getAccountStorage, SelectorData, toFunctionReferenceArray, toExecutionHook} from "./AccountStorage.sol"; +import {getAccountStorage, toFunctionReferenceArray, toExecutionHook} from "./AccountStorage.sol"; abstract contract AccountLoupe is IAccountLoupe { using EnumerableSet for EnumerableSet.Bytes32Set; @@ -39,15 +39,35 @@ abstract contract AccountLoupe is IAccountLoupe { override returns (ExecutionHook[] memory execHooks) { - SelectorData storage selectorData = getAccountStorage().selectorData[selector]; - uint256 executionHooksLength = selectorData.executionHooks.length(); + EnumerableSet.Bytes32Set storage hooks = getAccountStorage().selectorData[selector].executionHooks; + uint256 executionHooksLength = hooks.length(); execHooks = new ExecutionHook[](executionHooksLength); for (uint256 i = 0; i < executionHooksLength; ++i) { - bytes32 key = selectorData.executionHooks.at(i); + bytes32 key = hooks.at(i); ExecutionHook memory execHook = execHooks[i]; - (execHook.hookFunction, execHook.isPreHook, execHook.isPostHook) = toExecutionHook(key); + (execHook.hookFunction, execHook.isPreHook, execHook.isPostHook, execHook.requireUOContext) = + toExecutionHook(key); + } + } + + /// @inheritdoc IAccountLoupe + function getPermissionHooks(FunctionReference validationFunction) + external + view + override + returns (ExecutionHook[] memory permissionHooks) + { + EnumerableSet.Bytes32Set storage hooks = + getAccountStorage().validationData[validationFunction].permissionHooks; + uint256 executionHooksLength = hooks.length(); + permissionHooks = new ExecutionHook[](executionHooksLength); + for (uint256 i = 0; i < executionHooksLength; ++i) { + bytes32 key = hooks.at(i); + ExecutionHook memory execHook = permissionHooks[i]; + (execHook.hookFunction, execHook.isPreHook, execHook.isPostHook, execHook.requireUOContext) = + toExecutionHook(key); } } diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 5d08ab6d..70de42af 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -38,8 +38,12 @@ struct ValidationData { bool isDefault; // Whether or not this validation is a signature validator. bool isSignatureValidation; + // How many execution hooks require the UO context. + uint8 requireUOHookCount; // The pre validation hooks for this function selector. EnumerableSet.Bytes32Set preValidationHooks; + // Permission hooks for this validation function. + EnumerableSet.Bytes32Set permissionHooks; } struct AccountStorage { @@ -80,16 +84,18 @@ function toFunctionReference(bytes32 setValue) pure returns (FunctionReference) 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); + | bytes32(executionHook.isPostHook ? uint256(1) << 72 : 0) + | bytes32(executionHook.requireUOContext ? uint256(1) << 64 : 0); } function toExecutionHook(bytes32 setValue) pure - returns (FunctionReference hookFunction, bool isPreHook, bool isPostHook) + returns (FunctionReference hookFunction, bool isPreHook, bool isPostHook, bool requireUOContext) { hookFunction = FunctionReference.wrap(bytes21(setValue)); isPreHook = (uint256(setValue) >> 80) & 0xFF == 1; isPostHook = (uint256(setValue) >> 72) & 0xFF == 1; + requireUOContext = (uint256(setValue) >> 64) & 0xFF == 1; } /// @dev Helper function to get all elements of a set into memory. diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 05943e09..a2974e85 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -125,27 +125,39 @@ abstract contract PluginManagerInternals is IPluginManager { } function _addExecHooks( - bytes4 selector, + EnumerableSet.Bytes32Set storage hooks, FunctionReference hookFunction, bool isPreExecHook, - bool isPostExecHook + bool isPostExecHook, + bool requireUOContext ) internal { - getAccountStorage().selectorData[selector].executionHooks.add( + hooks.add( toSetValue( - ExecutionHook({hookFunction: hookFunction, isPreHook: isPreExecHook, isPostHook: isPostExecHook}) + ExecutionHook({ + hookFunction: hookFunction, + isPreHook: isPreExecHook, + isPostHook: isPostExecHook, + requireUOContext: requireUOContext + }) ) ); } function _removeExecHooks( - bytes4 selector, + EnumerableSet.Bytes32Set storage hooks, FunctionReference hookFunction, bool isPreExecHook, - bool isPostExecHook + bool isPostExecHook, + bool requireUOContext ) internal { - getAccountStorage().selectorData[selector].executionHooks.remove( + hooks.remove( toSetValue( - ExecutionHook({hookFunction: hookFunction, isPreHook: isPreExecHook, isPostHook: isPostExecHook}) + ExecutionHook({ + hookFunction: hookFunction, + isPreHook: isPreExecHook, + isPostHook: isPostExecHook, + requireUOContext: requireUOContext + }) ) ); } @@ -230,8 +242,9 @@ abstract contract PluginManagerInternals is IPluginManager { length = manifest.executionHooks.length; for (uint256 i = 0; i < length; ++i) { ManifestExecutionHook memory mh = manifest.executionHooks[i]; + EnumerableSet.Bytes32Set storage execHooks = _storage.selectorData[mh.executionSelector].executionHooks; FunctionReference hookFunction = FunctionReferenceLib.pack(plugin, mh.functionId); - _addExecHooks(mh.executionSelector, hookFunction, mh.isPreHook, mh.isPostHook); + _addExecHooks(execHooks, hookFunction, mh.isPreHook, mh.isPostHook, mh.requireUOContext); } length = manifest.interfaceIds.length; @@ -282,12 +295,12 @@ abstract contract PluginManagerInternals is IPluginManager { } // Remove components according to the manifest, in reverse order (by component type) of their installation. - length = manifest.executionHooks.length; for (uint256 i = 0; i < length; ++i) { ManifestExecutionHook memory mh = manifest.executionHooks[i]; FunctionReference hookFunction = FunctionReferenceLib.pack(plugin, mh.functionId); - _removeExecHooks(mh.executionSelector, hookFunction, mh.isPreHook, mh.isPostHook); + EnumerableSet.Bytes32Set storage execHooks = _storage.selectorData[mh.executionSelector].executionHooks; + _removeExecHooks(execHooks, hookFunction, mh.isPreHook, mh.isPostHook, mh.requireUOContext); } length = manifest.signatureValidationFunctions.length; diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 0c67ac81..cd938c93 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -22,7 +22,6 @@ import {AccountLoupe} from "./AccountLoupe.sol"; import { AccountStorage, getAccountStorage, - SelectorData, toSetValue, toFunctionReference, toExecutionHook @@ -70,6 +69,7 @@ contract UpgradeableModularAccount is error PostExecHookReverted(address plugin, uint8 functionId, bytes revertReason); error PreExecHookReverted(address plugin, uint8 functionId, bytes revertReason); error PreRuntimeValidationHookFailed(address plugin, uint8 functionId, bytes revertReason); + error RequireUserOperationContext(); error RuntimeValidationFunctionMissing(bytes4 selector); error RuntimeValidationFunctionReverted(address plugin, uint8 functionId, bytes revertReason); error SignatureValidationInvalid(address plugin, uint8 functionId); @@ -83,7 +83,8 @@ contract UpgradeableModularAccount is modifier wrapNativeFunction() { _checkPermittedCallerIfNotFromEP(); - PostExecToRun[] memory postExecHooks = _doPreExecHooks(msg.sig, msg.data); + PostExecToRun[] memory postExecHooks = + _doPreExecHooks(getAccountStorage().selectorData[msg.sig].executionHooks, msg.data); _; @@ -137,7 +138,7 @@ contract UpgradeableModularAccount is PostExecToRun[] memory postExecHooks; // Cache post-exec hooks in memory - postExecHooks = _doPreExecHooks(msg.sig, msg.data); + postExecHooks = _doPreExecHooks(getAccountStorage().selectorData[msg.sig].executionHooks, msg.data); // execute the function, bubbling up any reverts (bool execSuccess, bytes memory execReturnData) = execPlugin.call(msg.data); @@ -350,6 +351,17 @@ contract UpgradeableModularAccount is _checkIfValidationApplies(selector, userOpValidationFunction, isDefaultValidation); + // Check if there are exec hooks associated with the validator that require UO context, and revert if the + // call isn't to `executeUserOp` + // This check must be here because if context isn't passed, we wouldn't be able to get the exec hooks + // associated with the validator + if (getAccountStorage().validationData[userOpValidationFunction].requireUOHookCount > 0) { + /** + * && msg.sig != this.executeUserOp.selector + */ + revert RequireUserOperationContext(); + } + validationData = _doUserOpValidation(selector, userOpValidationFunction, userOp, userOp.signature[22:], userOpHash); } @@ -441,13 +453,11 @@ contract UpgradeableModularAccount is } } - function _doPreExecHooks(bytes4 selector, bytes calldata data) + function _doPreExecHooks(EnumerableSet.Bytes32Set storage executionHooks, bytes calldata data) internal returns (PostExecToRun[] memory postHooksToRun) { - SelectorData storage selectorData = getAccountStorage().selectorData[selector]; - - uint256 hooksLength = selectorData.executionHooks.length(); + uint256 hooksLength = executionHooks.length(); // Overallocate on length - not all of this may get filled up. We set the correct length later. postHooksToRun = new PostExecToRun[](hooksLength); @@ -455,8 +465,14 @@ contract UpgradeableModularAccount is // 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. for (uint256 i = 0; i < hooksLength; ++i) { - bytes32 key = selectorData.executionHooks.at(i); - (FunctionReference hookFunction,, bool isPostHook) = toExecutionHook(key); + bytes32 key = executionHooks.at(i); + (FunctionReference hookFunction,, bool isPostHook, bool requireUOContext) = toExecutionHook(key); + if (requireUOContext) { + /** + * && msg.sig != this.executeUserOp.selector + */ + revert RequireUserOperationContext(); + } if (isPostHook) { postHooksToRun[i].postExecHook = hookFunction; } @@ -465,8 +481,8 @@ contract UpgradeableModularAccount is // 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 < hooksLength; ++i) { - bytes32 key = selectorData.executionHooks.at(i); - (FunctionReference hookFunction, bool isPreHook, bool isPostHook) = toExecutionHook(key); + bytes32 key = executionHooks.at(i); + (FunctionReference hookFunction, bool isPreHook, bool isPostHook,) = toExecutionHook(key); if (isPreHook) { bytes memory preExecHookReturnData = _runPreExecHook(hookFunction, data); @@ -484,9 +500,8 @@ contract UpgradeableModularAccount is returns (bytes memory preExecHookReturnData) { (address plugin, uint8 functionId) = preExecHook.unpack(); - try IExecutionHook(plugin).preExecutionHook(functionId, msg.sender, msg.value, data) returns ( - bytes memory returnData - ) { + try IExecutionHook(plugin).preExecutionHook(functionId, abi.encodePacked(msg.sender, msg.value, data)) + returns (bytes memory returnData) { preExecHookReturnData = returnData; } catch (bytes memory revertReason) { revert PreExecHookReverted(plugin, functionId, revertReason); diff --git a/src/interfaces/IAccountLoupe.sol b/src/interfaces/IAccountLoupe.sol index 490b216c..1f1c354e 100644 --- a/src/interfaces/IAccountLoupe.sol +++ b/src/interfaces/IAccountLoupe.sol @@ -9,6 +9,7 @@ struct ExecutionHook { FunctionReference hookFunction; bool isPreHook; bool isPostHook; + bool requireUOContext; } interface IAccountLoupe { @@ -28,6 +29,14 @@ interface IAccountLoupe { /// @return The pre and post execution hooks for this selector. function getExecutionHooks(bytes4 selector) external view returns (ExecutionHook[] memory); + /// @notice Get the pre and post execution hooks for a validation function. + /// @param validationFunction The validation function to get the hooks for. + /// @return The pre and post execution hooks for this validation function. + function getPermissionHooks(FunctionReference validationFunction) + external + view + returns (ExecutionHook[] memory); + /// @notice Get the pre user op and runtime validation hooks associated with a selector. /// @param validationFunction The validation function to get the hooks for. /// @return preValidationHooks The pre validation hooks for this selector. diff --git a/src/interfaces/IExecutionHook.sol b/src/interfaces/IExecutionHook.sol index 3240c489..b0dd40c2 100644 --- a/src/interfaces/IExecutionHook.sol +++ b/src/interfaces/IExecutionHook.sol @@ -8,13 +8,10 @@ interface IExecutionHook is IPlugin { /// @dev To indicate the entire call should revert, the function MUST revert. /// @param functionId An identifier that routes the call to different internal implementations, should there be /// more than one. - /// @param sender The caller address. - /// @param value The call value. - /// @param data The calldata sent. + /// @param data If hook requires UO context, data is abi.encode(PackedUserOperation), else its + /// abi.encodePacked(sender, value, calldata) /// @return Context to pass to a post execution hook, if present. An empty bytes array MAY be returned. - function preExecutionHook(uint8 functionId, address sender, uint256 value, bytes calldata data) - external - returns (bytes memory); + function preExecutionHook(uint8 functionId, bytes calldata data) external returns (bytes memory); /// @notice Run the post execution hook specified by the `functionId`. /// @dev To indicate the entire call should revert, the function MUST revert. diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index 22d74546..248f48af 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -45,6 +45,7 @@ struct ManifestExecutionHook { uint8 functionId; bool isPreHook; bool isPostHook; + bool requireUOContext; } struct SelectorPermission { @@ -64,6 +65,8 @@ struct PluginMetadata { // String desciptions of the relative sensitivity of specific functions. The selectors MUST be selectors for // functions implemented by this plugin. SelectorPermission[] permissionDescriptors; + // A list of all ERC-7715 permission strings that the plugin could possibly use + string[] permissionRequest; } /// @dev A struct describing how the plugin should be installed on a modular account. diff --git a/test/account/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol index 6a3b7d8c..82bf1ae7 100644 --- a/test/account/AccountExecHooks.t.sol +++ b/test/account/AccountExecHooks.t.sol @@ -9,13 +9,13 @@ import { } from "../../src/interfaces/IPlugin.sol"; import {IExecutionHook} from "../../src/interfaces/IExecutionHook.sol"; import {FunctionReference} from "../../src/helpers/FunctionReferenceLib.sol"; +import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {MockPlugin} from "../mocks/MockPlugin.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; contract AccountExecHooksTest is AccountTestBase { MockPlugin public mockPlugin1; - MockPlugin public mockPlugin2; bytes32 public manifestHash1; bytes32 public manifestHash2; @@ -25,7 +25,6 @@ contract AccountExecHooksTest is AccountTestBase { uint8 internal constant _BOTH_HOOKS_FUNCTION_ID_3 = 3; PluginManifest internal _m1; - PluginManifest internal _m2; event PluginInstalled(address indexed plugin, bytes32 manifestHash, FunctionReference[] dependencies); event PluginUninstalled(address indexed plugin, bool indexed callbacksSucceeded); @@ -50,7 +49,8 @@ contract AccountExecHooksTest is AccountTestBase { executionSelector: _EXEC_SELECTOR, functionId: _PRE_HOOK_FUNCTION_ID_1, isPreHook: true, - isPostHook: false + isPostHook: false, + requireUOContext: false }) ); } @@ -65,9 +65,11 @@ contract AccountExecHooksTest is AccountTestBase { abi.encodeWithSelector( IExecutionHook.preExecutionHook.selector, _PRE_HOOK_FUNCTION_ID_1, - address(this), // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) + abi.encodePacked( + address(this), // caller + uint256(0), // msg.value in call to account + abi.encodeWithSelector(_EXEC_SELECTOR) + ) ), 0 // msg value in call to plugin ); @@ -88,7 +90,8 @@ contract AccountExecHooksTest is AccountTestBase { executionSelector: _EXEC_SELECTOR, functionId: _BOTH_HOOKS_FUNCTION_ID_3, isPreHook: true, - isPostHook: true + isPostHook: true, + requireUOContext: false }) ); } @@ -104,9 +107,11 @@ contract AccountExecHooksTest is AccountTestBase { abi.encodeWithSelector( IExecutionHook.preExecutionHook.selector, _BOTH_HOOKS_FUNCTION_ID_3, - address(this), // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) + abi.encodePacked( + address(this), // caller + uint256(0), // msg.value in call to account + abi.encodeWithSelector(_EXEC_SELECTOR) + ) ), 0 // msg value in call to plugin ); @@ -136,7 +141,8 @@ contract AccountExecHooksTest is AccountTestBase { executionSelector: _EXEC_SELECTOR, functionId: _POST_HOOK_FUNCTION_ID_2, isPreHook: false, - isPostHook: true + isPostHook: true, + requireUOContext: false }) ); } @@ -162,6 +168,23 @@ contract AccountExecHooksTest is AccountTestBase { _uninstallPlugin(mockPlugin1); } + function test_requireUOContextHook() public { + _installPlugin1WithHooks( + ManifestExecutionHook({ + executionSelector: _EXEC_SELECTOR, + functionId: _POST_HOOK_FUNCTION_ID_2, + isPreHook: false, + isPostHook: true, + requireUOContext: true + }) + ); + + (bool success, bytes memory errMsg) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + assertFalse(success); + assertEq(errMsg.length, 4); + assertEq(bytes4(errMsg), UpgradeableModularAccount.RequireUserOperationContext.selector); + } + function _installPlugin1WithHooks(ManifestExecutionHook memory execHooks) internal { _m1.executionHooks.push(execHooks); mockPlugin1 = new MockPlugin(_m1); diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol index fa92ab00..e21f232f 100644 --- a/test/account/AccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -117,21 +117,24 @@ contract AccountLoupeTest is AccountTestBase { address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.BOTH_EXECUTION_HOOKS) ), isPreHook: true, - isPostHook: true + isPostHook: true, + requireUOContext: false }), ExecutionHook({ hookFunction: FunctionReferenceLib.pack( address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.PRE_EXECUTION_HOOK) ), isPreHook: true, - isPostHook: false + isPostHook: false, + requireUOContext: false }), ExecutionHook({ hookFunction: FunctionReferenceLib.pack( address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.POST_EXECUTION_HOOK) ), isPreHook: false, - isPostHook: true + isPostHook: true, + requireUOContext: false }) ]; diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index 6ef654c7..2b742648 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -105,12 +105,7 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba revert NotImplemented(); } - function preExecutionHook(uint8 functionId, address, uint256, bytes calldata) - external - pure - override - returns (bytes memory) - { + function preExecutionHook(uint8 functionId, bytes calldata) external pure override returns (bytes memory) { if (functionId == uint8(FunctionId.PRE_EXECUTION_HOOK)) { return ""; } else if (functionId == uint8(FunctionId.BOTH_EXECUTION_HOOKS)) { @@ -154,19 +149,22 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba executionSelector: this.foo.selector, functionId: uint8(FunctionId.BOTH_EXECUTION_HOOKS), isPreHook: true, - isPostHook: true + isPostHook: true, + requireUOContext: false }); manifest.executionHooks[1] = ManifestExecutionHook({ executionSelector: this.foo.selector, functionId: uint8(FunctionId.PRE_EXECUTION_HOOK), isPreHook: true, - isPostHook: false + isPostHook: false, + requireUOContext: false }); manifest.executionHooks[2] = ManifestExecutionHook({ executionSelector: this.foo.selector, functionId: uint8(FunctionId.POST_EXECUTION_HOOK), isPreHook: false, - isPostHook: true + isPostHook: true, + requireUOContext: false }); return manifest; From ce2330e5504feb94b5a81d7b89353e608f1cbd15 Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Wed, 26 Jun 2024 17:07:13 -0400 Subject: [PATCH 4/7] docs: update exec hook documentation --- src/account/AccountStorage.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 70de42af..f55b4513 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -80,6 +80,7 @@ function toFunctionReference(bytes32 setValue) pure returns (FunctionReference) // 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF______________________ Hook Function Reference // 0x__________________________________________AA____________________ is pre hook // 0x____________________________________________BB__________________ is post hook +// 0x______________________________________________CC________________ require UO context function toSetValue(ExecutionHook memory executionHook) pure returns (bytes32) { return bytes32(FunctionReference.unwrap(executionHook.hookFunction)) From d6d82c0e349ef2baf84b497397f3ed9fcc659004 Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Thu, 27 Jun 2024 14:37:46 -0400 Subject: [PATCH 5/7] chore: remove require uo context param --- src/account/AccountLoupe.sol | 6 ++--- src/account/AccountStorage.sol | 7 ++---- src/account/PluginManagerInternals.sol | 24 +++++-------------- src/account/UpgradeableModularAccount.sol | 10 ++------ src/interfaces/IAccountLoupe.sol | 1 - src/interfaces/IPlugin.sol | 1 - test/account/AccountExecHooks.t.sol | 27 +++------------------- test/account/AccountLoupe.t.sol | 9 +++----- test/mocks/plugins/ComprehensivePlugin.sol | 9 +++----- 9 files changed, 21 insertions(+), 73 deletions(-) diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol index 99b5bd14..2afe6638 100644 --- a/src/account/AccountLoupe.sol +++ b/src/account/AccountLoupe.sol @@ -47,8 +47,7 @@ abstract contract AccountLoupe is IAccountLoupe { for (uint256 i = 0; i < executionHooksLength; ++i) { bytes32 key = hooks.at(i); ExecutionHook memory execHook = execHooks[i]; - (execHook.hookFunction, execHook.isPreHook, execHook.isPostHook, execHook.requireUOContext) = - toExecutionHook(key); + (execHook.hookFunction, execHook.isPreHook, execHook.isPostHook) = toExecutionHook(key); } } @@ -66,8 +65,7 @@ abstract contract AccountLoupe is IAccountLoupe { for (uint256 i = 0; i < executionHooksLength; ++i) { bytes32 key = hooks.at(i); ExecutionHook memory execHook = permissionHooks[i]; - (execHook.hookFunction, execHook.isPreHook, execHook.isPostHook, execHook.requireUOContext) = - toExecutionHook(key); + (execHook.hookFunction, execHook.isPreHook, execHook.isPostHook) = toExecutionHook(key); } } diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index f55b4513..352a3d5b 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -80,23 +80,20 @@ function toFunctionReference(bytes32 setValue) pure returns (FunctionReference) // 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF______________________ Hook Function Reference // 0x__________________________________________AA____________________ is pre hook // 0x____________________________________________BB__________________ is post hook -// 0x______________________________________________CC________________ require UO context 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) - | bytes32(executionHook.requireUOContext ? uint256(1) << 64 : 0); + | bytes32(executionHook.isPostHook ? uint256(1) << 72 : 0); } function toExecutionHook(bytes32 setValue) pure - returns (FunctionReference hookFunction, bool isPreHook, bool isPostHook, bool requireUOContext) + returns (FunctionReference hookFunction, bool isPreHook, bool isPostHook) { hookFunction = FunctionReference.wrap(bytes21(setValue)); isPreHook = (uint256(setValue) >> 80) & 0xFF == 1; isPostHook = (uint256(setValue) >> 72) & 0xFF == 1; - requireUOContext = (uint256(setValue) >> 64) & 0xFF == 1; } /// @dev Helper function to get all elements of a set into memory. diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index a2974e85..a9b79802 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -128,17 +128,11 @@ abstract contract PluginManagerInternals is IPluginManager { EnumerableSet.Bytes32Set storage hooks, FunctionReference hookFunction, bool isPreExecHook, - bool isPostExecHook, - bool requireUOContext + bool isPostExecHook ) internal { hooks.add( toSetValue( - ExecutionHook({ - hookFunction: hookFunction, - isPreHook: isPreExecHook, - isPostHook: isPostExecHook, - requireUOContext: requireUOContext - }) + ExecutionHook({hookFunction: hookFunction, isPreHook: isPreExecHook, isPostHook: isPostExecHook}) ) ); } @@ -147,17 +141,11 @@ abstract contract PluginManagerInternals is IPluginManager { EnumerableSet.Bytes32Set storage hooks, FunctionReference hookFunction, bool isPreExecHook, - bool isPostExecHook, - bool requireUOContext + bool isPostExecHook ) internal { hooks.remove( toSetValue( - ExecutionHook({ - hookFunction: hookFunction, - isPreHook: isPreExecHook, - isPostHook: isPostExecHook, - requireUOContext: requireUOContext - }) + ExecutionHook({hookFunction: hookFunction, isPreHook: isPreExecHook, isPostHook: isPostExecHook}) ) ); } @@ -244,7 +232,7 @@ abstract contract PluginManagerInternals is IPluginManager { ManifestExecutionHook memory mh = manifest.executionHooks[i]; EnumerableSet.Bytes32Set storage execHooks = _storage.selectorData[mh.executionSelector].executionHooks; FunctionReference hookFunction = FunctionReferenceLib.pack(plugin, mh.functionId); - _addExecHooks(execHooks, hookFunction, mh.isPreHook, mh.isPostHook, mh.requireUOContext); + _addExecHooks(execHooks, hookFunction, mh.isPreHook, mh.isPostHook); } length = manifest.interfaceIds.length; @@ -300,7 +288,7 @@ abstract contract PluginManagerInternals is IPluginManager { ManifestExecutionHook memory mh = manifest.executionHooks[i]; FunctionReference hookFunction = FunctionReferenceLib.pack(plugin, mh.functionId); EnumerableSet.Bytes32Set storage execHooks = _storage.selectorData[mh.executionSelector].executionHooks; - _removeExecHooks(execHooks, hookFunction, mh.isPreHook, mh.isPostHook, mh.requireUOContext); + _removeExecHooks(execHooks, hookFunction, mh.isPreHook, mh.isPostHook); } length = manifest.signatureValidationFunctions.length; diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index cd938c93..20ab0082 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -466,13 +466,7 @@ contract UpgradeableModularAccount is // be sure that the set of hooks to run will not be affected by state changes mid-execution. for (uint256 i = 0; i < hooksLength; ++i) { bytes32 key = executionHooks.at(i); - (FunctionReference hookFunction,, bool isPostHook, bool requireUOContext) = toExecutionHook(key); - if (requireUOContext) { - /** - * && msg.sig != this.executeUserOp.selector - */ - revert RequireUserOperationContext(); - } + (FunctionReference hookFunction,, bool isPostHook) = toExecutionHook(key); if (isPostHook) { postHooksToRun[i].postExecHook = hookFunction; } @@ -482,7 +476,7 @@ contract UpgradeableModularAccount is // exists. for (uint256 i = 0; i < hooksLength; ++i) { bytes32 key = executionHooks.at(i); - (FunctionReference hookFunction, bool isPreHook, bool isPostHook,) = toExecutionHook(key); + (FunctionReference hookFunction, bool isPreHook, bool isPostHook) = toExecutionHook(key); if (isPreHook) { bytes memory preExecHookReturnData = _runPreExecHook(hookFunction, data); diff --git a/src/interfaces/IAccountLoupe.sol b/src/interfaces/IAccountLoupe.sol index 1f1c354e..b172464a 100644 --- a/src/interfaces/IAccountLoupe.sol +++ b/src/interfaces/IAccountLoupe.sol @@ -9,7 +9,6 @@ struct ExecutionHook { FunctionReference hookFunction; bool isPreHook; bool isPostHook; - bool requireUOContext; } interface IAccountLoupe { diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index 248f48af..7259d748 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -45,7 +45,6 @@ struct ManifestExecutionHook { uint8 functionId; bool isPreHook; bool isPostHook; - bool requireUOContext; } struct SelectorPermission { diff --git a/test/account/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol index 82bf1ae7..124a87dc 100644 --- a/test/account/AccountExecHooks.t.sol +++ b/test/account/AccountExecHooks.t.sol @@ -9,7 +9,6 @@ import { } from "../../src/interfaces/IPlugin.sol"; import {IExecutionHook} from "../../src/interfaces/IExecutionHook.sol"; import {FunctionReference} from "../../src/helpers/FunctionReferenceLib.sol"; -import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {MockPlugin} from "../mocks/MockPlugin.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; @@ -49,8 +48,7 @@ contract AccountExecHooksTest is AccountTestBase { executionSelector: _EXEC_SELECTOR, functionId: _PRE_HOOK_FUNCTION_ID_1, isPreHook: true, - isPostHook: false, - requireUOContext: false + isPostHook: false }) ); } @@ -90,8 +88,7 @@ contract AccountExecHooksTest is AccountTestBase { executionSelector: _EXEC_SELECTOR, functionId: _BOTH_HOOKS_FUNCTION_ID_3, isPreHook: true, - isPostHook: true, - requireUOContext: false + isPostHook: true }) ); } @@ -141,8 +138,7 @@ contract AccountExecHooksTest is AccountTestBase { executionSelector: _EXEC_SELECTOR, functionId: _POST_HOOK_FUNCTION_ID_2, isPreHook: false, - isPostHook: true, - requireUOContext: false + isPostHook: true }) ); } @@ -168,23 +164,6 @@ contract AccountExecHooksTest is AccountTestBase { _uninstallPlugin(mockPlugin1); } - function test_requireUOContextHook() public { - _installPlugin1WithHooks( - ManifestExecutionHook({ - executionSelector: _EXEC_SELECTOR, - functionId: _POST_HOOK_FUNCTION_ID_2, - isPreHook: false, - isPostHook: true, - requireUOContext: true - }) - ); - - (bool success, bytes memory errMsg) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertFalse(success); - assertEq(errMsg.length, 4); - assertEq(bytes4(errMsg), UpgradeableModularAccount.RequireUserOperationContext.selector); - } - function _installPlugin1WithHooks(ManifestExecutionHook memory execHooks) internal { _m1.executionHooks.push(execHooks); mockPlugin1 = new MockPlugin(_m1); diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol index e21f232f..fa92ab00 100644 --- a/test/account/AccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -117,24 +117,21 @@ contract AccountLoupeTest is AccountTestBase { address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.BOTH_EXECUTION_HOOKS) ), isPreHook: true, - isPostHook: true, - requireUOContext: false + isPostHook: true }), ExecutionHook({ hookFunction: FunctionReferenceLib.pack( address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.PRE_EXECUTION_HOOK) ), isPreHook: true, - isPostHook: false, - requireUOContext: false + isPostHook: false }), ExecutionHook({ hookFunction: FunctionReferenceLib.pack( address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.POST_EXECUTION_HOOK) ), isPreHook: false, - isPostHook: true, - requireUOContext: false + isPostHook: true }) ]; diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index 2b742648..aa7e5314 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -149,22 +149,19 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba executionSelector: this.foo.selector, functionId: uint8(FunctionId.BOTH_EXECUTION_HOOKS), isPreHook: true, - isPostHook: true, - requireUOContext: false + isPostHook: true }); manifest.executionHooks[1] = ManifestExecutionHook({ executionSelector: this.foo.selector, functionId: uint8(FunctionId.PRE_EXECUTION_HOOK), isPreHook: true, - isPostHook: false, - requireUOContext: false + isPostHook: false }); manifest.executionHooks[2] = ManifestExecutionHook({ executionSelector: this.foo.selector, functionId: uint8(FunctionId.POST_EXECUTION_HOOK), isPreHook: false, - isPostHook: true, - requireUOContext: false + isPostHook: true }); return manifest; From e0d0c7f97dfc943c9524b1638affdd9b0e5b1561 Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Fri, 28 Jun 2024 16:04:28 -0400 Subject: [PATCH 6/7] chore: revert hook changes --- src/account/UpgradeableModularAccount.sol | 2 +- src/interfaces/IExecutionHook.sol | 9 ++++++--- test/mocks/plugins/ComprehensivePlugin.sol | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 20ab0082..4a274b3e 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -494,7 +494,7 @@ contract UpgradeableModularAccount is returns (bytes memory preExecHookReturnData) { (address plugin, uint8 functionId) = preExecHook.unpack(); - try IExecutionHook(plugin).preExecutionHook(functionId, abi.encodePacked(msg.sender, msg.value, data)) + try IExecutionHook(plugin).preExecutionHook(functionId, msg.sender, msg.value, data) returns (bytes memory returnData) { preExecHookReturnData = returnData; } catch (bytes memory revertReason) { diff --git a/src/interfaces/IExecutionHook.sol b/src/interfaces/IExecutionHook.sol index b0dd40c2..3240c489 100644 --- a/src/interfaces/IExecutionHook.sol +++ b/src/interfaces/IExecutionHook.sol @@ -8,10 +8,13 @@ interface IExecutionHook is IPlugin { /// @dev To indicate the entire call should revert, the function MUST revert. /// @param functionId An identifier that routes the call to different internal implementations, should there be /// more than one. - /// @param data If hook requires UO context, data is abi.encode(PackedUserOperation), else its - /// abi.encodePacked(sender, value, calldata) + /// @param sender The caller address. + /// @param value The call value. + /// @param data The calldata sent. /// @return Context to pass to a post execution hook, if present. An empty bytes array MAY be returned. - function preExecutionHook(uint8 functionId, bytes calldata data) external returns (bytes memory); + function preExecutionHook(uint8 functionId, address sender, uint256 value, bytes calldata data) + external + returns (bytes memory); /// @notice Run the post execution hook specified by the `functionId`. /// @dev To indicate the entire call should revert, the function MUST revert. diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index aa7e5314..e31d4c64 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -105,7 +105,7 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba revert NotImplemented(); } - function preExecutionHook(uint8 functionId, bytes calldata) external pure override returns (bytes memory) { + function preExecutionHook(uint8 functionId, address, uint256, bytes calldata) external pure override returns (bytes memory) { if (functionId == uint8(FunctionId.PRE_EXECUTION_HOOK)) { return ""; } else if (functionId == uint8(FunctionId.BOTH_EXECUTION_HOOKS)) { From d0447f08734a99c1cd45da66b2aaf2a6599a06dd Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Fri, 28 Jun 2024 16:06:49 -0400 Subject: [PATCH 7/7] fix: tests --- src/account/UpgradeableModularAccount.sol | 5 +++-- test/account/AccountExecHooks.t.sol | 16 ++++++---------- test/mocks/plugins/ComprehensivePlugin.sol | 7 ++++++- 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 4a274b3e..1a52b5dd 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -494,8 +494,9 @@ contract UpgradeableModularAccount is returns (bytes memory preExecHookReturnData) { (address plugin, uint8 functionId) = preExecHook.unpack(); - try IExecutionHook(plugin).preExecutionHook(functionId, msg.sender, msg.value, data) - returns (bytes memory returnData) { + try IExecutionHook(plugin).preExecutionHook(functionId, msg.sender, msg.value, data) returns ( + bytes memory returnData + ) { preExecHookReturnData = returnData; } catch (bytes memory revertReason) { revert PreExecHookReverted(plugin, functionId, revertReason); diff --git a/test/account/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol index 124a87dc..f2e6391f 100644 --- a/test/account/AccountExecHooks.t.sol +++ b/test/account/AccountExecHooks.t.sol @@ -63,11 +63,9 @@ contract AccountExecHooksTest is AccountTestBase { abi.encodeWithSelector( IExecutionHook.preExecutionHook.selector, _PRE_HOOK_FUNCTION_ID_1, - abi.encodePacked( - address(this), // caller - uint256(0), // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ) + address(this), // caller + uint256(0), // msg.value in call to account + abi.encodeWithSelector(_EXEC_SELECTOR) ), 0 // msg value in call to plugin ); @@ -104,11 +102,9 @@ contract AccountExecHooksTest is AccountTestBase { abi.encodeWithSelector( IExecutionHook.preExecutionHook.selector, _BOTH_HOOKS_FUNCTION_ID_3, - abi.encodePacked( - address(this), // caller - uint256(0), // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ) + address(this), // caller + uint256(0), // msg.value in call to account + abi.encodeWithSelector(_EXEC_SELECTOR) ), 0 // msg value in call to plugin ); diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index e31d4c64..6ef654c7 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -105,7 +105,12 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba revert NotImplemented(); } - function preExecutionHook(uint8 functionId, address, uint256, bytes calldata) external pure override returns (bytes memory) { + function preExecutionHook(uint8 functionId, address, uint256, bytes calldata) + external + pure + override + returns (bytes memory) + { if (functionId == uint8(FunctionId.PRE_EXECUTION_HOOK)) { return ""; } else if (functionId == uint8(FunctionId.BOTH_EXECUTION_HOOKS)) {