Skip to content

Commit

Permalink
remove validation functions from the manifest
Browse files Browse the repository at this point in the history
  • Loading branch information
@adamegyed
adamegyed committed Jul 19, 2024
1 parent 2a5de9d commit 610a6fc
Show file tree
Hide file tree
Showing 11 changed files with 85 additions and 149 deletions.
51 changes: 14 additions & 37 deletions src/account/ModuleManagerInternals.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@ import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165C

import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";

import {RESERVED_VALIDATION_DATA_INDEX, SELF_PERMIT_VALIDATION_FUNCTIONID} 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";

Expand All @@ -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);
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 > RESERVED_VALIDATION_DATA_INDEX) {
revert PreValidationHookLimitExceeded();
}
}
Expand All @@ -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]);
}
}
}
Expand All @@ -297,7 +274,7 @@ abstract contract ModuleManagerInternals is IModuleManager {
}
}

if (validationConfig.entityId() != _SELF_PERMIT_VALIDATION_FUNCTIONID) {
if (validationConfig.entityId() != SELF_PERMIT_VALIDATION_FUNCTIONID) {
// Only allow global validations and signature validations if they're not direct-call validations.

_validationData.isGlobal = validationConfig.isGlobal();
Expand Down Expand Up @@ -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;
Expand All @@ -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]);
}
}

Expand All @@ -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);
}
}
}
8 changes: 4 additions & 4 deletions src/account/UpgradeableModularAccount.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {SparseCalldataSegmentLib} from "../helpers/SparseCalldataSegmentLib.sol"
import {ValidationConfigLib} from "../helpers/ValidationConfigLib.sol";
import {_coalescePreValidation, _coalesceValidation} from "../helpers/ValidationResHelpers.sol";

import {RESERVED_VALIDATION_DATA_INDEX, SELF_PERMIT_VALIDATION_FUNCTIONID} from "../helpers/Constants.sol";
import {IExecutionHook} from "../interfaces/IExecutionHook.sol";
import {ModuleManifest} from "../interfaces/IModule.sol";
import {IModuleManager, ModuleEntity, ValidationConfig} from "../interfaces/IModuleManager.sol";
Expand All @@ -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
Expand Down Expand Up @@ -441,7 +441,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();
}

Expand Down Expand Up @@ -497,7 +497,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();
}

Expand Down Expand Up @@ -635,7 +635,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, SELF_PERMIT_VALIDATION_FUNCTIONID);

_checkIfValidationAppliesCallData(msg.data, directCallValidationKey, false);

Expand Down
8 changes: 8 additions & 0 deletions src/helpers/Constants.sol
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// 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 = 255;

// Magic value for the Entity ID of direct call validation.
uint32 constant SELF_PERMIT_VALIDATION_FUNCTIONID = type(uint32).max;
9 changes: 0 additions & 9 deletions src/interfaces/IModule.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down
16 changes: 10 additions & 6 deletions test/account/AccountLoupe.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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),
""
Expand Down
15 changes: 15 additions & 0 deletions test/account/AccountReturnData.t.sol
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.19;

import {SELF_PERMIT_VALIDATION_FUNCTIONID} 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,
Expand Down Expand Up @@ -38,6 +41,18 @@ 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), SELF_PERMIT_VALIDATION_FUNCTIONID, false, false
),
selectors,
"",
"",
""
);
vm.stopPrank();
}

Expand Down
12 changes: 9 additions & 3 deletions test/account/SelfCallAuthorization.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
39 changes: 19 additions & 20 deletions test/account/ValidationIntersection.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,19 +46,21 @@ contract ValidationIntersectionTest is AccountTestBase {
entityId: uint32(MockBaseUserOpValidationModule.EntityId.USER_OP_VALIDATION)
});

bytes4[] memory validationSelectors = new bytes4[](1);
validationSelectors[0] = MockUserOpValidationModule.foo.selector;

vm.startPrank(address(entryPoint));
account1.installModule({
module: address(noHookModule),
manifest: noHookModule.moduleManifest(),
moduleInstallData: ""
});
account1.installModule({
module: address(oneHookModule),
manifest: oneHookModule.moduleManifest(),
moduleInstallData: ""
});
// TODO: change with new install flow
// temporary fix to add the pre-validation hook
// Install noHookValidation
account1.installValidation(
ValidationConfigLib.pack(noHookValidation, true, true),
validationSelectors,
bytes(""),
bytes(""),
bytes("")
);

// Install oneHookValidation
validationSelectors[0] = MockUserOpValidation1HookModule.bar.selector;
ModuleEntity[] memory preValidationHooks = new ModuleEntity[](1);
preValidationHooks[0] = ModuleEntityLib.pack({
addr: address(oneHookModule),
Expand All @@ -67,17 +69,14 @@ contract ValidationIntersectionTest is AccountTestBase {
bytes[] memory installDatas = new bytes[](1);
account1.installValidation(
ValidationConfigLib.pack(oneHookValidation, true, true),
new bytes4[](0),
validationSelectors,
bytes(""),
abi.encode(preValidationHooks, installDatas),
bytes("")
);
account1.installModule({
module: address(twoHookModule),
manifest: twoHookModule.moduleManifest(),
moduleInstallData: ""
});
// temporary fix to add the pre-validation hook

// Install twoHookValidation
validationSelectors[0] = MockUserOpValidation2HookModule.baz.selector;
preValidationHooks = new ModuleEntity[](2);
preValidationHooks[0] = ModuleEntityLib.pack({
addr: address(twoHookModule),
Expand All @@ -90,7 +89,7 @@ contract ValidationIntersectionTest is AccountTestBase {
installDatas = new bytes[](2);
account1.installValidation(
ValidationConfigLib.pack(twoHookValidation, true, true),
new bytes4[](0),
validationSelectors,
bytes(""),
abi.encode(preValidationHooks, installDatas),
bytes("")
Expand Down
12 changes: 0 additions & 12 deletions test/mocks/modules/ComprehensiveModule.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import {IExecutionHook} from "../../../src/interfaces/IExecutionHook.sol";
import {
ManifestExecutionFunction,
ManifestExecutionHook,
ManifestValidation,
ModuleManifest,
ModuleMetadata
} from "../../../src/interfaces/IModule.sol";
Expand Down Expand Up @@ -140,17 +139,6 @@ contract ComprehensiveModule is IValidation, IValidationHook, IExecutionHook, Ba
allowGlobalValidation: false
});

bytes4[] memory validationSelectors = new bytes4[](1);
validationSelectors[0] = this.foo.selector;

manifest.validationFunctions = new ManifestValidation[](1);
manifest.validationFunctions[0] = ManifestValidation({
entityId: uint32(EntityId.VALIDATION),
isGlobal: true,
isSignatureValidation: false,
selectors: validationSelectors
});

manifest.executionHooks = new ManifestExecutionHook[](3);
manifest.executionHooks[0] = ManifestExecutionHook({
executionSelector: this.foo.selector,
Expand Down
Loading

0 comments on commit 610a6fc

Please sign in to comment.