Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: [v0.8-develop] Remove validation installation from the manifest 3/N #104

Merged
merged 3 commits into from
Jul 23, 2024
Merged

feat: [v0.8-develop] Remove validation installation from the manifest 3/N #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a6c70e1
remove validation functions from the manifest
adamegyed Jul 18, 2024
d236c1b
remove manifest-based
adamegyed Jul 22, 2024
22f26cf
update constant naming
adamegyed Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 14 additions & 37 deletions src/account/ModuleManagerInternals.sol
View file
Open in desktop
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 {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";

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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we get rid of preValidationHooks installation in installModule in a previous PR? Don't see them in code anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did, I think in the PR that introduced installValidation. Previously, preValidationHooks were associated with selectors, which also associated them to validations because there was a 1:1 mapping between selectors and validation. But when we allowed multiple validation functions, we needed a way to associate the preValidationHooks, so they became installValidation only.

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 > MAX_PRE_VALIDATION_HOOKS) {
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() != DIRECT_CALL_VALIDATION_ENTITYID) {
// 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);
}
}
}
30 changes: 4 additions & 26 deletions src/account/UpgradeableModularAccount.sol
View file
Open in desktop
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 {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";
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 @@ -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
Expand Down Expand Up @@ -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();
}

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

Expand Down Expand Up @@ -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);

Expand Down
11 changes: 11 additions & 0 deletions src/helpers/Constants.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.25;

// Index marking the start of the data for the validation function.
adamegyed marked this conversation as resolved.
Show resolved Hide resolved
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;
9 changes: 0 additions & 9 deletions src/interfaces/IModule.sol
View file
Open in desktop
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
Open in desktop
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
13 changes: 13 additions & 0 deletions test/account/AccountReturnData.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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();
}

Expand Down
12 changes: 9 additions & 3 deletions test/account/SelfCallAuthorization.t.sol
View file
Open in desktop
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
Loading
Loading