From c8775f18a691ebeacc276ae563f480e3d3a27d63 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Wed, 31 Jul 2024 09:57:42 -0700 Subject: [PATCH 1/5] refactor: rename selectorData to executionData --- src/account/AccountLoupe.sol | 4 ++-- src/account/AccountStorage.sol | 4 ++-- src/account/ModuleManagerInternals.sol | 26 ++++++++++++----------- src/account/UpgradeableModularAccount.sol | 8 +++---- 4 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol index 110e7e72..617b7af5 100644 --- a/src/account/AccountLoupe.sol +++ b/src/account/AccountLoupe.sol @@ -28,7 +28,7 @@ abstract contract AccountLoupe is IAccountLoupe { return address(this); } - return getAccountStorage().selectorData[selector].module; + return getAccountStorage().executionData[selector].module; } /// @inheritdoc IAccountLoupe @@ -51,7 +51,7 @@ abstract contract AccountLoupe is IAccountLoupe { override returns (ExecutionHook[] memory execHooks) { - EnumerableSet.Bytes32Set storage hooks = getAccountStorage().selectorData[selector].executionHooks; + EnumerableSet.Bytes32Set storage hooks = getAccountStorage().executionData[selector].executionHooks; uint256 executionHooksLength = hooks.length(); execHooks = new ExecutionHook[](executionHooksLength); diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index f7d279e4..f488e325 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -9,7 +9,7 @@ import {HookConfig, ModuleEntity} from "../interfaces/IModuleManager.sol"; bytes32 constant _ACCOUNT_STORAGE_SLOT = 0x9f09680beaa4e5c9f38841db2460c401499164f368baef687948c315d9073e40; // Represents data associated with a specifc function selector. -struct SelectorData { +struct ExecutionData { // The module that implements this execution function. // If this is a native function, the address must remain address(0). address module; @@ -42,7 +42,7 @@ struct AccountStorage { uint8 initialized; bool initializing; // Execution functions and their associated functions - mapping(bytes4 => SelectorData) selectorData; + mapping(bytes4 selector => ExecutionData) executionData; mapping(ModuleEntity validationFunction => ValidationData) validationData; // For ERC165 introspection mapping(bytes4 => uint256) supportedIfaces; diff --git a/src/account/ModuleManagerInternals.sol b/src/account/ModuleManagerInternals.sol index c064d1a9..fa679f59 100644 --- a/src/account/ModuleManagerInternals.sol +++ b/src/account/ModuleManagerInternals.sol @@ -15,7 +15,7 @@ import {IModule} from "../interfaces/IModule.sol"; import {HookConfig, IModuleManager, ModuleEntity, ValidationConfig} from "../interfaces/IModuleManager.sol"; import { AccountStorage, - SelectorData, + ExecutionData, ValidationData, getAccountStorage, toModuleEntity, @@ -46,9 +46,9 @@ abstract contract ModuleManagerInternals is IModuleManager { function _setExecutionFunction(bytes4 selector, bool isPublic, bool allowGlobalValidation, address module) internal { - SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; + ExecutionData storage _executionData = getAccountStorage().executionData[selector]; - if (_selectorData.module != address(0)) { + if (_executionData.module != address(0)) { revert ExecutionFunctionAlreadySet(selector); } @@ -72,17 +72,17 @@ abstract contract ModuleManagerInternals is IModuleManager { revert Erc4337FunctionNotAllowed(selector); } - _selectorData.module = module; - _selectorData.isPublic = isPublic; - _selectorData.allowGlobalValidation = allowGlobalValidation; + _executionData.module = module; + _executionData.isPublic = isPublic; + _executionData.allowGlobalValidation = allowGlobalValidation; } function _removeExecutionFunction(bytes4 selector) internal { - SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; + ExecutionData storage _executionData = getAccountStorage().executionData[selector]; - _selectorData.module = address(0); - _selectorData.isPublic = false; - _selectorData.allowGlobalValidation = false; + _executionData.module = address(0); + _executionData.isPublic = false; + _executionData.allowGlobalValidation = false; } function _addValidationFunction(ValidationConfig validationConfig, bytes4[] memory selectors) internal { @@ -152,7 +152,8 @@ abstract contract ModuleManagerInternals is IModuleManager { 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; + EnumerableSet.Bytes32Set storage execHooks = + _storage.executionData[mh.executionSelector].executionHooks; HookConfig hookConfig = HookConfigLib.packExecHook({ _module: module, _entityId: mh.entityId, @@ -187,7 +188,8 @@ abstract contract ModuleManagerInternals is IModuleManager { uint256 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; + EnumerableSet.Bytes32Set storage execHooks = + _storage.executionData[mh.executionSelector].executionHooks; HookConfig hookConfig = HookConfigLib.packExecHook({ _module: module, _entityId: mh.entityId, diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 5767f75f..7ca5b9d9 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -114,7 +114,7 @@ contract UpgradeableModularAccount is /// @dev We route calls to execution functions based on incoming msg.sig /// @dev If there's no module associated with this function selector, revert fallback(bytes calldata) external payable returns (bytes memory) { - address execModule = getAccountStorage().selectorData[msg.sig].module; + address execModule = getAccountStorage().executionData[msg.sig].module; if (execModule == address(0)) { revert UnrecognizedFunction(msg.sig); } @@ -604,7 +604,7 @@ contract UpgradeableModularAccount is // and the selector isn't public. if ( msg.sender != address(_ENTRY_POINT) && msg.sender != address(this) - && !_storage.selectorData[msg.sig].isPublic + && !_storage.executionData[msg.sig].isPublic ) { ModuleEntity directCallValidationKey = ModuleEntityLib.pack(msg.sender, DIRECT_CALL_VALIDATION_ENTITYID); @@ -629,7 +629,7 @@ contract UpgradeableModularAccount is // Exec hooks PostExecToRun[] memory postExecutionHooks = - _doPreHooks(_storage.selectorData[msg.sig].executionHooks, msg.data); + _doPreHooks(_storage.executionData[msg.sig].executionHooks, msg.data); return (postPermissionHooks, postExecutionHooks); } @@ -697,7 +697,7 @@ contract UpgradeableModularAccount is return true; } - return getAccountStorage().selectorData[selector].allowGlobalValidation; + return getAccountStorage().executionData[selector].allowGlobalValidation; } function _checkIfValidationAppliesSelector(bytes4 selector, ModuleEntity validationFunction, bool isGlobal) From 9a4b74b4c357b2f6ab1bc618d9c047926fb187fd Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Wed, 31 Jul 2024 15:25:56 -0700 Subject: [PATCH 2/5] fix: revamp account loup for data availability --- src/account/AccountLoupe.sol | 88 ++++----------- src/helpers/KnownSelectors.sol | 5 +- src/interfaces/IAccountLoupe.sol | 65 ++++++----- test/account/AccountLoupe.t.sol | 107 +++++++++---------- test/account/MultiValidation.t.sol | 4 +- test/account/UpgradeableModularAccount.t.sol | 12 +-- 6 files changed, 122 insertions(+), 159 deletions(-) diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol index 617b7af5..a26bdd8d 100644 --- a/src/account/AccountLoupe.sol +++ b/src/account/AccountLoupe.sol @@ -7,10 +7,12 @@ import {EnumerableMap} from "@openzeppelin/contracts/utils/structs/EnumerableMap import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import {HookConfigLib} from "../helpers/HookConfigLib.sol"; -import {ExecutionHook, IAccountLoupe} from "../interfaces/IAccountLoupe.sol"; +import { + ExecutionDataView, ExecutionHook, IAccountLoupe, ValidationDataView +} from "../interfaces/IAccountLoupe.sol"; import {HookConfig, IModuleManager, ModuleEntity} from "../interfaces/IModuleManager.sol"; import {IStandardExecutor} from "../interfaces/IStandardExecutor.sol"; -import {getAccountStorage, toHookConfig, toSelector} from "./AccountStorage.sol"; +import {ExecutionData, ValidationData, getAccountStorage, toHookConfig, toSelector} from "./AccountStorage.sol"; abstract contract AccountLoupe is IAccountLoupe { using EnumerableSet for EnumerableSet.Bytes32Set; @@ -18,84 +20,36 @@ abstract contract AccountLoupe is IAccountLoupe { using HookConfigLib for HookConfig; /// @inheritdoc IAccountLoupe - function getExecutionFunctionHandler(bytes4 selector) external view override returns (address module) { + function getExecutionData(bytes4 selector) external view override returns (ExecutionDataView memory data) { if ( selector == IStandardExecutor.execute.selector || selector == IStandardExecutor.executeBatch.selector || selector == UUPSUpgradeable.upgradeToAndCall.selector || selector == IModuleManager.installExecution.selector || selector == IModuleManager.uninstallExecution.selector ) { - return address(this); - } - - return getAccountStorage().executionData[selector].module; - } - - /// @inheritdoc IAccountLoupe - function getSelectors(ModuleEntity validationFunction) external view returns (bytes4[] memory) { - uint256 length = getAccountStorage().validationData[validationFunction].selectors.length(); - - bytes4[] memory selectors = new bytes4[](length); - - for (uint256 i = 0; i < length; ++i) { - selectors[i] = toSelector(getAccountStorage().validationData[validationFunction].selectors.at(i)); - } - - return selectors; - } - - /// @inheritdoc IAccountLoupe - function getExecutionHooks(bytes4 selector) - external - view - override - returns (ExecutionHook[] memory execHooks) - { - EnumerableSet.Bytes32Set storage hooks = getAccountStorage().executionData[selector].executionHooks; - uint256 executionHooksLength = hooks.length(); - - execHooks = new ExecutionHook[](executionHooksLength); - - for (uint256 i = 0; i < executionHooksLength; ++i) { - bytes32 key = hooks.at(i); - HookConfig hookConfig = toHookConfig(key); - execHooks[i] = ExecutionHook({ - hookFunction: hookConfig.moduleEntity(), - isPreHook: hookConfig.hasPreHook(), - isPostHook: hookConfig.hasPostHook() - }); - } - } - - /// @inheritdoc IAccountLoupe - function getPermissionHooks(ModuleEntity 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); - HookConfig hookConfig = toHookConfig(key); - permissionHooks[i] = ExecutionHook({ - hookFunction: hookConfig.moduleEntity(), - isPreHook: hookConfig.hasPreHook(), - isPostHook: hookConfig.hasPostHook() - }); + data.module = address(this); + data.allowGlobalValidation = true; + } else { + ExecutionData storage executionData = getAccountStorage().executionData[selector]; + data.module = executionData.module; + data.isPublic = executionData.isPublic; + data.allowGlobalValidation = executionData.allowGlobalValidation; + data.executionHooks = executionData.executionHooks.values(); } } /// @inheritdoc IAccountLoupe - function getPreValidationHooks(ModuleEntity validationFunction) + function getValidationData(ModuleEntity validationFunction) external view override - returns (ModuleEntity[] memory preValidationHooks) + returns (ValidationDataView memory data) { - preValidationHooks = getAccountStorage().validationData[validationFunction].preValidationHooks; + ValidationData storage validationData = getAccountStorage().validationData[validationFunction]; + data.isGlobal = validationData.isGlobal; + data.isSignatureValidation = validationData.isSignatureValidation; + data.preValidationHooks = validationData.preValidationHooks; + data.permissionHooks = validationData.permissionHooks.values(); + data.selectors = validationData.selectors.values(); } } diff --git a/src/helpers/KnownSelectors.sol b/src/helpers/KnownSelectors.sol index 6e1cfca0..73476a45 100644 --- a/src/helpers/KnownSelectors.sol +++ b/src/helpers/KnownSelectors.sol @@ -36,9 +36,8 @@ library KnownSelectors { || selector == IStandardExecutor.execute.selector || selector == IStandardExecutor.executeBatch.selector || selector == IStandardExecutor.executeWithAuthorization.selector // check against IAccountLoupe methods - || selector == IAccountLoupe.getExecutionFunctionHandler.selector - || selector == IAccountLoupe.getSelectors.selector || selector == IAccountLoupe.getExecutionHooks.selector - || selector == IAccountLoupe.getPreValidationHooks.selector; + || selector == IAccountLoupe.getExecutionData.selector + || selector == IAccountLoupe.getValidationData.selector; } function isErc4337Function(bytes4 selector) internal pure returns (bool) { diff --git a/src/interfaces/IAccountLoupe.sol b/src/interfaces/IAccountLoupe.sol index f076de61..c30f233c 100644 --- a/src/interfaces/IAccountLoupe.sol +++ b/src/interfaces/IAccountLoupe.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: CC0-1.0 pragma solidity ^0.8.25; -import {ModuleEntity} from "../interfaces/IModuleManager.sol"; +import {HookConfig, ModuleEntity} from "../interfaces/IModuleManager.sol"; /// @notice Pre and post hooks for a given selector. /// @dev It's possible for one of either `preExecHook` or `postExecHook` to be empty. @@ -11,33 +11,48 @@ struct ExecutionHook { bool isPostHook; } -interface IAccountLoupe { - /// @notice Get the module address for a selector. - /// @dev If the selector is a native function, the module address will be the address of the account. - /// @param selector The selector to get the configuration for. - /// @return module The module address for this selector. - function getExecutionFunctionHandler(bytes4 selector) external view returns (address module); - - /// @notice Get the selectors for a validation function. - /// @param validationFunction The validation function to get the selectors for. - /// @return The allowed selectors for this validation function. - function getSelectors(ModuleEntity validationFunction) external view returns (bytes4[] memory); +// Represents data associated with a specifc function selector. +struct ExecutionDataView { + // The module that implements this execution function. + // If this is a native function, the address must remain address(0). + address module; + // Whether or not the function needs runtime validation, or can be called by anyone. The function can still be + // state changing if this flag is set to true. + // Note that even if this is set to true, user op validation will still be required, otherwise anyone could + // drain the account of native tokens by wasting gas. + bool isPublic; + // Whether or not a global validation function may be used to validate this function. + bool allowGlobalValidation; + // The execution hooks for this function selector. + bytes32[] executionHooks; +} - /// @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 (ExecutionHook[] memory); +struct ValidationDataView { + // Whether or not this validation can be used as a global validation function. + bool isGlobal; + // Whether or not this validation is a signature validator. + bool isSignatureValidation; + // The pre validation hooks for this validation function. + ModuleEntity[] preValidationHooks; + // Permission hooks for this validation function. + bytes32[] permissionHooks; + // The set of selectors that may be validated by this validation function. + bytes32[] selectors; +} - /// @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(ModuleEntity validationFunction) external view returns (ExecutionHook[] memory); +interface IAccountLoupe { + /// @notice Get the execution data for a selector. + /// @dev If the selector is a native function, the module address will be the address of the account. + /// @param selector The selector to get the data for. + /// @return ExecutionData The module address for this selector. + function getExecutionData(bytes4 selector) external view returns (ExecutionDataView 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. - function getPreValidationHooks(ModuleEntity validationFunction) + /// @notice Get the validation data for a validation. + /// @dev If the selector is a native function, the module address will be the address of the account. + /// @param validationFunction The validationFunction to get the data for. + /// @return ValidationData The module address for this selector. + function getValidationData(ModuleEntity validationFunction) external view - returns (ModuleEntity[] memory preValidationHooks); + returns (ValidationDataView memory); } diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol index bc75d356..b03c7713 100644 --- a/test/account/AccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -5,8 +5,8 @@ import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeab import {HookConfigLib} from "../../src/helpers/HookConfigLib.sol"; import {ModuleEntity, ModuleEntityLib} from "../../src/helpers/ModuleEntityLib.sol"; -import {ExecutionHook} from "../../src/interfaces/IAccountLoupe.sol"; -import {IModuleManager} from "../../src/interfaces/IModuleManager.sol"; +import {ExecutionDataView, ExecutionHook, ValidationDataView} from "../../src/interfaces/IAccountLoupe.sol"; +import {HookConfig, IModuleManager} from "../../src/interfaces/IModuleManager.sol"; import {IStandardExecutor} from "../../src/interfaces/IStandardExecutor.sol"; import {ComprehensiveModule} from "../mocks/modules/ComprehensiveModule.sol"; @@ -31,7 +31,7 @@ contract AccountLoupeTest is CustomValidationTestBase { vm.stopPrank(); } - function test_moduleLoupe_getExecutionFunctionHandler_native() public { + function test_moduleLoupe_getExecutionData_native() public { bytes4[] memory selectorsToCheck = new bytes4[](5); selectorsToCheck[0] = IStandardExecutor.execute.selector; @@ -45,13 +45,14 @@ contract AccountLoupeTest is CustomValidationTestBase { selectorsToCheck[4] = IModuleManager.uninstallExecution.selector; for (uint256 i = 0; i < selectorsToCheck.length; i++) { - address module = account1.getExecutionFunctionHandler(selectorsToCheck[i]); - - assertEq(module, address(account1)); + ExecutionDataView memory data = account1.getExecutionData(selectorsToCheck[i]); + assertEq(data.module, address(account1)); + assertTrue(data.allowGlobalValidation); + assertFalse(data.isPublic); } } - function test_moduleLoupe_getExecutionFunctionConfig_module() public { + function test_moduleLoupe_getExecutionData_module() public { bytes4[] memory selectorsToCheck = new bytes4[](1); address[] memory expectedModuleAddress = new address[](1); @@ -59,61 +60,51 @@ contract AccountLoupeTest is CustomValidationTestBase { expectedModuleAddress[0] = address(comprehensiveModule); for (uint256 i = 0; i < selectorsToCheck.length; i++) { - address module = account1.getExecutionFunctionHandler(selectorsToCheck[i]); - - assertEq(module, expectedModuleAddress[i]); - } - } - - function test_moduleLoupe_getSelectors() public { - bytes4[] memory selectors = account1.getSelectors(comprehensiveModuleValidation); - - assertEq(selectors.length, 1); - assertEq(selectors[0], comprehensiveModule.foo.selector); - } - - function test_moduleLoupe_getExecutionHooks() public { - ExecutionHook[] memory hooks = account1.getExecutionHooks(comprehensiveModule.foo.selector); - ExecutionHook[3] memory expectedHooks = [ - ExecutionHook({ - hookFunction: ModuleEntityLib.pack( - address(comprehensiveModule), uint32(ComprehensiveModule.EntityId.BOTH_EXECUTION_HOOKS) + ExecutionDataView memory data = account1.getExecutionData(selectorsToCheck[i]); + assertEq(data.module, expectedModuleAddress[i]); + assertFalse(data.allowGlobalValidation); + assertFalse(data.isPublic); + + HookConfig[3] memory expectedHooks = [ + HookConfigLib.packExecHook( + ModuleEntityLib.pack( + address(comprehensiveModule), uint32(ComprehensiveModule.EntityId.BOTH_EXECUTION_HOOKS) + ), + true, + true ), - isPreHook: true, - isPostHook: true - }), - ExecutionHook({ - hookFunction: ModuleEntityLib.pack( - address(comprehensiveModule), uint32(ComprehensiveModule.EntityId.PRE_EXECUTION_HOOK) + HookConfigLib.packExecHook( + ModuleEntityLib.pack( + address(comprehensiveModule), uint32(ComprehensiveModule.EntityId.PRE_EXECUTION_HOOK) + ), + true, + false ), - isPreHook: true, - isPostHook: false - }), - ExecutionHook({ - hookFunction: ModuleEntityLib.pack( - address(comprehensiveModule), uint32(ComprehensiveModule.EntityId.POST_EXECUTION_HOOK) - ), - isPreHook: false, - isPostHook: true - }) - ]; - - assertEq(hooks.length, 3); - for (uint256 i = 0; i < hooks.length; i++) { - assertEq( - ModuleEntity.unwrap(hooks[i].hookFunction), ModuleEntity.unwrap(expectedHooks[i].hookFunction) - ); - assertEq(hooks[i].isPreHook, expectedHooks[i].isPreHook); - assertEq(hooks[i].isPostHook, expectedHooks[i].isPostHook); + HookConfigLib.packExecHook( + ModuleEntityLib.pack( + address(comprehensiveModule), uint32(ComprehensiveModule.EntityId.POST_EXECUTION_HOOK) + ), + false, + true + ) + ]; + + assertEq(data.executionHooks.length, 3); + for (uint256 j = 0; j < data.executionHooks.length; j++) { + assertEq(data.executionHooks[j], bytes32(HookConfig.unwrap(expectedHooks[j]))); + } } } - function test_moduleLoupe_getValidationHooks() public { - ModuleEntity[] memory hooks = account1.getPreValidationHooks(comprehensiveModuleValidation); + function test_moduleLoupe_getValidationData() public { + ValidationDataView memory data = account1.getValidationData(comprehensiveModuleValidation); + bytes32[] memory selectors = data.selectors; - assertEq(hooks.length, 2); + assertTrue(data.isGlobal); + assertTrue(data.isSignatureValidation); + assertEq(data.preValidationHooks.length, 2); assertEq( - ModuleEntity.unwrap(hooks[0]), + ModuleEntity.unwrap(data.preValidationHooks[0]), ModuleEntity.unwrap( ModuleEntityLib.pack( address(comprehensiveModule), uint32(ComprehensiveModule.EntityId.PRE_VALIDATION_HOOK_1) @@ -121,13 +112,17 @@ contract AccountLoupeTest is CustomValidationTestBase { ) ); assertEq( - ModuleEntity.unwrap(hooks[1]), + ModuleEntity.unwrap(data.preValidationHooks[1]), ModuleEntity.unwrap( ModuleEntityLib.pack( address(comprehensiveModule), uint32(ComprehensiveModule.EntityId.PRE_VALIDATION_HOOK_2) ) ) ); + + assertEq(data.permissionHooks.length, 0); + assertEq(selectors.length, 1); + assertEq(selectors[0], bytes32(comprehensiveModule.foo.selector)); } // Test config diff --git a/test/account/MultiValidation.t.sol b/test/account/MultiValidation.t.sol index 3e927ee6..b29bfb19 100644 --- a/test/account/MultiValidation.t.sol +++ b/test/account/MultiValidation.t.sol @@ -46,8 +46,8 @@ contract MultiValidationTest is AccountTestBase { validations[0] = ModuleEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID); validations[1] = ModuleEntityLib.pack(address(validator2), TEST_DEFAULT_VALIDATION_ENTITY_ID); - bytes4[] memory selectors0 = account1.getSelectors(validations[0]); - bytes4[] memory selectors1 = account1.getSelectors(validations[1]); + bytes32[] memory selectors0 = account1.getValidationData(validations[0]).selectors; + bytes32[] memory selectors1 = account1.getValidationData(validations[1]).selectors; assertEq(selectors0.length, selectors1.length); for (uint256 i = 0; i < selectors0.length; i++) { assertEq(selectors0[i], selectors1[i]); diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index 0bb281a2..a492e7ab 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -12,7 +12,7 @@ import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/Messa import {ModuleManagerInternals} from "../../src/account/ModuleManagerInternals.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; -import {IAccountLoupe} from "../../src/interfaces/IAccountLoupe.sol"; +import {ExecutionDataView, IAccountLoupe} from "../../src/interfaces/IAccountLoupe.sol"; import {ExecutionManifest} from "../../src/interfaces/IExecution.sol"; import {IModuleManager} from "../../src/interfaces/IModuleManager.sol"; import {Call} from "../../src/interfaces/IStandardExecutor.sol"; @@ -247,9 +247,9 @@ contract UpgradeableModularAccountTest is AccountTestBase { moduleInstallData: abi.encode(uint48(1 days)) }); - address handler = - IAccountLoupe(account1).getExecutionFunctionHandler(TokenReceiverModule.onERC721Received.selector); - assertEq(handler, address(tokenReceiverModule)); + ExecutionDataView memory data = + IAccountLoupe(account1).getExecutionData(TokenReceiverModule.onERC721Received.selector); + assertEq(data.module, address(tokenReceiverModule)); } function test_installExecution_PermittedCallSelectorNotInstalled() public { @@ -321,8 +321,8 @@ contract UpgradeableModularAccountTest is AccountTestBase { moduleUninstallData: "" }); - address handler = IAccountLoupe(account1).getExecutionFunctionHandler(module.foo.selector); - assertEq(handler, address(0)); + ExecutionDataView memory data = IAccountLoupe(account1).getExecutionData(module.foo.selector); + assertEq(data.module, address(0)); } function _installExecutionWithExecHooks() internal returns (MockModule module) { From 2fadf6cd0eadbbae28b344b97dcd4b27156ed231 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Wed, 31 Jul 2024 15:28:56 -0700 Subject: [PATCH 3/5] chore: clean up ExecutionHook usage --- src/account/AccountLoupe.sol | 4 +--- src/interfaces/IAccountLoupe.sol | 8 -------- test/account/AccountLoupe.t.sol | 2 +- test/module/ERC20TokenLimitModule.t.sol | 8 -------- 4 files changed, 2 insertions(+), 20 deletions(-) diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol index a26bdd8d..6471cf64 100644 --- a/src/account/AccountLoupe.sol +++ b/src/account/AccountLoupe.sol @@ -7,9 +7,7 @@ import {EnumerableMap} from "@openzeppelin/contracts/utils/structs/EnumerableMap import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import {HookConfigLib} from "../helpers/HookConfigLib.sol"; -import { - ExecutionDataView, ExecutionHook, IAccountLoupe, ValidationDataView -} from "../interfaces/IAccountLoupe.sol"; +import {ExecutionDataView, IAccountLoupe, ValidationDataView} from "../interfaces/IAccountLoupe.sol"; import {HookConfig, IModuleManager, ModuleEntity} from "../interfaces/IModuleManager.sol"; import {IStandardExecutor} from "../interfaces/IStandardExecutor.sol"; import {ExecutionData, ValidationData, getAccountStorage, toHookConfig, toSelector} from "./AccountStorage.sol"; diff --git a/src/interfaces/IAccountLoupe.sol b/src/interfaces/IAccountLoupe.sol index c30f233c..e4083116 100644 --- a/src/interfaces/IAccountLoupe.sol +++ b/src/interfaces/IAccountLoupe.sol @@ -3,14 +3,6 @@ pragma solidity ^0.8.25; import {HookConfig, ModuleEntity} from "../interfaces/IModuleManager.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 { - ModuleEntity hookFunction; - bool isPreHook; - bool isPostHook; -} - // Represents data associated with a specifc function selector. struct ExecutionDataView { // The module that implements this execution function. diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol index b03c7713..b44b1f33 100644 --- a/test/account/AccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -5,7 +5,7 @@ import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeab import {HookConfigLib} from "../../src/helpers/HookConfigLib.sol"; import {ModuleEntity, ModuleEntityLib} from "../../src/helpers/ModuleEntityLib.sol"; -import {ExecutionDataView, ExecutionHook, ValidationDataView} from "../../src/interfaces/IAccountLoupe.sol"; +import {ExecutionDataView, ValidationDataView} from "../../src/interfaces/IAccountLoupe.sol"; import {HookConfig, IModuleManager} from "../../src/interfaces/IModuleManager.sol"; import {IStandardExecutor} from "../../src/interfaces/IStandardExecutor.sol"; diff --git a/test/module/ERC20TokenLimitModule.t.sol b/test/module/ERC20TokenLimitModule.t.sol index 96c846e4..33a707f2 100644 --- a/test/module/ERC20TokenLimitModule.t.sol +++ b/test/module/ERC20TokenLimitModule.t.sol @@ -12,7 +12,6 @@ import {HookConfigLib} from "../../src/helpers/HookConfigLib.sol"; import {ModuleEntityLib} from "../../src/helpers/ModuleEntityLib.sol"; import {ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol"; -import {ExecutionHook} from "../../src/interfaces/IAccountLoupe.sol"; import {ExecutionManifest} from "../../src/interfaces/IExecution.sol"; import {Call, IStandardExecutor} from "../../src/interfaces/IStandardExecutor.sol"; import {ERC20TokenLimitModule} from "../../src/modules/ERC20TokenLimitModule.sol"; @@ -39,13 +38,6 @@ contract ERC20TokenLimitModuleTest is AccountTestBase { erc20 = new MockERC20(); erc20.mint(address(acct), 10 ether); - ExecutionHook[] memory permissionHooks = new ExecutionHook[](1); - permissionHooks[0] = ExecutionHook({ - hookFunction: ModuleEntityLib.pack(address(module), 0), - isPreHook: true, - isPostHook: false - }); - // arr idx 0 => functionId of 0 has that spend uint256[] memory limits = new uint256[](1); limits[0] = spendLimit; From e1a0783027dfa2a20ed248a6390cc68e13e83bc8 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Wed, 31 Jul 2024 15:47:54 -0700 Subject: [PATCH 4/5] chore: add events emision for validation install and uninstall --- src/account/AccountLoupe.sol | 2 +- src/account/ModuleManagerInternals.sol | 25 +++++++++++++++++------- src/interfaces/IAccountLoupe.sol | 2 +- src/interfaces/IModuleManager.sol | 3 ++- test/account/DirectCallsFromModule.t.sol | 4 ++++ test/module/SingleSignerValidation.t.sol | 6 ++++++ 6 files changed, 32 insertions(+), 10 deletions(-) diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol index 6471cf64..cbd60fab 100644 --- a/src/account/AccountLoupe.sol +++ b/src/account/AccountLoupe.sol @@ -10,7 +10,7 @@ import {HookConfigLib} from "../helpers/HookConfigLib.sol"; import {ExecutionDataView, IAccountLoupe, ValidationDataView} from "../interfaces/IAccountLoupe.sol"; import {HookConfig, IModuleManager, ModuleEntity} from "../interfaces/IModuleManager.sol"; import {IStandardExecutor} from "../interfaces/IStandardExecutor.sol"; -import {ExecutionData, ValidationData, getAccountStorage, toHookConfig, toSelector} from "./AccountStorage.sol"; +import {ExecutionData, ValidationData, getAccountStorage} from "./AccountStorage.sol"; abstract contract AccountLoupe is IAccountLoupe { using EnumerableSet for EnumerableSet.Bytes32Set; diff --git a/src/account/ModuleManagerInternals.sol b/src/account/ModuleManagerInternals.sol index fa679f59..c8a439a9 100644 --- a/src/account/ModuleManagerInternals.sol +++ b/src/account/ModuleManagerInternals.sol @@ -227,9 +227,15 @@ abstract contract ModuleManagerInternals is IModuleManager { } } - function _onUninstall(address module, bytes calldata data) internal { + function _onUninstall(address module, bytes calldata data) internal returns (bool onUninstallSuccess) { + onUninstallSuccess = true; if (data.length > 0) { - IModule(module).onUninstall(data); + // Clear the module storage for the account. + // solhint-disable-next-line no-empty-blocks + try IModule(module).onUninstall(data) {} + catch { + onUninstallSuccess = false; + } } } @@ -241,6 +247,7 @@ abstract contract ModuleManagerInternals is IModuleManager { ) internal { ValidationData storage _validationData = getAccountStorage().validationData[validationConfig.moduleEntity()]; + ModuleEntity moduleEntity = validationConfig.moduleEntity(); for (uint256 i = 0; i < hooks.length; ++i) { HookConfig hookConfig = HookConfig.wrap(bytes26(hooks[i][:26])); @@ -255,7 +262,7 @@ abstract contract ModuleManagerInternals is IModuleManager { } } // Hook is an execution hook else if (!_validationData.permissionHooks.add(toSetValue(hookConfig))) { - revert PermissionAlreadySet(validationConfig.moduleEntity(), hookConfig); + revert PermissionAlreadySet(moduleEntity, hookConfig); } _onInstall(hookConfig.module(), hookData); @@ -264,7 +271,7 @@ abstract contract ModuleManagerInternals is IModuleManager { for (uint256 i = 0; i < selectors.length; ++i) { bytes4 selector = selectors[i]; if (!_validationData.selectors.add(toSetValue(selector))) { - revert ValidationAlreadySet(selector, validationConfig.moduleEntity()); + revert ValidationAlreadySet(selector, moduleEntity); } } @@ -272,6 +279,7 @@ abstract contract ModuleManagerInternals is IModuleManager { _validationData.isSignatureValidation = validationConfig.isSignatureValidation(); _onInstall(validationConfig.module(), installData); + emit ValidationInstalled(moduleEntity); } function _uninstallValidation( @@ -280,6 +288,7 @@ abstract contract ModuleManagerInternals is IModuleManager { bytes[] calldata hookUninstallDatas ) internal { ValidationData storage _validationData = getAccountStorage().validationData[validationFunction]; + bool onUninstallSuccess = true; _removeValidationFunction(validationFunction); @@ -298,7 +307,7 @@ abstract contract ModuleManagerInternals is IModuleManager { for (uint256 i = 0; i < _validationData.preValidationHooks.length; ++i) { bytes calldata hookData = hookUninstallDatas[hookIndex]; (address hookModule,) = ModuleEntityLib.unpack(_validationData.preValidationHooks[i]); - _onUninstall(hookModule, hookData); + onUninstallSuccess = _onUninstall(hookModule, hookData); hookIndex++; } @@ -306,7 +315,7 @@ abstract contract ModuleManagerInternals is IModuleManager { bytes calldata hookData = hookUninstallDatas[hookIndex]; (address hookModule,) = ModuleEntityLib.unpack(toModuleEntity(_validationData.permissionHooks.at(i))); - _onUninstall(hookModule, hookData); + onUninstallSuccess = _onUninstall(hookModule, hookData); hookIndex++; } } @@ -329,6 +338,8 @@ abstract contract ModuleManagerInternals is IModuleManager { } (address module,) = ModuleEntityLib.unpack(validationFunction); - _onUninstall(module, uninstallData); + onUninstallSuccess = _onUninstall(module, uninstallData); + + emit ValidationUninstalled(validationFunction, onUninstallSuccess); } } diff --git a/src/interfaces/IAccountLoupe.sol b/src/interfaces/IAccountLoupe.sol index e4083116..b4811c3c 100644 --- a/src/interfaces/IAccountLoupe.sol +++ b/src/interfaces/IAccountLoupe.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: CC0-1.0 pragma solidity ^0.8.25; -import {HookConfig, ModuleEntity} from "../interfaces/IModuleManager.sol"; +import {ModuleEntity} from "../interfaces/IModuleManager.sol"; // Represents data associated with a specifc function selector. struct ExecutionDataView { diff --git a/src/interfaces/IModuleManager.sol b/src/interfaces/IModuleManager.sol index 74e3a338..8df05bc4 100644 --- a/src/interfaces/IModuleManager.sol +++ b/src/interfaces/IModuleManager.sol @@ -11,8 +11,9 @@ type HookConfig is bytes26; interface IModuleManager { event ModuleInstalled(address indexed module); - event ModuleUninstalled(address indexed module, bool indexed onUninstallSucceeded); + event ValidationInstalled(ModuleEntity indexed moduleEntity); + event ValidationUninstalled(ModuleEntity indexed moduleEntity, bool indexed onUninstallSucceeded); /// @notice Install a module to the modular account. /// @param module The module to install. diff --git a/test/account/DirectCallsFromModule.t.sol b/test/account/DirectCallsFromModule.t.sol index 889b65b9..0d81ba82 100644 --- a/test/account/DirectCallsFromModule.t.sol +++ b/test/account/DirectCallsFromModule.t.sol @@ -16,6 +16,8 @@ contract DirectCallsFromModuleTest is AccountTestBase { DirectCallModule internal _module; ModuleEntity internal _moduleEntity; + event ValidationUninstalled(ModuleEntity indexed moduleEntity, bool indexed onUninstallSucceeded); + function setUp() public { _module = new DirectCallModule(); assertFalse(_module.preHookRan()); @@ -124,6 +126,8 @@ contract DirectCallsFromModuleTest is AccountTestBase { function _uninstallExecution() internal { vm.prank(address(entryPoint)); + vm.expectEmit(true, true, true, true); + emit ValidationUninstalled(_moduleEntity, true); account1.uninstallValidation(_moduleEntity, "", new bytes[](1)); } diff --git a/test/module/SingleSignerValidation.t.sol b/test/module/SingleSignerValidation.t.sol index 9358b59f..89e5965c 100644 --- a/test/module/SingleSignerValidation.t.sol +++ b/test/module/SingleSignerValidation.t.sol @@ -7,6 +7,7 @@ import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/Messa import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {ModuleEntityLib} from "../../src/helpers/ModuleEntityLib.sol"; import {ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol"; +import {ModuleEntity} from "../../src/interfaces/IModuleManager.sol"; import {ContractOwner} from "../mocks/ContractOwner.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; @@ -24,6 +25,8 @@ contract SingleSignerValidationTest is AccountTestBase { ContractOwner public contractOwner; + event ValidationInstalled(ModuleEntity indexed moduleEntity); + function setUp() public { ethRecipient = makeAddr("ethRecipient"); (owner2, owner2Key) = makeAddrAndKey("owner2"); @@ -79,6 +82,9 @@ contract SingleSignerValidationTest is AccountTestBase { function test_runtime_with2SameValidationInstalled() public { uint32 newEntityId = TEST_DEFAULT_VALIDATION_ENTITY_ID + 1; vm.prank(address(entryPoint)); + + vm.expectEmit(true, true, true, true); + emit ValidationInstalled(ModuleEntityLib.pack(address(singleSignerValidation), newEntityId)); account.installValidation( ValidationConfigLib.pack(address(singleSignerValidation), newEntityId, true, false), new bytes4[](0), From a118f313cc55974e5181ebd8bb6c63ca90b5f4be Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Thu, 1 Aug 2024 14:09:55 -0700 Subject: [PATCH 5/5] fix loupe data type and uninstall success value --- src/account/AccountLoupe.sol | 22 +++++++++++++++++++--- src/account/ModuleManagerInternals.sol | 6 +++--- src/interfaces/IAccountLoupe.sol | 8 ++++---- test/account/AccountLoupe.t.sol | 6 +++--- test/account/MultiValidation.t.sol | 4 ++-- 5 files changed, 31 insertions(+), 15 deletions(-) diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol index cbd60fab..803d075d 100644 --- a/src/account/AccountLoupe.sol +++ b/src/account/AccountLoupe.sol @@ -32,7 +32,12 @@ abstract contract AccountLoupe is IAccountLoupe { data.module = executionData.module; data.isPublic = executionData.isPublic; data.allowGlobalValidation = executionData.allowGlobalValidation; - data.executionHooks = executionData.executionHooks.values(); + + uint256 executionHooksLen = executionData.executionHooks.length(); + data.executionHooks = new HookConfig[](executionHooksLen); + for (uint256 i = 0; i < executionHooksLen; ++i) { + data.executionHooks[i] = HookConfig.wrap(bytes26(executionData.executionHooks.at(i))); + } } } @@ -47,7 +52,18 @@ abstract contract AccountLoupe is IAccountLoupe { data.isGlobal = validationData.isGlobal; data.isSignatureValidation = validationData.isSignatureValidation; data.preValidationHooks = validationData.preValidationHooks; - data.permissionHooks = validationData.permissionHooks.values(); - data.selectors = validationData.selectors.values(); + + uint256 permissionHooksLen = validationData.permissionHooks.length(); + data.permissionHooks = new HookConfig[](permissionHooksLen); + for (uint256 i = 0; i < permissionHooksLen; ++i) { + data.permissionHooks[i] = HookConfig.wrap(bytes26(validationData.permissionHooks.at(i))); + } + + bytes32[] memory selectors = validationData.selectors.values(); + uint256 selectorsLen = selectors.length; + data.selectors = new bytes4[](selectorsLen); + for (uint256 j = 0; j < selectorsLen; ++j) { + data.selectors[j] = bytes4(selectors[j]); + } } } diff --git a/src/account/ModuleManagerInternals.sol b/src/account/ModuleManagerInternals.sol index c8a439a9..54cb2fc0 100644 --- a/src/account/ModuleManagerInternals.sol +++ b/src/account/ModuleManagerInternals.sol @@ -307,7 +307,7 @@ abstract contract ModuleManagerInternals is IModuleManager { for (uint256 i = 0; i < _validationData.preValidationHooks.length; ++i) { bytes calldata hookData = hookUninstallDatas[hookIndex]; (address hookModule,) = ModuleEntityLib.unpack(_validationData.preValidationHooks[i]); - onUninstallSuccess = _onUninstall(hookModule, hookData); + onUninstallSuccess = onUninstallSuccess && _onUninstall(hookModule, hookData); hookIndex++; } @@ -315,7 +315,7 @@ abstract contract ModuleManagerInternals is IModuleManager { bytes calldata hookData = hookUninstallDatas[hookIndex]; (address hookModule,) = ModuleEntityLib.unpack(toModuleEntity(_validationData.permissionHooks.at(i))); - onUninstallSuccess = _onUninstall(hookModule, hookData); + onUninstallSuccess = onUninstallSuccess && _onUninstall(hookModule, hookData); hookIndex++; } } @@ -338,7 +338,7 @@ abstract contract ModuleManagerInternals is IModuleManager { } (address module,) = ModuleEntityLib.unpack(validationFunction); - onUninstallSuccess = _onUninstall(module, uninstallData); + onUninstallSuccess = onUninstallSuccess && _onUninstall(module, uninstallData); emit ValidationUninstalled(validationFunction, onUninstallSuccess); } diff --git a/src/interfaces/IAccountLoupe.sol b/src/interfaces/IAccountLoupe.sol index b4811c3c..658eb5c9 100644 --- a/src/interfaces/IAccountLoupe.sol +++ b/src/interfaces/IAccountLoupe.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: CC0-1.0 pragma solidity ^0.8.25; -import {ModuleEntity} from "../interfaces/IModuleManager.sol"; +import {HookConfig, ModuleEntity} from "../interfaces/IModuleManager.sol"; // Represents data associated with a specifc function selector. struct ExecutionDataView { @@ -16,7 +16,7 @@ struct ExecutionDataView { // Whether or not a global validation function may be used to validate this function. bool allowGlobalValidation; // The execution hooks for this function selector. - bytes32[] executionHooks; + HookConfig[] executionHooks; } struct ValidationDataView { @@ -27,9 +27,9 @@ struct ValidationDataView { // The pre validation hooks for this validation function. ModuleEntity[] preValidationHooks; // Permission hooks for this validation function. - bytes32[] permissionHooks; + HookConfig[] permissionHooks; // The set of selectors that may be validated by this validation function. - bytes32[] selectors; + bytes4[] selectors; } interface IAccountLoupe { diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol index b44b1f33..ae988924 100644 --- a/test/account/AccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -91,14 +91,14 @@ contract AccountLoupeTest is CustomValidationTestBase { assertEq(data.executionHooks.length, 3); for (uint256 j = 0; j < data.executionHooks.length; j++) { - assertEq(data.executionHooks[j], bytes32(HookConfig.unwrap(expectedHooks[j]))); + assertEq(HookConfig.unwrap(data.executionHooks[j]), HookConfig.unwrap(expectedHooks[j])); } } } function test_moduleLoupe_getValidationData() public { ValidationDataView memory data = account1.getValidationData(comprehensiveModuleValidation); - bytes32[] memory selectors = data.selectors; + bytes4[] memory selectors = data.selectors; assertTrue(data.isGlobal); assertTrue(data.isSignatureValidation); @@ -122,7 +122,7 @@ contract AccountLoupeTest is CustomValidationTestBase { assertEq(data.permissionHooks.length, 0); assertEq(selectors.length, 1); - assertEq(selectors[0], bytes32(comprehensiveModule.foo.selector)); + assertEq(selectors[0], comprehensiveModule.foo.selector); } // Test config diff --git a/test/account/MultiValidation.t.sol b/test/account/MultiValidation.t.sol index b29bfb19..20862604 100644 --- a/test/account/MultiValidation.t.sol +++ b/test/account/MultiValidation.t.sol @@ -46,8 +46,8 @@ contract MultiValidationTest is AccountTestBase { validations[0] = ModuleEntityLib.pack(address(singleSignerValidation), TEST_DEFAULT_VALIDATION_ENTITY_ID); validations[1] = ModuleEntityLib.pack(address(validator2), TEST_DEFAULT_VALIDATION_ENTITY_ID); - bytes32[] memory selectors0 = account1.getValidationData(validations[0]).selectors; - bytes32[] memory selectors1 = account1.getValidationData(validations[1]).selectors; + bytes4[] memory selectors0 = account1.getValidationData(validations[0]).selectors; + bytes4[] memory selectors1 = account1.getValidationData(validations[1]).selectors; assertEq(selectors0.length, selectors1.length); for (uint256 i = 0; i < selectors0.length; i++) { assertEq(selectors0[i], selectors1[i]);