From 28421241059c6598a4705a71c45a20a9b397931a Mon Sep 17 00:00:00 2001 From: adam Date: Tue, 5 Mar 2024 12:32:07 -0500 Subject: [PATCH 1/8] Merge validation function assignments --- src/account/AccountLoupe.sol | 4 +- src/account/AccountStorage.sol | 4 +- src/account/PluginManagerInternals.sol | 84 ++++--------------- src/account/UpgradeableModularAccount.sol | 8 +- src/interfaces/IAccountLoupe.sol | 3 +- src/interfaces/IPlugin.sol | 3 +- src/plugins/TokenReceiverPlugin.sol | 10 +-- src/plugins/owner/ISingleOwnerPlugin.sol | 3 +- src/plugins/owner/SingleOwnerPlugin.sol | 78 +++++------------ .../plugins/ModularSessionKeyPlugin.sol | 51 ++++------- src/samples/plugins/TokenSessionKeyPlugin.sol | 22 ++--- .../plugins/interfaces/ISessionKeyPlugin.sol | 3 +- test/account/AccountExecHooks.t.sol | 2 +- test/account/AccountLoupe.t.sol | 57 ++++--------- test/account/ManifestValidity.t.sol | 17 ---- .../plugins/BadTransferOwnershipPlugin.sol | 4 +- test/mocks/plugins/ComprehensivePlugin.sol | 28 ++----- .../ExecFromPluginPermissionsMocks.sol | 8 +- test/mocks/plugins/ManifestValidityMocks.sol | 46 ++-------- test/mocks/plugins/ReturnDataPluginMocks.sol | 10 +-- test/mocks/plugins/ValidationPluginMocks.sol | 12 +-- test/plugin/SingleOwnerPlugin.t.sol | 8 +- .../plugins/ModularSessionKeyPlugin.t.sol | 21 ++--- 23 files changed, 132 insertions(+), 354 deletions(-) diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol index 8459c65a..8b6f42a2 100644 --- a/src/account/AccountLoupe.sol +++ b/src/account/AccountLoupe.sol @@ -36,9 +36,7 @@ abstract contract AccountLoupe is IAccountLoupe { config.plugin = _storage.selectorData[selector].plugin; } - config.userOpValidationFunction = _storage.selectorData[selector].userOpValidation; - - config.runtimeValidationFunction = _storage.selectorData[selector].runtimeValidation; + config.validationFunction = _storage.selectorData[selector].validation; } /// @inheritdoc IAccountLoupe diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 0fea6771..deb31228 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -35,8 +35,8 @@ struct SelectorData { // The plugin that implements this execution function. // If this is a native function, the address must remain address(0). address plugin; - FunctionReference userOpValidation; - FunctionReference runtimeValidation; + // User operation validation and runtime validation share a function reference. + FunctionReference validation; // The pre validation hooks for this function selector. EnumerableMap.Bytes32ToUintMap preUserOpValidationHooks; EnumerableMap.Bytes32ToUintMap preRuntimeValidationHooks; diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index e3062661..7ed6d768 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -41,8 +41,7 @@ abstract contract PluginManagerInternals is IPluginManager { error PluginInstallCallbackFailed(address plugin, bytes revertReason); error PluginInterfaceNotSupported(address plugin); error PluginNotInstalled(address plugin); - error RuntimeValidationFunctionAlreadySet(bytes4 selector, FunctionReference validationFunction); - error UserOpValidationFunctionAlreadySet(bytes4 selector, FunctionReference validationFunction); + error ValidationFunctionAlreadySet(bytes4 selector, FunctionReference validationFunction); modifier notNullFunction(FunctionReference functionReference) { if (functionReference.isEmpty()) { @@ -76,48 +75,26 @@ abstract contract PluginManagerInternals is IPluginManager { _selectorData.plugin = address(0); } - function _addUserOpValidationFunction(bytes4 selector, FunctionReference validationFunction) + function _addValidationFunction(bytes4 selector, FunctionReference validationFunction) internal notNullFunction(validationFunction) { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - if (!_selectorData.userOpValidation.isEmpty()) { - revert UserOpValidationFunctionAlreadySet(selector, validationFunction); + if (!_selectorData.validation.isEmpty()) { + revert ValidationFunctionAlreadySet(selector, validationFunction); } - _selectorData.userOpValidation = validationFunction; + _selectorData.validation = validationFunction; } - function _removeUserOpValidationFunction(bytes4 selector, FunctionReference validationFunction) + function _removeValidationFunction(bytes4 selector, FunctionReference validationFunction) internal notNullFunction(validationFunction) { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - _selectorData.userOpValidation = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; - } - - function _addRuntimeValidationFunction(bytes4 selector, FunctionReference validationFunction) - internal - notNullFunction(validationFunction) - { - SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - - if (!_selectorData.runtimeValidation.isEmpty()) { - revert RuntimeValidationFunctionAlreadySet(selector, validationFunction); - } - - _selectorData.runtimeValidation = validationFunction; - } - - function _removeRuntimeValidationFunction(bytes4 selector, FunctionReference validationFunction) - internal - notNullFunction(validationFunction) - { - SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - - _selectorData.runtimeValidation = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; + _selectorData.validation = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; } function _addExecHooks(bytes4 selector, FunctionReference preExecHook, FunctionReference postExecHook) @@ -319,31 +296,13 @@ abstract contract PluginManagerInternals is IPluginManager { } } - length = manifest.userOpValidationFunctions.length; - for (uint256 i = 0; i < length;) { - ManifestAssociatedFunction memory mv = manifest.userOpValidationFunctions[i]; - _addUserOpValidationFunction( - mv.executionSelector, - _resolveManifestFunction( - mv.associatedFunction, plugin, dependencies, ManifestAssociatedFunctionType.NONE - ) - ); - - unchecked { - ++i; - } - } - - length = manifest.runtimeValidationFunctions.length; + length = manifest.validationFunctions.length; for (uint256 i = 0; i < length;) { - ManifestAssociatedFunction memory mv = manifest.runtimeValidationFunctions[i]; - _addRuntimeValidationFunction( + ManifestAssociatedFunction memory mv = manifest.validationFunctions[i]; + _addValidationFunction( mv.executionSelector, _resolveManifestFunction( - mv.associatedFunction, - plugin, - dependencies, - ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW + mv.associatedFunction, plugin, dependencies, ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW ) ); @@ -521,10 +480,10 @@ abstract contract PluginManagerInternals is IPluginManager { } } - length = manifest.runtimeValidationFunctions.length; + length = manifest.validationFunctions.length; for (uint256 i = 0; i < length;) { - ManifestAssociatedFunction memory mv = manifest.runtimeValidationFunctions[i]; - _removeRuntimeValidationFunction( + ManifestAssociatedFunction memory mv = manifest.validationFunctions[i]; + _removeValidationFunction( mv.executionSelector, _resolveManifestFunction( mv.associatedFunction, @@ -539,21 +498,6 @@ abstract contract PluginManagerInternals is IPluginManager { } } - length = manifest.userOpValidationFunctions.length; - for (uint256 i = 0; i < length;) { - ManifestAssociatedFunction memory mv = manifest.userOpValidationFunctions[i]; - _removeUserOpValidationFunction( - mv.executionSelector, - _resolveManifestFunction( - mv.associatedFunction, plugin, dependencies, ManifestAssociatedFunctionType.NONE - ) - ); - - unchecked { - ++i; - } - } - // remove external call permissions if (manifest.permitAnyExternalAddress) { diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index a449ae99..022503c1 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -336,7 +336,7 @@ contract UpgradeableModularAccount is } bytes4 selector = bytes4(userOp.callData); - FunctionReference userOpValidationFunction = getAccountStorage().selectorData[selector].userOpValidation; + FunctionReference userOpValidationFunction = getAccountStorage().selectorData[selector].validation; validationData = _doUserOpValidation(selector, userOpValidationFunction, userOp, userOpHash); } @@ -395,7 +395,9 @@ contract UpgradeableModularAccount is validationData = currentValidationData; } } else { - // _RUNTIME_VALIDATION_ALWAYS_ALLOW and _PRE_HOOK_ALWAYS_DENY is not permitted here. + // _PRE_HOOK_ALWAYS_DENY is not permitted here. + // If this is _RUNTIME_VALIDATION_ALWAYS_ALLOW, the call should revert. + // Todo: this is the wrong error if it is set to _RUNTIME_VALIDATION_ALWAYS_ALLOW. revert InvalidConfiguration(); } } @@ -405,7 +407,7 @@ contract UpgradeableModularAccount is if (msg.sender == address(_ENTRY_POINT)) return; AccountStorage storage _storage = getAccountStorage(); - FunctionReference runtimeValidationFunction = _storage.selectorData[msg.sig].runtimeValidation; + FunctionReference runtimeValidationFunction = _storage.selectorData[msg.sig].validation; // run all preRuntimeValidation hooks EnumerableMap.Bytes32ToUintMap storage preRuntimeValidationHooks = getAccountStorage().selectorData[msg.sig].preRuntimeValidationHooks; diff --git a/src/interfaces/IAccountLoupe.sol b/src/interfaces/IAccountLoupe.sol index 25d50487..300a81b1 100644 --- a/src/interfaces/IAccountLoupe.sol +++ b/src/interfaces/IAccountLoupe.sol @@ -7,8 +7,7 @@ interface IAccountLoupe { /// @notice Config for an execution function, given a selector. struct ExecutionFunctionConfig { address plugin; - FunctionReference userOpValidationFunction; - FunctionReference runtimeValidationFunction; + FunctionReference validationFunction; } /// @notice Pre and post hooks for a given selector. diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index cf74fd89..e8edaf4a 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -90,8 +90,7 @@ struct PluginManifest { // plugin MUST still be able to spend up to the balance that it sends to the account in the same call. bool canSpendNativeToken; ManifestExternalCallPermission[] permittedExternalCalls; - ManifestAssociatedFunction[] userOpValidationFunctions; - ManifestAssociatedFunction[] runtimeValidationFunctions; + ManifestAssociatedFunction[] validationFunctions; ManifestAssociatedFunction[] preUserOpValidationHooks; ManifestAssociatedFunction[] preRuntimeValidationHooks; ManifestExecutionHook[] executionHooks; diff --git a/src/plugins/TokenReceiverPlugin.sol b/src/plugins/TokenReceiverPlugin.sol index ac9335fd..5574ddc8 100644 --- a/src/plugins/TokenReceiverPlugin.sol +++ b/src/plugins/TokenReceiverPlugin.sol @@ -84,20 +84,20 @@ contract TokenReceiverPlugin is BasePlugin, IERC721Receiver, IERC777Recipient, I functionId: 0, // Unused. dependencyIndex: 0 // Unused. }); - manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](4); - manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ + manifest.validationFunctions = new ManifestAssociatedFunction[](4); + manifest.validationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.tokensReceived.selector, associatedFunction: alwaysAllowFunction }); - manifest.runtimeValidationFunctions[1] = ManifestAssociatedFunction({ + manifest.validationFunctions[1] = ManifestAssociatedFunction({ executionSelector: this.onERC721Received.selector, associatedFunction: alwaysAllowFunction }); - manifest.runtimeValidationFunctions[2] = ManifestAssociatedFunction({ + manifest.validationFunctions[2] = ManifestAssociatedFunction({ executionSelector: this.onERC1155Received.selector, associatedFunction: alwaysAllowFunction }); - manifest.runtimeValidationFunctions[3] = ManifestAssociatedFunction({ + manifest.validationFunctions[3] = ManifestAssociatedFunction({ executionSelector: this.onERC1155BatchReceived.selector, associatedFunction: alwaysAllowFunction }); diff --git a/src/plugins/owner/ISingleOwnerPlugin.sol b/src/plugins/owner/ISingleOwnerPlugin.sol index c75ab8a3..fc0622c8 100644 --- a/src/plugins/owner/ISingleOwnerPlugin.sol +++ b/src/plugins/owner/ISingleOwnerPlugin.sol @@ -5,8 +5,7 @@ import {UserOperation} from "@eth-infinitism/account-abstraction/interfaces/User interface ISingleOwnerPlugin { enum FunctionId { - RUNTIME_VALIDATION_OWNER_OR_SELF, - USER_OP_VALIDATION_OWNER + VALIDATION_OWNER_OR_SELF } /// @notice This event is emitted when ownership of the account changes. diff --git a/src/plugins/owner/SingleOwnerPlugin.sol b/src/plugins/owner/SingleOwnerPlugin.sol index fa8ed177..a44d72b5 100644 --- a/src/plugins/owner/SingleOwnerPlugin.sol +++ b/src/plugins/owner/SingleOwnerPlugin.sol @@ -108,7 +108,7 @@ contract SingleOwnerPlugin is BasePlugin, ISingleOwnerPlugin, IERC1271 { view override { - if (functionId == uint8(FunctionId.RUNTIME_VALIDATION_OWNER_OR_SELF)) { + if (functionId == uint8(FunctionId.VALIDATION_OWNER_OR_SELF)) { // Validate that the sender is the owner of the account or self. if (sender != _owners[msg.sender] && sender != msg.sender) { revert NotAuthorized(); @@ -125,7 +125,7 @@ contract SingleOwnerPlugin is BasePlugin, ISingleOwnerPlugin, IERC1271 { override returns (uint256) { - if (functionId == uint8(FunctionId.USER_OP_VALIDATION_OWNER)) { + if (functionId == uint8(FunctionId.VALIDATION_OWNER_OR_SELF)) { // Validate the user op signature against the owner. (address signer,) = (userOpHash.toEthSignedMessageHash()).tryRecover(userOp.signature); if (signer == address(0) || signer != _owners[msg.sender]) { @@ -145,85 +145,47 @@ contract SingleOwnerPlugin is BasePlugin, ISingleOwnerPlugin, IERC1271 { manifest.executionFunctions[1] = this.isValidSignature.selector; manifest.executionFunctions[2] = this.owner.selector; - ManifestFunction memory ownerUserOpValidationFunction = ManifestFunction({ + ManifestFunction memory ownerValidationFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.USER_OP_VALIDATION_OWNER), + functionId: uint8(FunctionId.VALIDATION_OWNER_OR_SELF), dependencyIndex: 0 // Unused. }); - manifest.userOpValidationFunctions = new ManifestAssociatedFunction[](7); - manifest.userOpValidationFunctions[0] = ManifestAssociatedFunction({ + manifest.validationFunctions = new ManifestAssociatedFunction[](8); + manifest.validationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.transferOwnership.selector, - associatedFunction: ownerUserOpValidationFunction + associatedFunction: ownerValidationFunction }); - manifest.userOpValidationFunctions[1] = ManifestAssociatedFunction({ + manifest.validationFunctions[1] = ManifestAssociatedFunction({ executionSelector: IStandardExecutor.execute.selector, - associatedFunction: ownerUserOpValidationFunction + associatedFunction: ownerValidationFunction }); - manifest.userOpValidationFunctions[2] = ManifestAssociatedFunction({ + manifest.validationFunctions[2] = ManifestAssociatedFunction({ executionSelector: IStandardExecutor.executeBatch.selector, - associatedFunction: ownerUserOpValidationFunction + associatedFunction: ownerValidationFunction }); - manifest.userOpValidationFunctions[3] = ManifestAssociatedFunction({ + manifest.validationFunctions[3] = ManifestAssociatedFunction({ executionSelector: IPluginManager.installPlugin.selector, - associatedFunction: ownerUserOpValidationFunction + associatedFunction: ownerValidationFunction }); - manifest.userOpValidationFunctions[4] = ManifestAssociatedFunction({ + manifest.validationFunctions[4] = ManifestAssociatedFunction({ executionSelector: IPluginManager.uninstallPlugin.selector, - associatedFunction: ownerUserOpValidationFunction + associatedFunction: ownerValidationFunction }); - manifest.userOpValidationFunctions[5] = ManifestAssociatedFunction({ + manifest.validationFunctions[5] = ManifestAssociatedFunction({ executionSelector: UUPSUpgradeable.upgradeTo.selector, - associatedFunction: ownerUserOpValidationFunction + associatedFunction: ownerValidationFunction }); - manifest.userOpValidationFunctions[6] = ManifestAssociatedFunction({ + manifest.validationFunctions[6] = ManifestAssociatedFunction({ executionSelector: UUPSUpgradeable.upgradeToAndCall.selector, - associatedFunction: ownerUserOpValidationFunction + associatedFunction: ownerValidationFunction }); - ManifestFunction memory ownerOrSelfRuntimeValidationFunction = ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.RUNTIME_VALIDATION_OWNER_OR_SELF), - dependencyIndex: 0 // Unused. - }); ManifestFunction memory alwaysAllowFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, functionId: 0, // Unused. dependencyIndex: 0 // Unused. }); - manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](9); - manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ - executionSelector: this.transferOwnership.selector, - associatedFunction: ownerOrSelfRuntimeValidationFunction - }); - manifest.runtimeValidationFunctions[1] = ManifestAssociatedFunction({ - executionSelector: this.owner.selector, - associatedFunction: alwaysAllowFunction - }); - manifest.runtimeValidationFunctions[2] = ManifestAssociatedFunction({ - executionSelector: IStandardExecutor.execute.selector, - associatedFunction: ownerOrSelfRuntimeValidationFunction - }); - manifest.runtimeValidationFunctions[3] = ManifestAssociatedFunction({ - executionSelector: IStandardExecutor.executeBatch.selector, - associatedFunction: ownerOrSelfRuntimeValidationFunction - }); - manifest.runtimeValidationFunctions[4] = ManifestAssociatedFunction({ - executionSelector: IPluginManager.installPlugin.selector, - associatedFunction: ownerOrSelfRuntimeValidationFunction - }); - manifest.runtimeValidationFunctions[5] = ManifestAssociatedFunction({ - executionSelector: IPluginManager.uninstallPlugin.selector, - associatedFunction: ownerOrSelfRuntimeValidationFunction - }); - manifest.runtimeValidationFunctions[6] = ManifestAssociatedFunction({ - executionSelector: UUPSUpgradeable.upgradeTo.selector, - associatedFunction: ownerOrSelfRuntimeValidationFunction - }); - manifest.runtimeValidationFunctions[7] = ManifestAssociatedFunction({ - executionSelector: UUPSUpgradeable.upgradeToAndCall.selector, - associatedFunction: ownerOrSelfRuntimeValidationFunction - }); - manifest.runtimeValidationFunctions[8] = ManifestAssociatedFunction({ + manifest.validationFunctions[7] = ManifestAssociatedFunction({ executionSelector: this.isValidSignature.selector, associatedFunction: alwaysAllowFunction }); diff --git a/src/samples/plugins/ModularSessionKeyPlugin.sol b/src/samples/plugins/ModularSessionKeyPlugin.sol index e4a4d02b..7af6bb1c 100644 --- a/src/samples/plugins/ModularSessionKeyPlugin.sol +++ b/src/samples/plugins/ModularSessionKeyPlugin.sol @@ -192,7 +192,7 @@ contract ModularSessionKeyPlugin is BasePlugin, IModularSessionKeyPlugin { override returns (uint256) { - if (functionId == uint8(FunctionId.USER_OP_VALIDATION_TEMPORARY_OWNER)) { + if (functionId == uint8(FunctionId.VALIDATION_TEMPORARY_OWNER)) { (address signer, ECDSA.RecoverError err) = userOpHash.toEthSignedMessageHash().tryRecover(userOp.signature); if (err != ECDSA.RecoverError.NoError) { @@ -216,7 +216,7 @@ contract ModularSessionKeyPlugin is BasePlugin, IModularSessionKeyPlugin { view override { - if (functionId == uint8(FunctionId.RUNTIME_VALIDATION_TEMPORARY_OWNER)) { + if (functionId == uint8(FunctionId.VALIDATION_TEMPORARY_OWNER)) { bytes4 selector = bytes4(data[0:4]); bytes memory key = msg.sender.allocateAssociatedStorageKey(0, 1); StoragePointer ptr = key.associatedStorageLookup(keccak256(abi.encodePacked(sender, selector))); @@ -245,65 +245,42 @@ contract ModularSessionKeyPlugin is BasePlugin, IModularSessionKeyPlugin { manifest.executionFunctions[2] = this.addSessionKeyBatch.selector; manifest.executionFunctions[3] = this.removeSessionKeyBatch.selector; - ManifestFunction memory ownerUserOpValidationFunction = ManifestFunction({ + ManifestFunction memory ownerValidationFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.DEPENDENCY, functionId: 0, // Unused. dependencyIndex: 0 // Used as first index. }); - manifest.userOpValidationFunctions = new ManifestAssociatedFunction[](4); - manifest.userOpValidationFunctions[0] = ManifestAssociatedFunction({ + manifest.validationFunctions = new ManifestAssociatedFunction[](5); + manifest.validationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.addSessionKey.selector, - associatedFunction: ownerUserOpValidationFunction + associatedFunction: ownerValidationFunction }); - manifest.userOpValidationFunctions[1] = ManifestAssociatedFunction({ + manifest.validationFunctions[1] = ManifestAssociatedFunction({ executionSelector: this.removeSessionKey.selector, - associatedFunction: ownerUserOpValidationFunction + associatedFunction: ownerValidationFunction }); - manifest.userOpValidationFunctions[2] = ManifestAssociatedFunction({ + manifest.validationFunctions[2] = ManifestAssociatedFunction({ executionSelector: this.addSessionKeyBatch.selector, - associatedFunction: ownerUserOpValidationFunction + associatedFunction: ownerValidationFunction }); - manifest.userOpValidationFunctions[3] = ManifestAssociatedFunction({ + manifest.validationFunctions[3] = ManifestAssociatedFunction({ executionSelector: this.removeSessionKeyBatch.selector, - associatedFunction: ownerUserOpValidationFunction + associatedFunction: ownerValidationFunction }); - ManifestFunction memory ownerOrSelfRuntimeValidationFunction = ManifestFunction({ - functionType: ManifestAssociatedFunctionType.DEPENDENCY, - functionId: 0, // Unused. - dependencyIndex: 1 - }); ManifestFunction memory alwaysAllowFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, functionId: 0, // Unused. dependencyIndex: 0 // Unused. }); - manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](5); - manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ - executionSelector: this.addSessionKey.selector, - associatedFunction: ownerOrSelfRuntimeValidationFunction - }); - manifest.runtimeValidationFunctions[1] = ManifestAssociatedFunction({ - executionSelector: this.removeSessionKey.selector, - associatedFunction: ownerOrSelfRuntimeValidationFunction - }); - manifest.runtimeValidationFunctions[2] = ManifestAssociatedFunction({ - executionSelector: this.addSessionKeyBatch.selector, - associatedFunction: ownerOrSelfRuntimeValidationFunction - }); - manifest.runtimeValidationFunctions[3] = ManifestAssociatedFunction({ - executionSelector: this.removeSessionKeyBatch.selector, - associatedFunction: ownerOrSelfRuntimeValidationFunction - }); - manifest.runtimeValidationFunctions[4] = ManifestAssociatedFunction({ + manifest.validationFunctions[4] = ManifestAssociatedFunction({ executionSelector: this.getSessionDuration.selector, associatedFunction: alwaysAllowFunction }); - manifest.dependencyInterfaceIds = new bytes4[](2); + manifest.dependencyInterfaceIds = new bytes4[](1); manifest.dependencyInterfaceIds[0] = type(ISingleOwnerPlugin).interfaceId; - manifest.dependencyInterfaceIds[1] = type(ISingleOwnerPlugin).interfaceId; return manifest; } diff --git a/src/samples/plugins/TokenSessionKeyPlugin.sol b/src/samples/plugins/TokenSessionKeyPlugin.sol index 9e2e8abd..ebc58eda 100644 --- a/src/samples/plugins/TokenSessionKeyPlugin.sol +++ b/src/samples/plugins/TokenSessionKeyPlugin.sol @@ -68,32 +68,20 @@ contract TokenSessionKeyPlugin is BasePlugin, ITokenSessionKeyPlugin { manifest.executionFunctions = new bytes4[](1); manifest.executionFunctions[0] = this.transferFromSessionKey.selector; - ManifestFunction memory tempOwnerUserOpValidationFunction = ManifestFunction({ + ManifestFunction memory tempOwnerValidationFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.DEPENDENCY, functionId: 0, // Unused dependencyIndex: 0 // Used as first index }); - ManifestFunction memory tempOwnerRuntimeValidationFunction = ManifestFunction({ - functionType: ManifestAssociatedFunctionType.DEPENDENCY, - functionId: 0, // Unused - dependencyIndex: 1 // Used as second index - }); - - manifest.userOpValidationFunctions = new ManifestAssociatedFunction[](1); - manifest.userOpValidationFunctions[0] = ManifestAssociatedFunction({ - executionSelector: this.transferFromSessionKey.selector, - associatedFunction: tempOwnerUserOpValidationFunction - }); - manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](1); - manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ + manifest.validationFunctions = new ManifestAssociatedFunction[](1); + manifest.validationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.transferFromSessionKey.selector, - associatedFunction: tempOwnerRuntimeValidationFunction + associatedFunction: tempOwnerValidationFunction }); - manifest.dependencyInterfaceIds = new bytes4[](2); + manifest.dependencyInterfaceIds = new bytes4[](1); manifest.dependencyInterfaceIds[0] = type(IModularSessionKeyPlugin).interfaceId; - manifest.dependencyInterfaceIds[1] = type(IModularSessionKeyPlugin).interfaceId; bytes4[] memory permittedExternalSelectors = new bytes4[](1); permittedExternalSelectors[0] = TRANSFERFROM_SELECTOR; diff --git a/src/samples/plugins/interfaces/ISessionKeyPlugin.sol b/src/samples/plugins/interfaces/ISessionKeyPlugin.sol index 88d31f1a..18a2204c 100644 --- a/src/samples/plugins/interfaces/ISessionKeyPlugin.sol +++ b/src/samples/plugins/interfaces/ISessionKeyPlugin.sol @@ -5,8 +5,7 @@ import {UserOperation} from "@eth-infinitism/account-abstraction/interfaces/User interface IModularSessionKeyPlugin { enum FunctionId { - RUNTIME_VALIDATION_TEMPORARY_OWNER, - USER_OP_VALIDATION_TEMPORARY_OWNER + VALIDATION_TEMPORARY_OWNER } /// @notice This event is emitted when a session key is added to the account. diff --git a/test/account/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol index a9b23875..bf432e03 100644 --- a/test/account/AccountExecHooks.t.sol +++ b/test/account/AccountExecHooks.t.sol @@ -60,7 +60,7 @@ contract AccountExecHooksTest is OptimizedTest { m1.executionFunctions.push(_EXEC_SELECTOR); - m1.runtimeValidationFunctions.push( + m1.validationFunctions.push( ManifestAssociatedFunction({ executionSelector: _EXEC_SELECTOR, associatedFunction: ManifestFunction({ diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol index 17f66788..35c7b6e2 100644 --- a/test/account/AccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -31,8 +31,7 @@ contract AccountLoupeTest is OptimizedTest { UpgradeableModularAccount public account1; - FunctionReference public ownerUserOpValidation; - FunctionReference public ownerRuntimeValidation; + FunctionReference public ownerValidation; event ReceivedCall(bytes msgData, uint256 msgValue); @@ -48,11 +47,8 @@ contract AccountLoupeTest is OptimizedTest { bytes32 manifestHash = keccak256(abi.encode(comprehensivePlugin.pluginManifest())); account1.installPlugin(address(comprehensivePlugin), manifestHash, "", new FunctionReference[](0)); - ownerUserOpValidation = FunctionReferenceLib.pack( - address(singleOwnerPlugin), uint8(ISingleOwnerPlugin.FunctionId.USER_OP_VALIDATION_OWNER) - ); - ownerRuntimeValidation = FunctionReferenceLib.pack( - address(singleOwnerPlugin), uint8(ISingleOwnerPlugin.FunctionId.RUNTIME_VALIDATION_OWNER_OR_SELF) + ownerValidation = FunctionReferenceLib.pack( + address(singleOwnerPlugin), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER_OR_SELF) ); } @@ -67,28 +63,22 @@ contract AccountLoupeTest is OptimizedTest { function test_pluginLoupe_getExecutionFunctionConfig_native() public { bytes4[] memory selectorsToCheck = new bytes4[](5); - FunctionReference[] memory expectedUserOpValidations = new FunctionReference[](5); - FunctionReference[] memory expectedRuntimeValidations = new FunctionReference[](5); + FunctionReference[] memory expectedValidations = new FunctionReference[](5); selectorsToCheck[0] = IStandardExecutor.execute.selector; - expectedUserOpValidations[0] = ownerUserOpValidation; - expectedRuntimeValidations[0] = ownerRuntimeValidation; + expectedValidations[0] = ownerValidation; selectorsToCheck[1] = IStandardExecutor.executeBatch.selector; - expectedUserOpValidations[1] = ownerUserOpValidation; - expectedRuntimeValidations[1] = ownerRuntimeValidation; + expectedValidations[1] = ownerValidation; selectorsToCheck[2] = UUPSUpgradeable.upgradeToAndCall.selector; - expectedUserOpValidations[2] = ownerUserOpValidation; - expectedRuntimeValidations[2] = ownerRuntimeValidation; + expectedValidations[2] = ownerValidation; selectorsToCheck[3] = IPluginManager.installPlugin.selector; - expectedUserOpValidations[3] = ownerUserOpValidation; - expectedRuntimeValidations[3] = ownerRuntimeValidation; + expectedValidations[3] = ownerValidation; selectorsToCheck[4] = IPluginManager.uninstallPlugin.selector; - expectedUserOpValidations[4] = ownerUserOpValidation; - expectedRuntimeValidations[4] = ownerRuntimeValidation; + expectedValidations[4] = ownerValidation; for (uint256 i = 0; i < selectorsToCheck.length; i++) { IAccountLoupe.ExecutionFunctionConfig memory config = @@ -96,12 +86,8 @@ contract AccountLoupeTest is OptimizedTest { assertEq(config.plugin, address(account1)); assertEq( - FunctionReference.unwrap(config.userOpValidationFunction), - FunctionReference.unwrap(expectedUserOpValidations[i]) - ); - assertEq( - FunctionReference.unwrap(config.runtimeValidationFunction), - FunctionReference.unwrap(expectedRuntimeValidations[i]) + FunctionReference.unwrap(config.validationFunction), + FunctionReference.unwrap(expectedValidations[i]) ); } } @@ -109,22 +95,17 @@ contract AccountLoupeTest is OptimizedTest { function test_pluginLoupe_getExecutionFunctionConfig_plugin() public { bytes4[] memory selectorsToCheck = new bytes4[](2); address[] memory expectedPluginAddress = new address[](2); - FunctionReference[] memory expectedUserOpValidations = new FunctionReference[](2); - FunctionReference[] memory expectedRuntimeValidations = new FunctionReference[](2); + FunctionReference[] memory expectedValidations = new FunctionReference[](2); selectorsToCheck[0] = comprehensivePlugin.foo.selector; expectedPluginAddress[0] = address(comprehensivePlugin); - expectedUserOpValidations[0] = FunctionReferenceLib.pack( - address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.USER_OP_VALIDATION) - ); - expectedRuntimeValidations[0] = FunctionReferenceLib.pack( - address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.RUNTIME_VALIDATION) + expectedValidations[0] = FunctionReferenceLib.pack( + address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.VALIDATION) ); selectorsToCheck[1] = singleOwnerPlugin.transferOwnership.selector; expectedPluginAddress[1] = address(singleOwnerPlugin); - expectedUserOpValidations[1] = ownerUserOpValidation; - expectedRuntimeValidations[1] = ownerRuntimeValidation; + expectedValidations[1] = ownerValidation; for (uint256 i = 0; i < selectorsToCheck.length; i++) { IAccountLoupe.ExecutionFunctionConfig memory config = @@ -132,12 +113,8 @@ contract AccountLoupeTest is OptimizedTest { assertEq(config.plugin, expectedPluginAddress[i]); assertEq( - FunctionReference.unwrap(config.userOpValidationFunction), - FunctionReference.unwrap(expectedUserOpValidations[i]) - ); - assertEq( - FunctionReference.unwrap(config.runtimeValidationFunction), - FunctionReference.unwrap(expectedRuntimeValidations[i]) + FunctionReference.unwrap(config.validationFunction), + FunctionReference.unwrap(expectedValidations[i]) ); } } diff --git a/test/account/ManifestValidity.t.sol b/test/account/ManifestValidity.t.sol index 55b2c9ba..169d6c81 100644 --- a/test/account/ManifestValidity.t.sol +++ b/test/account/ManifestValidity.t.sol @@ -11,7 +11,6 @@ import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; import { - BadValidationMagicValue_UserOp_Plugin, BadValidationMagicValue_PreRuntimeValidationHook_Plugin, BadValidationMagicValue_PreUserOpValidationHook_Plugin, BadValidationMagicValue_PreExecHook_Plugin, @@ -39,22 +38,6 @@ contract ManifestValidityTest is OptimizedTest { account = factory.createAccount(address(this), 0); } - // Tests that the plugin manager rejects a plugin with a user op validationFunction set to "validation always - // allow" - function test_ManifestValidity_invalid_ValidationAlwaysAllow_UserOpValidationFunction() public { - BadValidationMagicValue_UserOp_Plugin plugin = new BadValidationMagicValue_UserOp_Plugin(); - - bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - - vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); - account.installPlugin({ - plugin: address(plugin), - manifestHash: manifestHash, - pluginInstallData: "", - dependencies: new FunctionReference[](0) - }); - } - // Tests that the plugin manager rejects a plugin with a pre-runtime validation hook set to "validation always // allow" function test_ManifestValidity_invalid_ValidationAlwaysAllow_PreRuntimeValidationHook() public { diff --git a/test/mocks/plugins/BadTransferOwnershipPlugin.sol b/test/mocks/plugins/BadTransferOwnershipPlugin.sol index 0b291c03..03da8019 100644 --- a/test/mocks/plugins/BadTransferOwnershipPlugin.sol +++ b/test/mocks/plugins/BadTransferOwnershipPlugin.sol @@ -47,8 +47,8 @@ contract BadTransferOwnershipPlugin is BasePlugin { manifest.permittedExecutionSelectors = new bytes4[](1); manifest.permittedExecutionSelectors[0] = ISingleOwnerPlugin.transferOwnership.selector; - manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](1); - manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ + manifest.validationFunctions = new ManifestAssociatedFunction[](1); + manifest.validationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.evilTransferOwnership.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index 30b656cd..442b39a6 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -18,10 +18,9 @@ contract ComprehensivePlugin is BasePlugin { enum FunctionId { PRE_USER_OP_VALIDATION_HOOK_1, PRE_USER_OP_VALIDATION_HOOK_2, - USER_OP_VALIDATION, PRE_RUNTIME_VALIDATION_HOOK_1, PRE_RUNTIME_VALIDATION_HOOK_2, - RUNTIME_VALIDATION, + VALIDATION, PRE_EXECUTION_HOOK, PRE_PERMITTED_CALL_EXECUTION_HOOK, POST_EXECUTION_HOOK, @@ -66,7 +65,7 @@ contract ComprehensivePlugin is BasePlugin { override returns (uint256) { - if (functionId == uint8(FunctionId.USER_OP_VALIDATION)) { + if (functionId == uint8(FunctionId.VALIDATION)) { return 0; } revert NotImplemented(); @@ -86,7 +85,7 @@ contract ComprehensivePlugin is BasePlugin { pure override { - if (functionId == uint8(FunctionId.RUNTIME_VALIDATION)) { + if (functionId == uint8(FunctionId.VALIDATION)) { return; } revert NotImplemented(); @@ -121,26 +120,15 @@ contract ComprehensivePlugin is BasePlugin { manifest.executionFunctions = new bytes4[](1); manifest.executionFunctions[0] = this.foo.selector; - ManifestFunction memory fooUserOpValidationFunction = ManifestFunction({ + ManifestFunction memory fooValidationFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.USER_OP_VALIDATION), + functionId: uint8(FunctionId.VALIDATION), dependencyIndex: 0 // Unused. }); - manifest.userOpValidationFunctions = new ManifestAssociatedFunction[](1); - manifest.userOpValidationFunctions[0] = ManifestAssociatedFunction({ + manifest.validationFunctions = new ManifestAssociatedFunction[](1); + manifest.validationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.foo.selector, - associatedFunction: fooUserOpValidationFunction - }); - - ManifestFunction memory fooRuntimeValidationFunction = ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.RUNTIME_VALIDATION), - dependencyIndex: 0 // Unused. - }); - manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](1); - manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ - executionSelector: this.foo.selector, - associatedFunction: fooRuntimeValidationFunction + associatedFunction: fooValidationFunction }); manifest.preUserOpValidationHooks = new ManifestAssociatedFunction[](4); diff --git a/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol b/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol index e858d8ee..3eea3ac0 100644 --- a/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol +++ b/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol @@ -45,7 +45,7 @@ contract EFPCallerPlugin is BaseTestPlugin { manifest.executionFunctions[9] = this.getNumberCounter3.selector; manifest.executionFunctions[10] = this.incrementCounter3.selector; - manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](11); + manifest.validationFunctions = new ManifestAssociatedFunction[](11); ManifestFunction memory alwaysAllowValidationFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, @@ -54,7 +54,7 @@ contract EFPCallerPlugin is BaseTestPlugin { }); for (uint256 i = 0; i < manifest.executionFunctions.length; i++) { - manifest.runtimeValidationFunctions[i] = ManifestAssociatedFunction({ + manifest.validationFunctions[i] = ManifestAssociatedFunction({ executionSelector: manifest.executionFunctions[i], associatedFunction: alwaysAllowValidationFunction }); @@ -181,8 +181,8 @@ contract EFPCallerPluginAnyExternal is BaseTestPlugin { manifest.executionFunctions = new bytes4[](1); manifest.executionFunctions[0] = this.passthroughExecute.selector; - manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](1); - manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ + manifest.validationFunctions = new ManifestAssociatedFunction[](1); + manifest.validationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.passthroughExecute.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, diff --git a/test/mocks/plugins/ManifestValidityMocks.sol b/test/mocks/plugins/ManifestValidityMocks.sol index 27243f79..f19c67ca 100644 --- a/test/mocks/plugins/ManifestValidityMocks.sol +++ b/test/mocks/plugins/ManifestValidityMocks.sol @@ -16,36 +16,6 @@ import {IPlugin} from "../../../src/interfaces/IPlugin.sol"; import {BaseTestPlugin} from "./BaseTestPlugin.sol"; -contract BadValidationMagicValue_UserOp_Plugin is BaseTestPlugin { - function onInstall(bytes calldata) external override {} - - function onUninstall(bytes calldata) external override {} - - function foo() external pure returns (bytes32) { - return keccak256("bar"); - } - - function pluginManifest() external pure override returns (PluginManifest memory) { - PluginManifest memory manifest; - - manifest.executionFunctions = new bytes4[](1); - manifest.executionFunctions[0] = this.foo.selector; - - manifest.userOpValidationFunctions = new ManifestAssociatedFunction[](1); - manifest.userOpValidationFunctions[0] = ManifestAssociatedFunction({ - executionSelector: this.foo.selector, - associatedFunction: ManifestFunction({ - // Illegal assignment: validation always allow only usable on runtime validation functions - functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - functionId: 0, - dependencyIndex: 0 - }) - }); - - return manifest; - } -} - contract BadValidationMagicValue_PreRuntimeValidationHook_Plugin is BaseTestPlugin { function onInstall(bytes calldata) external override {} @@ -61,8 +31,8 @@ contract BadValidationMagicValue_PreRuntimeValidationHook_Plugin is BaseTestPlug manifest.executionFunctions = new bytes4[](1); manifest.executionFunctions[0] = this.foo.selector; - manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](1); - manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ + manifest.validationFunctions = new ManifestAssociatedFunction[](1); + manifest.validationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.foo.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, @@ -101,8 +71,8 @@ contract BadValidationMagicValue_PreUserOpValidationHook_Plugin is BaseTestPlugi manifest.executionFunctions = new bytes4[](1); manifest.executionFunctions[0] = this.foo.selector; - manifest.userOpValidationFunctions = new ManifestAssociatedFunction[](1); - manifest.userOpValidationFunctions[0] = ManifestAssociatedFunction({ + manifest.validationFunctions = new ManifestAssociatedFunction[](1); + manifest.validationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.foo.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, @@ -212,8 +182,8 @@ contract BadHookMagicValue_UserOpValidationFunction_Plugin is BaseTestPlugin { manifest.executionFunctions = new bytes4[](1); manifest.executionFunctions[0] = this.foo.selector; - manifest.userOpValidationFunctions = new ManifestAssociatedFunction[](1); - manifest.userOpValidationFunctions[0] = ManifestAssociatedFunction({ + manifest.validationFunctions = new ManifestAssociatedFunction[](1); + manifest.validationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.foo.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, @@ -241,8 +211,8 @@ contract BadHookMagicValue_RuntimeValidationFunction_Plugin is BaseTestPlugin { manifest.executionFunctions = new bytes4[](1); manifest.executionFunctions[0] = this.foo.selector; - manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](1); - manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ + manifest.validationFunctions = new ManifestAssociatedFunction[](1); + manifest.validationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.foo.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, diff --git a/test/mocks/plugins/ReturnDataPluginMocks.sol b/test/mocks/plugins/ReturnDataPluginMocks.sol index 34ce8e28..85925c0e 100644 --- a/test/mocks/plugins/ReturnDataPluginMocks.sol +++ b/test/mocks/plugins/ReturnDataPluginMocks.sol @@ -45,8 +45,8 @@ contract ResultCreatorPlugin is BaseTestPlugin { manifest.executionFunctions[0] = this.foo.selector; manifest.executionFunctions[1] = this.bar.selector; - manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](1); - manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ + manifest.validationFunctions = new ManifestAssociatedFunction[](1); + manifest.validationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.foo.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, @@ -121,8 +121,8 @@ contract ResultConsumerPlugin is BaseTestPlugin { manifest.executionFunctions[0] = this.checkResultEFPFallback.selector; manifest.executionFunctions[1] = this.checkResultEFPExternal.selector; - manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](2); - manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ + manifest.validationFunctions = new ManifestAssociatedFunction[](2); + manifest.validationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.checkResultEFPFallback.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, @@ -130,7 +130,7 @@ contract ResultConsumerPlugin is BaseTestPlugin { dependencyIndex: 0 }) }); - manifest.runtimeValidationFunctions[1] = ManifestAssociatedFunction({ + manifest.validationFunctions[1] = ManifestAssociatedFunction({ executionSelector: this.checkResultEFPExternal.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, diff --git a/test/mocks/plugins/ValidationPluginMocks.sol b/test/mocks/plugins/ValidationPluginMocks.sol index 1e6cc941..4859a7e9 100644 --- a/test/mocks/plugins/ValidationPluginMocks.sol +++ b/test/mocks/plugins/ValidationPluginMocks.sol @@ -78,8 +78,8 @@ contract MockUserOpValidationPlugin is MockBaseUserOpValidationPlugin { manifest.executionFunctions = new bytes4[](1); manifest.executionFunctions[0] = this.foo.selector; - manifest.userOpValidationFunctions = new ManifestAssociatedFunction[](1); - manifest.userOpValidationFunctions[0] = ManifestAssociatedFunction({ + manifest.validationFunctions = new ManifestAssociatedFunction[](1); + manifest.validationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.foo.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, @@ -121,8 +121,8 @@ contract MockUserOpValidation1HookPlugin is MockBaseUserOpValidationPlugin { functionId: uint8(FunctionId.USER_OP_VALIDATION), dependencyIndex: 0 // Unused. }); - manifest.userOpValidationFunctions = new ManifestAssociatedFunction[](1); - manifest.userOpValidationFunctions[0] = ManifestAssociatedFunction({ + manifest.validationFunctions = new ManifestAssociatedFunction[](1); + manifest.validationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.bar.selector, associatedFunction: userOpValidationFunctionRef }); @@ -173,8 +173,8 @@ contract MockUserOpValidation2HookPlugin is MockBaseUserOpValidationPlugin { functionId: uint8(FunctionId.USER_OP_VALIDATION), dependencyIndex: 0 // Unused. }); - manifest.userOpValidationFunctions = new ManifestAssociatedFunction[](1); - manifest.userOpValidationFunctions[0] = ManifestAssociatedFunction({ + manifest.validationFunctions = new ManifestAssociatedFunction[](1); + manifest.validationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.baz.selector, associatedFunction: userOpValidationFunctionRef }); diff --git a/test/plugin/SingleOwnerPlugin.t.sol b/test/plugin/SingleOwnerPlugin.t.sol index 090e5be1..5784059e 100644 --- a/test/plugin/SingleOwnerPlugin.t.sol +++ b/test/plugin/SingleOwnerPlugin.t.sol @@ -113,13 +113,13 @@ contract SingleOwnerPluginTest is OptimizedTest { plugin.transferOwnership(owner1); assertEq(owner1, plugin.owner()); plugin.runtimeValidationFunction( - uint8(ISingleOwnerPlugin.FunctionId.RUNTIME_VALIDATION_OWNER_OR_SELF), owner1, 0, "" + uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER_OR_SELF), owner1, 0, "" ); vm.startPrank(b); vm.expectRevert(ISingleOwnerPlugin.NotAuthorized.selector); plugin.runtimeValidationFunction( - uint8(ISingleOwnerPlugin.FunctionId.RUNTIME_VALIDATION_OWNER_OR_SELF), owner1, 0, "" + uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER_OR_SELF), owner1, 0, "" ); } @@ -136,7 +136,7 @@ contract SingleOwnerPluginTest is OptimizedTest { // sig check should fail uint256 success = plugin.userOpValidationFunction( - uint8(ISingleOwnerPlugin.FunctionId.USER_OP_VALIDATION_OWNER), userOp, userOpHash + uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER_OR_SELF), userOp, userOpHash ); assertEq(success, 1); @@ -146,7 +146,7 @@ contract SingleOwnerPluginTest is OptimizedTest { // sig check should pass success = plugin.userOpValidationFunction( - uint8(ISingleOwnerPlugin.FunctionId.USER_OP_VALIDATION_OWNER), userOp, userOpHash + uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER_OR_SELF), userOp, userOpHash ); assertEq(success, 0); } diff --git a/test/samples/plugins/ModularSessionKeyPlugin.t.sol b/test/samples/plugins/ModularSessionKeyPlugin.t.sol index bdbcbfff..0af2c0b8 100644 --- a/test/samples/plugins/ModularSessionKeyPlugin.t.sol +++ b/test/samples/plugins/ModularSessionKeyPlugin.t.sol @@ -99,12 +99,9 @@ contract ModularSessionKeyPluginTest is Test { vm.deal(address(account), 1 ether); vm.startPrank(owner); - FunctionReference[] memory modularSessionDependency = new FunctionReference[](2); + FunctionReference[] memory modularSessionDependency = new FunctionReference[](1); modularSessionDependency[0] = FunctionReferenceLib.pack( - address(ownerPlugin), uint8(ISingleOwnerPlugin.FunctionId.USER_OP_VALIDATION_OWNER) - ); - modularSessionDependency[1] = FunctionReferenceLib.pack( - address(ownerPlugin), uint8(ISingleOwnerPlugin.FunctionId.RUNTIME_VALIDATION_OWNER_OR_SELF) + address(ownerPlugin), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER_OR_SELF) ); bytes32 modularSessionKeyManifestHash = keccak256(abi.encode(modularSessionKeyPlugin.pluginManifest())); @@ -130,14 +127,10 @@ contract ModularSessionKeyPluginTest is Test { dependencies: modularSessionDependency }); - FunctionReference[] memory tokenSessionDependency = new FunctionReference[](2); + FunctionReference[] memory tokenSessionDependency = new FunctionReference[](1); tokenSessionDependency[0] = FunctionReferenceLib.pack( address(modularSessionKeyPlugin), - uint8(IModularSessionKeyPlugin.FunctionId.USER_OP_VALIDATION_TEMPORARY_OWNER) - ); - tokenSessionDependency[1] = FunctionReferenceLib.pack( - address(modularSessionKeyPlugin), - uint8(IModularSessionKeyPlugin.FunctionId.RUNTIME_VALIDATION_TEMPORARY_OWNER) + uint8(IModularSessionKeyPlugin.FunctionId.VALIDATION_TEMPORARY_OWNER) ); bytes32 tokenSessionKeyManifestHash = keccak256(abi.encode(tokenSessionKeyPlugin.pluginManifest())); @@ -245,7 +238,7 @@ contract ModularSessionKeyPluginTest is Test { abi.encodeWithSelector( UpgradeableModularAccount.RuntimeValidationFunctionReverted.selector, address(modularSessionKeyPlugin), - IModularSessionKeyPlugin.FunctionId.RUNTIME_VALIDATION_TEMPORARY_OWNER, + IModularSessionKeyPlugin.FunctionId.VALIDATION_TEMPORARY_OWNER, revertReason ) ); @@ -286,7 +279,7 @@ contract ModularSessionKeyPluginTest is Test { abi.encodeWithSelector( UpgradeableModularAccount.RuntimeValidationFunctionReverted.selector, address(modularSessionKeyPlugin), - IModularSessionKeyPlugin.FunctionId.RUNTIME_VALIDATION_TEMPORARY_OWNER, + IModularSessionKeyPlugin.FunctionId.VALIDATION_TEMPORARY_OWNER, revertReason ) ); @@ -308,7 +301,7 @@ contract ModularSessionKeyPluginTest is Test { abi.encodeWithSelector( UpgradeableModularAccount.RuntimeValidationFunctionReverted.selector, address(modularSessionKeyPlugin), - IModularSessionKeyPlugin.FunctionId.RUNTIME_VALIDATION_TEMPORARY_OWNER, + IModularSessionKeyPlugin.FunctionId.VALIDATION_TEMPORARY_OWNER, revertReason ) ); From 01d544915a71bff123bc358b8a6e993c026a3028 Mon Sep 17 00:00:00 2001 From: adam Date: Tue, 5 Mar 2024 13:10:00 -0500 Subject: [PATCH 2/8] Update ERC-6900.md with validation merge --- standard/ERCs/erc-6900.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/standard/ERCs/erc-6900.md b/standard/ERCs/erc-6900.md index 9f9de2ce..ce775c15 100644 --- a/standard/ERCs/erc-6900.md +++ b/standard/ERCs/erc-6900.md @@ -215,8 +215,7 @@ interface IAccountLoupe { /// @notice Config for an execution function, given a selector. struct ExecutionFunctionConfig { address plugin; - FunctionReference userOpValidationFunction; - FunctionReference runtimeValidationFunction; + FunctionReference validationFunction; } /// @notice Pre and post hooks for a given selector. @@ -423,8 +422,7 @@ struct PluginManifest { // plugin MUST still be able to spend up to the balance that it sends to the account in the same call. bool canSpendNativeToken; ManifestExternalCallPermission[] permittedExternalCalls; - ManifestAssociatedFunction[] userOpValidationFunctions; - ManifestAssociatedFunction[] runtimeValidationFunctions; + ManifestAssociatedFunction[] validationFunctions; ManifestAssociatedFunction[] preUserOpValidationHooks; ManifestAssociatedFunction[] preRuntimeValidationHooks; ManifestExecutionHook[] executionHooks; From 81f3c51a986f2ff6f80759e87f58c847bcd1c085 Mon Sep 17 00:00:00 2001 From: adam Date: Tue, 5 Mar 2024 13:12:39 -0500 Subject: [PATCH 3/8] Confirm revert message for always-allow runtime validation --- src/account/UpgradeableModularAccount.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 022503c1..24a87ea1 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -397,7 +397,6 @@ contract UpgradeableModularAccount is } else { // _PRE_HOOK_ALWAYS_DENY is not permitted here. // If this is _RUNTIME_VALIDATION_ALWAYS_ALLOW, the call should revert. - // Todo: this is the wrong error if it is set to _RUNTIME_VALIDATION_ALWAYS_ALLOW. revert InvalidConfiguration(); } } From 0624f352e1463a7d0b36eb7435657fc54b3119f9 Mon Sep 17 00:00:00 2001 From: adam Date: Tue, 5 Mar 2024 13:35:00 -0500 Subject: [PATCH 4/8] forge fmt --- src/account/PluginManagerInternals.sol | 5 ++++- test/samples/plugins/ModularSessionKeyPlugin.t.sol | 3 +-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 7ed6d768..dc5fbb8a 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -302,7 +302,10 @@ abstract contract PluginManagerInternals is IPluginManager { _addValidationFunction( mv.executionSelector, _resolveManifestFunction( - mv.associatedFunction, plugin, dependencies, ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW + mv.associatedFunction, + plugin, + dependencies, + ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW ) ); diff --git a/test/samples/plugins/ModularSessionKeyPlugin.t.sol b/test/samples/plugins/ModularSessionKeyPlugin.t.sol index 0af2c0b8..e20cbfca 100644 --- a/test/samples/plugins/ModularSessionKeyPlugin.t.sol +++ b/test/samples/plugins/ModularSessionKeyPlugin.t.sol @@ -129,8 +129,7 @@ contract ModularSessionKeyPluginTest is Test { FunctionReference[] memory tokenSessionDependency = new FunctionReference[](1); tokenSessionDependency[0] = FunctionReferenceLib.pack( - address(modularSessionKeyPlugin), - uint8(IModularSessionKeyPlugin.FunctionId.VALIDATION_TEMPORARY_OWNER) + address(modularSessionKeyPlugin), uint8(IModularSessionKeyPlugin.FunctionId.VALIDATION_TEMPORARY_OWNER) ); bytes32 tokenSessionKeyManifestHash = keccak256(abi.encode(tokenSessionKeyPlugin.pluginManifest())); From a8442bec64df954dd0c0ad4ad9c7e7768abe88da Mon Sep 17 00:00:00 2001 From: adam Date: Fri, 8 Mar 2024 16:15:58 -0500 Subject: [PATCH 5/8] FunctionId removal MVP --- src/account/AccountLoupe.sol | 29 +- src/account/AccountStorage.sol | 23 +- src/account/PluginManagerInternals.sol | 125 ++-- src/account/UpgradeableModularAccount.sol | 94 ++- src/interfaces/IAccountLoupe.sol | 10 +- src/interfaces/IPlugin.sol | 37 +- src/interfaces/IPluginManager.sol | 4 +- src/plugins/BasePlugin.sol | 88 +-- src/plugins/TokenReceiverPlugin.sol | 1 - src/plugins/owner/ISingleOwnerPlugin.sol | 3 - src/plugins/owner/SingleOwnerPlugin.sol | 30 +- .../plugins/ModularSessionKeyPlugin.sol | 60 +- src/samples/plugins/TokenSessionKeyPlugin.sol | 1 - .../plugins/interfaces/ISessionKeyPlugin.sol | 3 - test/account/AccountExecHooks.t.sol | 47 +- test/account/AccountLoupe.t.sol | 123 ++-- test/account/AccountReturnData.t.sol | 4 +- .../ExecuteFromPluginPermissions.t.sol | 6 +- test/account/ManifestValidity.t.sol | 14 +- test/account/UpgradeableModularAccount.t.sol | 24 +- test/account/ValidationIntersection.t.sol | 555 +++++++++--------- .../plugins/BadTransferOwnershipPlugin.sol | 1 - test/mocks/plugins/ComprehensivePlugin.sol | 149 ++--- .../ExecFromPluginPermissionsMocks.sol | 2 - test/mocks/plugins/ManifestValidityMocks.sol | 12 - test/mocks/plugins/ReturnDataPluginMocks.sol | 3 - test/mocks/plugins/ValidationPluginMocks.sol | 356 +++++------ test/plugin/SingleOwnerPlugin.t.sol | 8 +- test/plugin/TokenReceiverPlugin.t.sol | 2 +- .../plugins/ModularSessionKeyPlugin.t.sol | 15 +- 30 files changed, 818 insertions(+), 1011 deletions(-) diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol index 8b6f42a2..c6cf1a91 100644 --- a/src/account/AccountLoupe.sol +++ b/src/account/AccountLoupe.sol @@ -6,9 +6,10 @@ import {EnumerableMap} from "@openzeppelin/contracts/utils/structs/EnumerableMap import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import {IAccountLoupe} from "../interfaces/IAccountLoupe.sol"; -import {FunctionReference, IPluginManager} from "../interfaces/IPluginManager.sol"; +import {IPlugin} from "../interfaces/IPlugin.sol"; +import {IPluginManager} from "../interfaces/IPluginManager.sol"; import {IStandardExecutor} from "../interfaces/IStandardExecutor.sol"; -import {AccountStorage, getAccountStorage, SelectorData, toFunctionReferenceArray} from "./AccountStorage.sol"; +import {AccountStorage, getAccountStorage, toAddressArray, toPlugin, SelectorData} from "./AccountStorage.sol"; abstract contract AccountLoupe is IAccountLoupe { using EnumerableMap for EnumerableMap.Bytes32ToUintMap; @@ -36,7 +37,7 @@ abstract contract AccountLoupe is IAccountLoupe { config.plugin = _storage.selectorData[selector].plugin; } - config.validationFunction = _storage.selectorData[selector].validation; + config.validationPlugin = address(_storage.selectorData[selector].validation); } /// @inheritdoc IAccountLoupe @@ -61,14 +62,14 @@ abstract contract AccountLoupe is IAccountLoupe { for (uint256 i = 0; i < preExecHooksLength;) { (bytes32 key,) = selectorData.preHooks.at(i); - FunctionReference preExecHook = FunctionReference.wrap(bytes21(key)); + IPlugin preExecHookPlugin = toPlugin(key); - uint256 associatedPostExecHooksLength = selectorData.associatedPostHooks[preExecHook].length(); + uint256 associatedPostExecHooksLength = selectorData.associatedPostHooks[preExecHookPlugin].length(); if (associatedPostExecHooksLength > 0) { for (uint256 j = 0; j < associatedPostExecHooksLength;) { - execHooks[actualExecHooksLength].preExecHook = preExecHook; - (key,) = selectorData.associatedPostHooks[preExecHook].at(j); - execHooks[actualExecHooksLength].postExecHook = FunctionReference.wrap(bytes21(key)); + execHooks[actualExecHooksLength].preExecHookPlugin = address(preExecHookPlugin); + (key,) = selectorData.associatedPostHooks[preExecHookPlugin].at(j); + execHooks[actualExecHooksLength].postExecHookPlugin = address(toPlugin(key)); unchecked { ++actualExecHooksLength; @@ -76,7 +77,7 @@ abstract contract AccountLoupe is IAccountLoupe { } } } else { - execHooks[actualExecHooksLength].preExecHook = preExecHook; + execHooks[actualExecHooksLength].preExecHookPlugin = address(preExecHookPlugin); unchecked { ++actualExecHooksLength; @@ -90,7 +91,7 @@ abstract contract AccountLoupe is IAccountLoupe { for (uint256 i = 0; i < postOnlyExecHooksLength;) { (bytes32 key,) = selectorData.postOnlyHooks.at(i); - execHooks[actualExecHooksLength].postExecHook = FunctionReference.wrap(bytes21(key)); + execHooks[actualExecHooksLength].postExecHookPlugin = address(toPlugin(key)); unchecked { ++actualExecHooksLength; @@ -109,14 +110,14 @@ abstract contract AccountLoupe is IAccountLoupe { external view returns ( - FunctionReference[] memory preUserOpValidationHooks, - FunctionReference[] memory preRuntimeValidationHooks + address[] memory preUserOpValidationHooks, + address[] memory preRuntimeValidationHooks ) { preUserOpValidationHooks = - toFunctionReferenceArray(getAccountStorage().selectorData[selector].preUserOpValidationHooks); + toAddressArray(getAccountStorage().selectorData[selector].preUserOpValidationHooks); preRuntimeValidationHooks = - toFunctionReferenceArray(getAccountStorage().selectorData[selector].preRuntimeValidationHooks); + toAddressArray(getAccountStorage().selectorData[selector].preRuntimeValidationHooks); } /// @inheritdoc IAccountLoupe diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index deb31228..0bd16f69 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -15,7 +15,7 @@ struct PluginData { // boolean to indicate if the plugin can spend native tokens from the account. bool canSpendNativeToken; bytes32 manifestHash; - FunctionReference[] dependencies; + address[] dependencies; // Tracks the number of times this plugin has been used as a dependency function uint256 dependentCount; } @@ -36,14 +36,14 @@ struct SelectorData { // If this is a native function, the address must remain address(0). address plugin; // User operation validation and runtime validation share a function reference. - FunctionReference validation; + IPlugin validation; // The pre validation hooks for this function selector. EnumerableMap.Bytes32ToUintMap preUserOpValidationHooks; EnumerableMap.Bytes32ToUintMap preRuntimeValidationHooks; // The execution hooks for this function selector. EnumerableMap.Bytes32ToUintMap preHooks; - // bytes21 key = pre hook function reference - mapping(FunctionReference => EnumerableMap.Bytes32ToUintMap) associatedPostHooks; + // bytes20 key = pre hook plugin address + mapping(IPlugin => EnumerableMap.Bytes32ToUintMap) associatedPostHooks; EnumerableMap.Bytes32ToUintMap postOnlyHooks; } @@ -53,7 +53,7 @@ struct AccountStorage { bool initializing; // Plugin metadata storage EnumerableSet.AddressSet plugins; - mapping(address => PluginData) pluginData; + mapping(IPlugin => PluginData) pluginData; // Execution functions and their associated functions mapping(bytes4 => SelectorData) selectorData; // bytes24 key = address(calling plugin) || bytes4(selector of execution function) @@ -74,18 +74,23 @@ function getPermittedCallKey(address addr, bytes4 selector) pure returns (bytes2 return bytes24(bytes20(addr)) | (bytes24(selector) >> 160); } + +function toPlugin(bytes32 key) pure returns (IPlugin) { + return IPlugin(address(bytes20(key))); +} + // Helper function to get all elements of a set into memory. using EnumerableMap for EnumerableMap.Bytes32ToUintMap; -function toFunctionReferenceArray(EnumerableMap.Bytes32ToUintMap storage map) +function toAddressArray(EnumerableMap.Bytes32ToUintMap storage map) view - returns (FunctionReference[] memory) + returns (address[] memory) { uint256 length = map.length(); - FunctionReference[] memory result = new FunctionReference[](length); + address[] memory result = new address[](length); for (uint256 i = 0; i < length;) { (bytes32 key,) = map.at(i); - result[i] = FunctionReference.wrap(bytes21(key)); + result[i] = address(bytes20(key)); unchecked { ++i; diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index dc5fbb8a..ca9ccb6d 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -21,7 +21,8 @@ import { getAccountStorage, SelectorData, getPermittedCallKey, - PermittedExternalCallData + PermittedExternalCallData, + toPlugin } from "./AccountStorage.sol"; abstract contract PluginManagerInternals is IPluginManager { @@ -41,17 +42,14 @@ abstract contract PluginManagerInternals is IPluginManager { error PluginInstallCallbackFailed(address plugin, bytes revertReason); error PluginInterfaceNotSupported(address plugin); error PluginNotInstalled(address plugin); - error ValidationFunctionAlreadySet(bytes4 selector, FunctionReference validationFunction); + error ValidationFunctionAlreadySet(bytes4 selector, IPlugin validation); - modifier notNullFunction(FunctionReference functionReference) { - if (functionReference.isEmpty()) { - revert NullFunctionReference(); - } - _; - } + IPlugin internal constant NULL_PLUGIN = IPlugin(address(0)); + IPlugin internal constant _RUNTIME_VALIDATION_ALWAYS_ALLOW = IPlugin(address(1)); + IPlugin internal constant _PRE_HOOK_ALWAYS_DENY = IPlugin(address(2)); - modifier notNullPlugin(address plugin) { - if (plugin == address(0)) { + modifier notNullPlugin(IPlugin plugin) { + if (address(plugin) == address(0)) { revert NullPlugin(); } _; @@ -59,7 +57,7 @@ abstract contract PluginManagerInternals is IPluginManager { // Storage update operations - function _setExecutionFunction(bytes4 selector, address plugin) internal notNullPlugin(plugin) { + function _setExecutionFunction(bytes4 selector, address plugin) internal notNullPlugin(IPlugin(plugin)) { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; if (_selectorData.plugin != address(0)) { @@ -75,58 +73,58 @@ abstract contract PluginManagerInternals is IPluginManager { _selectorData.plugin = address(0); } - function _addValidationFunction(bytes4 selector, FunctionReference validationFunction) + function _addValidationFunction(bytes4 selector, IPlugin validation) internal - notNullFunction(validationFunction) + notNullPlugin(validation) { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - if (!_selectorData.validation.isEmpty()) { - revert ValidationFunctionAlreadySet(selector, validationFunction); + if (_selectorData.validation != NULL_PLUGIN) { + revert ValidationFunctionAlreadySet(selector, validation); } - _selectorData.validation = validationFunction; + _selectorData.validation = validation; } - function _removeValidationFunction(bytes4 selector, FunctionReference validationFunction) + function _removeValidationFunction(bytes4 selector, IPlugin validation) internal - notNullFunction(validationFunction) + notNullPlugin(validation) { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - _selectorData.validation = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; + _selectorData.validation = NULL_PLUGIN; } - function _addExecHooks(bytes4 selector, FunctionReference preExecHook, FunctionReference postExecHook) + function _addExecHooks(bytes4 selector, IPlugin preExecHook, IPlugin postExecHook) internal { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - if (!preExecHook.isEmpty()) { + if (preExecHook != NULL_PLUGIN) { _addOrIncrement(_selectorData.preHooks, _toSetValue(preExecHook)); - if (!postExecHook.isEmpty()) { + if (postExecHook != NULL_PLUGIN) { _addOrIncrement(_selectorData.associatedPostHooks[preExecHook], _toSetValue(postExecHook)); } } else { - if (postExecHook.isEmpty()) { + if (postExecHook == NULL_PLUGIN) { // both pre and post hooks cannot be null - revert NullFunctionReference(); + revert NullPlugin(); } _addOrIncrement(_selectorData.postOnlyHooks, _toSetValue(postExecHook)); } } - function _removeExecHooks(bytes4 selector, FunctionReference preExecHook, FunctionReference postExecHook) + function _removeExecHooks(bytes4 selector, IPlugin preExecHook, IPlugin postExecHook) internal { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - if (!preExecHook.isEmpty()) { + if (preExecHook != NULL_PLUGIN) { _removeOrDecrement(_selectorData.preHooks, _toSetValue(preExecHook)); - if (!postExecHook.isEmpty()) { + if (postExecHook != NULL_PLUGIN) { _removeOrDecrement(_selectorData.associatedPostHooks[preExecHook], _toSetValue(postExecHook)); } } else { @@ -137,9 +135,9 @@ abstract contract PluginManagerInternals is IPluginManager { } } - function _addPreUserOpValidationHook(bytes4 selector, FunctionReference preUserOpValidationHook) + function _addPreUserOpValidationHook(bytes4 selector, IPlugin preUserOpValidationHook) internal - notNullFunction(preUserOpValidationHook) + notNullPlugin(preUserOpValidationHook) { _addOrIncrement( getAccountStorage().selectorData[selector].preUserOpValidationHooks, @@ -147,9 +145,9 @@ abstract contract PluginManagerInternals is IPluginManager { ); } - function _removePreUserOpValidationHook(bytes4 selector, FunctionReference preUserOpValidationHook) + function _removePreUserOpValidationHook(bytes4 selector, IPlugin preUserOpValidationHook) internal - notNullFunction(preUserOpValidationHook) + notNullPlugin(preUserOpValidationHook) { // May ignore return value, as the manifest hash is validated to ensure that the hook exists. _removeOrDecrement( @@ -158,9 +156,9 @@ abstract contract PluginManagerInternals is IPluginManager { ); } - function _addPreRuntimeValidationHook(bytes4 selector, FunctionReference preRuntimeValidationHook) + function _addPreRuntimeValidationHook(bytes4 selector, IPlugin preRuntimeValidationHook) internal - notNullFunction(preRuntimeValidationHook) + notNullPlugin(preRuntimeValidationHook) { _addOrIncrement( getAccountStorage().selectorData[selector].preRuntimeValidationHooks, @@ -168,9 +166,9 @@ abstract contract PluginManagerInternals is IPluginManager { ); } - function _removePreRuntimeValidationHook(bytes4 selector, FunctionReference preRuntimeValidationHook) + function _removePreRuntimeValidationHook(bytes4 selector, IPlugin preRuntimeValidationHook) internal - notNullFunction(preRuntimeValidationHook) + notNullPlugin(preRuntimeValidationHook) { // May ignore return value, as the manifest hash is validated to ensure that the hook exists. _removeOrDecrement( @@ -183,7 +181,7 @@ abstract contract PluginManagerInternals is IPluginManager { address plugin, bytes32 manifestHash, bytes memory pluginInstallData, - FunctionReference[] memory dependencies + address[] memory dependencies ) internal { AccountStorage storage _storage = getAccountStorage(); @@ -211,15 +209,15 @@ abstract contract PluginManagerInternals is IPluginManager { uint256 length = dependencies.length; for (uint256 i = 0; i < length;) { // Check the dependency interface id over the address of the dependency. - (address dependencyAddr,) = dependencies[i].unpack(); + IPlugin dependencyAddr = IPlugin(dependencies[i]); // Check that the dependency is installed. if (_storage.pluginData[dependencyAddr].manifestHash == bytes32(0)) { - revert MissingPluginDependency(dependencyAddr); + revert MissingPluginDependency(address(dependencyAddr)); } // Check that the dependency supports the expected interface. - if (!ERC165Checker.supportsInterface(dependencyAddr, manifest.dependencyInterfaceIds[i])) { + if (!ERC165Checker.supportsInterface(address(dependencyAddr), manifest.dependencyInterfaceIds[i])) { revert InvalidDependenciesProvided(); } @@ -232,14 +230,14 @@ abstract contract PluginManagerInternals is IPluginManager { } // Add the plugin metadata to the account - _storage.pluginData[plugin].manifestHash = manifestHash; - _storage.pluginData[plugin].dependencies = dependencies; + _storage.pluginData[IPlugin(plugin)].manifestHash = manifestHash; + _storage.pluginData[IPlugin(plugin)].dependencies = dependencies; // Update components according to the manifest. // Mark whether or not this plugin may spend native token amounts if (manifest.canSpendNativeToken) { - _storage.pluginData[plugin].canSpendNativeToken = true; + _storage.pluginData[IPlugin(plugin)].canSpendNativeToken = true; } length = manifest.executionFunctions.length; @@ -265,7 +263,7 @@ abstract contract PluginManagerInternals is IPluginManager { // Add the permitted external calls to the account. if (manifest.permitAnyExternalAddress) { - _storage.pluginData[plugin].anyExternalExecPermitted = true; + _storage.pluginData[IPlugin(plugin)].anyExternalExecPermitted = true; } else { // Only store the specific permitted external calls if "permit any" flag was not set. length = manifest.permittedExternalCalls.length; @@ -315,7 +313,7 @@ abstract contract PluginManagerInternals is IPluginManager { } // Hooks are not allowed to be provided as dependencies, so we use an empty array for resolving them. - FunctionReference[] memory emptyDependencies; + address[] memory emptyDependencies; length = manifest.preUserOpValidationHooks.length; for (uint256 i = 0; i < length;) { @@ -399,25 +397,24 @@ abstract contract PluginManagerInternals is IPluginManager { } // Check manifest hash. - bytes32 manifestHash = _storage.pluginData[plugin].manifestHash; + bytes32 manifestHash = _storage.pluginData[IPlugin(plugin)].manifestHash; if (!_isValidPluginManifest(manifest, manifestHash)) { revert InvalidPluginManifest(); } // Ensure that there are no dependent plugins. - if (_storage.pluginData[plugin].dependentCount != 0) { + if (_storage.pluginData[IPlugin(plugin)].dependentCount != 0) { revert PluginDependencyViolation(plugin); } // Remove this plugin as a dependent from its dependencies. - FunctionReference[] memory dependencies = _storage.pluginData[plugin].dependencies; + address[] memory dependencies = _storage.pluginData[IPlugin(plugin)].dependencies; uint256 length = dependencies.length; for (uint256 i = 0; i < length;) { - FunctionReference dependency = dependencies[i]; - (address dependencyAddr,) = dependency.unpack(); + address dependency = dependencies[i]; // Decrement the dependent count for the dependency function. - _storage.pluginData[dependencyAddr].dependentCount -= 1; + _storage.pluginData[IPlugin(dependency)].dependentCount -= 1; unchecked { ++i; @@ -427,7 +424,7 @@ abstract contract PluginManagerInternals is IPluginManager { // Remove components according to the manifest, in reverse order (by component type) of their installation. // Hooks are not allowed to be provided as dependencies, so we use an empty array for resolving them. - FunctionReference[] memory emptyDependencies; + address[] memory emptyDependencies; length = manifest.executionHooks.length; for (uint256 i = 0; i < length;) { @@ -505,7 +502,7 @@ abstract contract PluginManagerInternals is IPluginManager { if (manifest.permitAnyExternalAddress) { // Only clear if it was set during install time - _storage.pluginData[plugin].anyExternalExecPermitted = false; + _storage.pluginData[IPlugin(plugin)].anyExternalExecPermitted = false; } else { // Only clear the specific permitted external calls if "permit any" flag was not set. length = manifest.permittedExternalCalls.length; @@ -564,7 +561,7 @@ abstract contract PluginManagerInternals is IPluginManager { } // Remove the plugin metadata from the account. - delete _storage.pluginData[plugin]; + delete _storage.pluginData[IPlugin(plugin)]; // Clear the plugin storage for the account. bool onUninstallSuccess = true; @@ -596,14 +593,18 @@ abstract contract PluginManagerInternals is IPluginManager { return true; } - function _toSetValue(FunctionReference functionReference) internal pure returns (bytes32) { - return bytes32(FunctionReference.unwrap(functionReference)); + function _toSetValue(IPlugin plugin) internal pure returns (bytes32) { + return bytes32(bytes20(address(plugin))); } function _toFunctionReference(bytes32 setValue) internal pure returns (FunctionReference) { return FunctionReference.wrap(bytes21(setValue)); } + function _isEmptyOrMagicValue(IPlugin plugin) internal pure returns (bool) { + return address(plugin) <= address(2); + } + function _isValidPluginManifest(PluginManifest memory manifest, bytes32 manifestHash) internal pure @@ -615,31 +616,31 @@ abstract contract PluginManagerInternals is IPluginManager { function _resolveManifestFunction( ManifestFunction memory manifestFunction, address plugin, - FunctionReference[] memory dependencies, + address[] memory dependencies, // Indicates which magic value, if any, is permissible for the function to resolve. ManifestAssociatedFunctionType allowedMagicValue - ) internal pure returns (FunctionReference) { + ) internal pure returns (IPlugin) { if (manifestFunction.functionType == ManifestAssociatedFunctionType.SELF) { - return FunctionReferenceLib.pack(plugin, manifestFunction.functionId); + return IPlugin(plugin); } else if (manifestFunction.functionType == ManifestAssociatedFunctionType.DEPENDENCY) { if (manifestFunction.dependencyIndex >= dependencies.length) { revert InvalidPluginManifest(); } - return dependencies[manifestFunction.dependencyIndex]; + return IPlugin(dependencies[manifestFunction.dependencyIndex]); } else if (manifestFunction.functionType == ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW) { if (allowedMagicValue == ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW) { - return FunctionReferenceLib._RUNTIME_VALIDATION_ALWAYS_ALLOW; + return _RUNTIME_VALIDATION_ALWAYS_ALLOW; } else { revert InvalidPluginManifest(); } } else if (manifestFunction.functionType == ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY) { if (allowedMagicValue == ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY) { - return FunctionReferenceLib._PRE_HOOK_ALWAYS_DENY; + return _PRE_HOOK_ALWAYS_DENY; } else { revert InvalidPluginManifest(); } } - return FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; // Empty checks are done elsewhere + return IPlugin(address(0)); // Empty checks are done elsewhere } } diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 24a87ea1..718edb1f 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -17,7 +17,7 @@ import {FunctionReference, IPluginManager} from "../interfaces/IPluginManager.so import {IStandardExecutor, Call} from "../interfaces/IStandardExecutor.sol"; import {AccountExecutor} from "./AccountExecutor.sol"; import {AccountLoupe} from "./AccountLoupe.sol"; -import {AccountStorage, getAccountStorage, getPermittedCallKey, SelectorData} from "./AccountStorage.sol"; +import {AccountStorage, getAccountStorage, getPermittedCallKey, toPlugin, SelectorData} from "./AccountStorage.sol"; import {AccountStorageInitializable} from "./AccountStorageInitializable.sol"; import {PluginManagerInternals} from "./PluginManagerInternals.sol"; @@ -38,7 +38,7 @@ contract UpgradeableModularAccount is struct PostExecToRun { bytes preExecHookReturnData; - FunctionReference postExecHook; + IPlugin postExecHookPlugin; } IEntryPoint private immutable _ENTRY_POINT; @@ -55,12 +55,12 @@ contract UpgradeableModularAccount is error ExecFromPluginExternalNotPermitted(address plugin, address target, uint256 value, bytes data); error InvalidConfiguration(); error NativeTokenSpendingNotPermitted(address plugin); - error PostExecHookReverted(address plugin, uint8 functionId, bytes revertReason); - error PreExecHookReverted(address plugin, uint8 functionId, bytes revertReason); - error PreRuntimeValidationHookFailed(address plugin, uint8 functionId, bytes revertReason); + error PostExecHookReverted(address plugin, bytes revertReason); + error PreExecHookReverted(address plugin, bytes revertReason); + error PreRuntimeValidationHookFailed(address plugin, bytes revertReason); error RuntimeValidationFunctionMissing(bytes4 selector); - error RuntimeValidationFunctionReverted(address plugin, uint8 functionId, bytes revertReason); - error UnexpectedAggregator(address plugin, uint8 functionId, address aggregator); + error RuntimeValidationFunctionReverted(address plugin, bytes revertReason); + error UnexpectedAggregator(address plugin, address aggregator); error UnrecognizedFunction(bytes4 selector); error UserOpValidationFunctionMissing(bytes4 selector); @@ -99,7 +99,7 @@ contract UpgradeableModularAccount is revert ArrayLengthMismatch(); } - FunctionReference[] memory emptyDependencies = new FunctionReference[](0); + address[] memory emptyDependencies = new address[](0); for (uint256 i = 0; i < length;) { _installPlugin(plugins[i], manifestHashes[i], pluginInstallDatas[i], emptyDependencies); @@ -219,7 +219,7 @@ contract UpgradeableModularAccount is AccountStorage storage _storage = getAccountStorage(); // Make sure plugin is allowed to spend native token. - if (value > 0 && value > msg.value && !_storage.pluginData[msg.sender].canSpendNativeToken) { + if (value > 0 && value > msg.value && !_storage.pluginData[IPlugin(msg.sender)].canSpendNativeToken) { revert NativeTokenSpendingNotPermitted(msg.sender); } @@ -241,7 +241,7 @@ contract UpgradeableModularAccount is // If the target contract is not permitted, check if the caller plugin is permitted to make any external // calls. - if (!(targetContractPermittedCall || _storage.pluginData[msg.sender].anyExternalExecPermitted)) { + if (!(targetContractPermittedCall || _storage.pluginData[IPlugin(msg.sender)].anyExternalExecPermitted)) { revert ExecFromPluginExternalNotPermitted(msg.sender, target, value, data); } @@ -263,7 +263,7 @@ contract UpgradeableModularAccount is address plugin, bytes32 manifestHash, bytes calldata pluginInstallData, - FunctionReference[] calldata dependencies + address[] calldata dependencies ) external override wrapNativeFunction { _installPlugin(plugin, manifestHash, pluginInstallData, dependencies); } @@ -336,19 +336,19 @@ contract UpgradeableModularAccount is } bytes4 selector = bytes4(userOp.callData); - FunctionReference userOpValidationFunction = getAccountStorage().selectorData[selector].validation; + IPlugin userOpValidation = getAccountStorage().selectorData[selector].validation; - validationData = _doUserOpValidation(selector, userOpValidationFunction, userOp, userOpHash); + validationData = _doUserOpValidation(selector, userOpValidation, userOp, userOpHash); } // To support gas estimation, we don't fail early when the failure is caused by a signature failure function _doUserOpValidation( bytes4 selector, - FunctionReference userOpValidationFunction, + IPlugin userOpValidationPlugin, UserOperation calldata userOp, bytes32 userOpHash ) internal returns (uint256 validationData) { - if (userOpValidationFunction.isEmpty()) { + if (userOpValidationPlugin == NULL_PLUGIN) { revert UserOpValidationFunctionMissing(selector); } @@ -361,15 +361,14 @@ contract UpgradeableModularAccount is uint256 preUserOpValidationHooksLength = preUserOpValidationHooks.length(); for (uint256 i = 0; i < preUserOpValidationHooksLength;) { (bytes32 key,) = preUserOpValidationHooks.at(i); - FunctionReference preUserOpValidationHook = _toFunctionReference(key); + IPlugin preUserOpValidationHookPlugin = toPlugin(key); - if (!preUserOpValidationHook.isEmptyOrMagicValue()) { - (address plugin, uint8 functionId) = preUserOpValidationHook.unpack(); - currentValidationData = IPlugin(plugin).preUserOpValidationHook(functionId, userOp, userOpHash); + if (!_isEmptyOrMagicValue(preUserOpValidationHookPlugin)) { + currentValidationData = preUserOpValidationHookPlugin.preUserOpValidationHook(userOp, userOpHash); if (uint160(currentValidationData) > 1) { // If the aggregator is not 0 or 1, it is an unexpected value - revert UnexpectedAggregator(plugin, functionId, address(uint160(currentValidationData))); + revert UnexpectedAggregator(address(preUserOpValidationHookPlugin), address(uint160(currentValidationData))); } validationData = _coalescePreValidation(validationData, currentValidationData); } else { @@ -384,9 +383,8 @@ contract UpgradeableModularAccount is // Run the user op validationFunction { - if (!userOpValidationFunction.isEmptyOrMagicValue()) { - (address plugin, uint8 functionId) = userOpValidationFunction.unpack(); - currentValidationData = IPlugin(plugin).userOpValidationFunction(functionId, userOp, userOpHash); + if (!_isEmptyOrMagicValue(userOpValidationPlugin)) { + currentValidationData = IPlugin(userOpValidationPlugin).userOpValidationFunction(userOp, userOpHash); if (preUserOpValidationHooksLength != 0) { // If we have other validation data we need to coalesce with @@ -406,7 +404,7 @@ contract UpgradeableModularAccount is if (msg.sender == address(_ENTRY_POINT)) return; AccountStorage storage _storage = getAccountStorage(); - FunctionReference runtimeValidationFunction = _storage.selectorData[msg.sig].validation; + IPlugin runtimeValidationPlugin = _storage.selectorData[msg.sig].validation; // run all preRuntimeValidation hooks EnumerableMap.Bytes32ToUintMap storage preRuntimeValidationHooks = getAccountStorage().selectorData[msg.sig].preRuntimeValidationHooks; @@ -414,21 +412,20 @@ contract UpgradeableModularAccount is uint256 preRuntimeValidationHooksLength = preRuntimeValidationHooks.length(); for (uint256 i = 0; i < preRuntimeValidationHooksLength;) { (bytes32 key,) = preRuntimeValidationHooks.at(i); - FunctionReference preRuntimeValidationHook = _toFunctionReference(key); + IPlugin preRuntimeValidationHookPlugin = toPlugin(key); - if (!preRuntimeValidationHook.isEmptyOrMagicValue()) { - (address plugin, uint8 functionId) = preRuntimeValidationHook.unpack(); + if (!_isEmptyOrMagicValue(preRuntimeValidationHookPlugin)) { // solhint-disable-next-line no-empty-blocks - try IPlugin(plugin).preRuntimeValidationHook(functionId, msg.sender, msg.value, msg.data) {} + try preRuntimeValidationHookPlugin.preRuntimeValidationHook(msg.sender, msg.value, msg.data) {} catch (bytes memory revertReason) { - revert PreRuntimeValidationHookFailed(plugin, functionId, revertReason); + revert PreRuntimeValidationHookFailed(address(preRuntimeValidationHookPlugin), revertReason); } unchecked { ++i; } } else { - if (preRuntimeValidationHook.eq(FunctionReferenceLib._PRE_HOOK_ALWAYS_DENY)) { + if (preRuntimeValidationHookPlugin == _PRE_HOOK_ALWAYS_DENY) { revert AlwaysDenyRule(); } // Function reference cannot be 0 or _RUNTIME_VALIDATION_ALWAYS_ALLOW. @@ -438,17 +435,16 @@ contract UpgradeableModularAccount is // Identifier scope limiting { - if (!runtimeValidationFunction.isEmptyOrMagicValue()) { - (address plugin, uint8 functionId) = runtimeValidationFunction.unpack(); + if (!_isEmptyOrMagicValue(runtimeValidationPlugin)) { // solhint-disable-next-line no-empty-blocks - try IPlugin(plugin).runtimeValidationFunction(functionId, msg.sender, msg.value, msg.data) {} + try runtimeValidationPlugin.runtimeValidationFunction(msg.sender, msg.value, msg.data) {} catch (bytes memory revertReason) { - revert RuntimeValidationFunctionReverted(plugin, functionId, revertReason); + revert RuntimeValidationFunctionReverted(address(runtimeValidationPlugin), revertReason); } } else { - if (runtimeValidationFunction.isEmpty()) { + if (runtimeValidationPlugin == NULL_PLUGIN) { revert RuntimeValidationFunctionMissing(msg.sig); - } else if (runtimeValidationFunction.eq(FunctionReferenceLib._PRE_HOOK_ALWAYS_DENY)) { + } else if (runtimeValidationPlugin == _PRE_HOOK_ALWAYS_DENY) { revert InvalidConfiguration(); } // If _RUNTIME_VALIDATION_ALWAYS_ALLOW, just let the function finish. @@ -481,7 +477,7 @@ contract UpgradeableModularAccount is // Copy post-only hooks to the array. for (uint256 i = 0; i < postOnlyHooksLength;) { (bytes32 key,) = selectorData.postOnlyHooks.at(i); - postHooksToRun[actualPostHooksToRunLength].postExecHook = _toFunctionReference(key); + postHooksToRun[actualPostHooksToRunLength].postExecHookPlugin = toPlugin(key); unchecked { ++actualPostHooksToRunLength; ++i; @@ -492,21 +488,21 @@ contract UpgradeableModularAccount is // the array. for (uint256 i = 0; i < preExecHooksLength;) { (bytes32 key,) = selectorData.preHooks.at(i); - FunctionReference preExecHook = _toFunctionReference(key); + IPlugin preExecHookPlugin = toPlugin(key); - if (preExecHook.isEmptyOrMagicValue()) { + if (_isEmptyOrMagicValue(preExecHookPlugin)) { // The function reference must be PRE_HOOK_ALWAYS_DENY in this case, because zero and any other // magic value is unassignable here. revert AlwaysDenyRule(); } - bytes memory preExecHookReturnData = _runPreExecHook(preExecHook, data); + bytes memory preExecHookReturnData = _runPreExecHook(preExecHookPlugin, data); - uint256 associatedPostExecHooksLength = selectorData.associatedPostHooks[preExecHook].length(); + uint256 associatedPostExecHooksLength = selectorData.associatedPostHooks[preExecHookPlugin].length(); if (associatedPostExecHooksLength > 0) { for (uint256 j = 0; j < associatedPostExecHooksLength;) { - (key,) = selectorData.associatedPostHooks[preExecHook].at(j); - postHooksToRun[actualPostHooksToRunLength].postExecHook = _toFunctionReference(key); + (key,) = selectorData.associatedPostHooks[preExecHookPlugin].at(j); + postHooksToRun[actualPostHooksToRunLength].postExecHookPlugin = toPlugin(key); postHooksToRun[actualPostHooksToRunLength].preExecHookReturnData = preExecHookReturnData; unchecked { @@ -527,17 +523,16 @@ contract UpgradeableModularAccount is } } - function _runPreExecHook(FunctionReference preExecHook, bytes calldata data) + function _runPreExecHook(IPlugin preExecHookPlugin, bytes calldata data) internal returns (bytes memory preExecHookReturnData) { - (address plugin, uint8 functionId) = preExecHook.unpack(); - try IPlugin(plugin).preExecutionHook(functionId, msg.sender, msg.value, data) returns ( + try preExecHookPlugin.preExecutionHook(msg.sender, msg.value, data) returns ( bytes memory returnData ) { preExecHookReturnData = returnData; } catch (bytes memory revertReason) { - revert PreExecHookReverted(plugin, functionId, revertReason); + revert PreExecHookReverted(address(preExecHookPlugin), revertReason); } } @@ -550,11 +545,10 @@ contract UpgradeableModularAccount is } PostExecToRun memory postHookToRun = postHooksToRun[i]; - (address plugin, uint8 functionId) = postHookToRun.postExecHook.unpack(); // solhint-disable-next-line no-empty-blocks - try IPlugin(plugin).postExecutionHook(functionId, postHookToRun.preExecHookReturnData) {} + try postHookToRun.postExecHookPlugin.postExecutionHook(postHookToRun.preExecHookReturnData) {} catch (bytes memory revertReason) { - revert PostExecHookReverted(plugin, functionId, revertReason); + revert PostExecHookReverted(address(postHookToRun.postExecHookPlugin), revertReason); } } } diff --git a/src/interfaces/IAccountLoupe.sol b/src/interfaces/IAccountLoupe.sol index 300a81b1..bd54b2ef 100644 --- a/src/interfaces/IAccountLoupe.sol +++ b/src/interfaces/IAccountLoupe.sol @@ -7,14 +7,14 @@ interface IAccountLoupe { /// @notice Config for an execution function, given a selector. struct ExecutionFunctionConfig { address plugin; - FunctionReference validationFunction; + address validationPlugin; } /// @notice Pre and post hooks for a given selector. /// @dev It's possible for one of either `preExecHook` or `postExecHook` to be empty. struct ExecutionHooks { - FunctionReference preExecHook; - FunctionReference postExecHook; + address preExecHookPlugin; + address postExecHookPlugin; } /// @notice Get the validation functions and plugin address for a selector. @@ -36,8 +36,8 @@ interface IAccountLoupe { external view returns ( - FunctionReference[] memory preUserOpValidationHooks, - FunctionReference[] memory preRuntimeValidationHooks + address[] memory preUserOpValidationHooks, + address[] memory preRuntimeValidationHooks ); /// @notice Get an array of all installed plugins. diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index e8edaf4a..8b8572a2 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -31,7 +31,6 @@ enum ManifestAssociatedFunctionType { /// of the function at `dependencies[dependencyIndex]` during the call to `installPlugin(config)`. struct ManifestFunction { ManifestAssociatedFunctionType functionType; - uint8 functionId; uint256 dependencyIndex; } @@ -109,65 +108,53 @@ interface IPlugin { /// account. function onUninstall(bytes calldata data) external; - /// @notice Run the pre user operation validation hook specified by the `functionId`. + /// @notice Run the pre user operation validation hook. /// @dev Pre user operation validation hooks MUST NOT return an authorizer value other than 0 or 1. - /// @param functionId An identifier that routes the call to different internal implementations, should there be - /// more than one. /// @param userOp The user operation. /// @param userOpHash The user operation hash. /// @return Packed validation data for validAfter (6 bytes), validUntil (6 bytes), and authorizer (20 bytes). - function preUserOpValidationHook(uint8 functionId, UserOperation calldata userOp, bytes32 userOpHash) + function preUserOpValidationHook(UserOperation calldata userOp, bytes32 userOpHash) external returns (uint256); - /// @notice Run the user operation validationFunction specified by the `functionId`. - /// @param functionId An identifier that routes the call to different internal implementations, should there be - /// more than one. + /// @notice Run the user operation validation function. /// @param userOp The user operation. /// @param userOpHash The user operation hash. /// @return Packed validation data for validAfter (6 bytes), validUntil (6 bytes), and authorizer (20 bytes). - function userOpValidationFunction(uint8 functionId, UserOperation calldata userOp, bytes32 userOpHash) + function userOpValidationFunction(UserOperation calldata userOp, bytes32 userOpHash) external returns (uint256); - /// @notice Run the pre runtime validation hook specified by the `functionId`. + /// @notice Run the pre runtime validation hook. /// @dev To indicate the entire call should revert, the function MUST revert. - /// @param functionId An identifier that routes the call to different internal implementations, should there be - /// more than one. /// @param sender The caller address. /// @param value The call value. /// @param data The calldata sent. - function preRuntimeValidationHook(uint8 functionId, address sender, uint256 value, bytes calldata data) + function preRuntimeValidationHook(address sender, uint256 value, bytes calldata data) external; - /// @notice Run the runtime validationFunction specified by the `functionId`. + /// @notice Run the runtime validation function. /// @dev To indicate the entire call should revert, the function MUST revert. - /// @param functionId An identifier that routes the call to different internal implementations, should there be - /// more than one. /// @param sender The caller address. /// @param value The call value. /// @param data The calldata sent. - function runtimeValidationFunction(uint8 functionId, address sender, uint256 value, bytes calldata data) + function runtimeValidationFunction(address sender, uint256 value, bytes calldata data) external; - /// @notice Run the pre execution hook specified by the `functionId`. + /// @notice Run the pre execution hook. /// @dev To indicate the entire call should revert, the function MUST revert. - /// @param functionId An identifier that routes the call to different internal implementations, should there be - /// more than one. /// @param sender The caller address. /// @param value The call value. /// @param data The calldata sent. /// @return Context to pass to a post execution hook, if present. An empty bytes array MAY be returned. - function preExecutionHook(uint8 functionId, address sender, uint256 value, bytes calldata data) + function preExecutionHook(address sender, uint256 value, bytes calldata data) external returns (bytes memory); - /// @notice Run the post execution hook specified by the `functionId`. + /// @notice Run the post execution hook. /// @dev To indicate the entire call should revert, the function MUST revert. - /// @param functionId An identifier that routes the call to different internal implementations, should there be - /// more than one. /// @param preExecHookData The context returned by its associated pre execution hook. - function postExecutionHook(uint8 functionId, bytes calldata preExecHookData) external; + function postExecutionHook(bytes calldata preExecHookData) external; /// @notice Describe the contents and intended configuration of the plugin. /// @dev This manifest MUST stay constant over time. diff --git a/src/interfaces/IPluginManager.sol b/src/interfaces/IPluginManager.sol index 21054d27..d2c6c36c 100644 --- a/src/interfaces/IPluginManager.sol +++ b/src/interfaces/IPluginManager.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.19; type FunctionReference is bytes21; interface IPluginManager { - event PluginInstalled(address indexed plugin, bytes32 manifestHash, FunctionReference[] dependencies); + event PluginInstalled(address indexed plugin, bytes32 manifestHash, address[] dependencies); event PluginUninstalled(address indexed plugin, bool indexed onUninstallSucceeded); @@ -19,7 +19,7 @@ interface IPluginManager { address plugin, bytes32 manifestHash, bytes calldata pluginInstallData, - FunctionReference[] calldata dependencies + address[] calldata dependencies ) external; /// @notice Uninstall a plugin from the modular account. diff --git a/src/plugins/BasePlugin.sol b/src/plugins/BasePlugin.sol index 5dbcb2b3..dd926876 100644 --- a/src/plugins/BasePlugin.sol +++ b/src/plugins/BasePlugin.sol @@ -13,122 +13,78 @@ import {IPlugin, PluginManifest, PluginMetadata} from "../interfaces/IPlugin.sol abstract contract BasePlugin is ERC165, IPlugin { error NotImplemented(); - /// @notice Initialize plugin data for the modular account. - /// @dev Called by the modular account during `installPlugin`. - /// @param data Optional bytes array to be decoded and used by the plugin to setup initial plugin data for the - /// modular account. + /// @inheritdoc IPlugin function onInstall(bytes calldata data) external virtual { (data); revert NotImplemented(); } - /// @notice Clear plugin data for the modular account. - /// @dev Called by the modular account during `uninstallPlugin`. - /// @param data Optional bytes array to be decoded and used by the plugin to clear plugin data for the modular - /// account. + /// @inheritdoc IPlugin function onUninstall(bytes calldata data) external virtual { (data); revert NotImplemented(); } - /// @notice Run the pre user operation validation hook specified by the `functionId`. - /// @dev Pre user operation validation hooks MUST NOT return an authorizer value other than 0 or 1. - /// @param functionId An identifier that routes the call to different internal implementations, should there be - /// more than one. - /// @param userOp The user operation. - /// @param userOpHash The user operation hash. - /// @return Packed validation data for validAfter (6 bytes), validUntil (6 bytes), and authorizer (20 bytes). - function preUserOpValidationHook(uint8 functionId, UserOperation calldata userOp, bytes32 userOpHash) + /// @inheritdoc IPlugin + function preUserOpValidationHook(UserOperation calldata userOp, bytes32 userOpHash) external virtual returns (uint256) { - (functionId, userOp, userOpHash); + (userOp, userOpHash); revert NotImplemented(); } - /// @notice Run the user operation validationFunction specified by the `functionId`. - /// @param functionId An identifier that routes the call to different internal implementations, should there be - /// more than one. - /// @param userOp The user operation. - /// @param userOpHash The user operation hash. - /// @return Packed validation data for validAfter (6 bytes), validUntil (6 bytes), and authorizer (20 bytes). - function userOpValidationFunction(uint8 functionId, UserOperation calldata userOp, bytes32 userOpHash) + /// @inheritdoc IPlugin + function userOpValidationFunction(UserOperation calldata userOp, bytes32 userOpHash) external virtual returns (uint256) { - (functionId, userOp, userOpHash); + (userOp, userOpHash); revert NotImplemented(); } - /// @notice Run the pre runtime validation hook specified by the `functionId`. - /// @dev To indicate the entire call should revert, the function MUST revert. - /// @param functionId An identifier that routes the call to different internal implementations, should there be - /// more than one. - /// @param sender The caller address. - /// @param value The call value. - /// @param data The calldata sent. - function preRuntimeValidationHook(uint8 functionId, address sender, uint256 value, bytes calldata data) + /// @inheritdoc IPlugin + function preRuntimeValidationHook(address sender, uint256 value, bytes calldata data) external virtual { - (functionId, sender, value, data); + (sender, value, data); revert NotImplemented(); } - /// @notice Run the runtime validationFunction specified by the `functionId`. - /// @dev To indicate the entire call should revert, the function MUST revert. - /// @param functionId An identifier that routes the call to different internal implementations, should there be - /// more than one. - /// @param sender The caller address. - /// @param value The call value. - /// @param data The calldata sent. - function runtimeValidationFunction(uint8 functionId, address sender, uint256 value, bytes calldata data) + /// @inheritdoc IPlugin + function runtimeValidationFunction(address sender, uint256 value, bytes calldata data) external virtual { - (functionId, sender, value, data); + (sender, value, data); revert NotImplemented(); } - /// @notice Run the pre execution hook specified by the `functionId`. - /// @dev To indicate the entire call should revert, the function MUST revert. - /// @param functionId An identifier that routes the call to different internal implementations, should there be - /// more than one. - /// @param sender The caller address. - /// @param value The call value. - /// @param data The calldata sent. - /// @return Context to pass to a post execution hook, if present. An empty bytes array MAY be returned. - function preExecutionHook(uint8 functionId, address sender, uint256 value, bytes calldata data) + /// @inheritdoc IPlugin + function preExecutionHook(address sender, uint256 value, bytes calldata data) external virtual returns (bytes memory) { - (functionId, sender, value, data); + (sender, value, data); revert NotImplemented(); } - /// @notice Run the post execution hook specified by the `functionId`. - /// @dev To indicate the entire call should revert, the function MUST revert. - /// @param functionId An identifier that routes the call to different internal implementations, should there be - /// more than one. - /// @param preExecHookData The context returned by its associated pre execution hook. - function postExecutionHook(uint8 functionId, bytes calldata preExecHookData) external virtual { - (functionId, preExecHookData); + /// @inheritdoc IPlugin + function postExecutionHook(bytes calldata preExecHookData) external virtual { + (preExecHookData); revert NotImplemented(); } - /// @notice Describe the contents and intended configuration of the plugin. - /// @dev This manifest MUST stay constant over time. - /// @return A manifest describing the contents and intended configuration of the plugin. + /// @inheritdoc IPlugin function pluginManifest() external pure virtual returns (PluginManifest memory) { revert NotImplemented(); } - /// @notice Describe the metadata of the plugin. - /// @dev This metadata MUST stay constant over time. - /// @return A metadata struct describing the plugin. + /// @inheritdoc IPlugin function pluginMetadata() external pure virtual returns (PluginMetadata memory); /// @dev Returns true if this contract implements the interface defined by diff --git a/src/plugins/TokenReceiverPlugin.sol b/src/plugins/TokenReceiverPlugin.sol index 5574ddc8..b9237179 100644 --- a/src/plugins/TokenReceiverPlugin.sol +++ b/src/plugins/TokenReceiverPlugin.sol @@ -81,7 +81,6 @@ contract TokenReceiverPlugin is BasePlugin, IERC721Receiver, IERC777Recipient, I // Only runtime validationFunction is needed since callbacks come from token contracts only ManifestFunction memory alwaysAllowFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - functionId: 0, // Unused. dependencyIndex: 0 // Unused. }); manifest.validationFunctions = new ManifestAssociatedFunction[](4); diff --git a/src/plugins/owner/ISingleOwnerPlugin.sol b/src/plugins/owner/ISingleOwnerPlugin.sol index fc0622c8..48b7cd43 100644 --- a/src/plugins/owner/ISingleOwnerPlugin.sol +++ b/src/plugins/owner/ISingleOwnerPlugin.sol @@ -4,9 +4,6 @@ pragma solidity ^0.8.19; import {UserOperation} from "@eth-infinitism/account-abstraction/interfaces/UserOperation.sol"; interface ISingleOwnerPlugin { - enum FunctionId { - VALIDATION_OWNER_OR_SELF - } /// @notice This event is emitted when ownership of the account changes. /// @param account The account whose ownership changed. diff --git a/src/plugins/owner/SingleOwnerPlugin.sol b/src/plugins/owner/SingleOwnerPlugin.sol index a44d72b5..4b5d9d77 100644 --- a/src/plugins/owner/SingleOwnerPlugin.sol +++ b/src/plugins/owner/SingleOwnerPlugin.sol @@ -103,37 +103,31 @@ contract SingleOwnerPlugin is BasePlugin, ISingleOwnerPlugin, IERC1271 { } /// @inheritdoc BasePlugin - function runtimeValidationFunction(uint8 functionId, address sender, uint256, bytes calldata) + function runtimeValidationFunction(address sender, uint256, bytes calldata) external view override { - if (functionId == uint8(FunctionId.VALIDATION_OWNER_OR_SELF)) { - // Validate that the sender is the owner of the account or self. - if (sender != _owners[msg.sender] && sender != msg.sender) { - revert NotAuthorized(); - } - return; + // Validate that the sender is the owner of the account or self. + if (sender != _owners[msg.sender] && sender != msg.sender) { + revert NotAuthorized(); } - revert NotImplemented(); + return; } /// @inheritdoc BasePlugin - function userOpValidationFunction(uint8 functionId, UserOperation calldata userOp, bytes32 userOpHash) + function userOpValidationFunction(UserOperation calldata userOp, bytes32 userOpHash) external view override returns (uint256) { - if (functionId == uint8(FunctionId.VALIDATION_OWNER_OR_SELF)) { - // Validate the user op signature against the owner. - (address signer,) = (userOpHash.toEthSignedMessageHash()).tryRecover(userOp.signature); - if (signer == address(0) || signer != _owners[msg.sender]) { - return _SIG_VALIDATION_FAILED; - } - return _SIG_VALIDATION_PASSED; + // Validate the user op signature against the owner. + (address signer,) = (userOpHash.toEthSignedMessageHash()).tryRecover(userOp.signature); + if (signer == address(0) || signer != _owners[msg.sender]) { + return _SIG_VALIDATION_FAILED; } - revert NotImplemented(); + return _SIG_VALIDATION_PASSED; } /// @inheritdoc BasePlugin @@ -147,7 +141,6 @@ contract SingleOwnerPlugin is BasePlugin, ISingleOwnerPlugin, IERC1271 { ManifestFunction memory ownerValidationFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.VALIDATION_OWNER_OR_SELF), dependencyIndex: 0 // Unused. }); manifest.validationFunctions = new ManifestAssociatedFunction[](8); @@ -182,7 +175,6 @@ contract SingleOwnerPlugin is BasePlugin, ISingleOwnerPlugin, IERC1271 { ManifestFunction memory alwaysAllowFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - functionId: 0, // Unused. dependencyIndex: 0 // Unused. }); manifest.validationFunctions[7] = ManifestAssociatedFunction({ diff --git a/src/samples/plugins/ModularSessionKeyPlugin.sol b/src/samples/plugins/ModularSessionKeyPlugin.sol index 7af6bb1c..a6c83e9a 100644 --- a/src/samples/plugins/ModularSessionKeyPlugin.sol +++ b/src/samples/plugins/ModularSessionKeyPlugin.sol @@ -186,53 +186,47 @@ contract ModularSessionKeyPlugin is BasePlugin, IModularSessionKeyPlugin { } /// @inheritdoc BasePlugin - function userOpValidationFunction(uint8 functionId, UserOperation calldata userOp, bytes32 userOpHash) + function userOpValidationFunction(UserOperation calldata userOp, bytes32 userOpHash) external view override returns (uint256) { - if (functionId == uint8(FunctionId.VALIDATION_TEMPORARY_OWNER)) { - (address signer, ECDSA.RecoverError err) = - userOpHash.toEthSignedMessageHash().tryRecover(userOp.signature); - if (err != ECDSA.RecoverError.NoError) { - revert InvalidSignature(); - } - bytes4 selector = bytes4(userOp.callData[0:4]); - bytes memory key = msg.sender.allocateAssociatedStorageKey(0, 1); - StoragePointer ptr = key.associatedStorageLookup(keccak256(abi.encodePacked(signer, selector))); - SessionInfo storage duration = _castPtrToStruct(ptr); - uint48 validAfter = duration.validAfter; - uint48 validUntil = duration.validUntil; - - return _packValidationData(validUntil == 0, validUntil, validAfter); + (address signer, ECDSA.RecoverError err) = + userOpHash.toEthSignedMessageHash().tryRecover(userOp.signature); + if (err != ECDSA.RecoverError.NoError) { + revert InvalidSignature(); } - revert NotImplemented(); + bytes4 selector = bytes4(userOp.callData[0:4]); + bytes memory key = msg.sender.allocateAssociatedStorageKey(0, 1); + StoragePointer ptr = key.associatedStorageLookup(keccak256(abi.encodePacked(signer, selector))); + SessionInfo storage duration = _castPtrToStruct(ptr); + uint48 validAfter = duration.validAfter; + uint48 validUntil = duration.validUntil; + + return _packValidationData(validUntil == 0, validUntil, validAfter); } /// @inheritdoc BasePlugin - function runtimeValidationFunction(uint8 functionId, address sender, uint256, bytes calldata data) + function runtimeValidationFunction(address sender, uint256, bytes calldata data) external view override { - if (functionId == uint8(FunctionId.VALIDATION_TEMPORARY_OWNER)) { - bytes4 selector = bytes4(data[0:4]); - bytes memory key = msg.sender.allocateAssociatedStorageKey(0, 1); - StoragePointer ptr = key.associatedStorageLookup(keccak256(abi.encodePacked(sender, selector))); - SessionInfo storage duration = _castPtrToStruct(ptr); - uint48 validAfter = duration.validAfter; - uint48 validUntil = duration.validUntil; - - if (validUntil != 0) { - if (block.timestamp < validAfter || block.timestamp > validUntil) { - revert WrongTimeRangeForSession(); - } - return; + bytes4 selector = bytes4(data[0:4]); + bytes memory key = msg.sender.allocateAssociatedStorageKey(0, 1); + StoragePointer ptr = key.associatedStorageLookup(keccak256(abi.encodePacked(sender, selector))); + SessionInfo storage duration = _castPtrToStruct(ptr); + uint48 validAfter = duration.validAfter; + uint48 validUntil = duration.validUntil; + + if (validUntil != 0) { + if (block.timestamp < validAfter || block.timestamp > validUntil) { + revert WrongTimeRangeForSession(); } - revert NotAuthorized(); + return; } - revert NotImplemented(); + revert NotAuthorized(); } /// @inheritdoc BasePlugin @@ -247,7 +241,6 @@ contract ModularSessionKeyPlugin is BasePlugin, IModularSessionKeyPlugin { ManifestFunction memory ownerValidationFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.DEPENDENCY, - functionId: 0, // Unused. dependencyIndex: 0 // Used as first index. }); manifest.validationFunctions = new ManifestAssociatedFunction[](5); @@ -270,7 +263,6 @@ contract ModularSessionKeyPlugin is BasePlugin, IModularSessionKeyPlugin { ManifestFunction memory alwaysAllowFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - functionId: 0, // Unused. dependencyIndex: 0 // Unused. }); diff --git a/src/samples/plugins/TokenSessionKeyPlugin.sol b/src/samples/plugins/TokenSessionKeyPlugin.sol index ebc58eda..e4361d27 100644 --- a/src/samples/plugins/TokenSessionKeyPlugin.sol +++ b/src/samples/plugins/TokenSessionKeyPlugin.sol @@ -70,7 +70,6 @@ contract TokenSessionKeyPlugin is BasePlugin, ITokenSessionKeyPlugin { ManifestFunction memory tempOwnerValidationFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.DEPENDENCY, - functionId: 0, // Unused dependencyIndex: 0 // Used as first index }); diff --git a/src/samples/plugins/interfaces/ISessionKeyPlugin.sol b/src/samples/plugins/interfaces/ISessionKeyPlugin.sol index 18a2204c..412faf2e 100644 --- a/src/samples/plugins/interfaces/ISessionKeyPlugin.sol +++ b/src/samples/plugins/interfaces/ISessionKeyPlugin.sol @@ -4,9 +4,6 @@ pragma solidity ^0.8.19; import {UserOperation} from "@eth-infinitism/account-abstraction/interfaces/UserOperation.sol"; interface IModularSessionKeyPlugin { - enum FunctionId { - VALIDATION_TEMPORARY_OWNER - } /// @notice This event is emitted when a session key is added to the account. /// @param account The account whose session key is updated. diff --git a/test/account/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol index bf432e03..1144a3f6 100644 --- a/test/account/AccountExecHooks.t.sol +++ b/test/account/AccountExecHooks.t.sol @@ -36,15 +36,11 @@ contract AccountExecHooksTest is OptimizedTest { bytes32 public manifestHash2; bytes4 internal constant _EXEC_SELECTOR = bytes4(uint32(1)); - uint8 internal constant _PRE_HOOK_FUNCTION_ID_1 = 1; - uint8 internal constant _POST_HOOK_FUNCTION_ID_2 = 2; - uint8 internal constant _PRE_HOOK_FUNCTION_ID_3 = 3; - uint8 internal constant _POST_HOOK_FUNCTION_ID_4 = 4; PluginManifest public m1; PluginManifest public m2; - event PluginInstalled(address indexed plugin, bytes32 manifestHash, FunctionReference[] dependencies); + event PluginInstalled(address indexed plugin, bytes32 manifestHash, address[] dependencies); event PluginUninstalled(address indexed plugin, bool indexed callbacksSucceeded); // emitted by MockPlugin event ReceivedCall(bytes msgData, uint256 msgValue); @@ -65,7 +61,6 @@ contract AccountExecHooksTest is OptimizedTest { executionSelector: _EXEC_SELECTOR, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - functionId: 0, dependencyIndex: 0 }) }) @@ -77,10 +72,9 @@ contract AccountExecHooksTest is OptimizedTest { _EXEC_SELECTOR, ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: _PRE_HOOK_FUNCTION_ID_1, dependencyIndex: 0 }), - ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}) + ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, dependencyIndex: 0}) ); } @@ -93,7 +87,6 @@ contract AccountExecHooksTest is OptimizedTest { emit ReceivedCall( abi.encodeWithSelector( IPlugin.preExecutionHook.selector, - _PRE_HOOK_FUNCTION_ID_1, address(this), // caller 0, // msg.value in call to account abi.encodeWithSelector(_EXEC_SELECTOR) @@ -116,12 +109,10 @@ contract AccountExecHooksTest is OptimizedTest { _EXEC_SELECTOR, ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: _PRE_HOOK_FUNCTION_ID_1, dependencyIndex: 0 }), ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: _POST_HOOK_FUNCTION_ID_2, dependencyIndex: 0 }) ); @@ -137,7 +128,6 @@ contract AccountExecHooksTest is OptimizedTest { emit ReceivedCall( abi.encodeWithSelector( IPlugin.preExecutionHook.selector, - _PRE_HOOK_FUNCTION_ID_1, address(this), // caller 0, // msg.value in call to account abi.encodeWithSelector(_EXEC_SELECTOR) @@ -150,7 +140,7 @@ contract AccountExecHooksTest is OptimizedTest { vm.expectEmit(true, true, true, true); // post hook call emit ReceivedCall( - abi.encodeCall(IPlugin.postExecutionHook, (_POST_HOOK_FUNCTION_ID_2, "")), + abi.encodeCall(IPlugin.postExecutionHook, ("")), 0 // msg value in call to plugin ); @@ -167,10 +157,9 @@ contract AccountExecHooksTest is OptimizedTest { function test_postOnlyExecHook_install() public { _installPlugin1WithHooks( _EXEC_SELECTOR, - ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}), + ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, dependencyIndex: 0}), ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: _POST_HOOK_FUNCTION_ID_2, dependencyIndex: 0 }) ); @@ -183,7 +172,7 @@ contract AccountExecHooksTest is OptimizedTest { vm.expectEmit(true, true, true, true); emit ReceivedCall( - abi.encodeCall(IPlugin.postExecutionHook, (_POST_HOOK_FUNCTION_ID_2, "")), + abi.encodeCall(IPlugin.postExecutionHook, ("")), 0 // msg value in call to plugin ); @@ -203,10 +192,9 @@ contract AccountExecHooksTest is OptimizedTest { _EXEC_SELECTOR, ManifestFunction({ functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, - functionId: 0, dependencyIndex: 0 }), - ManifestFunction(ManifestAssociatedFunctionType.NONE, 0, 0) + ManifestFunction(ManifestAssociatedFunctionType.NONE, 0) ); // Install a second plugin that applies the same pre hook on the same selector. @@ -214,11 +202,10 @@ contract AccountExecHooksTest is OptimizedTest { _EXEC_SELECTOR, ManifestFunction({ functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, - functionId: 0, dependencyIndex: 0 }), - ManifestFunction(ManifestAssociatedFunctionType.NONE, 0, 0), - new FunctionReference[](0) + ManifestFunction(ManifestAssociatedFunctionType.NONE, 0), + new address[](0) ); vm.stopPrank(); @@ -253,32 +240,28 @@ contract AccountExecHooksTest is OptimizedTest { _EXEC_SELECTOR, ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: _PRE_HOOK_FUNCTION_ID_1, dependencyIndex: 0 }), ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: _POST_HOOK_FUNCTION_ID_2, dependencyIndex: 0 }) ); // Attempt to install a second plugin that applies the first plugin's hook pair (as dependencies) to the // same selector. This should revert. - FunctionReference[] memory dependencies = new FunctionReference[](2); - dependencies[0] = FunctionReferenceLib.pack(address(mockPlugin1), _PRE_HOOK_FUNCTION_ID_1); - dependencies[1] = FunctionReferenceLib.pack(address(mockPlugin1), _POST_HOOK_FUNCTION_ID_2); + address[] memory dependencies = new address[](2); + dependencies[0] = address(mockPlugin1); + dependencies[1] = address(mockPlugin1); _installPlugin2WithHooksExpectFail( _EXEC_SELECTOR, ManifestFunction({ functionType: ManifestAssociatedFunctionType.DEPENDENCY, - functionId: 0, dependencyIndex: 0 }), ManifestFunction({ functionType: ManifestAssociatedFunctionType.DEPENDENCY, - functionId: 0, dependencyIndex: 1 }), dependencies, @@ -300,13 +283,13 @@ contract AccountExecHooksTest is OptimizedTest { vm.expectEmit(true, true, true, true); emit ReceivedCall(abi.encodeCall(IPlugin.onInstall, (bytes(""))), 0); vm.expectEmit(true, true, true, true); - emit PluginInstalled(address(mockPlugin1), manifestHash1, new FunctionReference[](0)); + emit PluginInstalled(address(mockPlugin1), manifestHash1, new address[](0)); account.installPlugin({ plugin: address(mockPlugin1), manifestHash: manifestHash1, pluginInstallData: bytes(""), - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); } @@ -314,7 +297,7 @@ contract AccountExecHooksTest is OptimizedTest { bytes4 selector, ManifestFunction memory preHook, ManifestFunction memory postHook, - FunctionReference[] memory dependencies + address[] memory dependencies ) internal { if (preHook.functionType == ManifestAssociatedFunctionType.DEPENDENCY) { m2.dependencyInterfaceIds.push(type(IPlugin).interfaceId); @@ -345,7 +328,7 @@ contract AccountExecHooksTest is OptimizedTest { bytes4 selector, ManifestFunction memory preHook, ManifestFunction memory postHook, - FunctionReference[] memory dependencies, + address[] memory dependencies, bytes memory revertData ) internal { if (preHook.functionType == ManifestAssociatedFunctionType.DEPENDENCY) { diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol index 35c7b6e2..aebffdf9 100644 --- a/test/account/AccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -13,6 +13,7 @@ import { PluginManifest } from "../../src/interfaces/IPlugin.sol"; import {IAccountLoupe} from "../../src/interfaces/IAccountLoupe.sol"; +import {IPlugin} from "../../src/interfaces/IPlugin.sol"; import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; import {IStandardExecutor} from "../../src/interfaces/IStandardExecutor.sol"; import {ISingleOwnerPlugin} from "../../src/plugins/owner/ISingleOwnerPlugin.sol"; @@ -31,7 +32,7 @@ contract AccountLoupeTest is OptimizedTest { UpgradeableModularAccount public account1; - FunctionReference public ownerValidation; + IPlugin public ownerValidation; event ReceivedCall(bytes msgData, uint256 msgValue); @@ -45,11 +46,9 @@ contract AccountLoupeTest is OptimizedTest { account1 = factory.createAccount(address(this), 0); bytes32 manifestHash = keccak256(abi.encode(comprehensivePlugin.pluginManifest())); - account1.installPlugin(address(comprehensivePlugin), manifestHash, "", new FunctionReference[](0)); + account1.installPlugin(address(comprehensivePlugin), manifestHash, "", new address[](0)); - ownerValidation = FunctionReferenceLib.pack( - address(singleOwnerPlugin), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER_OR_SELF) - ); + ownerValidation = IPlugin(singleOwnerPlugin); } function test_pluginLoupe_getInstalledPlugins_initial() public { @@ -63,7 +62,7 @@ contract AccountLoupeTest is OptimizedTest { function test_pluginLoupe_getExecutionFunctionConfig_native() public { bytes4[] memory selectorsToCheck = new bytes4[](5); - FunctionReference[] memory expectedValidations = new FunctionReference[](5); + IPlugin[] memory expectedValidations = new IPlugin[](5); selectorsToCheck[0] = IStandardExecutor.execute.selector; expectedValidations[0] = ownerValidation; @@ -86,8 +85,8 @@ contract AccountLoupeTest is OptimizedTest { assertEq(config.plugin, address(account1)); assertEq( - FunctionReference.unwrap(config.validationFunction), - FunctionReference.unwrap(expectedValidations[i]) + config.validationPlugin, + address(expectedValidations[i]) ); } } @@ -95,13 +94,11 @@ contract AccountLoupeTest is OptimizedTest { function test_pluginLoupe_getExecutionFunctionConfig_plugin() public { bytes4[] memory selectorsToCheck = new bytes4[](2); address[] memory expectedPluginAddress = new address[](2); - FunctionReference[] memory expectedValidations = new FunctionReference[](2); + IPlugin[] memory expectedValidations = new IPlugin[](2); selectorsToCheck[0] = comprehensivePlugin.foo.selector; expectedPluginAddress[0] = address(comprehensivePlugin); - expectedValidations[0] = FunctionReferenceLib.pack( - address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.VALIDATION) - ); + expectedValidations[0] = IPlugin(comprehensivePlugin); selectorsToCheck[1] = singleOwnerPlugin.transferOwnership.selector; expectedPluginAddress[1] = address(singleOwnerPlugin); @@ -113,8 +110,8 @@ contract AccountLoupeTest is OptimizedTest { assertEq(config.plugin, expectedPluginAddress[i]); assertEq( - FunctionReference.unwrap(config.validationFunction), - FunctionReference.unwrap(expectedValidations[i]) + config.validationPlugin, + address(expectedValidations[i]) ); } } @@ -124,20 +121,12 @@ contract AccountLoupeTest is OptimizedTest { assertEq(hooks.length, 1); assertEq( - FunctionReference.unwrap(hooks[0].preExecHook), - FunctionReference.unwrap( - FunctionReferenceLib.pack( - address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.PRE_EXECUTION_HOOK) - ) - ) + hooks[0].preExecHookPlugin, + address(comprehensivePlugin) ); assertEq( - FunctionReference.unwrap(hooks[0].postExecHook), - FunctionReference.unwrap( - FunctionReferenceLib.pack( - address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.POST_EXECUTION_HOOK) - ) - ) + hooks[0].postExecHookPlugin, + address(comprehensivePlugin) ); } @@ -152,12 +141,10 @@ contract AccountLoupeTest is OptimizedTest { executionSelector: ComprehensivePlugin.foo.selector, preExecHook: ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: 0, dependencyIndex: 0 }), postExecHook: ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: 0, dependencyIndex: 0 }) }); @@ -165,7 +152,7 @@ contract AccountLoupeTest is OptimizedTest { MockPlugin mockPlugin = new MockPlugin(mockPluginManifest); bytes32 manifestHash = keccak256(abi.encode(mockPlugin.pluginManifest())); - account1.installPlugin(address(mockPlugin), manifestHash, "", new FunctionReference[](0)); + account1.installPlugin(address(mockPlugin), manifestHash, "", new address[](0)); // Assert that the returned execution hooks are what is expected @@ -173,76 +160,50 @@ contract AccountLoupeTest is OptimizedTest { assertEq(hooks.length, 2); assertEq( - FunctionReference.unwrap(hooks[0].preExecHook), - FunctionReference.unwrap( - FunctionReferenceLib.pack( - address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.PRE_EXECUTION_HOOK) - ) - ) + hooks[0].preExecHookPlugin, + address(comprehensivePlugin) ); assertEq( - FunctionReference.unwrap(hooks[0].postExecHook), - FunctionReference.unwrap( - FunctionReferenceLib.pack( - address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.POST_EXECUTION_HOOK) - ) - ) + hooks[0].postExecHookPlugin, + address(comprehensivePlugin) ); assertEq( - FunctionReference.unwrap(hooks[1].preExecHook), - FunctionReference.unwrap(FunctionReferenceLib.pack(address(mockPlugin), uint8(0))) + hooks[1].preExecHookPlugin, + address(mockPlugin) ); assertEq( - FunctionReference.unwrap(hooks[1].postExecHook), - FunctionReference.unwrap(FunctionReferenceLib.pack(address(mockPlugin), uint8(0))) + hooks[1].postExecHookPlugin, + address(mockPlugin) ); } function test_pluginLoupe_getPreUserOpValidationHooks() public { - (FunctionReference[] memory hooks,) = account1.getPreValidationHooks(comprehensivePlugin.foo.selector); + (address[] memory hooks,) = account1.getPreValidationHooks(comprehensivePlugin.foo.selector); - assertEq(hooks.length, 2); - assertEq( - FunctionReference.unwrap(hooks[0]), - FunctionReference.unwrap( - FunctionReferenceLib.pack( - address(comprehensivePlugin), - uint8(ComprehensivePlugin.FunctionId.PRE_USER_OP_VALIDATION_HOOK_1) - ) - ) - ); + assertEq(hooks.length, 1); assertEq( - FunctionReference.unwrap(hooks[1]), - FunctionReference.unwrap( - FunctionReferenceLib.pack( - address(comprehensivePlugin), - uint8(ComprehensivePlugin.FunctionId.PRE_USER_OP_VALIDATION_HOOK_2) - ) - ) + hooks[0], + address(comprehensivePlugin) ); + // todo: add a second hook to measure here + // assertEq( + // hooks[1], + // address(comprehensivePlugin) + // ); } function test_pluginLoupe_getPreRuntimeValidationHooks() public { - (, FunctionReference[] memory hooks) = account1.getPreValidationHooks(comprehensivePlugin.foo.selector); + (, address[] memory hooks) = account1.getPreValidationHooks(comprehensivePlugin.foo.selector); - assertEq(hooks.length, 2); - assertEq( - FunctionReference.unwrap(hooks[0]), - FunctionReference.unwrap( - FunctionReferenceLib.pack( - address(comprehensivePlugin), - uint8(ComprehensivePlugin.FunctionId.PRE_RUNTIME_VALIDATION_HOOK_1) - ) - ) - ); + assertEq(hooks.length, 1); assertEq( - FunctionReference.unwrap(hooks[1]), - FunctionReference.unwrap( - FunctionReferenceLib.pack( - address(comprehensivePlugin), - uint8(ComprehensivePlugin.FunctionId.PRE_RUNTIME_VALIDATION_HOOK_2) - ) - ) + hooks[0], + address(comprehensivePlugin) ); + // todo: add a second hook to measure here + // assertEq( + // hooks[1], + // address(comprehensivePlugin) + // ); } } diff --git a/test/account/AccountReturnData.t.sol b/test/account/AccountReturnData.t.sol index 2837b904..e110224f 100644 --- a/test/account/AccountReturnData.t.sol +++ b/test/account/AccountReturnData.t.sol @@ -47,7 +47,7 @@ contract AccountReturnDataTest is OptimizedTest { plugin: address(resultCreatorPlugin), manifestHash: resultCreatorManifestHash, pluginInstallData: "", - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); // Add the result consumer plugin to the account bytes32 resultConsumerManifestHash = keccak256(abi.encode(resultConsumerPlugin.pluginManifest())); @@ -55,7 +55,7 @@ contract AccountReturnDataTest is OptimizedTest { plugin: address(resultConsumerPlugin), manifestHash: resultConsumerManifestHash, pluginInstallData: "", - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); } diff --git a/test/account/ExecuteFromPluginPermissions.t.sol b/test/account/ExecuteFromPluginPermissions.t.sol index 88ce0c98..dcaab1f7 100644 --- a/test/account/ExecuteFromPluginPermissions.t.sol +++ b/test/account/ExecuteFromPluginPermissions.t.sol @@ -55,7 +55,7 @@ contract ExecuteFromPluginPermissionsTest is OptimizedTest { plugin: address(resultCreatorPlugin), manifestHash: resultCreatorManifestHash, pluginInstallData: "", - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); // Add the EFP caller plugin to the account bytes32 efpCallerManifestHash = keccak256(abi.encode(efpCallerPlugin.pluginManifest())); @@ -63,7 +63,7 @@ contract ExecuteFromPluginPermissionsTest is OptimizedTest { plugin: address(efpCallerPlugin), manifestHash: efpCallerManifestHash, pluginInstallData: "", - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); // Add the EFP caller plugin with any external permissions to the account @@ -73,7 +73,7 @@ contract ExecuteFromPluginPermissionsTest is OptimizedTest { plugin: address(efpCallerPluginAnyExternal), manifestHash: efpCallerAnyExternalManifestHash, pluginInstallData: "", - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); } diff --git a/test/account/ManifestValidity.t.sol b/test/account/ManifestValidity.t.sol index 169d6c81..0ae2a5c8 100644 --- a/test/account/ManifestValidity.t.sol +++ b/test/account/ManifestValidity.t.sol @@ -51,7 +51,7 @@ contract ManifestValidityTest is OptimizedTest { plugin: address(plugin), manifestHash: manifestHash, pluginInstallData: "", - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); } @@ -68,7 +68,7 @@ contract ManifestValidityTest is OptimizedTest { plugin: address(plugin), manifestHash: manifestHash, pluginInstallData: "", - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); } @@ -83,7 +83,7 @@ contract ManifestValidityTest is OptimizedTest { plugin: address(plugin), manifestHash: manifestHash, pluginInstallData: "", - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); } @@ -98,7 +98,7 @@ contract ManifestValidityTest is OptimizedTest { plugin: address(plugin), manifestHash: manifestHash, pluginInstallData: "", - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); } @@ -114,7 +114,7 @@ contract ManifestValidityTest is OptimizedTest { plugin: address(plugin), manifestHash: manifestHash, pluginInstallData: "", - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); } @@ -130,7 +130,7 @@ contract ManifestValidityTest is OptimizedTest { plugin: address(plugin), manifestHash: manifestHash, pluginInstallData: "", - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); } @@ -145,7 +145,7 @@ contract ManifestValidityTest is OptimizedTest { plugin: address(plugin), manifestHash: manifestHash, pluginInstallData: "", - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); } } diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index 1005cdf2..e45b63de 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -47,7 +47,7 @@ contract UpgradeableModularAccountTest is OptimizedTest { uint256 public constant CALL_GAS_LIMIT = 50000; uint256 public constant VERIFICATION_GAS_LIMIT = 1200000; - event PluginInstalled(address indexed plugin, bytes32 manifestHash, FunctionReference[] dependencies); + event PluginInstalled(address indexed plugin, bytes32 manifestHash, address[] dependencies); event PluginUninstalled(address indexed plugin, bool indexed callbacksSucceeded); event ReceivedCall(bytes msgData, uint256 msgValue); @@ -265,12 +265,12 @@ contract UpgradeableModularAccountTest is OptimizedTest { bytes32 manifestHash = keccak256(abi.encode(tokenReceiverPlugin.pluginManifest())); vm.expectEmit(true, true, true, true); - emit PluginInstalled(address(tokenReceiverPlugin), manifestHash, new FunctionReference[](0)); + emit PluginInstalled(address(tokenReceiverPlugin), manifestHash, new address[](0)); IPluginManager(account2).installPlugin({ plugin: address(tokenReceiverPlugin), manifestHash: manifestHash, pluginInstallData: abi.encode(uint48(1 days)), - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); address[] memory plugins = IAccountLoupe(account2).getInstalledPlugins(); @@ -293,7 +293,7 @@ contract UpgradeableModularAccountTest is OptimizedTest { plugin: address(mockPluginWithBadPermittedExec), manifestHash: manifestHash, pluginInstallData: "", - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); } @@ -305,7 +305,7 @@ contract UpgradeableModularAccountTest is OptimizedTest { plugin: address(tokenReceiverPlugin), manifestHash: bytes32(0), pluginInstallData: abi.encode(uint48(1 days)), - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); } @@ -320,7 +320,7 @@ contract UpgradeableModularAccountTest is OptimizedTest { plugin: address(badPlugin), manifestHash: bytes32(0), pluginInstallData: "", - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); } @@ -332,7 +332,7 @@ contract UpgradeableModularAccountTest is OptimizedTest { plugin: address(tokenReceiverPlugin), manifestHash: manifestHash, pluginInstallData: abi.encode(uint48(1 days)), - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); vm.expectRevert( @@ -344,7 +344,7 @@ contract UpgradeableModularAccountTest is OptimizedTest { plugin: address(tokenReceiverPlugin), manifestHash: manifestHash, pluginInstallData: abi.encode(uint48(1 days)), - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); } @@ -357,7 +357,7 @@ contract UpgradeableModularAccountTest is OptimizedTest { plugin: address(plugin), manifestHash: manifestHash, pluginInstallData: "", - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); vm.expectEmit(true, true, true, true); @@ -378,7 +378,7 @@ contract UpgradeableModularAccountTest is OptimizedTest { plugin: address(plugin), manifestHash: manifestHash, pluginInstallData: "", - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); vm.expectEmit(true, true, true, true); @@ -403,7 +403,7 @@ contract UpgradeableModularAccountTest is OptimizedTest { plugin: address(plugin), manifestHash: manifestHash, pluginInstallData: "", - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); // Attempt to uninstall with a blank manifest @@ -431,7 +431,7 @@ contract UpgradeableModularAccountTest is OptimizedTest { plugin: address(plugin), manifestHash: manifestHash, pluginInstallData: "", - dependencies: new FunctionReference[](0) + dependencies: new address[](0) }); vm.stopPrank(); diff --git a/test/account/ValidationIntersection.t.sol b/test/account/ValidationIntersection.t.sol index b5f001f7..2cb75a64 100644 --- a/test/account/ValidationIntersection.t.sol +++ b/test/account/ValidationIntersection.t.sol @@ -18,285 +18,284 @@ import { import {OptimizedTest} from "../utils/OptimizedTest.sol"; contract ValidationIntersectionTest is OptimizedTest { - uint256 internal constant _SIG_VALIDATION_FAILED = 1; - - EntryPoint public entryPoint; - - address public owner1; - uint256 public owner1Key; - UpgradeableModularAccount public account1; - MockUserOpValidationPlugin public noHookPlugin; - MockUserOpValidation1HookPlugin public oneHookPlugin; - MockUserOpValidation2HookPlugin public twoHookPlugin; - - function setUp() public { - entryPoint = new EntryPoint(); - owner1 = makeAddr("owner1"); - - SingleOwnerPlugin singleOwnerPlugin = _deploySingleOwnerPlugin(); - MSCAFactoryFixture factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); - - account1 = factory.createAccount(owner1, 0); - vm.deal(address(account1), 1 ether); - - noHookPlugin = new MockUserOpValidationPlugin(); - oneHookPlugin = new MockUserOpValidation1HookPlugin(); - twoHookPlugin = new MockUserOpValidation2HookPlugin(); - - vm.startPrank(address(owner1)); - account1.installPlugin({ - plugin: address(noHookPlugin), - manifestHash: keccak256(abi.encode(noHookPlugin.pluginManifest())), - pluginInstallData: "", - dependencies: new FunctionReference[](0) - }); - account1.installPlugin({ - plugin: address(oneHookPlugin), - manifestHash: keccak256(abi.encode(oneHookPlugin.pluginManifest())), - pluginInstallData: "", - dependencies: new FunctionReference[](0) - }); - account1.installPlugin({ - plugin: address(twoHookPlugin), - manifestHash: keccak256(abi.encode(twoHookPlugin.pluginManifest())), - pluginInstallData: "", - dependencies: new FunctionReference[](0) - }); - vm.stopPrank(); - } - - function testFuzz_validationIntersect_single(uint256 validationData) public { - noHookPlugin.setValidationData(validationData); - - UserOperation memory userOp; - userOp.callData = bytes.concat(noHookPlugin.foo.selector); - bytes32 uoHash = entryPoint.getUserOpHash(userOp); - - vm.prank(address(entryPoint)); - uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); - - assertEq(returnedValidationData, validationData); - } - - function test_validationIntersect_authorizer_sigfail_validationFunction() public { - oneHookPlugin.setValidationData( - _SIG_VALIDATION_FAILED, - 0 // returns OK - ); - - UserOperation memory userOp; - userOp.callData = bytes.concat(oneHookPlugin.bar.selector); - bytes32 uoHash = entryPoint.getUserOpHash(userOp); - - vm.prank(address(entryPoint)); - uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); - - // Down-cast to only check the authorizer - assertEq(uint160(returnedValidationData), _SIG_VALIDATION_FAILED); - } - - function test_validationIntersect_authorizer_sigfail_hook() public { - oneHookPlugin.setValidationData( - 0, // returns OK - _SIG_VALIDATION_FAILED - ); - - UserOperation memory userOp; - userOp.callData = bytes.concat(oneHookPlugin.bar.selector); - bytes32 uoHash = entryPoint.getUserOpHash(userOp); - - vm.prank(address(entryPoint)); - uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); - - // Down-cast to only check the authorizer - assertEq(uint160(returnedValidationData), _SIG_VALIDATION_FAILED); - } - - function test_validationIntersect_timeBounds_intersect_1() public { - uint48 start1 = uint48(10); - uint48 end1 = uint48(20); - - uint48 start2 = uint48(15); - uint48 end2 = uint48(25); - - oneHookPlugin.setValidationData( - _packValidationData(address(0), start1, end1), _packValidationData(address(0), start2, end2) - ); - - UserOperation memory userOp; - userOp.callData = bytes.concat(oneHookPlugin.bar.selector); - bytes32 uoHash = entryPoint.getUserOpHash(userOp); - - vm.prank(address(entryPoint)); - uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); - - assertEq(returnedValidationData, _packValidationData(address(0), start2, end1)); - } - - function test_validationIntersect_timeBounds_intersect_2() public { - uint48 start1 = uint48(10); - uint48 end1 = uint48(20); - - uint48 start2 = uint48(15); - uint48 end2 = uint48(25); - - oneHookPlugin.setValidationData( - _packValidationData(address(0), start2, end2), _packValidationData(address(0), start1, end1) - ); - - UserOperation memory userOp; - userOp.callData = bytes.concat(oneHookPlugin.bar.selector); - bytes32 uoHash = entryPoint.getUserOpHash(userOp); - - vm.prank(address(entryPoint)); - uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); - - assertEq(returnedValidationData, _packValidationData(address(0), start2, end1)); - } - - function test_validationIntersect_revert_unexpectedAuthorizer() public { - address badAuthorizer = makeAddr("badAuthorizer"); +// uint256 internal constant _SIG_VALIDATION_FAILED = 1; + +// EntryPoint public entryPoint; + +// address public owner1; +// uint256 public owner1Key; +// UpgradeableModularAccount public account1; +// MockUserOpValidationPlugin public noHookPlugin; +// MockUserOpValidation1HookPlugin public oneHookPlugin; +// MockUserOpValidation2HookPlugin public twoHookPlugin; + +// function setUp() public { +// entryPoint = new EntryPoint(); +// owner1 = makeAddr("owner1"); + +// SingleOwnerPlugin singleOwnerPlugin = _deploySingleOwnerPlugin(); +// MSCAFactoryFixture factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); + +// account1 = factory.createAccount(owner1, 0); +// vm.deal(address(account1), 1 ether); + +// noHookPlugin = new MockUserOpValidationPlugin(); +// oneHookPlugin = new MockUserOpValidation1HookPlugin(); +// twoHookPlugin = new MockUserOpValidation2HookPlugin(); + +// vm.startPrank(address(owner1)); +// account1.installPlugin({ +// plugin: address(noHookPlugin), +// manifestHash: keccak256(abi.encode(noHookPlugin.pluginManifest())), +// pluginInstallData: "", +// dependencies: new FunctionReference[](0) +// }); +// account1.installPlugin({ +// plugin: address(oneHookPlugin), +// manifestHash: keccak256(abi.encode(oneHookPlugin.pluginManifest())), +// pluginInstallData: "", +// dependencies: new FunctionReference[](0) +// }); +// account1.installPlugin({ +// plugin: address(twoHookPlugin), +// manifestHash: keccak256(abi.encode(twoHookPlugin.pluginManifest())), +// pluginInstallData: "", +// dependencies: new FunctionReference[](0) +// }); +// vm.stopPrank(); +// } + +// function testFuzz_validationIntersect_single(uint256 validationData) public { +// noHookPlugin.setValidationData(validationData); + +// UserOperation memory userOp; +// userOp.callData = bytes.concat(noHookPlugin.foo.selector); +// bytes32 uoHash = entryPoint.getUserOpHash(userOp); + +// vm.prank(address(entryPoint)); +// uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); + +// assertEq(returnedValidationData, validationData); +// } + +// function test_validationIntersect_authorizer_sigfail_validationFunction() public { +// oneHookPlugin.setValidationData( +// _SIG_VALIDATION_FAILED, +// 0 // returns OK +// ); + +// UserOperation memory userOp; +// userOp.callData = bytes.concat(oneHookPlugin.bar.selector); +// bytes32 uoHash = entryPoint.getUserOpHash(userOp); + +// vm.prank(address(entryPoint)); +// uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); + +// // Down-cast to only check the authorizer +// assertEq(uint160(returnedValidationData), _SIG_VALIDATION_FAILED); +// } + +// function test_validationIntersect_authorizer_sigfail_hook() public { +// oneHookPlugin.setValidationData( +// 0, // returns OK +// _SIG_VALIDATION_FAILED +// ); + +// UserOperation memory userOp; +// userOp.callData = bytes.concat(oneHookPlugin.bar.selector); +// bytes32 uoHash = entryPoint.getUserOpHash(userOp); + +// vm.prank(address(entryPoint)); +// uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); + +// // Down-cast to only check the authorizer +// assertEq(uint160(returnedValidationData), _SIG_VALIDATION_FAILED); +// } + +// function test_validationIntersect_timeBounds_intersect_1() public { +// uint48 start1 = uint48(10); +// uint48 end1 = uint48(20); + +// uint48 start2 = uint48(15); +// uint48 end2 = uint48(25); + +// oneHookPlugin.setValidationData( +// _packValidationData(address(0), start1, end1), _packValidationData(address(0), start2, end2) +// ); + +// UserOperation memory userOp; +// userOp.callData = bytes.concat(oneHookPlugin.bar.selector); +// bytes32 uoHash = entryPoint.getUserOpHash(userOp); + +// vm.prank(address(entryPoint)); +// uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); + +// assertEq(returnedValidationData, _packValidationData(address(0), start2, end1)); +// } + +// function test_validationIntersect_timeBounds_intersect_2() public { +// uint48 start1 = uint48(10); +// uint48 end1 = uint48(20); + +// uint48 start2 = uint48(15); +// uint48 end2 = uint48(25); + +// oneHookPlugin.setValidationData( +// _packValidationData(address(0), start2, end2), _packValidationData(address(0), start1, end1) +// ); + +// UserOperation memory userOp; +// userOp.callData = bytes.concat(oneHookPlugin.bar.selector); +// bytes32 uoHash = entryPoint.getUserOpHash(userOp); + +// vm.prank(address(entryPoint)); +// uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); + +// assertEq(returnedValidationData, _packValidationData(address(0), start2, end1)); +// } - oneHookPlugin.setValidationData( - 0, // returns OK - uint256(uint160(badAuthorizer)) // returns an aggregator, which preValidation hooks are not allowed to - // do. - ); +// function test_validationIntersect_revert_unexpectedAuthorizer() public { +// address badAuthorizer = makeAddr("badAuthorizer"); - UserOperation memory userOp; - userOp.callData = bytes.concat(oneHookPlugin.bar.selector); - bytes32 uoHash = entryPoint.getUserOpHash(userOp); +// oneHookPlugin.setValidationData( +// 0, // returns OK +// uint256(uint160(badAuthorizer)) // returns an aggregator, which preValidation hooks are not allowed to +// // do. +// ); - vm.prank(address(entryPoint)); - vm.expectRevert( - abi.encodeWithSelector( - UpgradeableModularAccount.UnexpectedAggregator.selector, - address(oneHookPlugin), - MockBaseUserOpValidationPlugin.FunctionId.PRE_USER_OP_VALIDATION_HOOK_1, - badAuthorizer - ) - ); - account1.validateUserOp(userOp, uoHash, 1 wei); - } - - function test_validationIntersect_validAuthorizer() public { - address goodAuthorizer = makeAddr("goodAuthorizer"); - - oneHookPlugin.setValidationData( - uint256(uint160(goodAuthorizer)), // returns a valid aggregator - 0 // returns OK - ); - - UserOperation memory userOp; - userOp.callData = bytes.concat(oneHookPlugin.bar.selector); - bytes32 uoHash = entryPoint.getUserOpHash(userOp); - - vm.prank(address(entryPoint)); - uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); - - assertEq(address(uint160(returnedValidationData)), goodAuthorizer); - } - - function test_validationIntersect_authorizerAndTimeRange() public { - uint48 start1 = uint48(10); - uint48 end1 = uint48(20); - - uint48 start2 = uint48(15); - uint48 end2 = uint48(25); - - address goodAuthorizer = makeAddr("goodAuthorizer"); - - oneHookPlugin.setValidationData( - _packValidationData(goodAuthorizer, start1, end1), _packValidationData(address(0), start2, end2) - ); - - UserOperation memory userOp; - userOp.callData = bytes.concat(oneHookPlugin.bar.selector); - bytes32 uoHash = entryPoint.getUserOpHash(userOp); - - vm.prank(address(entryPoint)); - uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); - - assertEq(returnedValidationData, _packValidationData(goodAuthorizer, start2, end1)); - } - - function test_validationIntersect_multiplePreValidationHooksIntersect() public { - uint48 start1 = uint48(10); - uint48 end1 = uint48(20); - - uint48 start2 = uint48(15); - uint48 end2 = uint48(25); - - twoHookPlugin.setValidationData( - 0, // returns OK - _packValidationData(address(0), start1, end1), - _packValidationData(address(0), start2, end2) - ); - - UserOperation memory userOp; - userOp.callData = bytes.concat(twoHookPlugin.baz.selector); - bytes32 uoHash = entryPoint.getUserOpHash(userOp); - - vm.prank(address(entryPoint)); - uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); - - assertEq(returnedValidationData, _packValidationData(address(0), start2, end1)); - } - - function test_validationIntersect_multiplePreValidationHooksSigFail() public { - twoHookPlugin.setValidationData( - 0, // returns OK - 0, // returns OK - _SIG_VALIDATION_FAILED - ); - - UserOperation memory userOp; - userOp.callData = bytes.concat(twoHookPlugin.baz.selector); - - bytes32 uoHash = entryPoint.getUserOpHash(userOp); - - vm.prank(address(entryPoint)); - uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); - - // Down-cast to only check the authorizer - assertEq(uint160(returnedValidationData), _SIG_VALIDATION_FAILED); - } - - function _unpackValidationData(uint256 validationData) - internal - pure - returns (address authorizer, uint48 validAfter, uint48 validUntil) - { - authorizer = address(uint160(validationData)); - validUntil = uint48(validationData >> 160); - if (validUntil == 0) { - validUntil = type(uint48).max; - } - validAfter = uint48(validationData >> (48 + 160)); - } - - function _packValidationData(address authorizer, uint48 validAfter, uint48 validUntil) - internal - pure - returns (uint256) - { - return uint160(authorizer) | (uint256(validUntil) << 160) | (uint256(validAfter) << (160 + 48)); - } - - function _intersectTimeRange(uint48 validafter1, uint48 validuntil1, uint48 validafter2, uint48 validuntil2) - internal - pure - returns (uint48 validAfter, uint48 validUntil) - { - if (validafter1 < validafter2) { - validAfter = validafter2; - } else { - validAfter = validafter1; - } - if (validuntil1 > validuntil2) { - validUntil = validuntil2; - } else { - validUntil = validuntil1; - } - } +// UserOperation memory userOp; +// userOp.callData = bytes.concat(oneHookPlugin.bar.selector); +// bytes32 uoHash = entryPoint.getUserOpHash(userOp); + +// vm.prank(address(entryPoint)); +// vm.expectRevert( +// abi.encodeWithSelector( +// UpgradeableModularAccount.UnexpectedAggregator.selector, +// address(oneHookPlugin), +// badAuthorizer +// ) +// ); +// account1.validateUserOp(userOp, uoHash, 1 wei); +// } + +// function test_validationIntersect_validAuthorizer() public { +// address goodAuthorizer = makeAddr("goodAuthorizer"); + +// oneHookPlugin.setValidationData( +// uint256(uint160(goodAuthorizer)), // returns a valid aggregator +// 0 // returns OK +// ); + +// UserOperation memory userOp; +// userOp.callData = bytes.concat(oneHookPlugin.bar.selector); +// bytes32 uoHash = entryPoint.getUserOpHash(userOp); + +// vm.prank(address(entryPoint)); +// uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); + +// assertEq(address(uint160(returnedValidationData)), goodAuthorizer); +// } + +// function test_validationIntersect_authorizerAndTimeRange() public { +// uint48 start1 = uint48(10); +// uint48 end1 = uint48(20); + +// uint48 start2 = uint48(15); +// uint48 end2 = uint48(25); + +// address goodAuthorizer = makeAddr("goodAuthorizer"); + +// oneHookPlugin.setValidationData( +// _packValidationData(goodAuthorizer, start1, end1), _packValidationData(address(0), start2, end2) +// ); + +// UserOperation memory userOp; +// userOp.callData = bytes.concat(oneHookPlugin.bar.selector); +// bytes32 uoHash = entryPoint.getUserOpHash(userOp); + +// vm.prank(address(entryPoint)); +// uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); + +// assertEq(returnedValidationData, _packValidationData(goodAuthorizer, start2, end1)); +// } + +// function test_validationIntersect_multiplePreValidationHooksIntersect() public { +// uint48 start1 = uint48(10); +// uint48 end1 = uint48(20); + +// uint48 start2 = uint48(15); +// uint48 end2 = uint48(25); + +// twoHookPlugin.setValidationData( +// 0, // returns OK +// _packValidationData(address(0), start1, end1), +// _packValidationData(address(0), start2, end2) +// ); + +// UserOperation memory userOp; +// userOp.callData = bytes.concat(twoHookPlugin.baz.selector); +// bytes32 uoHash = entryPoint.getUserOpHash(userOp); + +// vm.prank(address(entryPoint)); +// uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); + +// assertEq(returnedValidationData, _packValidationData(address(0), start2, end1)); +// } + +// function test_validationIntersect_multiplePreValidationHooksSigFail() public { +// twoHookPlugin.setValidationData( +// 0, // returns OK +// 0, // returns OK +// _SIG_VALIDATION_FAILED +// ); + +// UserOperation memory userOp; +// userOp.callData = bytes.concat(twoHookPlugin.baz.selector); + +// bytes32 uoHash = entryPoint.getUserOpHash(userOp); + +// vm.prank(address(entryPoint)); +// uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); + +// // Down-cast to only check the authorizer +// assertEq(uint160(returnedValidationData), _SIG_VALIDATION_FAILED); +// } + +// function _unpackValidationData(uint256 validationData) +// internal +// pure +// returns (address authorizer, uint48 validAfter, uint48 validUntil) +// { +// authorizer = address(uint160(validationData)); +// validUntil = uint48(validationData >> 160); +// if (validUntil == 0) { +// validUntil = type(uint48).max; +// } +// validAfter = uint48(validationData >> (48 + 160)); +// } + +// function _packValidationData(address authorizer, uint48 validAfter, uint48 validUntil) +// internal +// pure +// returns (uint256) +// { +// return uint160(authorizer) | (uint256(validUntil) << 160) | (uint256(validAfter) << (160 + 48)); +// } + +// function _intersectTimeRange(uint48 validafter1, uint48 validuntil1, uint48 validafter2, uint48 validuntil2) +// internal +// pure +// returns (uint48 validAfter, uint48 validUntil) +// { +// if (validafter1 < validafter2) { +// validAfter = validafter2; +// } else { +// validAfter = validafter1; +// } +// if (validuntil1 > validuntil2) { +// validUntil = validuntil2; +// } else { +// validUntil = validuntil1; +// } +// } } diff --git a/test/mocks/plugins/BadTransferOwnershipPlugin.sol b/test/mocks/plugins/BadTransferOwnershipPlugin.sol index 03da8019..7725c8e5 100644 --- a/test/mocks/plugins/BadTransferOwnershipPlugin.sol +++ b/test/mocks/plugins/BadTransferOwnershipPlugin.sol @@ -52,7 +52,6 @@ contract BadTransferOwnershipPlugin is BasePlugin { executionSelector: this.evilTransferOwnership.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - functionId: 0, // Unused. dependencyIndex: 0 // Unused. }) }); diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index 442b39a6..9af70618 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -15,17 +15,18 @@ import {IStandardExecutor} from "../../../src/interfaces/IStandardExecutor.sol"; import {BasePlugin} from "../../../src/plugins/BasePlugin.sol"; contract ComprehensivePlugin is BasePlugin { - enum FunctionId { - PRE_USER_OP_VALIDATION_HOOK_1, - PRE_USER_OP_VALIDATION_HOOK_2, - PRE_RUNTIME_VALIDATION_HOOK_1, - PRE_RUNTIME_VALIDATION_HOOK_2, - VALIDATION, - PRE_EXECUTION_HOOK, - PRE_PERMITTED_CALL_EXECUTION_HOOK, - POST_EXECUTION_HOOK, - POST_PERMITTED_CALL_EXECUTION_HOOK - } + // todo: remove + // enum FunctionId { + // PRE_USER_OP_VALIDATION_HOOK_1, + // PRE_USER_OP_VALIDATION_HOOK_2, + // PRE_RUNTIME_VALIDATION_HOOK_1, + // PRE_RUNTIME_VALIDATION_HOOK_2, + // VALIDATION, + // PRE_EXECUTION_HOOK, + // PRE_PERMITTED_CALL_EXECUTION_HOOK, + // POST_EXECUTION_HOOK, + // POST_PERMITTED_CALL_EXECUTION_HOOK + // } string public constant NAME = "Comprehensive Plugin"; string public constant VERSION = "1.0.0"; @@ -45,73 +46,76 @@ contract ComprehensivePlugin is BasePlugin { function onUninstall(bytes calldata) external override {} - function preUserOpValidationHook(uint8 functionId, UserOperation calldata, bytes32) + function preUserOpValidationHook(UserOperation calldata, bytes32) external pure override returns (uint256) { - if (functionId == uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_1)) { - return 0; - } else if (functionId == uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_2)) { - return 0; - } - revert NotImplemented(); + // if (functionId == uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_1)) { + // return 0; + // } else if (functionId == uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_2)) { + // return 0; + // } + // revert NotImplemented(); + // Todo: is there a logic step missing here, with the two different hooks? + return 0; } - function userOpValidationFunction(uint8 functionId, UserOperation calldata, bytes32) + function userOpValidationFunction(UserOperation calldata, bytes32) external pure override returns (uint256) { - if (functionId == uint8(FunctionId.VALIDATION)) { - return 0; - } - revert NotImplemented(); + return 0; } - function preRuntimeValidationHook(uint8 functionId, address, uint256, bytes calldata) external pure override { - if (functionId == uint8(FunctionId.PRE_RUNTIME_VALIDATION_HOOK_1)) { - return; - } else if (functionId == uint8(FunctionId.PRE_RUNTIME_VALIDATION_HOOK_2)) { - return; - } - revert NotImplemented(); + function preRuntimeValidationHook(address, uint256, bytes calldata) external pure override { + // if (functionId == uint8(FunctionId.PRE_RUNTIME_VALIDATION_HOOK_1)) { + // return; + // } else if (functionId == uint8(FunctionId.PRE_RUNTIME_VALIDATION_HOOK_2)) { + // return; + // } + // revert NotImplemented(); + // Todo: is there a logic step missing here, with the two different hooks? + return; } - function runtimeValidationFunction(uint8 functionId, address, uint256, bytes calldata) + function runtimeValidationFunction(address, uint256, bytes calldata) external pure override { - if (functionId == uint8(FunctionId.VALIDATION)) { - return; - } - revert NotImplemented(); + return; } - function preExecutionHook(uint8 functionId, address, uint256, bytes calldata) + function preExecutionHook(address, uint256, bytes calldata) external pure override returns (bytes memory) { - if (functionId == uint8(FunctionId.PRE_EXECUTION_HOOK)) { - return ""; - } else if (functionId == uint8(FunctionId.PRE_PERMITTED_CALL_EXECUTION_HOOK)) { - return ""; - } - revert NotImplemented(); + // if (functionId == uint8(FunctionId.PRE_EXECUTION_HOOK)) { + // return ""; + // } else if (functionId == uint8(FunctionId.PRE_PERMITTED_CALL_EXECUTION_HOOK)) { + // return ""; + // } + // revert NotImplemented(); + // Todo: is there a logic step missing here, with the two different hooks? + return ""; } - function postExecutionHook(uint8 functionId, bytes calldata) external pure override { - if (functionId == uint8(FunctionId.POST_EXECUTION_HOOK)) { - return; - } else if (functionId == uint8(FunctionId.POST_PERMITTED_CALL_EXECUTION_HOOK)) { - return; - } - revert NotImplemented(); + function postExecutionHook(bytes calldata) external pure override { + // if (functionId == uint8(FunctionId.POST_EXECUTION_HOOK)) { + // return; + // } else if (functionId == uint8(FunctionId.POST_PERMITTED_CALL_EXECUTION_HOOK)) { + // return; + // } + // revert NotImplemented(); + // Todo: is there a logic step missing here, with the two different hooks? + return; + } function pluginManifest() external pure override returns (PluginManifest memory) { @@ -122,7 +126,6 @@ contract ComprehensivePlugin is BasePlugin { ManifestFunction memory fooValidationFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.VALIDATION), dependencyIndex: 0 // Unused. }); manifest.validationFunctions = new ManifestAssociatedFunction[](1); @@ -131,85 +134,51 @@ contract ComprehensivePlugin is BasePlugin { associatedFunction: fooValidationFunction }); - manifest.preUserOpValidationHooks = new ManifestAssociatedFunction[](4); + manifest.preUserOpValidationHooks = new ManifestAssociatedFunction[](2); manifest.preUserOpValidationHooks[0] = ManifestAssociatedFunction({ executionSelector: this.foo.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_1), dependencyIndex: 0 // Unused. }) }); + // todo: manifest.preUserOpValidationHooks[1] = manifest.preUserOpValidationHooks[1] = ManifestAssociatedFunction({ - executionSelector: this.foo.selector, - associatedFunction: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_2), - dependencyIndex: 0 // Unused. - }) - }); - manifest.preUserOpValidationHooks[2] = ManifestAssociatedFunction({ - executionSelector: IStandardExecutor.execute.selector, - associatedFunction: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_1), - dependencyIndex: 0 // Unused. - }) - }); - manifest.preUserOpValidationHooks[3] = ManifestAssociatedFunction({ executionSelector: IStandardExecutor.execute.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_2), dependencyIndex: 0 // Unused. }) }); + // todo: manifest.preUserOpValidationHooks[3] = - manifest.preRuntimeValidationHooks = new ManifestAssociatedFunction[](4); + manifest.preRuntimeValidationHooks = new ManifestAssociatedFunction[](2); manifest.preRuntimeValidationHooks[0] = ManifestAssociatedFunction({ executionSelector: this.foo.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_RUNTIME_VALIDATION_HOOK_1), dependencyIndex: 0 // Unused. }) }); + // todo: manifest.preRuntimeValidationHooks[1] = manifest.preRuntimeValidationHooks[1] = ManifestAssociatedFunction({ - executionSelector: this.foo.selector, - associatedFunction: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_RUNTIME_VALIDATION_HOOK_2), - dependencyIndex: 0 // Unused. - }) - }); - manifest.preRuntimeValidationHooks[2] = ManifestAssociatedFunction({ - executionSelector: IStandardExecutor.execute.selector, - associatedFunction: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_RUNTIME_VALIDATION_HOOK_1), - dependencyIndex: 0 // Unused. - }) - }); - manifest.preRuntimeValidationHooks[3] = ManifestAssociatedFunction({ executionSelector: IStandardExecutor.execute.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_RUNTIME_VALIDATION_HOOK_2), dependencyIndex: 0 // Unused. }) }); + // todo: manifest.preRuntimeValidationHooks[3] = manifest.executionHooks = new ManifestExecutionHook[](1); manifest.executionHooks[0] = ManifestExecutionHook({ executionSelector: this.foo.selector, preExecHook: ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_EXECUTION_HOOK), dependencyIndex: 0 // Unused. }), postExecHook: ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.POST_EXECUTION_HOOK), dependencyIndex: 0 // Unused. }) }); diff --git a/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol b/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol index 3eea3ac0..2015702c 100644 --- a/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol +++ b/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol @@ -49,7 +49,6 @@ contract EFPCallerPlugin is BaseTestPlugin { ManifestFunction memory alwaysAllowValidationFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - functionId: 0, dependencyIndex: 0 }); @@ -186,7 +185,6 @@ contract EFPCallerPluginAnyExternal is BaseTestPlugin { executionSelector: this.passthroughExecute.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - functionId: 0, dependencyIndex: 0 }) }); diff --git a/test/mocks/plugins/ManifestValidityMocks.sol b/test/mocks/plugins/ManifestValidityMocks.sol index f19c67ca..18a962cf 100644 --- a/test/mocks/plugins/ManifestValidityMocks.sol +++ b/test/mocks/plugins/ManifestValidityMocks.sol @@ -36,7 +36,6 @@ contract BadValidationMagicValue_PreRuntimeValidationHook_Plugin is BaseTestPlug executionSelector: this.foo.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: 0, dependencyIndex: 0 }) }); @@ -47,7 +46,6 @@ contract BadValidationMagicValue_PreRuntimeValidationHook_Plugin is BaseTestPlug executionSelector: this.foo.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - functionId: 0, dependencyIndex: 0 }) }); @@ -76,7 +74,6 @@ contract BadValidationMagicValue_PreUserOpValidationHook_Plugin is BaseTestPlugi executionSelector: this.foo.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: 0, dependencyIndex: 0 }) }); @@ -87,7 +84,6 @@ contract BadValidationMagicValue_PreUserOpValidationHook_Plugin is BaseTestPlugi executionSelector: this.foo.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - functionId: 0, dependencyIndex: 0 }) }); @@ -118,12 +114,10 @@ contract BadValidationMagicValue_PreExecHook_Plugin is BaseTestPlugin { executionSelector: this.foo.selector, preExecHook: ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - functionId: 0, dependencyIndex: 0 }), postExecHook: ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: 0, // Dummy unimplemented function id, but can be added correctly dependencyIndex: 0 }) }); @@ -153,12 +147,10 @@ contract BadValidationMagicValue_PostExecHook_Plugin is BaseTestPlugin { executionSelector: this.foo.selector, preExecHook: ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: 0, // Dummy unimplemented function id, but can be added correctly dependencyIndex: 0 }), postExecHook: ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - functionId: 0, dependencyIndex: 0 }) }); @@ -187,7 +179,6 @@ contract BadHookMagicValue_UserOpValidationFunction_Plugin is BaseTestPlugin { executionSelector: this.foo.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, - functionId: 0, dependencyIndex: 0 }) }); @@ -216,7 +207,6 @@ contract BadHookMagicValue_RuntimeValidationFunction_Plugin is BaseTestPlugin { executionSelector: this.foo.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, - functionId: 0, dependencyIndex: 0 }) }); @@ -246,12 +236,10 @@ contract BadHookMagicValue_PostExecHook_Plugin is BaseTestPlugin { executionSelector: this.foo.selector, preExecHook: ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, - functionId: 0, // Dummy unimplemented function id, but can be added correctly dependencyIndex: 0 }), postExecHook: ManifestFunction({ functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, - functionId: 0, dependencyIndex: 0 }) }); diff --git a/test/mocks/plugins/ReturnDataPluginMocks.sol b/test/mocks/plugins/ReturnDataPluginMocks.sol index 85925c0e..132b7df4 100644 --- a/test/mocks/plugins/ReturnDataPluginMocks.sol +++ b/test/mocks/plugins/ReturnDataPluginMocks.sol @@ -50,7 +50,6 @@ contract ResultCreatorPlugin is BaseTestPlugin { executionSelector: this.foo.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - functionId: 0, dependencyIndex: 0 }) }); @@ -126,7 +125,6 @@ contract ResultConsumerPlugin is BaseTestPlugin { executionSelector: this.checkResultEFPFallback.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - functionId: 0, dependencyIndex: 0 }) }); @@ -134,7 +132,6 @@ contract ResultConsumerPlugin is BaseTestPlugin { executionSelector: this.checkResultEFPExternal.selector, associatedFunction: ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - functionId: 0, dependencyIndex: 0 }) }); diff --git a/test/mocks/plugins/ValidationPluginMocks.sol b/test/mocks/plugins/ValidationPluginMocks.sol index 4859a7e9..e413c5a1 100644 --- a/test/mocks/plugins/ValidationPluginMocks.sol +++ b/test/mocks/plugins/ValidationPluginMocks.sol @@ -12,191 +12,191 @@ import { import {BaseTestPlugin} from "./BaseTestPlugin.sol"; abstract contract MockBaseUserOpValidationPlugin is BaseTestPlugin { - enum FunctionId { - USER_OP_VALIDATION, - PRE_USER_OP_VALIDATION_HOOK_1, - PRE_USER_OP_VALIDATION_HOOK_2 - } - - uint256 internal _userOpValidationFunctionData; - uint256 internal _preUserOpValidationHook1Data; - uint256 internal _preUserOpValidationHook2Data; - - // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ - // ┃ Plugin interface functions ┃ - // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - - function onInstall(bytes calldata) external override {} - - function onUninstall(bytes calldata) external override {} - - function preUserOpValidationHook(uint8 functionId, UserOperation calldata, bytes32) - external - view - override - returns (uint256) - { - if (functionId == uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_1)) { - return _preUserOpValidationHook1Data; - } else if (functionId == uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_2)) { - return _preUserOpValidationHook2Data; - } - revert NotImplemented(); - } - - function userOpValidationFunction(uint8 functionId, UserOperation calldata, bytes32) - external - view - override - returns (uint256) - { - if (functionId == uint8(FunctionId.USER_OP_VALIDATION)) { - return _userOpValidationFunctionData; - } - revert NotImplemented(); - } + // enum FunctionId { + // USER_OP_VALIDATION, + // PRE_USER_OP_VALIDATION_HOOK_1, + // PRE_USER_OP_VALIDATION_HOOK_2 + // } + + // uint256 internal _userOpValidationFunctionData; + // uint256 internal _preUserOpValidationHook1Data; + // uint256 internal _preUserOpValidationHook2Data; + + // // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ + // // ┃ Plugin interface functions ┃ + // // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + + // function onInstall(bytes calldata) external override {} + + // function onUninstall(bytes calldata) external override {} + + // function preUserOpValidationHook(uint8 functionId, UserOperation calldata, bytes32) + // external + // view + // override + // returns (uint256) + // { + // if (functionId == uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_1)) { + // return _preUserOpValidationHook1Data; + // } else if (functionId == uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_2)) { + // return _preUserOpValidationHook2Data; + // } + // revert NotImplemented(); + // } + + // function userOpValidationFunction(uint8 functionId, UserOperation calldata, bytes32) + // external + // view + // override + // returns (uint256) + // { + // if (functionId == uint8(FunctionId.USER_OP_VALIDATION)) { + // return _userOpValidationFunctionData; + // } + // revert NotImplemented(); + // } } contract MockUserOpValidationPlugin is MockBaseUserOpValidationPlugin { - function setValidationData(uint256 userOpValidationFunctionData) external { - _userOpValidationFunctionData = userOpValidationFunctionData; - } - - // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ - // ┃ Execution functions ┃ - // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - - function foo() external {} - - // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ - // ┃ Plugin interface functions ┃ - // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - - function pluginManifest() external pure override returns (PluginManifest memory) { - PluginManifest memory manifest; - - manifest.executionFunctions = new bytes4[](1); - manifest.executionFunctions[0] = this.foo.selector; - - manifest.validationFunctions = new ManifestAssociatedFunction[](1); - manifest.validationFunctions[0] = ManifestAssociatedFunction({ - executionSelector: this.foo.selector, - associatedFunction: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.USER_OP_VALIDATION), - dependencyIndex: 0 // Unused. - }) - }); - - return manifest; - } + // function setValidationData(uint256 userOpValidationFunctionData) external { + // _userOpValidationFunctionData = userOpValidationFunctionData; + // } + + // // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ + // // ┃ Execution functions ┃ + // // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + + // function foo() external {} + + // // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ + // // ┃ Plugin interface functions ┃ + // // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + + // function pluginManifest() external pure override returns (PluginManifest memory) { + // PluginManifest memory manifest; + + // manifest.executionFunctions = new bytes4[](1); + // manifest.executionFunctions[0] = this.foo.selector; + + // manifest.validationFunctions = new ManifestAssociatedFunction[](1); + // manifest.validationFunctions[0] = ManifestAssociatedFunction({ + // executionSelector: this.foo.selector, + // associatedFunction: ManifestFunction({ + // functionType: ManifestAssociatedFunctionType.SELF, + // functionId: uint8(FunctionId.USER_OP_VALIDATION), + // dependencyIndex: 0 // Unused. + // }) + // }); + + // return manifest; + // } } contract MockUserOpValidation1HookPlugin is MockBaseUserOpValidationPlugin { - function setValidationData(uint256 userOpValidationFunctionData, uint256 preUserOpValidationHook1Data) - external - { - _userOpValidationFunctionData = userOpValidationFunctionData; - _preUserOpValidationHook1Data = preUserOpValidationHook1Data; - } - - // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ - // ┃ Execution functions ┃ - // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - - function bar() external {} - - // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ - // ┃ Plugin interface functions ┃ - // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - - function pluginManifest() external pure override returns (PluginManifest memory) { - PluginManifest memory manifest; - - manifest.executionFunctions = new bytes4[](1); - manifest.executionFunctions[0] = this.bar.selector; - - ManifestFunction memory userOpValidationFunctionRef = ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.USER_OP_VALIDATION), - dependencyIndex: 0 // Unused. - }); - manifest.validationFunctions = new ManifestAssociatedFunction[](1); - manifest.validationFunctions[0] = ManifestAssociatedFunction({ - executionSelector: this.bar.selector, - associatedFunction: userOpValidationFunctionRef - }); - - manifest.preUserOpValidationHooks = new ManifestAssociatedFunction[](1); - manifest.preUserOpValidationHooks[0] = ManifestAssociatedFunction({ - executionSelector: this.bar.selector, - associatedFunction: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_1), - dependencyIndex: 0 // Unused. - }) - }); - - return manifest; - } +// function setValidationData(uint256 userOpValidationFunctionData, uint256 preUserOpValidationHook1Data) +// external +// { +// _userOpValidationFunctionData = userOpValidationFunctionData; +// _preUserOpValidationHook1Data = preUserOpValidationHook1Data; +// } + +// // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ +// // ┃ Execution functions ┃ +// // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + +// function bar() external {} + +// // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ +// // ┃ Plugin interface functions ┃ +// // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + +// function pluginManifest() external pure override returns (PluginManifest memory) { +// PluginManifest memory manifest; + +// manifest.executionFunctions = new bytes4[](1); +// manifest.executionFunctions[0] = this.bar.selector; + +// ManifestFunction memory userOpValidationFunctionRef = ManifestFunction({ +// functionType: ManifestAssociatedFunctionType.SELF, +// functionId: uint8(FunctionId.USER_OP_VALIDATION), +// dependencyIndex: 0 // Unused. +// }); +// manifest.validationFunctions = new ManifestAssociatedFunction[](1); +// manifest.validationFunctions[0] = ManifestAssociatedFunction({ +// executionSelector: this.bar.selector, +// associatedFunction: userOpValidationFunctionRef +// }); + +// manifest.preUserOpValidationHooks = new ManifestAssociatedFunction[](1); +// manifest.preUserOpValidationHooks[0] = ManifestAssociatedFunction({ +// executionSelector: this.bar.selector, +// associatedFunction: ManifestFunction({ +// functionType: ManifestAssociatedFunctionType.SELF, +// functionId: uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_1), +// dependencyIndex: 0 // Unused. +// }) +// }); + +// return manifest; +// } } contract MockUserOpValidation2HookPlugin is MockBaseUserOpValidationPlugin { - function setValidationData( - uint256 userOpValidationFunctionData, - uint256 preUserOpValidationHook1Data, - uint256 preUserOpValidationHook2Data - ) external { - _userOpValidationFunctionData = userOpValidationFunctionData; - _preUserOpValidationHook1Data = preUserOpValidationHook1Data; - _preUserOpValidationHook2Data = preUserOpValidationHook2Data; - } - - // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ - // ┃ Execution functions ┃ - // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - - function baz() external {} - - // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ - // ┃ Plugin interface functions ┃ - // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - - function pluginManifest() external pure override returns (PluginManifest memory) { - PluginManifest memory manifest; - - manifest.executionFunctions = new bytes4[](1); - manifest.executionFunctions[0] = this.baz.selector; - - ManifestFunction memory userOpValidationFunctionRef = ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.USER_OP_VALIDATION), - dependencyIndex: 0 // Unused. - }); - manifest.validationFunctions = new ManifestAssociatedFunction[](1); - manifest.validationFunctions[0] = ManifestAssociatedFunction({ - executionSelector: this.baz.selector, - associatedFunction: userOpValidationFunctionRef - }); - - manifest.preUserOpValidationHooks = new ManifestAssociatedFunction[](2); - manifest.preUserOpValidationHooks[0] = ManifestAssociatedFunction({ - executionSelector: this.baz.selector, - associatedFunction: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_1), - dependencyIndex: 0 // Unused. - }) - }); - manifest.preUserOpValidationHooks[1] = ManifestAssociatedFunction({ - executionSelector: this.baz.selector, - associatedFunction: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_2), - dependencyIndex: 0 // Unused. - }) - }); - - return manifest; - } +// function setValidationData( +// uint256 userOpValidationFunctionData, +// uint256 preUserOpValidationHook1Data, +// uint256 preUserOpValidationHook2Data +// ) external { +// _userOpValidationFunctionData = userOpValidationFunctionData; +// _preUserOpValidationHook1Data = preUserOpValidationHook1Data; +// _preUserOpValidationHook2Data = preUserOpValidationHook2Data; +// } + +// // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ +// // ┃ Execution functions ┃ +// // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + +// function baz() external {} + +// // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ +// // ┃ Plugin interface functions ┃ +// // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + +// function pluginManifest() external pure override returns (PluginManifest memory) { +// PluginManifest memory manifest; + +// manifest.executionFunctions = new bytes4[](1); +// manifest.executionFunctions[0] = this.baz.selector; + +// ManifestFunction memory userOpValidationFunctionRef = ManifestFunction({ +// functionType: ManifestAssociatedFunctionType.SELF, +// functionId: uint8(FunctionId.USER_OP_VALIDATION), +// dependencyIndex: 0 // Unused. +// }); +// manifest.validationFunctions = new ManifestAssociatedFunction[](1); +// manifest.validationFunctions[0] = ManifestAssociatedFunction({ +// executionSelector: this.baz.selector, +// associatedFunction: userOpValidationFunctionRef +// }); + +// manifest.preUserOpValidationHooks = new ManifestAssociatedFunction[](2); +// manifest.preUserOpValidationHooks[0] = ManifestAssociatedFunction({ +// executionSelector: this.baz.selector, +// associatedFunction: ManifestFunction({ +// functionType: ManifestAssociatedFunctionType.SELF, +// functionId: uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_1), +// dependencyIndex: 0 // Unused. +// }) +// }); +// manifest.preUserOpValidationHooks[1] = ManifestAssociatedFunction({ +// executionSelector: this.baz.selector, +// associatedFunction: ManifestFunction({ +// functionType: ManifestAssociatedFunctionType.SELF, +// functionId: uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_2), +// dependencyIndex: 0 // Unused. +// }) +// }); + +// return manifest; +// } } diff --git a/test/plugin/SingleOwnerPlugin.t.sol b/test/plugin/SingleOwnerPlugin.t.sol index 5784059e..3bcf859c 100644 --- a/test/plugin/SingleOwnerPlugin.t.sol +++ b/test/plugin/SingleOwnerPlugin.t.sol @@ -113,13 +113,13 @@ contract SingleOwnerPluginTest is OptimizedTest { plugin.transferOwnership(owner1); assertEq(owner1, plugin.owner()); plugin.runtimeValidationFunction( - uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER_OR_SELF), owner1, 0, "" + owner1, 0, "" ); vm.startPrank(b); vm.expectRevert(ISingleOwnerPlugin.NotAuthorized.selector); plugin.runtimeValidationFunction( - uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER_OR_SELF), owner1, 0, "" + owner1, 0, "" ); } @@ -136,7 +136,7 @@ contract SingleOwnerPluginTest is OptimizedTest { // sig check should fail uint256 success = plugin.userOpValidationFunction( - uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER_OR_SELF), userOp, userOpHash + userOp, userOpHash ); assertEq(success, 1); @@ -146,7 +146,7 @@ contract SingleOwnerPluginTest is OptimizedTest { // sig check should pass success = plugin.userOpValidationFunction( - uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER_OR_SELF), userOp, userOpHash + userOp, userOpHash ); assertEq(success, 0); } diff --git a/test/plugin/TokenReceiverPlugin.t.sol b/test/plugin/TokenReceiverPlugin.t.sol index 49692e34..215422f5 100644 --- a/test/plugin/TokenReceiverPlugin.t.sol +++ b/test/plugin/TokenReceiverPlugin.t.sol @@ -60,7 +60,7 @@ contract TokenReceiverPluginTest is OptimizedTest, IERC1155Receiver { function _initPlugin() internal { bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - acct.installPlugin(address(plugin), manifestHash, "", new FunctionReference[](0)); + acct.installPlugin(address(plugin), manifestHash, "", new address[](0)); } function test_failERC721Transfer() public { diff --git a/test/samples/plugins/ModularSessionKeyPlugin.t.sol b/test/samples/plugins/ModularSessionKeyPlugin.t.sol index e20cbfca..7038c3d3 100644 --- a/test/samples/plugins/ModularSessionKeyPlugin.t.sol +++ b/test/samples/plugins/ModularSessionKeyPlugin.t.sol @@ -99,10 +99,8 @@ contract ModularSessionKeyPluginTest is Test { vm.deal(address(account), 1 ether); vm.startPrank(owner); - FunctionReference[] memory modularSessionDependency = new FunctionReference[](1); - modularSessionDependency[0] = FunctionReferenceLib.pack( - address(ownerPlugin), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER_OR_SELF) - ); + address[] memory modularSessionDependency = new address[](1); + modularSessionDependency[0] = address(ownerPlugin); bytes32 modularSessionKeyManifestHash = keccak256(abi.encode(modularSessionKeyPlugin.pluginManifest())); @@ -127,10 +125,8 @@ contract ModularSessionKeyPluginTest is Test { dependencies: modularSessionDependency }); - FunctionReference[] memory tokenSessionDependency = new FunctionReference[](1); - tokenSessionDependency[0] = FunctionReferenceLib.pack( - address(modularSessionKeyPlugin), uint8(IModularSessionKeyPlugin.FunctionId.VALIDATION_TEMPORARY_OWNER) - ); + address[] memory tokenSessionDependency = new address[](1); + tokenSessionDependency[0] = address(modularSessionKeyPlugin); bytes32 tokenSessionKeyManifestHash = keccak256(abi.encode(tokenSessionKeyPlugin.pluginManifest())); account.installPlugin({ @@ -237,7 +233,6 @@ contract ModularSessionKeyPluginTest is Test { abi.encodeWithSelector( UpgradeableModularAccount.RuntimeValidationFunctionReverted.selector, address(modularSessionKeyPlugin), - IModularSessionKeyPlugin.FunctionId.VALIDATION_TEMPORARY_OWNER, revertReason ) ); @@ -278,7 +273,6 @@ contract ModularSessionKeyPluginTest is Test { abi.encodeWithSelector( UpgradeableModularAccount.RuntimeValidationFunctionReverted.selector, address(modularSessionKeyPlugin), - IModularSessionKeyPlugin.FunctionId.VALIDATION_TEMPORARY_OWNER, revertReason ) ); @@ -300,7 +294,6 @@ contract ModularSessionKeyPluginTest is Test { abi.encodeWithSelector( UpgradeableModularAccount.RuntimeValidationFunctionReverted.selector, address(modularSessionKeyPlugin), - IModularSessionKeyPlugin.FunctionId.VALIDATION_TEMPORARY_OWNER, revertReason ) ); From 1fcafab27855604f4a1c43ddbd8b98cd132a5826 Mon Sep 17 00:00:00 2001 From: adam Date: Mon, 11 Mar 2024 11:43:29 -0400 Subject: [PATCH 6/8] Delete functionReference, simplify associated post hooks --- src/account/AccountLoupe.sol | 39 +- src/account/AccountStorage.sol | 9 +- src/account/PluginManagerInternals.sol | 59 +- src/account/UpgradeableModularAccount.sol | 51 +- src/helpers/FunctionReferenceLib.sol | 40 -- src/interfaces/IAccountLoupe.sol | 7 +- src/interfaces/IPlugin.sol | 6 +- src/plugins/BasePlugin.sol | 10 +- src/plugins/owner/ISingleOwnerPlugin.sol | 1 - src/plugins/owner/SingleOwnerPlugin.sol | 6 +- .../plugins/ModularSessionKeyPlugin.sol | 9 +- .../plugins/interfaces/ISessionKeyPlugin.sol | 1 - test/account/AccountExecHooks.t.sol | 41 +- test/account/AccountLoupe.t.sol | 62 +- test/account/AccountReturnData.t.sol | 1 - .../ExecuteFromPluginPermissions.t.sol | 1 - test/account/ManifestValidity.t.sol | 1 - test/account/UpgradeableModularAccount.t.sol | 1 - test/account/ValidationIntersection.t.sol | 558 +++++++++--------- test/libraries/FunctionReferenceLib.t.sol | 40 -- test/mocks/plugins/ComprehensivePlugin.sol | 28 +- .../ExecFromPluginPermissionsMocks.sol | 1 - test/mocks/plugins/ManifestValidityMocks.sol | 16 +- test/mocks/plugins/ReturnDataPluginMocks.sol | 1 - test/mocks/plugins/ValidationPluginMocks.sol | 299 ++++------ test/plugin/SingleOwnerPlugin.t.sol | 16 +- test/plugin/TokenReceiverPlugin.t.sol | 1 - .../plugins/ModularSessionKeyPlugin.t.sol | 1 - 28 files changed, 487 insertions(+), 819 deletions(-) delete mode 100644 src/helpers/FunctionReferenceLib.sol delete mode 100644 test/libraries/FunctionReferenceLib.t.sol diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol index c6cf1a91..b9135b43 100644 --- a/src/account/AccountLoupe.sol +++ b/src/account/AccountLoupe.sol @@ -45,16 +45,7 @@ abstract contract AccountLoupe is IAccountLoupe { SelectorData storage selectorData = getAccountStorage().selectorData[selector]; uint256 preExecHooksLength = selectorData.preHooks.length(); uint256 postOnlyExecHooksLength = selectorData.postOnlyHooks.length(); - uint256 maxExecHooksLength = postOnlyExecHooksLength; - - // There can only be as many associated post hooks to run as there are pre hooks. - for (uint256 i = 0; i < preExecHooksLength;) { - (, uint256 count) = selectorData.preHooks.at(i); - unchecked { - maxExecHooksLength += (count + 1); - ++i; - } - } + uint256 maxExecHooksLength = postOnlyExecHooksLength + preExecHooksLength; // Overallocate on length - not all of this may get filled up. We set the correct length later. execHooks = new ExecutionHooks[](maxExecHooksLength); @@ -64,27 +55,14 @@ abstract contract AccountLoupe is IAccountLoupe { (bytes32 key,) = selectorData.preHooks.at(i); IPlugin preExecHookPlugin = toPlugin(key); - uint256 associatedPostExecHooksLength = selectorData.associatedPostHooks[preExecHookPlugin].length(); - if (associatedPostExecHooksLength > 0) { - for (uint256 j = 0; j < associatedPostExecHooksLength;) { - execHooks[actualExecHooksLength].preExecHookPlugin = address(preExecHookPlugin); - (key,) = selectorData.associatedPostHooks[preExecHookPlugin].at(j); - execHooks[actualExecHooksLength].postExecHookPlugin = address(toPlugin(key)); - - unchecked { - ++actualExecHooksLength; - ++j; - } - } - } else { - execHooks[actualExecHooksLength].preExecHookPlugin = address(preExecHookPlugin); - - unchecked { - ++actualExecHooksLength; - } + execHooks[actualExecHooksLength].preExecHookPlugin = address(preExecHookPlugin); + + if (selectorData.hasAssociatedPostHook[preExecHookPlugin]) { + execHooks[actualExecHooksLength].postExecHookPlugin = address(preExecHookPlugin); } unchecked { + ++actualExecHooksLength; ++i; } } @@ -109,10 +87,7 @@ abstract contract AccountLoupe is IAccountLoupe { function getPreValidationHooks(bytes4 selector) external view - returns ( - address[] memory preUserOpValidationHooks, - address[] memory preRuntimeValidationHooks - ) + returns (address[] memory preUserOpValidationHooks, address[] memory preRuntimeValidationHooks) { preUserOpValidationHooks = toAddressArray(getAccountStorage().selectorData[selector].preUserOpValidationHooks); diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 0bd16f69..3a8eb0d1 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -5,7 +5,6 @@ import {EnumerableMap} from "@openzeppelin/contracts/utils/structs/EnumerableMap import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import {IPlugin} from "../interfaces/IPlugin.sol"; -import {FunctionReference} from "../interfaces/IPluginManager.sol"; // bytes = keccak256("ERC6900.UpgradeableModularAccount.Storage") bytes32 constant _ACCOUNT_STORAGE_SLOT = 0x9f09680beaa4e5c9f38841db2460c401499164f368baef687948c315d9073e40; @@ -43,7 +42,7 @@ struct SelectorData { // The execution hooks for this function selector. EnumerableMap.Bytes32ToUintMap preHooks; // bytes20 key = pre hook plugin address - mapping(IPlugin => EnumerableMap.Bytes32ToUintMap) associatedPostHooks; + mapping(IPlugin => bool) hasAssociatedPostHook; EnumerableMap.Bytes32ToUintMap postOnlyHooks; } @@ -74,7 +73,6 @@ function getPermittedCallKey(address addr, bytes4 selector) pure returns (bytes2 return bytes24(bytes20(addr)) | (bytes24(selector) >> 160); } - function toPlugin(bytes32 key) pure returns (IPlugin) { return IPlugin(address(bytes20(key))); } @@ -82,10 +80,7 @@ function toPlugin(bytes32 key) pure returns (IPlugin) { // Helper function to get all elements of a set into memory. using EnumerableMap for EnumerableMap.Bytes32ToUintMap; -function toAddressArray(EnumerableMap.Bytes32ToUintMap storage map) - view - returns (address[] memory) -{ +function toAddressArray(EnumerableMap.Bytes32ToUintMap storage map) view returns (address[] memory) { uint256 length = map.length(); address[] memory result = new address[](length); for (uint256 i = 0; i < length;) { diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index ca9ccb6d..f97946c9 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -5,7 +5,6 @@ import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165C import {EnumerableMap} from "@openzeppelin/contracts/utils/structs/EnumerableMap.sol"; import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; -import {FunctionReferenceLib} from "../helpers/FunctionReferenceLib.sol"; import { IPlugin, ManifestExecutionHook, @@ -15,20 +14,22 @@ import { ManifestExternalCallPermission, PluginManifest } from "../interfaces/IPlugin.sol"; -import {FunctionReference, IPluginManager} from "../interfaces/IPluginManager.sol"; +import {IPluginManager} from "../interfaces/IPluginManager.sol"; import { AccountStorage, getAccountStorage, SelectorData, getPermittedCallKey, - PermittedExternalCallData, - toPlugin + PermittedExternalCallData } from "./AccountStorage.sol"; abstract contract PluginManagerInternals is IPluginManager { using EnumerableMap for EnumerableMap.Bytes32ToUintMap; using EnumerableSet for EnumerableSet.AddressSet; - using FunctionReferenceLib for FunctionReference; + + IPlugin internal constant _NULL_PLUGIN = IPlugin(address(0)); + IPlugin internal constant _RUNTIME_VALIDATION_ALWAYS_ALLOW = IPlugin(address(1)); + IPlugin internal constant _PRE_HOOK_ALWAYS_DENY = IPlugin(address(2)); error ArrayLengthMismatch(); error ExecutionFunctionAlreadySet(bytes4 selector); @@ -44,10 +45,6 @@ abstract contract PluginManagerInternals is IPluginManager { error PluginNotInstalled(address plugin); error ValidationFunctionAlreadySet(bytes4 selector, IPlugin validation); - IPlugin internal constant NULL_PLUGIN = IPlugin(address(0)); - IPlugin internal constant _RUNTIME_VALIDATION_ALWAYS_ALLOW = IPlugin(address(1)); - IPlugin internal constant _PRE_HOOK_ALWAYS_DENY = IPlugin(address(2)); - modifier notNullPlugin(IPlugin plugin) { if (address(plugin) == address(0)) { revert NullPlugin(); @@ -73,41 +70,37 @@ abstract contract PluginManagerInternals is IPluginManager { _selectorData.plugin = address(0); } - function _addValidationFunction(bytes4 selector, IPlugin validation) - internal - notNullPlugin(validation) - { + function _addValidationFunction(bytes4 selector, IPlugin validation) internal notNullPlugin(validation) { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - if (_selectorData.validation != NULL_PLUGIN) { + if (_selectorData.validation != _NULL_PLUGIN) { revert ValidationFunctionAlreadySet(selector, validation); } _selectorData.validation = validation; } - function _removeValidationFunction(bytes4 selector, IPlugin validation) - internal - notNullPlugin(validation) - { + function _removeValidationFunction(bytes4 selector, IPlugin validation) internal notNullPlugin(validation) { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - _selectorData.validation = NULL_PLUGIN; + _selectorData.validation = _NULL_PLUGIN; } - function _addExecHooks(bytes4 selector, IPlugin preExecHook, IPlugin postExecHook) - internal - { + function _addExecHooks(bytes4 selector, IPlugin preExecHook, IPlugin postExecHook) internal { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - if (preExecHook != NULL_PLUGIN) { + if (preExecHook != _NULL_PLUGIN) { _addOrIncrement(_selectorData.preHooks, _toSetValue(preExecHook)); - if (postExecHook != NULL_PLUGIN) { - _addOrIncrement(_selectorData.associatedPostHooks[preExecHook], _toSetValue(postExecHook)); + if (postExecHook != _NULL_PLUGIN) { + if (preExecHook == _PRE_HOOK_ALWAYS_DENY) { + // todo: more elegant handling of this case. + revert InvalidPluginManifest(); + } + _selectorData.hasAssociatedPostHook[preExecHook] = true; } } else { - if (postExecHook == NULL_PLUGIN) { + if (postExecHook == _NULL_PLUGIN) { // both pre and post hooks cannot be null revert NullPlugin(); } @@ -116,16 +109,14 @@ abstract contract PluginManagerInternals is IPluginManager { } } - function _removeExecHooks(bytes4 selector, IPlugin preExecHook, IPlugin postExecHook) - internal - { + function _removeExecHooks(bytes4 selector, IPlugin preExecHook, IPlugin postExecHook) internal { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - if (preExecHook != NULL_PLUGIN) { + if (preExecHook != _NULL_PLUGIN) { _removeOrDecrement(_selectorData.preHooks, _toSetValue(preExecHook)); - if (postExecHook != NULL_PLUGIN) { - _removeOrDecrement(_selectorData.associatedPostHooks[preExecHook], _toSetValue(postExecHook)); + if (postExecHook != _NULL_PLUGIN) { + _selectorData.hasAssociatedPostHook[preExecHook] = false; } } else { // The case where both pre and post hooks are null was checked during installation. @@ -597,10 +588,6 @@ abstract contract PluginManagerInternals is IPluginManager { return bytes32(bytes20(address(plugin))); } - function _toFunctionReference(bytes32 setValue) internal pure returns (FunctionReference) { - return FunctionReference.wrap(bytes21(setValue)); - } - function _isEmptyOrMagicValue(IPlugin plugin) internal pure returns (bool) { return address(plugin) <= address(2); } diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 718edb1f..3b38204e 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -9,15 +9,16 @@ import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; import {EnumerableMap} from "@openzeppelin/contracts/utils/structs/EnumerableMap.sol"; import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; -import {FunctionReferenceLib} from "../helpers/FunctionReferenceLib.sol"; import {_coalescePreValidation, _coalesceValidation} from "../helpers/ValidationDataHelpers.sol"; import {IPlugin, PluginManifest} from "../interfaces/IPlugin.sol"; import {IPluginExecutor} from "../interfaces/IPluginExecutor.sol"; -import {FunctionReference, IPluginManager} from "../interfaces/IPluginManager.sol"; +import {IPluginManager} from "../interfaces/IPluginManager.sol"; import {IStandardExecutor, Call} from "../interfaces/IStandardExecutor.sol"; import {AccountExecutor} from "./AccountExecutor.sol"; import {AccountLoupe} from "./AccountLoupe.sol"; -import {AccountStorage, getAccountStorage, getPermittedCallKey, toPlugin, SelectorData} from "./AccountStorage.sol"; +import { + AccountStorage, getAccountStorage, getPermittedCallKey, toPlugin, SelectorData +} from "./AccountStorage.sol"; import {AccountStorageInitializable} from "./AccountStorageInitializable.sol"; import {PluginManagerInternals} from "./PluginManagerInternals.sol"; @@ -34,7 +35,6 @@ contract UpgradeableModularAccount is { using EnumerableMap for EnumerableMap.Bytes32ToUintMap; using EnumerableSet for EnumerableSet.Bytes32Set; - using FunctionReferenceLib for FunctionReference; struct PostExecToRun { bytes preExecHookReturnData; @@ -348,7 +348,7 @@ contract UpgradeableModularAccount is UserOperation calldata userOp, bytes32 userOpHash ) internal returns (uint256 validationData) { - if (userOpValidationPlugin == NULL_PLUGIN) { + if (userOpValidationPlugin == _NULL_PLUGIN) { revert UserOpValidationFunctionMissing(selector); } @@ -368,7 +368,9 @@ contract UpgradeableModularAccount is if (uint160(currentValidationData) > 1) { // If the aggregator is not 0 or 1, it is an unexpected value - revert UnexpectedAggregator(address(preUserOpValidationHookPlugin), address(uint160(currentValidationData))); + revert UnexpectedAggregator( + address(preUserOpValidationHookPlugin), address(uint160(currentValidationData)) + ); } validationData = _coalescePreValidation(validationData, currentValidationData); } else { @@ -384,7 +386,8 @@ contract UpgradeableModularAccount is // Run the user op validationFunction { if (!_isEmptyOrMagicValue(userOpValidationPlugin)) { - currentValidationData = IPlugin(userOpValidationPlugin).userOpValidationFunction(userOp, userOpHash); + currentValidationData = + IPlugin(userOpValidationPlugin).userOpValidationFunction(userOp, userOpHash); if (preUserOpValidationHooksLength != 0) { // If we have other validation data we need to coalesce with @@ -442,7 +445,7 @@ contract UpgradeableModularAccount is revert RuntimeValidationFunctionReverted(address(runtimeValidationPlugin), revertReason); } } else { - if (runtimeValidationPlugin == NULL_PLUGIN) { + if (runtimeValidationPlugin == _NULL_PLUGIN) { revert RuntimeValidationFunctionMissing(msg.sig); } else if (runtimeValidationPlugin == _PRE_HOOK_ALWAYS_DENY) { revert InvalidConfiguration(); @@ -459,16 +462,7 @@ contract UpgradeableModularAccount is SelectorData storage selectorData = getAccountStorage().selectorData[selector]; uint256 preExecHooksLength = selectorData.preHooks.length(); uint256 postOnlyHooksLength = selectorData.postOnlyHooks.length(); - uint256 maxPostExecHooksLength = postOnlyHooksLength; - - // There can only be as many associated post hooks to run as there are pre hooks. - for (uint256 i = 0; i < preExecHooksLength;) { - (, uint256 count) = selectorData.preHooks.at(i); - unchecked { - maxPostExecHooksLength += (count + 1); - ++i; - } - } + uint256 maxPostExecHooksLength = postOnlyHooksLength + preExecHooksLength; // Overallocate on length - not all of this may get filled up. We set the correct length later. postHooksToRun = new PostExecToRun[](maxPostExecHooksLength); @@ -498,17 +492,12 @@ contract UpgradeableModularAccount is bytes memory preExecHookReturnData = _runPreExecHook(preExecHookPlugin, data); - uint256 associatedPostExecHooksLength = selectorData.associatedPostHooks[preExecHookPlugin].length(); - if (associatedPostExecHooksLength > 0) { - for (uint256 j = 0; j < associatedPostExecHooksLength;) { - (key,) = selectorData.associatedPostHooks[preExecHookPlugin].at(j); - postHooksToRun[actualPostHooksToRunLength].postExecHookPlugin = toPlugin(key); - postHooksToRun[actualPostHooksToRunLength].preExecHookReturnData = preExecHookReturnData; - - unchecked { - ++actualPostHooksToRunLength; - ++j; - } + if (selectorData.hasAssociatedPostHook[preExecHookPlugin]) { + postHooksToRun[actualPostHooksToRunLength].postExecHookPlugin = preExecHookPlugin; + postHooksToRun[actualPostHooksToRunLength].preExecHookReturnData = preExecHookReturnData; + + unchecked { + ++actualPostHooksToRunLength; } } @@ -527,9 +516,7 @@ contract UpgradeableModularAccount is internal returns (bytes memory preExecHookReturnData) { - try preExecHookPlugin.preExecutionHook(msg.sender, msg.value, data) returns ( - bytes memory returnData - ) { + try preExecHookPlugin.preExecutionHook(msg.sender, msg.value, data) returns (bytes memory returnData) { preExecHookReturnData = returnData; } catch (bytes memory revertReason) { revert PreExecHookReverted(address(preExecHookPlugin), revertReason); diff --git a/src/helpers/FunctionReferenceLib.sol b/src/helpers/FunctionReferenceLib.sol deleted file mode 100644 index 07a0abbc..00000000 --- a/src/helpers/FunctionReferenceLib.sol +++ /dev/null @@ -1,40 +0,0 @@ -// SPDX-License-Identifier: GPL-3.0 -pragma solidity ^0.8.19; - -import {FunctionReference} from "../interfaces/IPluginManager.sol"; - -library FunctionReferenceLib { - // Empty or unset function reference. - FunctionReference internal constant _EMPTY_FUNCTION_REFERENCE = FunctionReference.wrap(bytes21(0)); - // Magic value for runtime validation functions that always allow access. - FunctionReference internal constant _RUNTIME_VALIDATION_ALWAYS_ALLOW = - FunctionReference.wrap(bytes21(uint168(1))); - // Magic value for hooks that should always revert. - FunctionReference internal constant _PRE_HOOK_ALWAYS_DENY = FunctionReference.wrap(bytes21(uint168(2))); - - function pack(address addr, uint8 functionId) internal pure returns (FunctionReference) { - return FunctionReference.wrap(bytes21(bytes20(addr)) | bytes21(uint168(functionId))); - } - - function unpack(FunctionReference fr) internal pure returns (address addr, uint8 functionId) { - bytes21 underlying = FunctionReference.unwrap(fr); - addr = address(bytes20(underlying)); - functionId = uint8(bytes1(underlying << 160)); - } - - function isEmpty(FunctionReference fr) internal pure returns (bool) { - return FunctionReference.unwrap(fr) == bytes21(0); - } - - function isEmptyOrMagicValue(FunctionReference fr) internal pure returns (bool) { - return FunctionReference.unwrap(fr) <= bytes21(uint168(2)); - } - - function eq(FunctionReference a, FunctionReference b) internal pure returns (bool) { - return FunctionReference.unwrap(a) == FunctionReference.unwrap(b); - } - - function notEq(FunctionReference a, FunctionReference b) internal pure returns (bool) { - return FunctionReference.unwrap(a) != FunctionReference.unwrap(b); - } -} diff --git a/src/interfaces/IAccountLoupe.sol b/src/interfaces/IAccountLoupe.sol index bd54b2ef..13be4c19 100644 --- a/src/interfaces/IAccountLoupe.sol +++ b/src/interfaces/IAccountLoupe.sol @@ -1,8 +1,6 @@ // SPDX-License-Identifier: CC0-1.0 pragma solidity ^0.8.19; -import {FunctionReference} from "../interfaces/IPluginManager.sol"; - interface IAccountLoupe { /// @notice Config for an execution function, given a selector. struct ExecutionFunctionConfig { @@ -35,10 +33,7 @@ interface IAccountLoupe { function getPreValidationHooks(bytes4 selector) external view - returns ( - address[] memory preUserOpValidationHooks, - address[] memory preRuntimeValidationHooks - ); + returns (address[] memory preUserOpValidationHooks, address[] memory preRuntimeValidationHooks); /// @notice Get an array of all installed plugins. /// @return The addresses of all installed plugins. diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index 8b8572a2..1bf78e0c 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -130,16 +130,14 @@ interface IPlugin { /// @param sender The caller address. /// @param value The call value. /// @param data The calldata sent. - function preRuntimeValidationHook(address sender, uint256 value, bytes calldata data) - external; + function preRuntimeValidationHook(address sender, uint256 value, bytes calldata data) external; /// @notice Run the runtime validation function. /// @dev To indicate the entire call should revert, the function MUST revert. /// @param sender The caller address. /// @param value The call value. /// @param data The calldata sent. - function runtimeValidationFunction(address sender, uint256 value, bytes calldata data) - external; + function runtimeValidationFunction(address sender, uint256 value, bytes calldata data) external; /// @notice Run the pre execution hook. /// @dev To indicate the entire call should revert, the function MUST revert. diff --git a/src/plugins/BasePlugin.sol b/src/plugins/BasePlugin.sol index dd926876..41988373 100644 --- a/src/plugins/BasePlugin.sol +++ b/src/plugins/BasePlugin.sol @@ -46,19 +46,13 @@ abstract contract BasePlugin is ERC165, IPlugin { } /// @inheritdoc IPlugin - function preRuntimeValidationHook(address sender, uint256 value, bytes calldata data) - external - virtual - { + function preRuntimeValidationHook(address sender, uint256 value, bytes calldata data) external virtual { (sender, value, data); revert NotImplemented(); } /// @inheritdoc IPlugin - function runtimeValidationFunction(address sender, uint256 value, bytes calldata data) - external - virtual - { + function runtimeValidationFunction(address sender, uint256 value, bytes calldata data) external virtual { (sender, value, data); revert NotImplemented(); } diff --git a/src/plugins/owner/ISingleOwnerPlugin.sol b/src/plugins/owner/ISingleOwnerPlugin.sol index 48b7cd43..02cb0168 100644 --- a/src/plugins/owner/ISingleOwnerPlugin.sol +++ b/src/plugins/owner/ISingleOwnerPlugin.sol @@ -4,7 +4,6 @@ pragma solidity ^0.8.19; import {UserOperation} from "@eth-infinitism/account-abstraction/interfaces/UserOperation.sol"; interface ISingleOwnerPlugin { - /// @notice This event is emitted when ownership of the account changes. /// @param account The account whose ownership changed. /// @param previousOwner The address of the previous owner. diff --git a/src/plugins/owner/SingleOwnerPlugin.sol b/src/plugins/owner/SingleOwnerPlugin.sol index 4b5d9d77..377436c1 100644 --- a/src/plugins/owner/SingleOwnerPlugin.sol +++ b/src/plugins/owner/SingleOwnerPlugin.sol @@ -103,11 +103,7 @@ contract SingleOwnerPlugin is BasePlugin, ISingleOwnerPlugin, IERC1271 { } /// @inheritdoc BasePlugin - function runtimeValidationFunction(address sender, uint256, bytes calldata) - external - view - override - { + function runtimeValidationFunction(address sender, uint256, bytes calldata) external view override { // Validate that the sender is the owner of the account or self. if (sender != _owners[msg.sender] && sender != msg.sender) { revert NotAuthorized(); diff --git a/src/samples/plugins/ModularSessionKeyPlugin.sol b/src/samples/plugins/ModularSessionKeyPlugin.sol index a6c83e9a..372653c4 100644 --- a/src/samples/plugins/ModularSessionKeyPlugin.sol +++ b/src/samples/plugins/ModularSessionKeyPlugin.sol @@ -192,8 +192,7 @@ contract ModularSessionKeyPlugin is BasePlugin, IModularSessionKeyPlugin { override returns (uint256) { - (address signer, ECDSA.RecoverError err) = - userOpHash.toEthSignedMessageHash().tryRecover(userOp.signature); + (address signer, ECDSA.RecoverError err) = userOpHash.toEthSignedMessageHash().tryRecover(userOp.signature); if (err != ECDSA.RecoverError.NoError) { revert InvalidSignature(); } @@ -208,11 +207,7 @@ contract ModularSessionKeyPlugin is BasePlugin, IModularSessionKeyPlugin { } /// @inheritdoc BasePlugin - function runtimeValidationFunction(address sender, uint256, bytes calldata data) - external - view - override - { + function runtimeValidationFunction(address sender, uint256, bytes calldata data) external view override { bytes4 selector = bytes4(data[0:4]); bytes memory key = msg.sender.allocateAssociatedStorageKey(0, 1); StoragePointer ptr = key.associatedStorageLookup(keccak256(abi.encodePacked(sender, selector))); diff --git a/src/samples/plugins/interfaces/ISessionKeyPlugin.sol b/src/samples/plugins/interfaces/ISessionKeyPlugin.sol index 412faf2e..9beacffc 100644 --- a/src/samples/plugins/interfaces/ISessionKeyPlugin.sol +++ b/src/samples/plugins/interfaces/ISessionKeyPlugin.sol @@ -4,7 +4,6 @@ pragma solidity ^0.8.19; import {UserOperation} from "@eth-infinitism/account-abstraction/interfaces/UserOperation.sol"; interface IModularSessionKeyPlugin { - /// @notice This event is emitted when a session key is added to the account. /// @param account The account whose session key is updated. /// @param sessionKey The address of the session key. diff --git a/test/account/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol index 1144a3f6..f5f71adb 100644 --- a/test/account/AccountExecHooks.t.sol +++ b/test/account/AccountExecHooks.t.sol @@ -15,7 +15,6 @@ import { import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {PluginManagerInternals} from "../../src/account/PluginManagerInternals.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; -import {FunctionReference, FunctionReferenceLib} from "../../src/helpers/FunctionReferenceLib.sol"; import {MockPlugin} from "../mocks/MockPlugin.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; @@ -70,10 +69,7 @@ contract AccountExecHooksTest is OptimizedTest { function test_preExecHook_install() public { _installPlugin1WithHooks( _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - dependencyIndex: 0 - }), + ManifestFunction({functionType: ManifestAssociatedFunctionType.SELF, dependencyIndex: 0}), ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, dependencyIndex: 0}) ); } @@ -107,14 +103,8 @@ contract AccountExecHooksTest is OptimizedTest { function test_execHookPair_install() public { _installPlugin1WithHooks( _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - dependencyIndex: 0 - }), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - dependencyIndex: 0 - }) + ManifestFunction({functionType: ManifestAssociatedFunctionType.SELF, dependencyIndex: 0}), + ManifestFunction({functionType: ManifestAssociatedFunctionType.SELF, dependencyIndex: 0}) ); } @@ -158,10 +148,7 @@ contract AccountExecHooksTest is OptimizedTest { _installPlugin1WithHooks( _EXEC_SELECTOR, ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, dependencyIndex: 0}), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - dependencyIndex: 0 - }) + ManifestFunction({functionType: ManifestAssociatedFunctionType.SELF, dependencyIndex: 0}) ); } @@ -238,14 +225,8 @@ contract AccountExecHooksTest is OptimizedTest { // Install the first plugin. _installPlugin1WithHooks( _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - dependencyIndex: 0 - }), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - dependencyIndex: 0 - }) + ManifestFunction({functionType: ManifestAssociatedFunctionType.SELF, dependencyIndex: 0}), + ManifestFunction({functionType: ManifestAssociatedFunctionType.SELF, dependencyIndex: 0}) ); // Attempt to install a second plugin that applies the first plugin's hook pair (as dependencies) to the @@ -256,14 +237,8 @@ contract AccountExecHooksTest is OptimizedTest { _installPlugin2WithHooksExpectFail( _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.DEPENDENCY, - dependencyIndex: 0 - }), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.DEPENDENCY, - dependencyIndex: 1 - }), + ManifestFunction({functionType: ManifestAssociatedFunctionType.DEPENDENCY, dependencyIndex: 0}), + ManifestFunction({functionType: ManifestAssociatedFunctionType.DEPENDENCY, dependencyIndex: 1}), dependencies, abi.encodePacked(PluginManagerInternals.InvalidPluginManifest.selector) ); diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol index aebffdf9..5ad64802 100644 --- a/test/account/AccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -5,7 +5,6 @@ import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeab import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; -import {FunctionReference, FunctionReferenceLib} from "../../src/helpers/FunctionReferenceLib.sol"; import { ManifestAssociatedFunctionType, ManifestExecutionHook, @@ -16,7 +15,6 @@ import {IAccountLoupe} from "../../src/interfaces/IAccountLoupe.sol"; import {IPlugin} from "../../src/interfaces/IPlugin.sol"; import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; import {IStandardExecutor} from "../../src/interfaces/IStandardExecutor.sol"; -import {ISingleOwnerPlugin} from "../../src/plugins/owner/ISingleOwnerPlugin.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; @@ -84,10 +82,7 @@ contract AccountLoupeTest is OptimizedTest { account1.getExecutionFunctionConfig(selectorsToCheck[i]); assertEq(config.plugin, address(account1)); - assertEq( - config.validationPlugin, - address(expectedValidations[i]) - ); + assertEq(config.validationPlugin, address(expectedValidations[i])); } } @@ -109,10 +104,7 @@ contract AccountLoupeTest is OptimizedTest { account1.getExecutionFunctionConfig(selectorsToCheck[i]); assertEq(config.plugin, expectedPluginAddress[i]); - assertEq( - config.validationPlugin, - address(expectedValidations[i]) - ); + assertEq(config.validationPlugin, address(expectedValidations[i])); } } @@ -120,14 +112,8 @@ contract AccountLoupeTest is OptimizedTest { IAccountLoupe.ExecutionHooks[] memory hooks = account1.getExecutionHooks(comprehensivePlugin.foo.selector); assertEq(hooks.length, 1); - assertEq( - hooks[0].preExecHookPlugin, - address(comprehensivePlugin) - ); - assertEq( - hooks[0].postExecHookPlugin, - address(comprehensivePlugin) - ); + assertEq(hooks[0].preExecHookPlugin, address(comprehensivePlugin)); + assertEq(hooks[0].postExecHookPlugin, address(comprehensivePlugin)); } function test_pluginLoupe_getHooks_multiple() public { @@ -139,14 +125,8 @@ contract AccountLoupeTest is OptimizedTest { mockPluginManifest.executionHooks = new ManifestExecutionHook[](1); mockPluginManifest.executionHooks[0] = ManifestExecutionHook({ executionSelector: ComprehensivePlugin.foo.selector, - preExecHook: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - dependencyIndex: 0 - }), - postExecHook: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - dependencyIndex: 0 - }) + preExecHook: ManifestFunction({functionType: ManifestAssociatedFunctionType.SELF, dependencyIndex: 0}), + postExecHook: ManifestFunction({functionType: ManifestAssociatedFunctionType.SELF, dependencyIndex: 0}) }); MockPlugin mockPlugin = new MockPlugin(mockPluginManifest); @@ -159,32 +139,17 @@ contract AccountLoupeTest is OptimizedTest { IAccountLoupe.ExecutionHooks[] memory hooks = account1.getExecutionHooks(comprehensivePlugin.foo.selector); assertEq(hooks.length, 2); - assertEq( - hooks[0].preExecHookPlugin, - address(comprehensivePlugin) - ); - assertEq( - hooks[0].postExecHookPlugin, - address(comprehensivePlugin) - ); - assertEq( - hooks[1].preExecHookPlugin, - address(mockPlugin) - ); - assertEq( - hooks[1].postExecHookPlugin, - address(mockPlugin) - ); + assertEq(hooks[0].preExecHookPlugin, address(comprehensivePlugin)); + assertEq(hooks[0].postExecHookPlugin, address(comprehensivePlugin)); + assertEq(hooks[1].preExecHookPlugin, address(mockPlugin)); + assertEq(hooks[1].postExecHookPlugin, address(mockPlugin)); } function test_pluginLoupe_getPreUserOpValidationHooks() public { (address[] memory hooks,) = account1.getPreValidationHooks(comprehensivePlugin.foo.selector); assertEq(hooks.length, 1); - assertEq( - hooks[0], - address(comprehensivePlugin) - ); + assertEq(hooks[0], address(comprehensivePlugin)); // todo: add a second hook to measure here // assertEq( // hooks[1], @@ -196,10 +161,7 @@ contract AccountLoupeTest is OptimizedTest { (, address[] memory hooks) = account1.getPreValidationHooks(comprehensivePlugin.foo.selector); assertEq(hooks.length, 1); - assertEq( - hooks[0], - address(comprehensivePlugin) - ); + assertEq(hooks[0], address(comprehensivePlugin)); // todo: add a second hook to measure here // assertEq( // hooks[1], diff --git a/test/account/AccountReturnData.t.sol b/test/account/AccountReturnData.t.sol index e110224f..4ca8a7d1 100644 --- a/test/account/AccountReturnData.t.sol +++ b/test/account/AccountReturnData.t.sol @@ -4,7 +4,6 @@ pragma solidity ^0.8.19; import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; -import {FunctionReference} from "../../src/helpers/FunctionReferenceLib.sol"; import {Call} from "../../src/interfaces/IStandardExecutor.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; diff --git a/test/account/ExecuteFromPluginPermissions.t.sol b/test/account/ExecuteFromPluginPermissions.t.sol index dcaab1f7..ba74ced3 100644 --- a/test/account/ExecuteFromPluginPermissions.t.sol +++ b/test/account/ExecuteFromPluginPermissions.t.sol @@ -6,7 +6,6 @@ import {console} from "forge-std/Test.sol"; import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; -import {FunctionReference} from "../../src/helpers/FunctionReferenceLib.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; diff --git a/test/account/ManifestValidity.t.sol b/test/account/ManifestValidity.t.sol index 0ae2a5c8..3b0ba122 100644 --- a/test/account/ManifestValidity.t.sol +++ b/test/account/ManifestValidity.t.sol @@ -6,7 +6,6 @@ import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.so import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {PluginManagerInternals} from "../../src/account/PluginManagerInternals.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; -import {FunctionReference} from "../../src/helpers/FunctionReferenceLib.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index e45b63de..94dd2d0e 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -9,7 +9,6 @@ import {UserOperation} from "@eth-infinitism/account-abstraction/interfaces/User import {PluginManagerInternals} from "../../src/account/PluginManagerInternals.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; -import {FunctionReference} from "../../src/helpers/FunctionReferenceLib.sol"; import {IPlugin, PluginManifest} from "../../src/interfaces/IPlugin.sol"; import {IAccountLoupe} from "../../src/interfaces/IAccountLoupe.sol"; import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; diff --git a/test/account/ValidationIntersection.t.sol b/test/account/ValidationIntersection.t.sol index 2cb75a64..643b07a3 100644 --- a/test/account/ValidationIntersection.t.sol +++ b/test/account/ValidationIntersection.t.sol @@ -5,297 +5,295 @@ import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.so import {UserOperation} from "@eth-infinitism/account-abstraction/interfaces/UserOperation.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; -import {FunctionReference} from "../../src/helpers/FunctionReferenceLib.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; import { - MockBaseUserOpValidationPlugin, - MockUserOpValidation1HookPlugin, - MockUserOpValidation2HookPlugin, + MockUserOpValidationWithPreHookPlugin, + MockOnlyPreUserOpValidationHookPlugin, MockUserOpValidationPlugin } from "../mocks/plugins/ValidationPluginMocks.sol"; import {OptimizedTest} from "../utils/OptimizedTest.sol"; contract ValidationIntersectionTest is OptimizedTest { -// uint256 internal constant _SIG_VALIDATION_FAILED = 1; - -// EntryPoint public entryPoint; - -// address public owner1; -// uint256 public owner1Key; -// UpgradeableModularAccount public account1; -// MockUserOpValidationPlugin public noHookPlugin; -// MockUserOpValidation1HookPlugin public oneHookPlugin; -// MockUserOpValidation2HookPlugin public twoHookPlugin; - -// function setUp() public { -// entryPoint = new EntryPoint(); -// owner1 = makeAddr("owner1"); - -// SingleOwnerPlugin singleOwnerPlugin = _deploySingleOwnerPlugin(); -// MSCAFactoryFixture factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); - -// account1 = factory.createAccount(owner1, 0); -// vm.deal(address(account1), 1 ether); - -// noHookPlugin = new MockUserOpValidationPlugin(); -// oneHookPlugin = new MockUserOpValidation1HookPlugin(); -// twoHookPlugin = new MockUserOpValidation2HookPlugin(); - -// vm.startPrank(address(owner1)); -// account1.installPlugin({ -// plugin: address(noHookPlugin), -// manifestHash: keccak256(abi.encode(noHookPlugin.pluginManifest())), -// pluginInstallData: "", -// dependencies: new FunctionReference[](0) -// }); -// account1.installPlugin({ -// plugin: address(oneHookPlugin), -// manifestHash: keccak256(abi.encode(oneHookPlugin.pluginManifest())), -// pluginInstallData: "", -// dependencies: new FunctionReference[](0) -// }); -// account1.installPlugin({ -// plugin: address(twoHookPlugin), -// manifestHash: keccak256(abi.encode(twoHookPlugin.pluginManifest())), -// pluginInstallData: "", -// dependencies: new FunctionReference[](0) -// }); -// vm.stopPrank(); -// } - -// function testFuzz_validationIntersect_single(uint256 validationData) public { -// noHookPlugin.setValidationData(validationData); - -// UserOperation memory userOp; -// userOp.callData = bytes.concat(noHookPlugin.foo.selector); -// bytes32 uoHash = entryPoint.getUserOpHash(userOp); - -// vm.prank(address(entryPoint)); -// uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); - -// assertEq(returnedValidationData, validationData); -// } - -// function test_validationIntersect_authorizer_sigfail_validationFunction() public { -// oneHookPlugin.setValidationData( -// _SIG_VALIDATION_FAILED, -// 0 // returns OK -// ); - -// UserOperation memory userOp; -// userOp.callData = bytes.concat(oneHookPlugin.bar.selector); -// bytes32 uoHash = entryPoint.getUserOpHash(userOp); - -// vm.prank(address(entryPoint)); -// uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); - -// // Down-cast to only check the authorizer -// assertEq(uint160(returnedValidationData), _SIG_VALIDATION_FAILED); -// } - -// function test_validationIntersect_authorizer_sigfail_hook() public { -// oneHookPlugin.setValidationData( -// 0, // returns OK -// _SIG_VALIDATION_FAILED -// ); - -// UserOperation memory userOp; -// userOp.callData = bytes.concat(oneHookPlugin.bar.selector); -// bytes32 uoHash = entryPoint.getUserOpHash(userOp); - -// vm.prank(address(entryPoint)); -// uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); - -// // Down-cast to only check the authorizer -// assertEq(uint160(returnedValidationData), _SIG_VALIDATION_FAILED); -// } - -// function test_validationIntersect_timeBounds_intersect_1() public { -// uint48 start1 = uint48(10); -// uint48 end1 = uint48(20); - -// uint48 start2 = uint48(15); -// uint48 end2 = uint48(25); - -// oneHookPlugin.setValidationData( -// _packValidationData(address(0), start1, end1), _packValidationData(address(0), start2, end2) -// ); - -// UserOperation memory userOp; -// userOp.callData = bytes.concat(oneHookPlugin.bar.selector); -// bytes32 uoHash = entryPoint.getUserOpHash(userOp); - -// vm.prank(address(entryPoint)); -// uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); - -// assertEq(returnedValidationData, _packValidationData(address(0), start2, end1)); -// } - -// function test_validationIntersect_timeBounds_intersect_2() public { -// uint48 start1 = uint48(10); -// uint48 end1 = uint48(20); - -// uint48 start2 = uint48(15); -// uint48 end2 = uint48(25); - -// oneHookPlugin.setValidationData( -// _packValidationData(address(0), start2, end2), _packValidationData(address(0), start1, end1) -// ); - -// UserOperation memory userOp; -// userOp.callData = bytes.concat(oneHookPlugin.bar.selector); -// bytes32 uoHash = entryPoint.getUserOpHash(userOp); - -// vm.prank(address(entryPoint)); -// uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); - -// assertEq(returnedValidationData, _packValidationData(address(0), start2, end1)); -// } + uint256 internal constant _SIG_VALIDATION_FAILED = 1; + + EntryPoint public entryPoint; + + address public owner1; + uint256 public owner1Key; + UpgradeableModularAccount public account1; + MockUserOpValidationPlugin public uoPlugin; + MockUserOpValidationWithPreHookPlugin public uoPvhPlugin; + MockOnlyPreUserOpValidationHookPlugin public pvhPlugin; + + function setUp() public { + entryPoint = new EntryPoint(); + owner1 = makeAddr("owner1"); + + SingleOwnerPlugin singleOwnerPlugin = _deploySingleOwnerPlugin(); + MSCAFactoryFixture factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); + + account1 = factory.createAccount(owner1, 0); + vm.deal(address(account1), 1 ether); + + uoPlugin = new MockUserOpValidationPlugin(); + uoPvhPlugin = new MockUserOpValidationWithPreHookPlugin(); + pvhPlugin = new MockOnlyPreUserOpValidationHookPlugin(); + + vm.startPrank(address(owner1)); + account1.installPlugin({ + plugin: address(uoPlugin), + manifestHash: keccak256(abi.encode(uoPlugin.pluginManifest())), + pluginInstallData: "", + dependencies: new address[](0) + }); + account1.installPlugin({ + plugin: address(uoPvhPlugin), + manifestHash: keccak256(abi.encode(uoPvhPlugin.pluginManifest())), + pluginInstallData: "", + dependencies: new address[](0) + }); + account1.installPlugin({ + plugin: address(pvhPlugin), + manifestHash: keccak256(abi.encode(pvhPlugin.pluginManifest())), + pluginInstallData: "", + dependencies: new address[](0) + }); + vm.stopPrank(); + } + + function testFuzz_validationIntersect_single(uint256 validationData) public { + uoPlugin.setValidationData(validationData); + + UserOperation memory userOp; + userOp.callData = bytes.concat(uoPlugin.foo.selector); + bytes32 uoHash = entryPoint.getUserOpHash(userOp); + + vm.prank(address(entryPoint)); + uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); + + assertEq(returnedValidationData, validationData); + } + + function test_validationIntersect_authorizer_sigfail_validationFunction() public { + uoPvhPlugin.setValidationData( + _SIG_VALIDATION_FAILED, + 0 // returns OK + ); + + UserOperation memory userOp; + userOp.callData = bytes.concat(uoPvhPlugin.bar.selector); + bytes32 uoHash = entryPoint.getUserOpHash(userOp); + + vm.prank(address(entryPoint)); + uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); + + // Down-cast to only check the authorizer + assertEq(uint160(returnedValidationData), _SIG_VALIDATION_FAILED); + } + + function test_validationIntersect_authorizer_sigfail_hook() public { + uoPvhPlugin.setValidationData( + 0, // returns OK + _SIG_VALIDATION_FAILED + ); + + UserOperation memory userOp; + userOp.callData = bytes.concat(uoPvhPlugin.bar.selector); + bytes32 uoHash = entryPoint.getUserOpHash(userOp); + + vm.prank(address(entryPoint)); + uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); + + // Down-cast to only check the authorizer + assertEq(uint160(returnedValidationData), _SIG_VALIDATION_FAILED); + } + + function test_validationIntersect_timeBounds_intersect_1() public { + uint48 start1 = uint48(10); + uint48 end1 = uint48(20); + + uint48 start2 = uint48(15); + uint48 end2 = uint48(25); + + uoPvhPlugin.setValidationData( + _packValidationData(address(0), start1, end1), _packValidationData(address(0), start2, end2) + ); + + UserOperation memory userOp; + userOp.callData = bytes.concat(uoPvhPlugin.bar.selector); + bytes32 uoHash = entryPoint.getUserOpHash(userOp); + + vm.prank(address(entryPoint)); + uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); + + assertEq(returnedValidationData, _packValidationData(address(0), start2, end1)); + } + + function test_validationIntersect_timeBounds_intersect_2() public { + uint48 start1 = uint48(10); + uint48 end1 = uint48(20); + + uint48 start2 = uint48(15); + uint48 end2 = uint48(25); + + uoPvhPlugin.setValidationData( + _packValidationData(address(0), start2, end2), _packValidationData(address(0), start1, end1) + ); + + UserOperation memory userOp; + userOp.callData = bytes.concat(uoPvhPlugin.bar.selector); + bytes32 uoHash = entryPoint.getUserOpHash(userOp); + + vm.prank(address(entryPoint)); + uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); -// function test_validationIntersect_revert_unexpectedAuthorizer() public { -// address badAuthorizer = makeAddr("badAuthorizer"); + assertEq(returnedValidationData, _packValidationData(address(0), start2, end1)); + } -// oneHookPlugin.setValidationData( -// 0, // returns OK -// uint256(uint160(badAuthorizer)) // returns an aggregator, which preValidation hooks are not allowed to -// // do. -// ); + function test_validationIntersect_revert_unexpectedAuthorizer() public { + address badAuthorizer = makeAddr("badAuthorizer"); -// UserOperation memory userOp; -// userOp.callData = bytes.concat(oneHookPlugin.bar.selector); -// bytes32 uoHash = entryPoint.getUserOpHash(userOp); + uoPvhPlugin.setValidationData( + 0, // returns OK + uint256(uint160(badAuthorizer)) // returns an aggregator, which preValidation hooks are not allowed to + // do. + ); -// vm.prank(address(entryPoint)); -// vm.expectRevert( -// abi.encodeWithSelector( -// UpgradeableModularAccount.UnexpectedAggregator.selector, -// address(oneHookPlugin), -// badAuthorizer -// ) -// ); -// account1.validateUserOp(userOp, uoHash, 1 wei); -// } - -// function test_validationIntersect_validAuthorizer() public { -// address goodAuthorizer = makeAddr("goodAuthorizer"); - -// oneHookPlugin.setValidationData( -// uint256(uint160(goodAuthorizer)), // returns a valid aggregator -// 0 // returns OK -// ); - -// UserOperation memory userOp; -// userOp.callData = bytes.concat(oneHookPlugin.bar.selector); -// bytes32 uoHash = entryPoint.getUserOpHash(userOp); - -// vm.prank(address(entryPoint)); -// uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); - -// assertEq(address(uint160(returnedValidationData)), goodAuthorizer); -// } - -// function test_validationIntersect_authorizerAndTimeRange() public { -// uint48 start1 = uint48(10); -// uint48 end1 = uint48(20); - -// uint48 start2 = uint48(15); -// uint48 end2 = uint48(25); - -// address goodAuthorizer = makeAddr("goodAuthorizer"); - -// oneHookPlugin.setValidationData( -// _packValidationData(goodAuthorizer, start1, end1), _packValidationData(address(0), start2, end2) -// ); - -// UserOperation memory userOp; -// userOp.callData = bytes.concat(oneHookPlugin.bar.selector); -// bytes32 uoHash = entryPoint.getUserOpHash(userOp); - -// vm.prank(address(entryPoint)); -// uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); - -// assertEq(returnedValidationData, _packValidationData(goodAuthorizer, start2, end1)); -// } - -// function test_validationIntersect_multiplePreValidationHooksIntersect() public { -// uint48 start1 = uint48(10); -// uint48 end1 = uint48(20); - -// uint48 start2 = uint48(15); -// uint48 end2 = uint48(25); - -// twoHookPlugin.setValidationData( -// 0, // returns OK -// _packValidationData(address(0), start1, end1), -// _packValidationData(address(0), start2, end2) -// ); - -// UserOperation memory userOp; -// userOp.callData = bytes.concat(twoHookPlugin.baz.selector); -// bytes32 uoHash = entryPoint.getUserOpHash(userOp); - -// vm.prank(address(entryPoint)); -// uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); - -// assertEq(returnedValidationData, _packValidationData(address(0), start2, end1)); -// } - -// function test_validationIntersect_multiplePreValidationHooksSigFail() public { -// twoHookPlugin.setValidationData( -// 0, // returns OK -// 0, // returns OK -// _SIG_VALIDATION_FAILED -// ); - -// UserOperation memory userOp; -// userOp.callData = bytes.concat(twoHookPlugin.baz.selector); - -// bytes32 uoHash = entryPoint.getUserOpHash(userOp); - -// vm.prank(address(entryPoint)); -// uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); - -// // Down-cast to only check the authorizer -// assertEq(uint160(returnedValidationData), _SIG_VALIDATION_FAILED); -// } - -// function _unpackValidationData(uint256 validationData) -// internal -// pure -// returns (address authorizer, uint48 validAfter, uint48 validUntil) -// { -// authorizer = address(uint160(validationData)); -// validUntil = uint48(validationData >> 160); -// if (validUntil == 0) { -// validUntil = type(uint48).max; -// } -// validAfter = uint48(validationData >> (48 + 160)); -// } - -// function _packValidationData(address authorizer, uint48 validAfter, uint48 validUntil) -// internal -// pure -// returns (uint256) -// { -// return uint160(authorizer) | (uint256(validUntil) << 160) | (uint256(validAfter) << (160 + 48)); -// } - -// function _intersectTimeRange(uint48 validafter1, uint48 validuntil1, uint48 validafter2, uint48 validuntil2) -// internal -// pure -// returns (uint48 validAfter, uint48 validUntil) -// { -// if (validafter1 < validafter2) { -// validAfter = validafter2; -// } else { -// validAfter = validafter1; -// } -// if (validuntil1 > validuntil2) { -// validUntil = validuntil2; -// } else { -// validUntil = validuntil1; -// } -// } + UserOperation memory userOp; + userOp.callData = bytes.concat(uoPvhPlugin.bar.selector); + bytes32 uoHash = entryPoint.getUserOpHash(userOp); + + vm.prank(address(entryPoint)); + vm.expectRevert( + abi.encodeWithSelector( + UpgradeableModularAccount.UnexpectedAggregator.selector, address(uoPvhPlugin), badAuthorizer + ) + ); + account1.validateUserOp(userOp, uoHash, 1 wei); + } + + function test_validationIntersect_validAuthorizer() public { + address goodAuthorizer = makeAddr("goodAuthorizer"); + + uoPvhPlugin.setValidationData( + uint256(uint160(goodAuthorizer)), // returns a valid aggregator + 0 // returns OK + ); + + UserOperation memory userOp; + userOp.callData = bytes.concat(uoPvhPlugin.bar.selector); + bytes32 uoHash = entryPoint.getUserOpHash(userOp); + + vm.prank(address(entryPoint)); + uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); + + assertEq(address(uint160(returnedValidationData)), goodAuthorizer); + } + + function test_validationIntersect_authorizerAndTimeRange() public { + uint48 start1 = uint48(10); + uint48 end1 = uint48(20); + + uint48 start2 = uint48(15); + uint48 end2 = uint48(25); + + address goodAuthorizer = makeAddr("goodAuthorizer"); + + uoPvhPlugin.setValidationData( + _packValidationData(goodAuthorizer, start1, end1), _packValidationData(address(0), start2, end2) + ); + + UserOperation memory userOp; + userOp.callData = bytes.concat(uoPvhPlugin.bar.selector); + bytes32 uoHash = entryPoint.getUserOpHash(userOp); + + vm.prank(address(entryPoint)); + uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); + + assertEq(returnedValidationData, _packValidationData(goodAuthorizer, start2, end1)); + } + + function test_validationIntersect_multiplePreValidationHooksIntersect() public { + uint48 start1 = uint48(10); + uint48 end1 = uint48(20); + + uint48 start2 = uint48(15); + uint48 end2 = uint48(25); + + uoPvhPlugin.setValidationData( + 0, // returns OK + _packValidationData(address(0), start1, end1) + ); + + pvhPlugin.setValidationData(_packValidationData(address(0), start2, end2)); + + UserOperation memory userOp; + userOp.callData = bytes.concat(uoPvhPlugin.bar.selector); + bytes32 uoHash = entryPoint.getUserOpHash(userOp); + + vm.prank(address(entryPoint)); + uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); + + assertEq(returnedValidationData, _packValidationData(address(0), start2, end1)); + } + + function test_validationIntersect_multiplePreValidationHooksSigFail() public { + uoPvhPlugin.setValidationData( + 0, // returns OK + 0 // returns OK + ); + + pvhPlugin.setValidationData(_SIG_VALIDATION_FAILED); + + UserOperation memory userOp; + userOp.callData = bytes.concat(uoPvhPlugin.bar.selector); + + bytes32 uoHash = entryPoint.getUserOpHash(userOp); + + vm.prank(address(entryPoint)); + uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei); + + // Down-cast to only check the authorizer + assertEq(uint160(returnedValidationData), _SIG_VALIDATION_FAILED); + } + + function _unpackValidationData(uint256 validationData) + internal + pure + returns (address authorizer, uint48 validAfter, uint48 validUntil) + { + authorizer = address(uint160(validationData)); + validUntil = uint48(validationData >> 160); + if (validUntil == 0) { + validUntil = type(uint48).max; + } + validAfter = uint48(validationData >> (48 + 160)); + } + + function _packValidationData(address authorizer, uint48 validAfter, uint48 validUntil) + internal + pure + returns (uint256) + { + return uint160(authorizer) | (uint256(validUntil) << 160) | (uint256(validAfter) << (160 + 48)); + } + + function _intersectTimeRange(uint48 validafter1, uint48 validuntil1, uint48 validafter2, uint48 validuntil2) + internal + pure + returns (uint48 validAfter, uint48 validUntil) + { + if (validafter1 < validafter2) { + validAfter = validafter2; + } else { + validAfter = validafter1; + } + if (validuntil1 > validuntil2) { + validUntil = validuntil2; + } else { + validUntil = validuntil1; + } + } } diff --git a/test/libraries/FunctionReferenceLib.t.sol b/test/libraries/FunctionReferenceLib.t.sol deleted file mode 100644 index 6471fbd0..00000000 --- a/test/libraries/FunctionReferenceLib.t.sol +++ /dev/null @@ -1,40 +0,0 @@ -// SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.19; - -import {Test} from "forge-std/Test.sol"; - -import {FunctionReferenceLib} from "../../src/helpers/FunctionReferenceLib.sol"; -import {FunctionReference} from "../../src/interfaces/IPluginManager.sol"; - -contract FunctionReferenceLibTest is Test { - using FunctionReferenceLib for FunctionReference; - - function testFuzz_functionReference_packing(address addr, uint8 functionId) public { - // console.log("addr: ", addr); - // console.log("functionId: ", vm.toString(functionId)); - FunctionReference fr = FunctionReferenceLib.pack(addr, functionId); - // console.log("packed: ", vm.toString(FunctionReference.unwrap(fr))); - (address addr2, uint8 functionId2) = FunctionReferenceLib.unpack(fr); - // console.log("addr2: ", addr2); - // console.log("functionId2: ", vm.toString(functionId2)); - assertEq(addr, addr2); - assertEq(functionId, functionId2); - } - - function testFuzz_functionReference_operators(FunctionReference a, FunctionReference b) public { - assertTrue(a.eq(a)); - assertTrue(b.eq(b)); - - if (FunctionReference.unwrap(a) == FunctionReference.unwrap(b)) { - assertTrue(a.eq(b)); - assertTrue(b.eq(a)); - assertFalse(a.notEq(b)); - assertFalse(b.notEq(a)); - } else { - assertTrue(a.notEq(b)); - assertTrue(b.notEq(a)); - assertFalse(a.eq(b)); - assertFalse(b.eq(a)); - } - } -} diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index 9af70618..10cfca04 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -46,12 +46,7 @@ contract ComprehensivePlugin is BasePlugin { function onUninstall(bytes calldata) external override {} - function preUserOpValidationHook(UserOperation calldata, bytes32) - external - pure - override - returns (uint256) - { + function preUserOpValidationHook(UserOperation calldata, bytes32) external pure override returns (uint256) { // if (functionId == uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_1)) { // return 0; // } else if (functionId == uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_2)) { @@ -62,12 +57,7 @@ contract ComprehensivePlugin is BasePlugin { return 0; } - function userOpValidationFunction(UserOperation calldata, bytes32) - external - pure - override - returns (uint256) - { + function userOpValidationFunction(UserOperation calldata, bytes32) external pure override returns (uint256) { return 0; } @@ -82,20 +72,11 @@ contract ComprehensivePlugin is BasePlugin { return; } - function runtimeValidationFunction(address, uint256, bytes calldata) - external - pure - override - { + function runtimeValidationFunction(address, uint256, bytes calldata) external pure override { return; } - function preExecutionHook(address, uint256, bytes calldata) - external - pure - override - returns (bytes memory) - { + function preExecutionHook(address, uint256, bytes calldata) external pure override returns (bytes memory) { // if (functionId == uint8(FunctionId.PRE_EXECUTION_HOOK)) { // return ""; // } else if (functionId == uint8(FunctionId.PRE_PERMITTED_CALL_EXECUTION_HOOK)) { @@ -115,7 +96,6 @@ contract ComprehensivePlugin is BasePlugin { // revert NotImplemented(); // Todo: is there a logic step missing here, with the two different hooks? return; - } function pluginManifest() external pure override returns (PluginManifest memory) { diff --git a/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol b/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol index 2015702c..8125cb76 100644 --- a/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol +++ b/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol @@ -1,7 +1,6 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; -import {FunctionReference} from "../../../src/helpers/FunctionReferenceLib.sol"; import { ManifestFunction, ManifestAssociatedFunctionType, diff --git a/test/mocks/plugins/ManifestValidityMocks.sol b/test/mocks/plugins/ManifestValidityMocks.sol index 18a962cf..b0a9c73d 100644 --- a/test/mocks/plugins/ManifestValidityMocks.sol +++ b/test/mocks/plugins/ManifestValidityMocks.sol @@ -1,7 +1,6 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; -import {FunctionReference} from "../../../src/helpers/FunctionReferenceLib.sol"; import { ManifestFunction, ManifestAssociatedFunctionType, @@ -116,10 +115,7 @@ contract BadValidationMagicValue_PreExecHook_Plugin is BaseTestPlugin { functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, dependencyIndex: 0 }), - postExecHook: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - dependencyIndex: 0 - }) + postExecHook: ManifestFunction({functionType: ManifestAssociatedFunctionType.SELF, dependencyIndex: 0}) }); return manifest; @@ -145,10 +141,7 @@ contract BadValidationMagicValue_PostExecHook_Plugin is BaseTestPlugin { // Illegal assignment: validation always allow only usable on runtime validation functions manifest.executionHooks[0] = ManifestExecutionHook({ executionSelector: this.foo.selector, - preExecHook: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - dependencyIndex: 0 - }), + preExecHook: ManifestFunction({functionType: ManifestAssociatedFunctionType.SELF, dependencyIndex: 0}), postExecHook: ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, dependencyIndex: 0 @@ -234,10 +227,7 @@ contract BadHookMagicValue_PostExecHook_Plugin is BaseTestPlugin { // Illegal assignment: hook always deny only usable on runtime validation functions manifest.executionHooks[0] = ManifestExecutionHook({ executionSelector: this.foo.selector, - preExecHook: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - dependencyIndex: 0 - }), + preExecHook: ManifestFunction({functionType: ManifestAssociatedFunctionType.SELF, dependencyIndex: 0}), postExecHook: ManifestFunction({ functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, dependencyIndex: 0 diff --git a/test/mocks/plugins/ReturnDataPluginMocks.sol b/test/mocks/plugins/ReturnDataPluginMocks.sol index 132b7df4..b92c3f39 100644 --- a/test/mocks/plugins/ReturnDataPluginMocks.sol +++ b/test/mocks/plugins/ReturnDataPluginMocks.sol @@ -1,7 +1,6 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; -import {FunctionReference} from "../../../src/helpers/FunctionReferenceLib.sol"; import { ManifestFunction, ManifestAssociatedFunctionType, diff --git a/test/mocks/plugins/ValidationPluginMocks.sol b/test/mocks/plugins/ValidationPluginMocks.sol index e413c5a1..c0f30a84 100644 --- a/test/mocks/plugins/ValidationPluginMocks.sol +++ b/test/mocks/plugins/ValidationPluginMocks.sol @@ -12,191 +12,130 @@ import { import {BaseTestPlugin} from "./BaseTestPlugin.sol"; abstract contract MockBaseUserOpValidationPlugin is BaseTestPlugin { - // enum FunctionId { - // USER_OP_VALIDATION, - // PRE_USER_OP_VALIDATION_HOOK_1, - // PRE_USER_OP_VALIDATION_HOOK_2 - // } - - // uint256 internal _userOpValidationFunctionData; - // uint256 internal _preUserOpValidationHook1Data; - // uint256 internal _preUserOpValidationHook2Data; - - // // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ - // // ┃ Plugin interface functions ┃ - // // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - - // function onInstall(bytes calldata) external override {} - - // function onUninstall(bytes calldata) external override {} - - // function preUserOpValidationHook(uint8 functionId, UserOperation calldata, bytes32) - // external - // view - // override - // returns (uint256) - // { - // if (functionId == uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_1)) { - // return _preUserOpValidationHook1Data; - // } else if (functionId == uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_2)) { - // return _preUserOpValidationHook2Data; - // } - // revert NotImplemented(); - // } - - // function userOpValidationFunction(uint8 functionId, UserOperation calldata, bytes32) - // external - // view - // override - // returns (uint256) - // { - // if (functionId == uint8(FunctionId.USER_OP_VALIDATION)) { - // return _userOpValidationFunctionData; - // } - // revert NotImplemented(); - // } + uint256 internal _userOpValidationFunctionData; + uint256 internal _preUserOpValidationHookData; + + // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ + // ┃ Plugin interface functions ┃ + // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + + function onInstall(bytes calldata) external override {} + + function onUninstall(bytes calldata) external override {} + + function preUserOpValidationHook(UserOperation calldata, bytes32) external view override returns (uint256) { + // todo: is there a test case we don't cover by not having multiple hooks? + return _preUserOpValidationHookData; + } + + function userOpValidationFunction(UserOperation calldata, bytes32) external view override returns (uint256) { + return _userOpValidationFunctionData; + } } contract MockUserOpValidationPlugin is MockBaseUserOpValidationPlugin { - // function setValidationData(uint256 userOpValidationFunctionData) external { - // _userOpValidationFunctionData = userOpValidationFunctionData; - // } - - // // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ - // // ┃ Execution functions ┃ - // // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - - // function foo() external {} - - // // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ - // // ┃ Plugin interface functions ┃ - // // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - - // function pluginManifest() external pure override returns (PluginManifest memory) { - // PluginManifest memory manifest; - - // manifest.executionFunctions = new bytes4[](1); - // manifest.executionFunctions[0] = this.foo.selector; - - // manifest.validationFunctions = new ManifestAssociatedFunction[](1); - // manifest.validationFunctions[0] = ManifestAssociatedFunction({ - // executionSelector: this.foo.selector, - // associatedFunction: ManifestFunction({ - // functionType: ManifestAssociatedFunctionType.SELF, - // functionId: uint8(FunctionId.USER_OP_VALIDATION), - // dependencyIndex: 0 // Unused. - // }) - // }); - - // return manifest; - // } + function setValidationData(uint256 userOpValidationFunctionData) external { + _userOpValidationFunctionData = userOpValidationFunctionData; + } + + // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ + // ┃ Execution functions ┃ + // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + + function foo() external {} + + // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ + // ┃ Plugin interface functions ┃ + // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + + function pluginManifest() external pure override returns (PluginManifest memory) { + PluginManifest memory manifest; + + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.foo.selector; + + manifest.validationFunctions = new ManifestAssociatedFunction[](1); + manifest.validationFunctions[0] = ManifestAssociatedFunction({ + executionSelector: this.foo.selector, + associatedFunction: ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + dependencyIndex: 0 // Unused. + }) + }); + + return manifest; + } } -contract MockUserOpValidation1HookPlugin is MockBaseUserOpValidationPlugin { -// function setValidationData(uint256 userOpValidationFunctionData, uint256 preUserOpValidationHook1Data) -// external -// { -// _userOpValidationFunctionData = userOpValidationFunctionData; -// _preUserOpValidationHook1Data = preUserOpValidationHook1Data; -// } - -// // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ -// // ┃ Execution functions ┃ -// // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - -// function bar() external {} - -// // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ -// // ┃ Plugin interface functions ┃ -// // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - -// function pluginManifest() external pure override returns (PluginManifest memory) { -// PluginManifest memory manifest; - -// manifest.executionFunctions = new bytes4[](1); -// manifest.executionFunctions[0] = this.bar.selector; - -// ManifestFunction memory userOpValidationFunctionRef = ManifestFunction({ -// functionType: ManifestAssociatedFunctionType.SELF, -// functionId: uint8(FunctionId.USER_OP_VALIDATION), -// dependencyIndex: 0 // Unused. -// }); -// manifest.validationFunctions = new ManifestAssociatedFunction[](1); -// manifest.validationFunctions[0] = ManifestAssociatedFunction({ -// executionSelector: this.bar.selector, -// associatedFunction: userOpValidationFunctionRef -// }); - -// manifest.preUserOpValidationHooks = new ManifestAssociatedFunction[](1); -// manifest.preUserOpValidationHooks[0] = ManifestAssociatedFunction({ -// executionSelector: this.bar.selector, -// associatedFunction: ManifestFunction({ -// functionType: ManifestAssociatedFunctionType.SELF, -// functionId: uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_1), -// dependencyIndex: 0 // Unused. -// }) -// }); - -// return manifest; -// } +contract MockUserOpValidationWithPreHookPlugin is MockBaseUserOpValidationPlugin { + function setValidationData(uint256 userOpValidationFunctionData, uint256 preUserOpValidationHookData) + external + { + _userOpValidationFunctionData = userOpValidationFunctionData; + _preUserOpValidationHookData = preUserOpValidationHookData; + } + + // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ + // ┃ Execution functions ┃ + // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + + function bar() external {} + + // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ + // ┃ Plugin interface functions ┃ + // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + + function pluginManifest() external pure override returns (PluginManifest memory) { + PluginManifest memory manifest; + + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.bar.selector; + + ManifestFunction memory userOpValidationFunctionRef = ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + dependencyIndex: 0 // Unused. + }); + manifest.validationFunctions = new ManifestAssociatedFunction[](1); + manifest.validationFunctions[0] = ManifestAssociatedFunction({ + executionSelector: this.bar.selector, + associatedFunction: userOpValidationFunctionRef + }); + + manifest.preUserOpValidationHooks = new ManifestAssociatedFunction[](1); + manifest.preUserOpValidationHooks[0] = ManifestAssociatedFunction({ + executionSelector: this.bar.selector, + associatedFunction: ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + dependencyIndex: 0 // Unused. + }) + }); + + return manifest; + } } -contract MockUserOpValidation2HookPlugin is MockBaseUserOpValidationPlugin { -// function setValidationData( -// uint256 userOpValidationFunctionData, -// uint256 preUserOpValidationHook1Data, -// uint256 preUserOpValidationHook2Data -// ) external { -// _userOpValidationFunctionData = userOpValidationFunctionData; -// _preUserOpValidationHook1Data = preUserOpValidationHook1Data; -// _preUserOpValidationHook2Data = preUserOpValidationHook2Data; -// } - -// // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ -// // ┃ Execution functions ┃ -// // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - -// function baz() external {} - -// // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ -// // ┃ Plugin interface functions ┃ -// // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - -// function pluginManifest() external pure override returns (PluginManifest memory) { -// PluginManifest memory manifest; - -// manifest.executionFunctions = new bytes4[](1); -// manifest.executionFunctions[0] = this.baz.selector; - -// ManifestFunction memory userOpValidationFunctionRef = ManifestFunction({ -// functionType: ManifestAssociatedFunctionType.SELF, -// functionId: uint8(FunctionId.USER_OP_VALIDATION), -// dependencyIndex: 0 // Unused. -// }); -// manifest.validationFunctions = new ManifestAssociatedFunction[](1); -// manifest.validationFunctions[0] = ManifestAssociatedFunction({ -// executionSelector: this.baz.selector, -// associatedFunction: userOpValidationFunctionRef -// }); - -// manifest.preUserOpValidationHooks = new ManifestAssociatedFunction[](2); -// manifest.preUserOpValidationHooks[0] = ManifestAssociatedFunction({ -// executionSelector: this.baz.selector, -// associatedFunction: ManifestFunction({ -// functionType: ManifestAssociatedFunctionType.SELF, -// functionId: uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_1), -// dependencyIndex: 0 // Unused. -// }) -// }); -// manifest.preUserOpValidationHooks[1] = ManifestAssociatedFunction({ -// executionSelector: this.baz.selector, -// associatedFunction: ManifestFunction({ -// functionType: ManifestAssociatedFunctionType.SELF, -// functionId: uint8(FunctionId.PRE_USER_OP_VALIDATION_HOOK_2), -// dependencyIndex: 0 // Unused. -// }) -// }); - -// return manifest; -// } +// Applies a second pre validation hook over the `bar()` function from MockUserOpValidationWithPreHookPlugin. +contract MockOnlyPreUserOpValidationHookPlugin is MockBaseUserOpValidationPlugin { + function setValidationData(uint256 preUserOpValidationHookData) external { + _preUserOpValidationHookData = preUserOpValidationHookData; + } + + // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ + // ┃ Plugin interface functions ┃ + // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + + function pluginManifest() external pure override returns (PluginManifest memory) { + PluginManifest memory manifest; + + manifest.preUserOpValidationHooks = new ManifestAssociatedFunction[](1); + manifest.preUserOpValidationHooks[0] = ManifestAssociatedFunction({ + executionSelector: MockUserOpValidationWithPreHookPlugin.bar.selector, + associatedFunction: ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + dependencyIndex: 0 // Unused. + }) + }); + + return manifest; + } } diff --git a/test/plugin/SingleOwnerPlugin.t.sol b/test/plugin/SingleOwnerPlugin.t.sol index 3bcf859c..1306425f 100644 --- a/test/plugin/SingleOwnerPlugin.t.sol +++ b/test/plugin/SingleOwnerPlugin.t.sol @@ -112,15 +112,11 @@ contract SingleOwnerPluginTest is OptimizedTest { assertEq(address(0), plugin.owner()); plugin.transferOwnership(owner1); assertEq(owner1, plugin.owner()); - plugin.runtimeValidationFunction( - owner1, 0, "" - ); + plugin.runtimeValidationFunction(owner1, 0, ""); vm.startPrank(b); vm.expectRevert(ISingleOwnerPlugin.NotAuthorized.selector); - plugin.runtimeValidationFunction( - owner1, 0, "" - ); + plugin.runtimeValidationFunction(owner1, 0, ""); } function testFuzz_validateUserOpSig(string memory salt, UserOperation memory userOp) public { @@ -135,9 +131,7 @@ contract SingleOwnerPluginTest is OptimizedTest { userOp.signature = abi.encodePacked(r, s, v); // sig check should fail - uint256 success = plugin.userOpValidationFunction( - userOp, userOpHash - ); + uint256 success = plugin.userOpValidationFunction(userOp, userOpHash); assertEq(success, 1); // transfer ownership to signer @@ -145,9 +139,7 @@ contract SingleOwnerPluginTest is OptimizedTest { assertEq(signer, plugin.owner()); // sig check should pass - success = plugin.userOpValidationFunction( - userOp, userOpHash - ); + success = plugin.userOpValidationFunction(userOp, userOpHash); assertEq(success, 0); } diff --git a/test/plugin/TokenReceiverPlugin.t.sol b/test/plugin/TokenReceiverPlugin.t.sol index 215422f5..f31a6f1a 100644 --- a/test/plugin/TokenReceiverPlugin.t.sol +++ b/test/plugin/TokenReceiverPlugin.t.sol @@ -9,7 +9,6 @@ import {IERC777Recipient} from "@openzeppelin/contracts/token/ERC777/IERC777Reci import {IERC1155Receiver} from "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; -import {FunctionReference} from "../../src/helpers/FunctionReferenceLib.sol"; import {TokenReceiverPlugin} from "../../src/plugins/TokenReceiverPlugin.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; diff --git a/test/samples/plugins/ModularSessionKeyPlugin.t.sol b/test/samples/plugins/ModularSessionKeyPlugin.t.sol index 7038c3d3..fc65b462 100644 --- a/test/samples/plugins/ModularSessionKeyPlugin.t.sol +++ b/test/samples/plugins/ModularSessionKeyPlugin.t.sol @@ -16,7 +16,6 @@ import {ITokenSessionKeyPlugin} from "../../../src/samples/plugins/interfaces/IT import {UpgradeableModularAccount} from "../../../src/account/UpgradeableModularAccount.sol"; import {MSCAFactoryFixture} from "../../mocks/MSCAFactoryFixture.sol"; -import {FunctionReference, FunctionReferenceLib} from "../../../src/helpers/FunctionReferenceLib.sol"; import {IPluginManager} from "../../../src/interfaces/IPluginManager.sol"; import {MockERC20} from "../../mocks/MockERC20.sol"; From 29e39d53bf9821a4367179859e73f162b5616fa0 Mon Sep 17 00:00:00 2001 From: adam Date: Mon, 11 Mar 2024 11:50:46 -0400 Subject: [PATCH 7/8] Update ERC --- src/interfaces/IPlugin.sol | 2 +- standard/ERCs/erc-6900.md | 55 +++++++++++++------------------------- 2 files changed, 19 insertions(+), 38 deletions(-) diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index 1bf78e0c..666694d4 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -65,7 +65,7 @@ struct PluginMetadata { // The author field SHOULD be a username representing the identity of the user or organization // that created this plugin. string author; - // String desciptions of the relative sensitivity of specific functions. The selectors MUST be selectors for + // String descriptions of the relative sensitivity of specific functions. The selectors MUST be selectors for // functions implemented by this plugin. SelectorPermission[] permissionDescriptors; } diff --git a/standard/ERCs/erc-6900.md b/standard/ERCs/erc-6900.md index ce775c15..3115505f 100644 --- a/standard/ERCs/erc-6900.md +++ b/standard/ERCs/erc-6900.md @@ -105,11 +105,8 @@ Each step is modular, supporting different implementations for each execution fu Plugin manager interface. Modular Smart Contract Accounts **MUST** implement this interface to support installing and uninstalling plugins. ```solidity -// Treats the first 20 bytes as an address, and the last byte as a function identifier. -type FunctionReference is bytes21; - interface IPluginManager { - event PluginInstalled(address indexed plugin, bytes32 manifestHash, FunctionReference[] dependencies); + event PluginInstalled(address indexed plugin, bytes32 manifestHash, address[] dependencies); event PluginUninstalled(address indexed plugin, bool indexed onUninstallSucceeded); @@ -124,7 +121,7 @@ interface IPluginManager { address plugin, bytes32 manifestHash, bytes calldata pluginInstallData, - FunctionReference[] calldata dependencies + address[] calldata dependencies ) external; /// @notice Uninstall a plugin from the modular account. @@ -135,7 +132,6 @@ interface IPluginManager { /// modular account. function uninstallPlugin(address plugin, bytes calldata config, bytes calldata pluginUninstallData) external; } - ``` #### `IStandardExecutor.sol` @@ -215,14 +211,14 @@ interface IAccountLoupe { /// @notice Config for an execution function, given a selector. struct ExecutionFunctionConfig { address plugin; - FunctionReference validationFunction; + address validationPlugin; } /// @notice Pre and post hooks for a given selector. /// @dev It's possible for one of either `preExecHook` or `postExecHook` to be empty. struct ExecutionHooks { - FunctionReference preExecHook; - FunctionReference postExecHook; + address preExecHookPlugin; + address postExecHookPlugin; } /// @notice Get the validation functions and plugin address for a selector. @@ -243,10 +239,7 @@ interface IAccountLoupe { function getPreValidationHooks(bytes4 selector) external view - returns ( - FunctionReference[] memory preUserOpValidationHooks, - FunctionReference[] memory preRuntimeValidationHooks - ); + returns (address[] memory preUserOpValidationHooks, address[] memory preRuntimeValidationHooks); /// @notice Get an array of all installed plugins. /// @return The addresses of all installed plugins. @@ -270,56 +263,45 @@ interface IPlugin { /// @param data Optional bytes array to be decoded and used by the plugin to clear plugin data for the modular account. function onUninstall(bytes calldata data) external; - /// @notice Run the pre user operation validation hook specified by the `functionId`. + /// @notice Run the pre user operation validation hook. /// @dev Pre user operation validation hooks MUST NOT return an authorizer value other than 0 or 1. - /// @param functionId An identifier that routes the call to different internal implementations, should there be more than one. /// @param userOp The user operation. /// @param userOpHash The user operation hash. /// @return Packed validation data for validAfter (6 bytes), validUntil (6 bytes), and authorizer (20 bytes). - function preUserOpValidationHook(uint8 functionId, UserOperation memory userOp, bytes32 userOpHash) external returns (uint256); + function preUserOpValidationHook(UserOperation calldata userOp, bytes32 userOpHash) external returns (uint256); - /// @notice Run the user operation validationFunction specified by the `functionId`. - /// @param functionId An identifier that routes the call to different internal implementations, should there be - /// more than one. + /// @notice Run the user operation validation function. /// @param userOp The user operation. /// @param userOpHash The user operation hash. /// @return Packed validation data for validAfter (6 bytes), validUntil (6 bytes), and authorizer (20 bytes). - function userOpValidationFunction(uint8 functionId, UserOperation calldata userOp, bytes32 userOpHash) - external - returns (uint256); + function userOpValidationFunction(UserOperation calldata userOp, bytes32 userOpHash) external returns (uint256); - /// @notice Run the pre runtime validation hook specified by the `functionId`. + /// @notice Run the pre runtime validation hook. /// @dev To indicate the entire call should revert, the function MUST revert. - /// @param functionId An identifier that routes the call to different internal implementations, should there be more than one. /// @param sender The caller address. /// @param value The call value. /// @param data The calldata sent. - function preRuntimeValidationHook(uint8 functionId, address sender, uint256 value, bytes calldata data) external; + function preRuntimeValidationHook(address sender, uint256 value, bytes calldata data) external; - /// @notice Run the runtime validationFunction specified by the `functionId`. + /// @notice Run the runtime validation function. /// @dev To indicate the entire call should revert, the function MUST revert. - /// @param functionId An identifier that routes the call to different internal implementations, should there be - /// more than one. /// @param sender The caller address. /// @param value The call value. /// @param data The calldata sent. - function runtimeValidationFunction(uint8 functionId, address sender, uint256 value, bytes calldata data) - external; + function runtimeValidationFunction(address sender, uint256 value, bytes calldata data) external; - /// @notice Run the pre execution hook specified by the `functionId`. + /// @notice Run the pre execution hook. /// @dev To indicate the entire call should revert, the function MUST revert. - /// @param functionId An identifier that routes the call to different internal implementations, should there be more than one. /// @param sender The caller address. /// @param value The call value. /// @param data The calldata sent. /// @return Context to pass to a post execution hook, if present. An empty bytes array MAY be returned. - function preExecutionHook(uint8 functionId, address sender, uint256 value, bytes calldata data) external returns (bytes memory); + function preExecutionHook(address sender, uint256 value, bytes calldata data) external returns (bytes memory); - /// @notice Run the post execution hook specified by the `functionId`. + /// @notice Run the post execution hook. /// @dev To indicate the entire call should revert, the function MUST revert. - /// @param functionId An identifier that routes the call to different internal implementations, should there be more than one. /// @param preExecHookData The context returned by its associated pre execution hook. - function postExecutionHook(uint8 functionId, bytes calldata preExecHookData) external; + function postExecutionHook(bytes calldata preExecHookData) external; /// @notice Describe the contents and intended configuration of the plugin. /// @dev This manifest MUST stay constant over time. @@ -363,7 +345,6 @@ enum ManifestAssociatedFunctionType { /// of the function at `dependencies[dependencyIndex]` during the call to `installPlugin(config)`. struct ManifestFunction { ManifestAssociatedFunctionType functionType; - uint8 functionId; uint256 dependencyIndex; } From d0e3b7ec1d7234d06084f025c42b12cab2b4b0cc Mon Sep 17 00:00:00 2001 From: adam Date: Mon, 11 Mar 2024 13:36:29 -0400 Subject: [PATCH 8/8] Update validation function names --- src/account/UpgradeableModularAccount.sol | 17 ++++++++--------- src/interfaces/IPlugin.sol | 6 ++---- src/plugins/BasePlugin.sol | 4 ++-- src/plugins/owner/SingleOwnerPlugin.sol | 4 ++-- src/samples/plugins/ModularSessionKeyPlugin.sol | 4 ++-- standard/ERCs/erc-6900.md | 4 ++-- test/account/ManifestValidity.t.sol | 14 ++++++-------- test/mocks/MockPlugin.sol | 3 +-- test/mocks/plugins/ComprehensivePlugin.sol | 4 ++-- test/mocks/plugins/ManifestValidityMocks.sol | 4 ++-- test/mocks/plugins/ValidationPluginMocks.sol | 2 +- test/plugin/SingleOwnerPlugin.t.sol | 8 ++++---- .../plugins/ModularSessionKeyPlugin.t.sol | 6 +++--- 13 files changed, 37 insertions(+), 43 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 3b38204e..1fb60f13 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -58,11 +58,11 @@ contract UpgradeableModularAccount is error PostExecHookReverted(address plugin, bytes revertReason); error PreExecHookReverted(address plugin, bytes revertReason); error PreRuntimeValidationHookFailed(address plugin, bytes revertReason); - error RuntimeValidationFunctionMissing(bytes4 selector); - error RuntimeValidationFunctionReverted(address plugin, bytes revertReason); + error RuntimeValidationMissing(bytes4 selector); + error RuntimeValidationReverted(address plugin, bytes revertReason); error UnexpectedAggregator(address plugin, address aggregator); error UnrecognizedFunction(bytes4 selector); - error UserOpValidationFunctionMissing(bytes4 selector); + error UserOpValidationMissing(bytes4 selector); // Wraps execution of a native function with runtime validation and hooks // Used for upgradeTo, upgradeToAndCall, execute, executeBatch, installPlugin, uninstallPlugin @@ -349,7 +349,7 @@ contract UpgradeableModularAccount is bytes32 userOpHash ) internal returns (uint256 validationData) { if (userOpValidationPlugin == _NULL_PLUGIN) { - revert UserOpValidationFunctionMissing(selector); + revert UserOpValidationMissing(selector); } uint256 currentValidationData; @@ -386,8 +386,7 @@ contract UpgradeableModularAccount is // Run the user op validationFunction { if (!_isEmptyOrMagicValue(userOpValidationPlugin)) { - currentValidationData = - IPlugin(userOpValidationPlugin).userOpValidationFunction(userOp, userOpHash); + currentValidationData = IPlugin(userOpValidationPlugin).validateUserOp(userOp, userOpHash); if (preUserOpValidationHooksLength != 0) { // If we have other validation data we need to coalesce with @@ -440,13 +439,13 @@ contract UpgradeableModularAccount is { if (!_isEmptyOrMagicValue(runtimeValidationPlugin)) { // solhint-disable-next-line no-empty-blocks - try runtimeValidationPlugin.runtimeValidationFunction(msg.sender, msg.value, msg.data) {} + try runtimeValidationPlugin.validateRuntime(msg.sender, msg.value, msg.data) {} catch (bytes memory revertReason) { - revert RuntimeValidationFunctionReverted(address(runtimeValidationPlugin), revertReason); + revert RuntimeValidationReverted(address(runtimeValidationPlugin), revertReason); } } else { if (runtimeValidationPlugin == _NULL_PLUGIN) { - revert RuntimeValidationFunctionMissing(msg.sig); + revert RuntimeValidationMissing(msg.sig); } else if (runtimeValidationPlugin == _PRE_HOOK_ALWAYS_DENY) { revert InvalidConfiguration(); } diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index 666694d4..fff4066a 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -121,9 +121,7 @@ interface IPlugin { /// @param userOp The user operation. /// @param userOpHash The user operation hash. /// @return Packed validation data for validAfter (6 bytes), validUntil (6 bytes), and authorizer (20 bytes). - function userOpValidationFunction(UserOperation calldata userOp, bytes32 userOpHash) - external - returns (uint256); + function validateUserOp(UserOperation calldata userOp, bytes32 userOpHash) external returns (uint256); /// @notice Run the pre runtime validation hook. /// @dev To indicate the entire call should revert, the function MUST revert. @@ -137,7 +135,7 @@ interface IPlugin { /// @param sender The caller address. /// @param value The call value. /// @param data The calldata sent. - function runtimeValidationFunction(address sender, uint256 value, bytes calldata data) external; + function validateRuntime(address sender, uint256 value, bytes calldata data) external; /// @notice Run the pre execution hook. /// @dev To indicate the entire call should revert, the function MUST revert. diff --git a/src/plugins/BasePlugin.sol b/src/plugins/BasePlugin.sol index 41988373..ffb15d74 100644 --- a/src/plugins/BasePlugin.sol +++ b/src/plugins/BasePlugin.sol @@ -36,7 +36,7 @@ abstract contract BasePlugin is ERC165, IPlugin { } /// @inheritdoc IPlugin - function userOpValidationFunction(UserOperation calldata userOp, bytes32 userOpHash) + function validateUserOp(UserOperation calldata userOp, bytes32 userOpHash) external virtual returns (uint256) @@ -52,7 +52,7 @@ abstract contract BasePlugin is ERC165, IPlugin { } /// @inheritdoc IPlugin - function runtimeValidationFunction(address sender, uint256 value, bytes calldata data) external virtual { + function validateRuntime(address sender, uint256 value, bytes calldata data) external virtual { (sender, value, data); revert NotImplemented(); } diff --git a/src/plugins/owner/SingleOwnerPlugin.sol b/src/plugins/owner/SingleOwnerPlugin.sol index 377436c1..b6427c3f 100644 --- a/src/plugins/owner/SingleOwnerPlugin.sol +++ b/src/plugins/owner/SingleOwnerPlugin.sol @@ -103,7 +103,7 @@ contract SingleOwnerPlugin is BasePlugin, ISingleOwnerPlugin, IERC1271 { } /// @inheritdoc BasePlugin - function runtimeValidationFunction(address sender, uint256, bytes calldata) external view override { + function validateRuntime(address sender, uint256, bytes calldata) external view override { // Validate that the sender is the owner of the account or self. if (sender != _owners[msg.sender] && sender != msg.sender) { revert NotAuthorized(); @@ -112,7 +112,7 @@ contract SingleOwnerPlugin is BasePlugin, ISingleOwnerPlugin, IERC1271 { } /// @inheritdoc BasePlugin - function userOpValidationFunction(UserOperation calldata userOp, bytes32 userOpHash) + function validateUserOp(UserOperation calldata userOp, bytes32 userOpHash) external view override diff --git a/src/samples/plugins/ModularSessionKeyPlugin.sol b/src/samples/plugins/ModularSessionKeyPlugin.sol index 372653c4..47b9cd99 100644 --- a/src/samples/plugins/ModularSessionKeyPlugin.sol +++ b/src/samples/plugins/ModularSessionKeyPlugin.sol @@ -186,7 +186,7 @@ contract ModularSessionKeyPlugin is BasePlugin, IModularSessionKeyPlugin { } /// @inheritdoc BasePlugin - function userOpValidationFunction(UserOperation calldata userOp, bytes32 userOpHash) + function validateUserOp(UserOperation calldata userOp, bytes32 userOpHash) external view override @@ -207,7 +207,7 @@ contract ModularSessionKeyPlugin is BasePlugin, IModularSessionKeyPlugin { } /// @inheritdoc BasePlugin - function runtimeValidationFunction(address sender, uint256, bytes calldata data) external view override { + function validateRuntime(address sender, uint256, bytes calldata data) external view override { bytes4 selector = bytes4(data[0:4]); bytes memory key = msg.sender.allocateAssociatedStorageKey(0, 1); StoragePointer ptr = key.associatedStorageLookup(keccak256(abi.encodePacked(sender, selector))); diff --git a/standard/ERCs/erc-6900.md b/standard/ERCs/erc-6900.md index 3115505f..f56860ea 100644 --- a/standard/ERCs/erc-6900.md +++ b/standard/ERCs/erc-6900.md @@ -274,7 +274,7 @@ interface IPlugin { /// @param userOp The user operation. /// @param userOpHash The user operation hash. /// @return Packed validation data for validAfter (6 bytes), validUntil (6 bytes), and authorizer (20 bytes). - function userOpValidationFunction(UserOperation calldata userOp, bytes32 userOpHash) external returns (uint256); + function validateUserOp(UserOperation calldata userOp, bytes32 userOpHash) external returns (uint256); /// @notice Run the pre runtime validation hook. /// @dev To indicate the entire call should revert, the function MUST revert. @@ -288,7 +288,7 @@ interface IPlugin { /// @param sender The caller address. /// @param value The call value. /// @param data The calldata sent. - function runtimeValidationFunction(address sender, uint256 value, bytes calldata data) external; + function validateRuntime(address sender, uint256 value, bytes calldata data) external; /// @notice Run the pre execution hook. /// @dev To indicate the entire call should revert, the function MUST revert. diff --git a/test/account/ManifestValidity.t.sol b/test/account/ManifestValidity.t.sol index 3b0ba122..21cb3863 100644 --- a/test/account/ManifestValidity.t.sol +++ b/test/account/ManifestValidity.t.sol @@ -14,8 +14,8 @@ import { BadValidationMagicValue_PreUserOpValidationHook_Plugin, BadValidationMagicValue_PreExecHook_Plugin, BadValidationMagicValue_PostExecHook_Plugin, - BadHookMagicValue_UserOpValidationFunction_Plugin, - BadHookMagicValue_RuntimeValidationFunction_Plugin, + BadHookMagicValue_UserOpValidation_Plugin, + BadHookMagicValue_RuntimeValidation_Plugin, BadHookMagicValue_PostExecHook_Plugin } from "../mocks/plugins/ManifestValidityMocks.sol"; import {OptimizedTest} from "../utils/OptimizedTest.sol"; @@ -103,8 +103,7 @@ contract ManifestValidityTest is OptimizedTest { // Tests that the plugin manager rejects a plugin with a user op validationFunction set to "hook always deny" function test_ManifestValidity_invalid_HookAlwaysDeny_UserOpValidation() public { - BadHookMagicValue_UserOpValidationFunction_Plugin plugin = - new BadHookMagicValue_UserOpValidationFunction_Plugin(); + BadHookMagicValue_UserOpValidation_Plugin plugin = new BadHookMagicValue_UserOpValidation_Plugin(); bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); @@ -117,10 +116,9 @@ contract ManifestValidityTest is OptimizedTest { }); } - // Tests that the plugin manager rejects a plugin with a runtime validationFunction set to "hook always deny" - function test_ManifestValidity_invalid_HookAlwaysDeny_RuntimeValidationFunction() public { - BadHookMagicValue_RuntimeValidationFunction_Plugin plugin = - new BadHookMagicValue_RuntimeValidationFunction_Plugin(); + // Tests that the plugin manager rejects a plugin with a runtime validation function set to "hook always deny" + function test_ManifestValidity_invalid_HookAlwaysDeny_RuntimeValidation() public { + BadHookMagicValue_RuntimeValidation_Plugin plugin = new BadHookMagicValue_RuntimeValidation_Plugin(); bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); diff --git a/test/mocks/MockPlugin.sol b/test/mocks/MockPlugin.sol index 67891502..4b0ffd71 100644 --- a/test/mocks/MockPlugin.sol +++ b/test/mocks/MockPlugin.sol @@ -79,8 +79,7 @@ contract MockPlugin is ERC165 { fallback() external payable { emit ReceivedCall(msg.data, msg.value); if ( - msg.sig == IPlugin.userOpValidationFunction.selector - || msg.sig == IPlugin.runtimeValidationFunction.selector + msg.sig == IPlugin.validateUserOp.selector || msg.sig == IPlugin.validateRuntime.selector || msg.sig == IPlugin.preExecutionHook.selector ) { // return 0 for userOp/runtimeVal case, return bytes("") for preExecutionHook case diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index 10cfca04..2b93b9bf 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -57,7 +57,7 @@ contract ComprehensivePlugin is BasePlugin { return 0; } - function userOpValidationFunction(UserOperation calldata, bytes32) external pure override returns (uint256) { + function validateUserOp(UserOperation calldata, bytes32) external pure override returns (uint256) { return 0; } @@ -72,7 +72,7 @@ contract ComprehensivePlugin is BasePlugin { return; } - function runtimeValidationFunction(address, uint256, bytes calldata) external pure override { + function validateRuntime(address, uint256, bytes calldata) external pure override { return; } diff --git a/test/mocks/plugins/ManifestValidityMocks.sol b/test/mocks/plugins/ManifestValidityMocks.sol index b0a9c73d..15622cfb 100644 --- a/test/mocks/plugins/ManifestValidityMocks.sol +++ b/test/mocks/plugins/ManifestValidityMocks.sol @@ -152,7 +152,7 @@ contract BadValidationMagicValue_PostExecHook_Plugin is BaseTestPlugin { } } -contract BadHookMagicValue_UserOpValidationFunction_Plugin is BaseTestPlugin { +contract BadHookMagicValue_UserOpValidation_Plugin is BaseTestPlugin { function onInstall(bytes calldata) external override {} function onUninstall(bytes calldata) external override {} @@ -180,7 +180,7 @@ contract BadHookMagicValue_UserOpValidationFunction_Plugin is BaseTestPlugin { } } -contract BadHookMagicValue_RuntimeValidationFunction_Plugin is BaseTestPlugin { +contract BadHookMagicValue_RuntimeValidation_Plugin is BaseTestPlugin { function onInstall(bytes calldata) external override {} function onUninstall(bytes calldata) external override {} diff --git a/test/mocks/plugins/ValidationPluginMocks.sol b/test/mocks/plugins/ValidationPluginMocks.sol index c0f30a84..5c5c7e9f 100644 --- a/test/mocks/plugins/ValidationPluginMocks.sol +++ b/test/mocks/plugins/ValidationPluginMocks.sol @@ -28,7 +28,7 @@ abstract contract MockBaseUserOpValidationPlugin is BaseTestPlugin { return _preUserOpValidationHookData; } - function userOpValidationFunction(UserOperation calldata, bytes32) external view override returns (uint256) { + function validateUserOp(UserOperation calldata, bytes32) external view override returns (uint256) { return _userOpValidationFunctionData; } } diff --git a/test/plugin/SingleOwnerPlugin.t.sol b/test/plugin/SingleOwnerPlugin.t.sol index 1306425f..cb709fa4 100644 --- a/test/plugin/SingleOwnerPlugin.t.sol +++ b/test/plugin/SingleOwnerPlugin.t.sol @@ -112,11 +112,11 @@ contract SingleOwnerPluginTest is OptimizedTest { assertEq(address(0), plugin.owner()); plugin.transferOwnership(owner1); assertEq(owner1, plugin.owner()); - plugin.runtimeValidationFunction(owner1, 0, ""); + plugin.validateRuntime(owner1, 0, ""); vm.startPrank(b); vm.expectRevert(ISingleOwnerPlugin.NotAuthorized.selector); - plugin.runtimeValidationFunction(owner1, 0, ""); + plugin.validateRuntime(owner1, 0, ""); } function testFuzz_validateUserOpSig(string memory salt, UserOperation memory userOp) public { @@ -131,7 +131,7 @@ contract SingleOwnerPluginTest is OptimizedTest { userOp.signature = abi.encodePacked(r, s, v); // sig check should fail - uint256 success = plugin.userOpValidationFunction(userOp, userOpHash); + uint256 success = plugin.validateUserOp(userOp, userOpHash); assertEq(success, 1); // transfer ownership to signer @@ -139,7 +139,7 @@ contract SingleOwnerPluginTest is OptimizedTest { assertEq(signer, plugin.owner()); // sig check should pass - success = plugin.userOpValidationFunction(userOp, userOpHash); + success = plugin.validateUserOp(userOp, userOpHash); assertEq(success, 0); } diff --git a/test/samples/plugins/ModularSessionKeyPlugin.t.sol b/test/samples/plugins/ModularSessionKeyPlugin.t.sol index fc65b462..19a5cf0a 100644 --- a/test/samples/plugins/ModularSessionKeyPlugin.t.sol +++ b/test/samples/plugins/ModularSessionKeyPlugin.t.sol @@ -230,7 +230,7 @@ contract ModularSessionKeyPluginTest is Test { bytes memory revertReason = abi.encodeWithSelector(IModularSessionKeyPlugin.NotAuthorized.selector); vm.expectRevert( abi.encodeWithSelector( - UpgradeableModularAccount.RuntimeValidationFunctionReverted.selector, + UpgradeableModularAccount.RuntimeValidationReverted.selector, address(modularSessionKeyPlugin), revertReason ) @@ -270,7 +270,7 @@ contract ModularSessionKeyPluginTest is Test { vm.expectRevert( abi.encodeWithSelector( - UpgradeableModularAccount.RuntimeValidationFunctionReverted.selector, + UpgradeableModularAccount.RuntimeValidationReverted.selector, address(modularSessionKeyPlugin), revertReason ) @@ -291,7 +291,7 @@ contract ModularSessionKeyPluginTest is Test { vm.expectRevert( abi.encodeWithSelector( - UpgradeableModularAccount.RuntimeValidationFunctionReverted.selector, + UpgradeableModularAccount.RuntimeValidationReverted.selector, address(modularSessionKeyPlugin), revertReason )