feat: [v0.8-develop] Remove validation installation from the manifest…

… 3/N (#104)
erc6900 · Jul 26, 2024 · 16b6e97 · 16b6e97
1 parent 125920f
commit 16b6e97
Show file tree

Hide file tree

Showing 11 changed files with 86 additions and 171 deletions.
diff --git a/src/account/ModuleManagerInternals.sol b/src/account/ModuleManagerInternals.sol
@@ -5,11 +5,12 @@ import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165C
 
 import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
 
+import {DIRECT_CALL_VALIDATION_ENTITYID, MAX_PRE_VALIDATION_HOOKS} from "../helpers/Constants.sol";
 import {KnownSelectors} from "../helpers/KnownSelectors.sol";
 import {ModuleEntityLib} from "../helpers/ModuleEntityLib.sol";
 import {ValidationConfigLib} from "../helpers/ValidationConfigLib.sol";
 import {ExecutionHook} from "../interfaces/IAccountLoupe.sol";
-import {IModule, ManifestExecutionHook, ManifestValidation, ModuleManifest} from "../interfaces/IModule.sol";
+import {IModule, ManifestExecutionHook, ModuleManifest} from "../interfaces/IModule.sol";
 import {IModuleManager, ModuleEntity, ValidationConfig} from "../interfaces/IModuleManager.sol";
 import {AccountStorage, SelectorData, ValidationData, getAccountStorage, toSetValue} from "./AccountStorage.sol";
 
@@ -18,12 +19,6 @@ abstract contract ModuleManagerInternals is IModuleManager {
     using ModuleEntityLib for ModuleEntity;
     using ValidationConfigLib for ValidationConfig;
 
-    // Index marking the start of the data for the validation function.
-    uint8 internal constant _RESERVED_VALIDATION_DATA_INDEX = 255;
-
-    // Magic value for the Entity ID of direct call validation.
-    uint32 internal constant _SELF_PERMIT_VALIDATION_FUNCTIONID = type(uint32).max;
-
     error ArrayLengthMismatch();
     error Erc4337FunctionNotAllowed(bytes4 selector);
     error ExecutionFunctionAlreadySet(bytes4 selector);
@@ -163,17 +158,6 @@ abstract contract ModuleManagerInternals is IModuleManager {
             _setExecutionFunction(selector, isPublic, allowGlobalValidation, module);
         }
 
-        length = manifest.validationFunctions.length;
-        for (uint256 i = 0; i < length; ++i) {
-            // Todo: limit this to only "direct runtime call" validation path (old EFP),
-            // and add a way for the user to specify permission/pre-val hooks here.
-            ManifestValidation memory mv = manifest.validationFunctions[i];
-
-            ValidationConfig validationConfig =
-                ValidationConfigLib.pack(module, mv.entityId, mv.isGlobal, mv.isSignatureValidation);
-            _addValidationFunction(validationConfig, mv.selectors);
-        }
-
         length = manifest.executionHooks.length;
         for (uint256 i = 0; i < length; ++i) {
             ManifestExecutionHook memory mh = manifest.executionHooks[i];
@@ -212,13 +196,6 @@ abstract contract ModuleManagerInternals is IModuleManager {
             _removeExecHooks(execHooks, hookFunction, mh.isPreHook, mh.isPostHook);
         }
 
-        length = manifest.validationFunctions.length;
-        for (uint256 i = 0; i < length; ++i) {
-            ModuleEntity validationFunction =
-                ModuleEntityLib.pack(module, manifest.validationFunctions[i].entityId);
-            _removeValidationFunction(validationFunction);
-        }
-
         length = manifest.executionFunctions.length;
         for (uint256 i = 0; i < length; ++i) {
             bytes4 selector = manifest.executionFunctions[i].executionSelector;
@@ -261,13 +238,13 @@ abstract contract ModuleManagerInternals is IModuleManager {
                 _validationData.preValidationHooks.push(preValidationFunction);
 
                 if (initDatas[i].length > 0) {
-                    (address preValidationPlugin,) = ModuleEntityLib.unpack(preValidationFunction);
-                    IModule(preValidationPlugin).onInstall(initDatas[i]);
+                    (address preValidationModule,) = ModuleEntityLib.unpack(preValidationFunction);
+                    IModule(preValidationModule).onInstall(initDatas[i]);
                 }
             }
 
             // Avoid collision between reserved index and actual indices
-            if (_validationData.preValidationHooks.length > _RESERVED_VALIDATION_DATA_INDEX) {
+            if (_validationData.preValidationHooks.length > MAX_PRE_VALIDATION_HOOKS) {
                 revert PreValidationHookLimitExceeded();
             }
         }
@@ -284,8 +261,8 @@ abstract contract ModuleManagerInternals is IModuleManager {
                 }
 
                 if (initDatas[i].length > 0) {
-                    (address executionPlugin,) = ModuleEntityLib.unpack(permissionFunction.hookFunction);
-                    IModule(executionPlugin).onInstall(initDatas[i]);
+                    (address executionModule,) = ModuleEntityLib.unpack(permissionFunction.hookFunction);
+                    IModule(executionModule).onInstall(initDatas[i]);
                 }
             }
         }
@@ -297,7 +274,7 @@ abstract contract ModuleManagerInternals is IModuleManager {
             }
         }
 
-        if (validationConfig.entityId() != _SELF_PERMIT_VALIDATION_FUNCTIONID) {
+        if (validationConfig.entityId() != DIRECT_CALL_VALIDATION_ENTITYID) {
             // Only allow global validations and signature validations if they're not direct-call validations.
 
             _validationData.isGlobal = validationConfig.isGlobal();
@@ -326,8 +303,8 @@ abstract contract ModuleManagerInternals is IModuleManager {
             for (uint256 i = 0; i < preValidationHooks.length; ++i) {
                 ModuleEntity preValidationFunction = preValidationHooks[i];
                 if (preValidationHookUninstallDatas[0].length > 0) {
-                    (address preValidationPlugin,) = ModuleEntityLib.unpack(preValidationFunction);
-                    IModule(preValidationPlugin).onUninstall(preValidationHookUninstallDatas[0]);
+                    (address preValidationModule,) = ModuleEntityLib.unpack(preValidationFunction);
+                    IModule(preValidationModule).onUninstall(preValidationHookUninstallDatas[0]);
                 }
             }
             delete _validationData.preValidationHooks;
@@ -342,8 +319,8 @@ abstract contract ModuleManagerInternals is IModuleManager {
             for (uint256 i = 0; i < permissionHookLen; ++i) {
                 bytes32 permissionHook = permissionHooks.at(0);
                 permissionHooks.remove(permissionHook);
-                address permissionHookPlugin = address(uint160(bytes20(permissionHook)));
-                IModule(permissionHookPlugin).onUninstall(permissionHookUninstallDatas[i]);
+                address permissionHookModule = address(uint160(bytes20(permissionHook)));
+                IModule(permissionHookModule).onUninstall(permissionHookUninstallDatas[i]);
             }
         }
 
@@ -355,8 +332,8 @@ abstract contract ModuleManagerInternals is IModuleManager {
         }
 
         if (uninstallData.length > 0) {
-            (address plugin,) = ModuleEntityLib.unpack(validationFunction);
-            IModule(plugin).onUninstall(uninstallData);
+            (address module,) = ModuleEntityLib.unpack(validationFunction);
+            IModule(module).onUninstall(uninstallData);
         }
     }
 }
diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol
@@ -18,6 +18,7 @@ import {SparseCalldataSegmentLib} from "../helpers/SparseCalldataSegmentLib.sol"
 import {ValidationConfigLib} from "../helpers/ValidationConfigLib.sol";
 import {_coalescePreValidation, _coalesceValidation} from "../helpers/ValidationResHelpers.sol";
 
+import {DIRECT_CALL_VALIDATION_ENTITYID, RESERVED_VALIDATION_DATA_INDEX} from "../helpers/Constants.sol";
 import {IExecutionHook} from "../interfaces/IExecutionHook.sol";
 import {ModuleManifest} from "../interfaces/IModule.sol";
 import {IModuleManager, ModuleEntity, ValidationConfig} from "../interfaces/IModuleManager.sol";
@@ -28,7 +29,6 @@ import {AccountExecutor} from "./AccountExecutor.sol";
 import {AccountLoupe} from "./AccountLoupe.sol";
 import {AccountStorage, getAccountStorage, toExecutionHook, toSetValue} from "./AccountStorage.sol";
 import {AccountStorageInitializable} from "./AccountStorageInitializable.sol";
-
 import {ModuleManagerInternals} from "./ModuleManagerInternals.sol";
 
 contract UpgradeableModularAccount is
@@ -105,28 +105,6 @@ contract UpgradeableModularAccount is
 
     // EXTERNAL FUNCTIONS
 
-    /// @notice Initializes the account with a set of modules
-    /// @param modules The modules to install
-    /// @param manifests The manifests of the modules to install
-    /// @param moduleInstallDatas The module install datas of the modules to install
-    function initialize(
-        address[] memory modules,
-        ModuleManifest[] calldata manifests,
-        bytes[] memory moduleInstallDatas
-    ) external initializer {
-        uint256 length = modules.length;
-
-        if (length != manifests.length || length != moduleInstallDatas.length) {
-            revert ArrayLengthMismatch();
-        }
-
-        for (uint256 i = 0; i < length; ++i) {
-            _installModule(modules[i], manifests[i], moduleInstallDatas[i]);
-        }
-
-        emit ModularAccountInitialized(_ENTRY_POINT);
-    }
-
     receive() external payable {}
 
     /// @notice Fallback function
@@ -441,7 +419,7 @@ contract UpgradeableModularAccount is
 
         // Run the user op validationFunction
         {
-            if (signatureSegment.getIndex() != _RESERVED_VALIDATION_DATA_INDEX) {
+            if (signatureSegment.getIndex() != RESERVED_VALIDATION_DATA_INDEX) {
                 revert ValidationSignatureSegmentMissing();
             }
 
@@ -497,7 +475,7 @@ contract UpgradeableModularAccount is
             _doPreRuntimeValidationHook(preRuntimeValidationHooks[i], callData, currentAuthData);
         }
 
-        if (authSegment.getIndex() != _RESERVED_VALIDATION_DATA_INDEX) {
+        if (authSegment.getIndex() != RESERVED_VALIDATION_DATA_INDEX) {
             revert ValidationSignatureSegmentMissing();
         }
 
@@ -635,7 +613,7 @@ contract UpgradeableModularAccount is
             return (new PostExecToRun[](0), new PostExecToRun[](0));
         }
 
-        ModuleEntity directCallValidationKey = ModuleEntityLib.pack(msg.sender, _SELF_PERMIT_VALIDATION_FUNCTIONID);
+        ModuleEntity directCallValidationKey = ModuleEntityLib.pack(msg.sender, DIRECT_CALL_VALIDATION_ENTITYID);
 
         _checkIfValidationAppliesCallData(msg.data, directCallValidationKey, false);
 

diff --git a/src/helpers/Constants.sol b/src/helpers/Constants.sol
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity ^0.8.25;
+
+// Index marking the start of the data for the validation function.
+uint8 constant RESERVED_VALIDATION_DATA_INDEX = type(uint8).max;
+
+// Maximum number of pre-validation hooks that can be registered.
+uint8 constant MAX_PRE_VALIDATION_HOOKS = type(uint8).max;
+
+// Magic value for the Entity ID of direct call validation.
+uint32 constant DIRECT_CALL_VALIDATION_ENTITYID = type(uint32).max;
diff --git a/src/interfaces/IModule.sol b/src/interfaces/IModule.sol
@@ -13,14 +13,6 @@ struct ManifestExecutionFunction {
     bool allowGlobalValidation;
 }
 
-// todo: do we need these at all? Or do we fully switch to `installValidation`?
-struct ManifestValidation {
-    uint32 entityId;
-    bool isGlobal;
-    bool isSignatureValidation;
-    bytes4[] selectors;
-}
-
 struct ManifestExecutionHook {
     // TODO(erc6900 spec): These fields can be packed into a single word
     bytes4 executionSelector;
@@ -54,7 +46,6 @@ struct ModuleMetadata {
 struct ModuleManifest {
     // Execution functions defined in this module to be installed on the MSCA.
     ManifestExecutionFunction[] executionFunctions;
-    ManifestValidation[] validationFunctions;
     ManifestExecutionHook[] executionHooks;
     // List of ERC-165 interface IDs to add to account to support introspection checks. This MUST NOT include
     // IModule's interface ID.

diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol
@@ -16,8 +16,12 @@ contract AccountLoupeTest is CustomValidationTestBase {
 
     event ReceivedCall(bytes msgData, uint256 msgValue);
 
+    ModuleEntity public comprehensiveModuleValidation;
+
     function setUp() public {
         comprehensiveModule = new ComprehensiveModule();
+        comprehensiveModuleValidation =
+            ModuleEntityLib.pack(address(comprehensiveModule), uint32(ComprehensiveModule.EntityId.VALIDATION));
 
         _customValidationSetup();
 
@@ -61,9 +65,6 @@ contract AccountLoupeTest is CustomValidationTestBase {
     }
 
     function test_moduleLoupe_getSelectors() public {
-        ModuleEntity comprehensiveModuleValidation =
-            ModuleEntityLib.pack(address(comprehensiveModule), uint32(ComprehensiveModule.EntityId.VALIDATION));
-
         bytes4[] memory selectors = account1.getSelectors(comprehensiveModuleValidation);
 
         assertEq(selectors.length, 1);
@@ -107,7 +108,7 @@ contract AccountLoupeTest is CustomValidationTestBase {
     }
 
     function test_moduleLoupe_getValidationHooks() public {
-        ModuleEntity[] memory hooks = account1.getPreValidationHooks(_signerValidation);
+        ModuleEntity[] memory hooks = account1.getPreValidationHooks(comprehensiveModuleValidation);
 
         assertEq(hooks.length, 2);
         assertEq(
@@ -144,12 +145,15 @@ contract AccountLoupeTest is CustomValidationTestBase {
             address(comprehensiveModule), uint32(ComprehensiveModule.EntityId.PRE_VALIDATION_HOOK_2)
         );
 
+        bytes4[] memory selectors = new bytes4[](1);
+        selectors[0] = comprehensiveModule.foo.selector;
+
         bytes[] memory installDatas = new bytes[](2);
         return (
-            _signerValidation,
+            comprehensiveModuleValidation,
             true,
             true,
-            new bytes4[](0),
+            selectors,
             bytes(""),
             abi.encode(preValidationHooks, installDatas),
             ""

diff --git a/test/account/AccountReturnData.t.sol b/test/account/AccountReturnData.t.sol
@@ -1,8 +1,11 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity ^0.8.19;
 
+import {DIRECT_CALL_VALIDATION_ENTITYID} from "../../src/helpers/Constants.sol";
 import {ModuleEntityLib} from "../../src/helpers/ModuleEntityLib.sol";
+import {ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol";
 import {Call} from "../../src/interfaces/IStandardExecutor.sol";
+import {IStandardExecutor} from "../../src/interfaces/IStandardExecutor.sol";
 
 import {
     RegularResultContract,
@@ -38,6 +41,16 @@ contract AccountReturnDataTest is AccountTestBase {
             manifest: resultConsumerModule.moduleManifest(),
             moduleInstallData: ""
         });
+        // Allow the result consumer module to perform direct calls to the account
+        bytes4[] memory selectors = new bytes4[](1);
+        selectors[0] = IStandardExecutor.execute.selector;
+        account1.installValidation(
+            ValidationConfigLib.pack(address(resultConsumerModule), DIRECT_CALL_VALIDATION_ENTITYID, false, false),
+            selectors,
+            "",
+            "",
+            ""
+        );
         vm.stopPrank();
     }
 

diff --git a/test/account/SelfCallAuthorization.t.sol b/test/account/SelfCallAuthorization.t.sol
@@ -24,12 +24,18 @@ contract SelfCallAuthorizationTest is AccountTestBase {
 
         comprehensiveModule = new ComprehensiveModule();
 
+        comprehensiveModuleValidation =
+            ModuleEntityLib.pack(address(comprehensiveModule), uint32(ComprehensiveModule.EntityId.VALIDATION));
+
+        bytes4[] memory validationSelectors = new bytes4[](1);
+        validationSelectors[0] = ComprehensiveModule.foo.selector;
+
         vm.startPrank(address(entryPoint));
         account1.installModule(address(comprehensiveModule), comprehensiveModule.moduleManifest(), "");
+        account1.installValidation(
+            ValidationConfigLib.pack(comprehensiveModuleValidation, false, false), validationSelectors, "", "", ""
+        );
         vm.stopPrank();
-
-        comprehensiveModuleValidation =
-            ModuleEntityLib.pack(address(comprehensiveModule), uint32(ComprehensiveModule.EntityId.VALIDATION));
     }
 
     function test_selfCallFails_userOp() public {