From d06cfbcdd125145787a56776b01dd036cfc2f004 Mon Sep 17 00:00:00 2001 From: Jay Paik Date: Mon, 30 Sep 2024 19:06:42 -0400 Subject: [PATCH 1/2] feat: consistency in hook UDVTs for ValidationDataView --- src/account/AccountStorage.sol | 4 ++-- src/account/ModularAccountView.sol | 2 +- src/account/ModuleManagerInternals.sol | 12 ++++++------ src/account/ReferenceModularAccount.sol | 24 ++++++++++++------------ src/interfaces/IModularAccountView.sol | 4 ++-- standard/ERCs/erc-6900.md | 4 ++-- test/account/ModularAccountView.t.sol | 14 +++++++------- test/module/NativeTokenLimitModule.t.sol | 6 +++--- test/utils/AccountTestBase.sol | 4 +++- 9 files changed, 38 insertions(+), 36 deletions(-) diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 8d763d71..12e8f10e 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -31,8 +31,8 @@ struct ValidationData { bool isSignatureValidation; // Whether or not this validation is allowed to validate ERC-4337 user operations. bool isUserOpValidation; - // The pre validation hooks for this validation function. - ModuleEntity[] preValidationHooks; + // The validation hooks for this validation function. + HookConfig[] validationHooks; // Execution hooks to run with this validation function. EnumerableSet.Bytes32Set executionHooks; // The set of selectors that may be validated by this validation function. diff --git a/src/account/ModularAccountView.sol b/src/account/ModularAccountView.sol index 5756c968..3e0cf1df 100644 --- a/src/account/ModularAccountView.sol +++ b/src/account/ModularAccountView.sol @@ -50,7 +50,7 @@ abstract contract ModularAccountView is IModularAccountView { data.isGlobal = validationData.isGlobal; data.isSignatureValidation = validationData.isSignatureValidation; data.isUserOpValidation = validationData.isUserOpValidation; - data.preValidationHooks = validationData.preValidationHooks; + data.validationHooks = validationData.validationHooks; uint256 execHooksLen = validationData.executionHooks.length(); data.executionHooks = new HookConfig[](execHooksLen); diff --git a/src/account/ModuleManagerInternals.sol b/src/account/ModuleManagerInternals.sol index e92525e6..ee6b10d8 100644 --- a/src/account/ModuleManagerInternals.sol +++ b/src/account/ModuleManagerInternals.sol @@ -233,10 +233,10 @@ abstract contract ModuleManagerInternals is IModularAccount { bytes calldata hookData = hooks[i][25:]; if (hookConfig.isValidationHook()) { - _validationData.preValidationHooks.push(hookConfig.moduleEntity()); + _validationData.validationHooks.push(hookConfig); // Avoid collision between reserved index and actual indices - if (_validationData.preValidationHooks.length > MAX_PRE_VALIDATION_HOOKS) { + if (_validationData.validationHooks.length > MAX_PRE_VALIDATION_HOOKS) { revert PreValidationHookLimitExceeded(); } @@ -280,16 +280,16 @@ abstract contract ModuleManagerInternals is IModularAccount { // If any uninstall data is provided, assert it is of the correct length. if ( hookUninstallDatas.length - != _validationData.preValidationHooks.length + _validationData.executionHooks.length() + != _validationData.validationHooks.length + _validationData.executionHooks.length() ) { revert ArrayLengthMismatch(); } // Hook uninstall data is provided in the order of pre validation hooks, then execution hooks. uint256 hookIndex = 0; - for (uint256 i = 0; i < _validationData.preValidationHooks.length; ++i) { + for (uint256 i = 0; i < _validationData.validationHooks.length; ++i) { bytes calldata hookData = hookUninstallDatas[hookIndex]; - (address hookModule,) = ModuleEntityLib.unpack(_validationData.preValidationHooks[i]); + (address hookModule,) = ModuleEntityLib.unpack(_validationData.validationHooks[i].moduleEntity()); onUninstallSuccess = onUninstallSuccess && _onUninstall(hookModule, hookData); hookIndex++; } @@ -304,7 +304,7 @@ abstract contract ModuleManagerInternals is IModularAccount { } // Clear all stored hooks - delete _validationData.preValidationHooks; + delete _validationData.validationHooks; EnumerableSet.Bytes32Set storage executionHooks = _validationData.executionHooks; uint256 executionHookLen = executionHooks.length(); diff --git a/src/account/ReferenceModularAccount.sol b/src/account/ReferenceModularAccount.sol index 96aff5d6..168cca44 100644 --- a/src/account/ReferenceModularAccount.sol +++ b/src/account/ReferenceModularAccount.sol @@ -311,11 +311,11 @@ contract ReferenceModularAccount is ModuleEntity sigValidation = ModuleEntity.wrap(bytes24(signature)); signature = signature[24:]; - ModuleEntity[] memory preSignatureValidationHooks = - getAccountStorage().validationData[sigValidation].preValidationHooks; + HookConfig[] memory preSignatureValidationHooks = + getAccountStorage().validationData[sigValidation].validationHooks; for (uint256 i = 0; i < preSignatureValidationHooks.length; ++i) { - (address hookModule, uint32 hookEntityId) = preSignatureValidationHooks[i].unpack(); + (address hookModule, uint32 hookEntityId) = preSignatureValidationHooks[i].moduleEntity().unpack(); bytes memory currentSignatureSegment; @@ -384,13 +384,13 @@ contract ReferenceModularAccount is uint256 validationRes; // Do preUserOpValidation hooks - ModuleEntity[] memory preUserOpValidationHooks = - getAccountStorage().validationData[userOpValidationFunction].preValidationHooks; + HookConfig[] memory preUserOpValidationHooks = + getAccountStorage().validationData[userOpValidationFunction].validationHooks; for (uint256 i = 0; i < preUserOpValidationHooks.length; ++i) { (userOp.signature, signature) = signature.advanceSegmentIfAtIndex(uint8(i)); - (address module, uint32 entityId) = preUserOpValidationHooks[i].unpack(); + (address module, uint32 entityId) = preUserOpValidationHooks[i].moduleEntity().unpack(); uint256 currentValidationRes = IValidationHookModule(module).preUserOpValidationHook(entityId, userOp, userOpHash); @@ -424,15 +424,15 @@ contract ReferenceModularAccount is bytes calldata authorizationData ) internal { // run all preRuntimeValidation hooks - ModuleEntity[] memory preRuntimeValidationHooks = - getAccountStorage().validationData[runtimeValidationFunction].preValidationHooks; + HookConfig[] memory preRuntimeValidationHooks = + getAccountStorage().validationData[runtimeValidationFunction].validationHooks; for (uint256 i = 0; i < preRuntimeValidationHooks.length; ++i) { bytes memory currentAuthSegment; (currentAuthSegment, authorizationData) = authorizationData.advanceSegmentIfAtIndex(uint8(i)); - _doPreRuntimeValidationHook(preRuntimeValidationHooks[i], callData, currentAuthSegment); + _doPreRuntimeValidationHook(preRuntimeValidationHooks[i].moduleEntity(), callData, currentAuthSegment); } authorizationData = authorizationData.getFinalSegment(); @@ -569,12 +569,12 @@ contract ReferenceModularAccount is // Direct call is allowed, run associated execution & validation hooks // Validation hooks - ModuleEntity[] memory preRuntimeValidationHooks = - _storage.validationData[directCallValidationKey].preValidationHooks; + HookConfig[] memory preRuntimeValidationHooks = + _storage.validationData[directCallValidationKey].validationHooks; uint256 hookLen = preRuntimeValidationHooks.length; for (uint256 i = 0; i < hookLen; ++i) { - _doPreRuntimeValidationHook(preRuntimeValidationHooks[i], msg.data, ""); + _doPreRuntimeValidationHook(preRuntimeValidationHooks[i].moduleEntity(), msg.data, ""); } // Execution hooks associated with the validator diff --git a/src/interfaces/IModularAccountView.sol b/src/interfaces/IModularAccountView.sol index 4370cab3..2da36e5c 100644 --- a/src/interfaces/IModularAccountView.sol +++ b/src/interfaces/IModularAccountView.sol @@ -26,8 +26,8 @@ struct ValidationDataView { bool isSignatureValidation; // Whether or not this validation function is a user operation validation function. bool isUserOpValidation; - // The pre validation hooks for this validation function. - ModuleEntity[] preValidationHooks; + // The validation hooks for this validation function. + HookConfig[] validationHooks; // Execution hooks to run with this validation function. HookConfig[] executionHooks; // The set of selectors that may be validated by this validation function. diff --git a/standard/ERCs/erc-6900.md b/standard/ERCs/erc-6900.md index 015a954f..0988a95a 100644 --- a/standard/ERCs/erc-6900.md +++ b/standard/ERCs/erc-6900.md @@ -245,8 +245,8 @@ struct ValidationDataView { bool isSignatureValidation; // Whether or not this validation function is a user operation validation function. bool isUserOpValidation; - // The pre validation hooks for this validation function. - ModuleEntity[] preValidationHooks; + // The validation hooks for this validation function. + HookConfig[] validationHooks; // Execution hooks to run with this validation function. HookConfig[] executionHooks; // The set of selectors that may be validated by this validation function. diff --git a/test/account/ModularAccountView.t.sol b/test/account/ModularAccountView.t.sol index 97d864a8..3c0a2805 100644 --- a/test/account/ModularAccountView.t.sol +++ b/test/account/ModularAccountView.t.sol @@ -103,19 +103,19 @@ contract ModularAccountViewTest is CustomValidationTestBase { assertTrue(data.isGlobal); assertTrue(data.isSignatureValidation); assertTrue(data.isUserOpValidation); - assertEq(data.preValidationHooks.length, 2); + assertEq(data.validationHooks.length, 2); assertEq( - ModuleEntity.unwrap(data.preValidationHooks[0]), - ModuleEntity.unwrap( - ModuleEntityLib.pack( + HookConfig.unwrap(data.validationHooks[0]), + HookConfig.unwrap( + HookConfigLib.packValidationHook( address(comprehensiveModule), uint32(ComprehensiveModule.EntityId.PRE_VALIDATION_HOOK_1) ) ) ); assertEq( - ModuleEntity.unwrap(data.preValidationHooks[1]), - ModuleEntity.unwrap( - ModuleEntityLib.pack( + HookConfig.unwrap(data.validationHooks[1]), + HookConfig.unwrap( + HookConfigLib.packValidationHook( address(comprehensiveModule), uint32(ComprehensiveModule.EntityId.PRE_VALIDATION_HOOK_2) ) ) diff --git a/test/module/NativeTokenLimitModule.t.sol b/test/module/NativeTokenLimitModule.t.sol index 32642f4d..c4bcf927 100644 --- a/test/module/NativeTokenLimitModule.t.sol +++ b/test/module/NativeTokenLimitModule.t.sol @@ -11,7 +11,7 @@ import {ModuleEntityLib} from "../../src/helpers/ModuleEntityLib.sol"; import {HookConfigLib} from "../../src/helpers/HookConfigLib.sol"; import {ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol"; import {ExecutionManifest} from "../../src/interfaces/IExecutionModule.sol"; -import {Call, IModularAccount} from "../../src/interfaces/IModularAccount.sol"; +import {Call, HookConfig, IModularAccount} from "../../src/interfaces/IModularAccount.sol"; import {NativeTokenLimitModule} from "../../src/modules/NativeTokenLimitModule.sol"; import {MockModule} from "../mocks/MockModule.sol"; @@ -35,8 +35,8 @@ contract NativeTokenLimitModuleTest is AccountTestBase { vm.deal(address(acct), 10 ether); - ModuleEntity[] memory preValidationHooks = new ModuleEntity[](1); - preValidationHooks[0] = ModuleEntityLib.pack(address(module), 0); + HookConfig[] memory validationHooks = new HookConfig[](1); + validationHooks[0] = HookConfigLib.packValidationHook(address(module), 0); uint256[] memory spendLimits = new uint256[](1); spendLimits[0] = spendLimit; diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index b6253311..2aeef788 100644 --- a/test/utils/AccountTestBase.sol +++ b/test/utils/AccountTestBase.sol @@ -7,8 +7,9 @@ import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/Messa import {ReferenceModularAccount} from "../../src/account/ReferenceModularAccount.sol"; import {SemiModularAccount} from "../../src/account/SemiModularAccount.sol"; +import {HookConfigLib} from "../../src/helpers/HookConfigLib.sol"; import {ModuleEntity, ModuleEntityLib} from "../../src/helpers/ModuleEntityLib.sol"; -import {Call, IModularAccount} from "../../src/interfaces/IModularAccount.sol"; +import {Call, HookConfig, IModularAccount} from "../../src/interfaces/IModularAccount.sol"; import {SingleSignerValidationModule} from "../../src/modules/validation/SingleSignerValidationModule.sol"; import {OptimizedTest} from "./OptimizedTest.sol"; @@ -20,6 +21,7 @@ import {SingleSignerFactoryFixture} from "../mocks/SingleSignerFactoryFixture.so /// @dev This contract handles common boilerplate setup for tests using ReferenceModularAccount with /// SingleSignerValidationModule. abstract contract AccountTestBase is OptimizedTest { + using HookConfigLib for HookConfig; using ModuleEntityLib for ModuleEntity; using MessageHashUtils for bytes32; From fd241dc9e3a3b331b7ce8fc5c3ef9c7ad2f86732 Mon Sep 17 00:00:00 2001 From: Jay Paik Date: Tue, 1 Oct 2024 16:45:30 -0400 Subject: [PATCH 2/2] fix: remove unused HookConfigLib in AccountTestBase --- test/utils/AccountTestBase.sol | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index 2aeef788..b6253311 100644 --- a/test/utils/AccountTestBase.sol +++ b/test/utils/AccountTestBase.sol @@ -7,9 +7,8 @@ import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/Messa import {ReferenceModularAccount} from "../../src/account/ReferenceModularAccount.sol"; import {SemiModularAccount} from "../../src/account/SemiModularAccount.sol"; -import {HookConfigLib} from "../../src/helpers/HookConfigLib.sol"; import {ModuleEntity, ModuleEntityLib} from "../../src/helpers/ModuleEntityLib.sol"; -import {Call, HookConfig, IModularAccount} from "../../src/interfaces/IModularAccount.sol"; +import {Call, IModularAccount} from "../../src/interfaces/IModularAccount.sol"; import {SingleSignerValidationModule} from "../../src/modules/validation/SingleSignerValidationModule.sol"; import {OptimizedTest} from "./OptimizedTest.sol"; @@ -21,7 +20,6 @@ import {SingleSignerFactoryFixture} from "../mocks/SingleSignerFactoryFixture.so /// @dev This contract handles common boilerplate setup for tests using ReferenceModularAccount with /// SingleSignerValidationModule. abstract contract AccountTestBase is OptimizedTest { - using HookConfigLib for HookConfig; using ModuleEntityLib for ModuleEntity; using MessageHashUtils for bytes32;