From a3a952d7d82e97c3f985d89f93d83e5dac76fd3c Mon Sep 17 00:00:00 2001 From: adam Date: Tue, 28 Nov 2023 16:00:18 -0500 Subject: [PATCH 01/20] Rename Execution to Call, and update IStandardExecutor --- src/account/UpgradeableModularAccount.sol | 26 ++++++++------------ src/interfaces/IStandardExecutor.sol | 23 ++++++++++++----- src/libraries/ERC6900TypeUtils.sol | 11 --------- test/account/AccountReturnData.t.sol | 13 +++++----- test/account/UpgradeableModularAccount.t.sol | 26 +++++++------------- 5 files changed, 42 insertions(+), 57 deletions(-) delete mode 100644 src/libraries/ERC6900TypeUtils.sol diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index be4dc126..1cdc0444 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -10,10 +10,9 @@ import {BaseAccount} from "@eth-infinitism/account-abstraction/core/BaseAccount. import {BaseModularAccount} from "./BaseModularAccount.sol"; import {BaseModularAccountLoupe} from "./BaseModularAccountLoupe.sol"; import {IPlugin, PluginManifest} from "../interfaces/IPlugin.sol"; -import {IStandardExecutor} from "../interfaces/IStandardExecutor.sol"; +import {IStandardExecutor, Call} from "../interfaces/IStandardExecutor.sol"; import {IPluginExecutor} from "../interfaces/IPluginExecutor.sol"; import {AccountStorage, getAccountStorage, getPermittedCallKey} from "../libraries/AccountStorage.sol"; -import {Execution} from "../libraries/ERC6900TypeUtils.sol"; import {FunctionReference, FunctionReferenceLib} from "../libraries/FunctionReferenceLib.sol"; import {AccountStorageInitializable} from "./AccountStorageInitializable.sol"; import {IPluginManager} from "../interfaces/IPluginManager.sol"; @@ -137,35 +136,30 @@ contract UpgradeableModularAccount is return execReturnData; } - /// @notice Executes a transaction from the account - /// @param execution The execution to perform - /// @return result The result of the execution - function execute(Execution calldata execution) + /// @inheritdoc IStandardExecutor + function execute(address target, uint256 value, bytes calldata data) external payable override wrapNativeFunction returns (bytes memory result) { - result = _exec(execution.target, execution.value, execution.data); + result = _exec(target, value, data); } - /// @notice Executes a batch of transactions from the account - /// @dev If any of the transactions revert, the entire batch reverts - /// @param executions The executions to perform - /// @return results The results of the executions - function executeBatch(Execution[] calldata executions) + /// @inheritdoc IStandardExecutor + function executeBatch(Call[] calldata calls) external payable override wrapNativeFunction returns (bytes[] memory results) { - uint256 executionsLength = executions.length; - results = new bytes[](executionsLength); + uint256 callsLength = calls.length; + results = new bytes[](callsLength); - for (uint256 i = 0; i < executionsLength;) { - results[i] = _exec(executions[i].target, executions[i].value, executions[i].data); + for (uint256 i = 0; i < callsLength;) { + results[i] = _exec(calls[i].target, calls[i].value, calls[i].data); unchecked { ++i; diff --git a/src/interfaces/IStandardExecutor.sol b/src/interfaces/IStandardExecutor.sol index 15f84872..5c3c52b3 100644 --- a/src/interfaces/IStandardExecutor.sol +++ b/src/interfaces/IStandardExecutor.sol @@ -1,18 +1,29 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.19; -import {Execution} from "../libraries/ERC6900TypeUtils.sol"; +struct Call { + // The target address for account to call. + address target; + // The value sent with the call. + uint256 value; + // The call data for the call. + bytes data; +} +/// @title Standard Executor Interface interface IStandardExecutor { /// @notice Standard execute method. /// @dev If the target is a plugin, the call SHOULD revert. - /// @param execution The execution information. + /// @param target The target address for account to call. + /// @param value The value sent with the call. + /// @param data The call data for the call. /// @return The return data from the call. - function execute(Execution calldata execution) external payable returns (bytes memory); + function execute(address target, uint256 value, bytes calldata data) external payable returns (bytes memory); /// @notice Standard executeBatch method. - /// @dev If the target is a plugin, the call SHOULD revert. - /// @param executions The array of executions. + /// @dev If the target is a plugin, the call SHOULD revert. If any of the transactions revert, the entire batch + /// reverts. + /// @param calls The array of calls. /// @return An array containing the return data from the calls. - function executeBatch(Execution[] calldata executions) external payable returns (bytes[] memory); + function executeBatch(Call[] calldata calls) external payable returns (bytes[] memory); } diff --git a/src/libraries/ERC6900TypeUtils.sol b/src/libraries/ERC6900TypeUtils.sol deleted file mode 100644 index 9e088121..00000000 --- a/src/libraries/ERC6900TypeUtils.sol +++ /dev/null @@ -1,11 +0,0 @@ -// SPDX-License-Identifier: GPL-3.0 -pragma solidity ^0.8.19; - -struct Execution { - // The target contract for account to execute. - address target; - // The value for the execution. - uint256 value; - // The call data for the execution. - bytes data; -} diff --git a/test/account/AccountReturnData.t.sol b/test/account/AccountReturnData.t.sol index d969c2c8..dc5c350f 100644 --- a/test/account/AccountReturnData.t.sol +++ b/test/account/AccountReturnData.t.sol @@ -9,7 +9,7 @@ import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {FunctionReference} from "../../src/libraries/FunctionReferenceLib.sol"; -import {Execution} from "../../src/libraries/ERC6900TypeUtils.sol"; +import {Call} from "../../src/interfaces/IStandardExecutor.sol"; import { RegularResultContract, @@ -72,9 +72,8 @@ contract AccountReturnDataTest is Test { // Tests the ability to read the results of contracts called via IStandardExecutor.execute function test_returnData_singular_execute() public { - bytes memory returnData = account.execute( - Execution(address(regularResultContract), 0, abi.encodeCall(RegularResultContract.foo, ())) - ); + bytes memory returnData = + account.execute(address(regularResultContract), 0, abi.encodeCall(RegularResultContract.foo, ())); bytes32 result = abi.decode(returnData, (bytes32)); @@ -83,13 +82,13 @@ contract AccountReturnDataTest is Test { // Tests the ability to read the results of multiple contract calls via IStandardExecutor.executeBatch function test_returnData_executeBatch() public { - Execution[] memory calls = new Execution[](2); - calls[0] = Execution({ + Call[] memory calls = new Call[](2); + calls[0] = Call({ target: address(regularResultContract), value: 0, data: abi.encodeCall(RegularResultContract.foo, ()) }); - calls[1] = Execution({ + calls[1] = Call({ target: address(regularResultContract), value: 0, data: abi.encodeCall(RegularResultContract.bar, ()) diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index 0da80e81..3b584181 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -15,7 +15,7 @@ import {PluginManifest} from "../../src/interfaces/IPlugin.sol"; import {IPluginLoupe} from "../../src/interfaces/IPluginLoupe.sol"; import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; import {IPluginExecutor} from "../../src/interfaces/IPluginExecutor.sol"; -import {Execution} from "../../src/libraries/ERC6900TypeUtils.sol"; +import {Call} from "../../src/interfaces/IStandardExecutor.sol"; import {FunctionReference} from "../../src/libraries/FunctionReferenceLib.sol"; import {IPlugin, PluginManifest} from "../../src/interfaces/IPlugin.sol"; @@ -122,9 +122,7 @@ contract UpgradeableModularAccountTest is Test { sender: address(account1), nonce: 0, initCode: abi.encodePacked(address(factory), abi.encodeCall(factory.createAccount, (owner1, 0))), - callData: abi.encodeCall( - UpgradeableModularAccount(payable(account1)).execute, Execution(recipient, 1 wei, "") - ), + callData: abi.encodeCall(UpgradeableModularAccount.execute, (recipient, 1 wei, "")), callGasLimit: CALL_GAS_LIMIT, verificationGasLimit: VERIFICATION_GAS_LIMIT, preVerificationGas: 0, @@ -152,9 +150,7 @@ contract UpgradeableModularAccountTest is Test { sender: address(account2), nonce: 0, initCode: "", - callData: abi.encodeCall( - UpgradeableModularAccount(payable(account2)).execute, Execution(ethRecipient, 1 wei, "") - ), + callData: abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, "")), callGasLimit: CALL_GAS_LIMIT, verificationGasLimit: VERIFICATION_GAS_LIMIT, preVerificationGas: 0, @@ -182,9 +178,7 @@ contract UpgradeableModularAccountTest is Test { sender: address(account2), nonce: 0, initCode: "", - callData: abi.encodeCall( - UpgradeableModularAccount(payable(account2)).execute, Execution(ethRecipient, 1 wei, "") - ), + callData: abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, "")), callGasLimit: CALL_GAS_LIMIT, verificationGasLimit: VERIFICATION_GAS_LIMIT, preVerificationGas: 0, @@ -213,8 +207,7 @@ contract UpgradeableModularAccountTest is Test { nonce: 0, initCode: "", callData: abi.encodeCall( - UpgradeableModularAccount(payable(account2)).execute, - Execution(address(counter), 0, abi.encodeCall(counter.increment, ())) + UpgradeableModularAccount.execute, (address(counter), 0, abi.encodeCall(counter.increment, ())) ), callGasLimit: CALL_GAS_LIMIT, verificationGasLimit: VERIFICATION_GAS_LIMIT, @@ -240,16 +233,15 @@ contract UpgradeableModularAccountTest is Test { function test_batchExecute() public { // Performs both an eth send and a contract interaction with counter - Execution[] memory executions = new Execution[](2); - executions[0] = Execution({target: ethRecipient, value: 1 wei, data: ""}); - executions[1] = - Execution({target: address(counter), value: 0, data: abi.encodeCall(counter.increment, ())}); + Call[] memory calls = new Call[](2); + calls[0] = Call({target: ethRecipient, value: 1 wei, data: ""}); + calls[1] = Call({target: address(counter), value: 0, data: abi.encodeCall(counter.increment, ())}); UserOperation memory userOp = UserOperation({ sender: address(account2), nonce: 0, initCode: "", - callData: abi.encodeCall(UpgradeableModularAccount(payable(account2)).executeBatch, (executions)), + callData: abi.encodeCall(UpgradeableModularAccount.executeBatch, (calls)), callGasLimit: CALL_GAS_LIMIT, verificationGasLimit: VERIFICATION_GAS_LIMIT, preVerificationGas: 0, From 0e4b0ab6521367a45dd2d750e3b1925af56b6269 Mon Sep 17 00:00:00 2001 From: adam Date: Wed, 29 Nov 2023 11:00:18 -0500 Subject: [PATCH 02/20] Update IPluginExecutor NatSpec --- src/account/UpgradeableModularAccount.sol | 12 ++---------- src/interfaces/IPluginExecutor.sol | 18 ++++++++++-------- 2 files changed, 12 insertions(+), 18 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 1cdc0444..312c1d46 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -167,10 +167,7 @@ contract UpgradeableModularAccount is } } - /// @notice Executes a call from a plugin to another plugin - /// @dev Permissions must be granted to the calling plugin for the call to go through - /// @param data calldata to send to the plugin - /// @return The result of the call + /// @inheritdoc IPluginExecutor function executeFromPlugin(bytes calldata data) external payable override returns (bytes memory) { bytes4 selector = bytes4(data[:4]); address callingPlugin = msg.sender; @@ -207,12 +204,7 @@ contract UpgradeableModularAccount is return returnData; } - /// @notice Executes a call from a plugin to a non-plugin address - /// @dev Permissions must be granted to the calling plugin for the call to go through - /// @param target address of the target to call - /// @param value value to send with the call - /// @param data calldata to send to the target - /// @return The result of the call + /// @inheritdoc IPluginExecutor function executeFromPluginExternal(address target, uint256 value, bytes calldata data) external payable diff --git a/src/interfaces/IPluginExecutor.sol b/src/interfaces/IPluginExecutor.sol index c5e3b7e7..31269e00 100644 --- a/src/interfaces/IPluginExecutor.sol +++ b/src/interfaces/IPluginExecutor.sol @@ -2,16 +2,18 @@ pragma solidity ^0.8.19; interface IPluginExecutor { - /// @notice Method from calls made from plugins. - /// @param data The call data for the call. - /// @return The return data from the call. + /// @notice Executes a call from a plugin to another plugin. + /// @dev Permissions must be granted to the calling plugin for the call to go through. + /// @param data calldata to send to the plugin + /// @return The result of the call function executeFromPlugin(bytes calldata data) external payable returns (bytes memory); - /// @notice Method from calls made from plugins. - /// @dev If the target is a plugin, the call SHOULD revert. - /// @param target The target of the external contract to be called. - /// @param value The value to pass. - /// @param data The data to pass. + /// @notice Executes a call from a plugin to a non-plugin address. + /// @dev Permissions must be granted to the calling plugin for the call to go through. + /// @param target address of the target to call + /// @param value value to send with the call + /// @param data calldata to send to the target + /// @return The result of the call function executeFromPluginExternal(address target, uint256 value, bytes calldata data) external payable From 02f04fdde806ee66fea72198367da171a804bb16 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Tue, 28 Nov 2023 11:44:31 -0800 Subject: [PATCH 03/20] update IPluginManager --- src/account/BaseModularAccount.sol | 4 +-- src/interfaces/IPluginManager.sol | 17 ++++++++++--- test/account/UpgradeableModularAccount.t.sol | 26 ++++++++++++++------ 3 files changed, 33 insertions(+), 14 deletions(-) diff --git a/src/account/BaseModularAccount.sol b/src/account/BaseModularAccount.sol index b0b4f512..9f1bfb5c 100644 --- a/src/account/BaseModularAccount.sol +++ b/src/account/BaseModularAccount.sol @@ -615,7 +615,7 @@ abstract contract BaseModularAccount is IPluginManager, AccountExecutor, IERC165 revert PluginInstallCallbackFailed(plugin, revertReason); } - emit PluginInstalled(plugin, manifestHash); + emit PluginInstalled(plugin, manifestHash, dependencies, injectedHooks); } function _uninstallPlugin( @@ -893,7 +893,7 @@ abstract contract BaseModularAccount is IPluginManager, AccountExecutor, IERC165 onUninstallSuccess = false; } - emit PluginUninstalled(plugin, manifestHash, onUninstallSuccess); + emit PluginUninstalled(plugin, onUninstallSuccess); } function _toSetValue(FunctionReference functionReference) internal pure returns (bytes32) { diff --git a/src/interfaces/IPluginManager.sol b/src/interfaces/IPluginManager.sol index e646a501..7b7dd85f 100644 --- a/src/interfaces/IPluginManager.sol +++ b/src/interfaces/IPluginManager.sol @@ -3,9 +3,10 @@ pragma solidity ^0.8.19; import {FunctionReference} from "../libraries/FunctionReferenceLib.sol"; +/// @title Plugin Manager Interface interface IPluginManager { - // Pre/post exec hooks added by the user to limit the scope a plugin has - // These hooks are injected at plugin install time + /// @dev Pre/post exec hooks added by the user to limit the scope of a plugin. These hooks are injected at + /// plugin install time struct InjectedHook { // The plugin that provides the hook address providingPlugin; @@ -21,8 +22,15 @@ interface IPluginManager { uint8 postExecHookFunctionId; } - event PluginInstalled(address indexed plugin, bytes32 manifestHash); - event PluginUninstalled(address indexed plugin, bytes32 manifestHash, bool onUninstallSucceeded); + /// @dev Note that we strip hookApplyData from InjectedHooks in this event for gas savings + event PluginInstalled( + address indexed plugin, + bytes32 manifestHash, + FunctionReference[] dependencies, + InjectedHook[] injectedHooks + ); + + event PluginUninstalled(address indexed plugin, bool indexed callbacksSucceeded); /// @notice Install a plugin to the modular account. /// @param plugin The plugin to install. @@ -40,6 +48,7 @@ interface IPluginManager { ) external; /// @notice Uninstall a plugin from the modular account. + /// @dev Uninstalling owner plugins outside of a replace operation via executeBatch risks losing the account! /// @param plugin The plugin to uninstall. /// @param config An optional, implementation-specific field that accounts may use to ensure consistency /// guarantees. diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index 3b584181..bfde3560 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -53,8 +53,13 @@ contract UpgradeableModularAccountTest is Test { uint256 public constant CALL_GAS_LIMIT = 50000; uint256 public constant VERIFICATION_GAS_LIMIT = 1200000; - event PluginInstalled(address indexed plugin, bytes32 manifestHash); - event PluginUninstalled(address indexed plugin, bytes32 manifestHash, bool onUninstallSucceeded); + event PluginInstalled( + address indexed plugin, + bytes32 manifestHash, + FunctionReference[] dependencies, + IPluginManager.InjectedHook[] injectedHooks + ); + event PluginUninstalled(address indexed plugin, bool indexed callbacksSucceeded); event ReceivedCall(bytes msgData, uint256 msgValue); function setUp() public { @@ -271,7 +276,12 @@ contract UpgradeableModularAccountTest is Test { bytes32 manifestHash = keccak256(abi.encode(tokenReceiverPlugin.pluginManifest())); vm.expectEmit(true, true, true, true); - emit PluginInstalled(address(tokenReceiverPlugin), manifestHash); + emit PluginInstalled( + address(tokenReceiverPlugin), + manifestHash, + new FunctionReference[](0), + new IPluginManager.InjectedHook[](0) + ); IPluginManager(account2).installPlugin({ plugin: address(tokenReceiverPlugin), manifestHash: manifestHash, @@ -381,7 +391,7 @@ contract UpgradeableModularAccountTest is Test { }); vm.expectEmit(true, true, true, true); - emit PluginUninstalled(address(plugin), manifestHash, true); + emit PluginUninstalled(address(plugin), true); IPluginManager(account2).uninstallPlugin({ plugin: address(plugin), config: "", @@ -408,7 +418,7 @@ contract UpgradeableModularAccountTest is Test { }); vm.expectEmit(true, true, true, true); - emit PluginUninstalled(address(plugin), manifestHash, true); + emit PluginUninstalled(address(plugin), true); IPluginManager(account2).uninstallPlugin({ plugin: address(plugin), config: serializedManifest, @@ -485,7 +495,7 @@ contract UpgradeableModularAccountTest is Test { vm.prank(owner2); vm.expectEmit(true, true, true, true); - emit PluginInstalled(address(newPlugin), manifestHash); + emit PluginInstalled(address(newPlugin), manifestHash, new FunctionReference[](0), hooks); emit ReceivedCall(abi.encodeCall(IPlugin.onHookApply, (address(newPlugin), injectedHooksInfo, "")), 0); IPluginManager(account2).installPlugin({ plugin: address(newPlugin), @@ -539,7 +549,7 @@ contract UpgradeableModularAccountTest is Test { ); vm.expectEmit(true, true, true, true); - emit PluginInstalled(address(newPlugin), manifestHash); + emit PluginInstalled(address(newPlugin), manifestHash, new FunctionReference[](0), hooks); emit ReceivedCall( abi.encodeCall(IPlugin.onHookApply, (address(newPlugin), injectedHooksInfo, onApplyData)), 0 ); @@ -583,7 +593,7 @@ contract UpgradeableModularAccountTest is Test { (, MockPlugin newPlugin, bytes32 manifestHash) = _installWithInjectHooks(); vm.expectEmit(true, true, true, true); - emit PluginUninstalled(address(newPlugin), manifestHash, true); + emit PluginUninstalled(address(newPlugin), true); vm.prank(owner2); IPluginManager(account2).uninstallPlugin({ plugin: address(newPlugin), From c06dd7f260afa9803f5c73b1c02956dabc8da143 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Tue, 28 Nov 2023 15:19:18 -0800 Subject: [PATCH 04/20] update name IPluginLoupe to IAccountLoupe --- src/account/BaseModularAccountLoupe.sol | 4 ++-- .../{IPluginLoupe.sol => IAccountLoupe.sol} | 2 +- test/account/ModularAccountLoupe.t.sol | 12 ++++++------ test/account/UpgradeableModularAccount.t.sol | 10 +++++----- 4 files changed, 14 insertions(+), 14 deletions(-) rename src/interfaces/{IPluginLoupe.sol => IAccountLoupe.sol} (97%) diff --git a/src/account/BaseModularAccountLoupe.sol b/src/account/BaseModularAccountLoupe.sol index 39ac5a77..b4c7174e 100644 --- a/src/account/BaseModularAccountLoupe.sol +++ b/src/account/BaseModularAccountLoupe.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.19; import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; -import {IPluginLoupe} from "../interfaces/IPluginLoupe.sol"; +import {IAccountLoupe} from "../interfaces/IAccountLoupe.sol"; import {IPluginManager} from "../interfaces/IPluginManager.sol"; import {IStandardExecutor} from "../interfaces/IStandardExecutor.sol"; import { @@ -15,7 +15,7 @@ import { } from "../libraries/AccountStorage.sol"; import {FunctionReference} from "../libraries/FunctionReferenceLib.sol"; -abstract contract BaseModularAccountLoupe is IPluginLoupe { +abstract contract BaseModularAccountLoupe is IAccountLoupe { using EnumerableSet for EnumerableSet.AddressSet; error ManifestDiscrepancy(address plugin); diff --git a/src/interfaces/IPluginLoupe.sol b/src/interfaces/IAccountLoupe.sol similarity index 97% rename from src/interfaces/IPluginLoupe.sol rename to src/interfaces/IAccountLoupe.sol index 797c5567..837ec3b7 100644 --- a/src/interfaces/IPluginLoupe.sol +++ b/src/interfaces/IAccountLoupe.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.19; import {FunctionReference} from "../libraries/FunctionReferenceLib.sol"; -interface IPluginLoupe { +interface IAccountLoupe { // Config for a Plugin Execution function struct ExecutionFunctionConfig { address plugin; diff --git a/test/account/ModularAccountLoupe.t.sol b/test/account/ModularAccountLoupe.t.sol index 5757c001..786b560b 100644 --- a/test/account/ModularAccountLoupe.t.sol +++ b/test/account/ModularAccountLoupe.t.sol @@ -15,7 +15,7 @@ import { ManifestFunction, PluginManifest } from "../../src/interfaces/IPlugin.sol"; -import {IPluginLoupe} from "../../src/interfaces/IPluginLoupe.sol"; +import {IAccountLoupe} from "../../src/interfaces/IAccountLoupe.sol"; import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; import {IStandardExecutor} from "../../src/interfaces/IStandardExecutor.sol"; import {FunctionReference, FunctionReferenceLib} from "../../src/libraries/FunctionReferenceLib.sol"; @@ -98,7 +98,7 @@ contract ModularAccountLoupeTest is Test { expectedRuntimeValidations[4] = ownerRuntimeValidation; for (uint256 i = 0; i < selectorsToCheck.length; i++) { - IPluginLoupe.ExecutionFunctionConfig memory config = + IAccountLoupe.ExecutionFunctionConfig memory config = account1.getExecutionFunctionConfig(selectorsToCheck[i]); assertEq(config.plugin, address(account1)); @@ -134,7 +134,7 @@ contract ModularAccountLoupeTest is Test { expectedRuntimeValidations[1] = ownerRuntimeValidation; for (uint256 i = 0; i < selectorsToCheck.length; i++) { - IPluginLoupe.ExecutionFunctionConfig memory config = + IAccountLoupe.ExecutionFunctionConfig memory config = account1.getExecutionFunctionConfig(selectorsToCheck[i]); assertEq(config.plugin, expectedPluginAddress[i]); @@ -150,7 +150,7 @@ contract ModularAccountLoupeTest is Test { } function test_pluginLoupe_getExecutionHooks() public { - IPluginLoupe.ExecutionHooks[] memory hooks = account1.getExecutionHooks(comprehensivePlugin.foo.selector); + IAccountLoupe.ExecutionHooks[] memory hooks = account1.getExecutionHooks(comprehensivePlugin.foo.selector); assertEq(hooks.length, 1); assertEq( @@ -172,7 +172,7 @@ contract ModularAccountLoupeTest is Test { } function test_pluginLoupe_getPermittedCallHooks() public { - IPluginLoupe.ExecutionHooks[] memory hooks = + IAccountLoupe.ExecutionHooks[] memory hooks = account1.getPermittedCallHooks(address(comprehensivePlugin), comprehensivePlugin.foo.selector); assertEq(hooks.length, 1); @@ -243,7 +243,7 @@ contract ModularAccountLoupeTest is Test { // Assert that the returned execution hooks are what is expected - IPluginLoupe.ExecutionHooks[] memory hooks = account1.getExecutionHooks(comprehensivePlugin.foo.selector); + IAccountLoupe.ExecutionHooks[] memory hooks = account1.getExecutionHooks(comprehensivePlugin.foo.selector); assertEq(hooks.length, 2); assertEq( diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index bfde3560..879babc5 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -12,7 +12,7 @@ import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAcc import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {TokenReceiverPlugin} from "../../src/plugins/TokenReceiverPlugin.sol"; import {PluginManifest} from "../../src/interfaces/IPlugin.sol"; -import {IPluginLoupe} from "../../src/interfaces/IPluginLoupe.sol"; +import {IAccountLoupe} from "../../src/interfaces/IAccountLoupe.sol"; import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; import {IPluginExecutor} from "../../src/interfaces/IPluginExecutor.sol"; import {Call} from "../../src/interfaces/IStandardExecutor.sol"; @@ -290,7 +290,7 @@ contract UpgradeableModularAccountTest is Test { injectedHooks: new IPluginManager.InjectedHook[](0) }); - address[] memory plugins = IPluginLoupe(account2).getInstalledPlugins(); + address[] memory plugins = IAccountLoupe(account2).getInstalledPlugins(); assertEq(plugins.length, 2); assertEq(plugins[0], address(singleOwnerPlugin)); assertEq(plugins[1], address(tokenReceiverPlugin)); @@ -398,7 +398,7 @@ contract UpgradeableModularAccountTest is Test { pluginUninstallData: "", hookUnapplyData: new bytes[](0) }); - address[] memory plugins = IPluginLoupe(account2).getInstalledPlugins(); + address[] memory plugins = IAccountLoupe(account2).getInstalledPlugins(); assertEq(plugins.length, 1); assertEq(plugins[0], address(singleOwnerPlugin)); } @@ -425,7 +425,7 @@ contract UpgradeableModularAccountTest is Test { pluginUninstallData: "", hookUnapplyData: new bytes[](0) }); - address[] memory plugins = IPluginLoupe(account2).getInstalledPlugins(); + address[] memory plugins = IAccountLoupe(account2).getInstalledPlugins(); assertEq(plugins.length, 1); assertEq(plugins[0], address(singleOwnerPlugin)); } @@ -454,7 +454,7 @@ contract UpgradeableModularAccountTest is Test { pluginUninstallData: "", hookUnapplyData: new bytes[](0) }); - address[] memory plugins = IPluginLoupe(account2).getInstalledPlugins(); + address[] memory plugins = IAccountLoupe(account2).getInstalledPlugins(); assertEq(plugins.length, 2); assertEq(plugins[0], address(singleOwnerPlugin)); assertEq(plugins[1], address(plugin)); From 3415be00c94be42fd256ea346eba3a996cff78ed Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Tue, 28 Nov 2023 15:22:52 -0800 Subject: [PATCH 05/20] update IAccountLoupe interface to spec update 6 --- ...dularAccountLoupe.sol => AccountLoupe.sol} | 43 ++++++------------- src/account/UpgradeableModularAccount.sol | 4 +- src/interfaces/IAccountLoupe.sol | 31 +++++++++++-- ...rAccountLoupe.t.sol => AccountLoupe.t.sol} | 6 +-- 4 files changed, 45 insertions(+), 39 deletions(-) rename src/account/{BaseModularAccountLoupe.sol => AccountLoupe.sol} (68%) rename test/account/{ModularAccountLoupe.t.sol => AccountLoupe.t.sol} (98%) diff --git a/src/account/BaseModularAccountLoupe.sol b/src/account/AccountLoupe.sol similarity index 68% rename from src/account/BaseModularAccountLoupe.sol rename to src/account/AccountLoupe.sol index b4c7174e..526fe297 100644 --- a/src/account/BaseModularAccountLoupe.sol +++ b/src/account/AccountLoupe.sol @@ -15,14 +15,12 @@ import { } from "../libraries/AccountStorage.sol"; import {FunctionReference} from "../libraries/FunctionReferenceLib.sol"; -abstract contract BaseModularAccountLoupe is IAccountLoupe { +abstract contract AccountLoupe is IAccountLoupe { using EnumerableSet for EnumerableSet.AddressSet; error ManifestDiscrepancy(address plugin); - /// @notice Gets the validator and plugin configuration for a selector - /// @param selector The selector to get the configuration for - /// @return config The configuration for this selector + /// @inheritdoc IAccountLoupe function getExecutionFunctionConfig(bytes4 selector) external view @@ -47,9 +45,7 @@ abstract contract BaseModularAccountLoupe is IAccountLoupe { config.runtimeValidationFunction = _storage.selectorData[selector].runtimeValidation; } - /// @notice Gets the pre and post execution hooks for a selector - /// @param selector The selector to get the hooks for - /// @return execHooks The pre and post execution hooks for this selector + /// @inheritdoc IAccountLoupe function getExecutionHooks(bytes4 selector) external view returns (ExecutionHooks[] memory execHooks) { AccountStorage storage _storage = getAccountStorage(); @@ -69,10 +65,7 @@ abstract contract BaseModularAccountLoupe is IAccountLoupe { } } - /// @notice Gets the pre and post permitted call hooks applied for a plugin calling this selector - /// @param callingPlugin The plugin that is calling the selector - /// @param selector The selector the plugin is calling - /// @return execHooks The pre and post permitted call hooks for this selector + /// @inheritdoc IAccountLoupe function getPermittedCallHooks(address callingPlugin, bytes4 selector) external view @@ -99,32 +92,22 @@ abstract contract BaseModularAccountLoupe is IAccountLoupe { } } - /// @notice Gets the pre user op validation hooks associated with a selector - /// @param selector The selector to get the hooks for - /// @return preValidationHooks The pre user op validation hooks for this selector - function getPreUserOpValidationHooks(bytes4 selector) + /// @inheritdoc IAccountLoupe + function getPreValidationHooks(bytes4 selector) external view - returns (FunctionReference[] memory preValidationHooks) + returns ( + FunctionReference[] memory preUserOpValidationHooks, + FunctionReference[] memory preRuntimeValidationHooks + ) { - preValidationHooks = + preUserOpValidationHooks = toFunctionReferenceArray(getAccountStorage().selectorData[selector].preUserOpValidationHooks); - } - - /// @notice Gets the pre runtime validation hooks associated with a selector - /// @param selector The selector to get the hooks for - /// @return preValidationHooks The pre runtime validation hooks for this selector - function getPreRuntimeValidationHooks(bytes4 selector) - external - view - returns (FunctionReference[] memory preValidationHooks) - { - preValidationHooks = + preRuntimeValidationHooks = toFunctionReferenceArray(getAccountStorage().selectorData[selector].preRuntimeValidationHooks); } - /// @notice Gets an array of all installed plugins - /// @return pluginAddresses The addresses of all installed plugins + /// @inheritdoc IAccountLoupe function getInstalledPlugins() external view returns (address[] memory pluginAddresses) { pluginAddresses = getAccountStorage().plugins.values(); } diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 312c1d46..b80f5fb8 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -8,7 +8,7 @@ import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntry import {UserOperation} from "@eth-infinitism/account-abstraction/interfaces/UserOperation.sol"; import {BaseAccount} from "@eth-infinitism/account-abstraction/core/BaseAccount.sol"; import {BaseModularAccount} from "./BaseModularAccount.sol"; -import {BaseModularAccountLoupe} from "./BaseModularAccountLoupe.sol"; +import {AccountLoupe} from "./AccountLoupe.sol"; import {IPlugin, PluginManifest} from "../interfaces/IPlugin.sol"; import {IStandardExecutor, Call} from "../interfaces/IStandardExecutor.sol"; import {IPluginExecutor} from "../interfaces/IPluginExecutor.sol"; @@ -22,7 +22,7 @@ contract UpgradeableModularAccount is IPluginManager, BaseAccount, BaseModularAccount, - BaseModularAccountLoupe, + AccountLoupe, UUPSUpgradeable, AccountStorageInitializable, IStandardExecutor, diff --git a/src/interfaces/IAccountLoupe.sol b/src/interfaces/IAccountLoupe.sol index 837ec3b7..cdf1525f 100644 --- a/src/interfaces/IAccountLoupe.sol +++ b/src/interfaces/IAccountLoupe.sol @@ -4,30 +4,53 @@ pragma solidity ^0.8.19; import {FunctionReference} from "../libraries/FunctionReferenceLib.sol"; interface IAccountLoupe { - // Config for a Plugin Execution function + /// @notice Config for an execution function, given a selector struct ExecutionFunctionConfig { address plugin; FunctionReference userOpValidationFunction; FunctionReference runtimeValidationFunction; } + /// @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; } + /// @notice Gets the validation functions and plugin address for a selector + /// @dev If the selector is a native function, the plugin address will be the address of the account + /// @param selector The selector to get the configuration for + /// @return The configuration for this selector function getExecutionFunctionConfig(bytes4 selector) external view returns (ExecutionFunctionConfig memory); + /// @notice Gets the pre and post execution hooks for a selector + /// @param selector The selector to get the hooks for + /// @return The pre and post execution hooks for this selector function getExecutionHooks(bytes4 selector) external view returns (ExecutionHooks[] memory); + /// @notice Gets the pre and post permitted call hooks applied for a plugin calling this selector + /// @param callingPlugin The plugin that is calling the selector + /// @param selector The selector the plugin is calling + /// @return The pre and post permitted call hooks for this selector function getPermittedCallHooks(address callingPlugin, bytes4 selector) external view returns (ExecutionHooks[] memory); - function getPreUserOpValidationHooks(bytes4 selector) external view returns (FunctionReference[] memory); - - function getPreRuntimeValidationHooks(bytes4 selector) external view returns (FunctionReference[] memory); + /// @notice Gets the pre user op and runtime validation hooks associated with a selector + /// @param selector The selector to get the hooks for + /// @return preUserOpValidationHooks The pre user op validation hooks for this selector + /// @return preRuntimeValidationHooks The pre runtime validation hooks for this selector + function getPreValidationHooks(bytes4 selector) + external + view + returns ( + FunctionReference[] memory preUserOpValidationHooks, + FunctionReference[] memory preRuntimeValidationHooks + ); + /// @notice Gets an array of all installed plugins + /// @return The addresses of all installed plugins function getInstalledPlugins() external view returns (address[] memory); } diff --git a/test/account/ModularAccountLoupe.t.sol b/test/account/AccountLoupe.t.sol similarity index 98% rename from test/account/ModularAccountLoupe.t.sol rename to test/account/AccountLoupe.t.sol index 786b560b..31d08bb1 100644 --- a/test/account/ModularAccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -24,7 +24,7 @@ import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; import {ComprehensivePlugin} from "../mocks/plugins/ComprehensivePlugin.sol"; import {MockPlugin} from "../mocks/MockPlugin.sol"; -contract ModularAccountLoupeTest is Test { +contract AccountLoupeTest is Test { EntryPoint public entryPoint; SingleOwnerPlugin public singleOwnerPlugin; MSCAFactoryFixture public factory; @@ -295,7 +295,7 @@ contract ModularAccountLoupeTest is Test { } function test_pluginLoupe_getPreUserOpValidationHooks() public { - FunctionReference[] memory hooks = account1.getPreUserOpValidationHooks(comprehensivePlugin.foo.selector); + (FunctionReference[] memory hooks,) = account1.getPreValidationHooks(comprehensivePlugin.foo.selector); assertEq(hooks.length, 2); assertEq( @@ -319,7 +319,7 @@ contract ModularAccountLoupeTest is Test { } function test_pluginLoupe_getPreRuntimeValidationHooks() public { - FunctionReference[] memory hooks = account1.getPreRuntimeValidationHooks(comprehensivePlugin.foo.selector); + (, FunctionReference[] memory hooks) = account1.getPreValidationHooks(comprehensivePlugin.foo.selector); assertEq(hooks.length, 2); assertEq( From 8382b42e4779e09243131f011e6588bed30e0510 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Tue, 28 Nov 2023 15:44:41 -0800 Subject: [PATCH 06/20] update BaseModularAccount to PluginManagerInternals and clean it up --- src/account/AccountStorageInitializable.sol | 1 + ...Account.sol => PluginManagerInternals.sol} | 12 ++++------ src/account/UpgradeableModularAccount.sol | 23 ++++++++++++++----- src/interfaces/IPlugin.sol | 1 + test/account/ManifestValidity.t.sol | 18 +++++++-------- test/account/UpgradeableModularAccount.t.sol | 18 +++++++-------- 6 files changed, 41 insertions(+), 32 deletions(-) rename src/account/{BaseModularAccount.sol => PluginManagerInternals.sol} (99%) diff --git a/src/account/AccountStorageInitializable.sol b/src/account/AccountStorageInitializable.sol index 68805ffe..49d5b54a 100644 --- a/src/account/AccountStorageInitializable.sol +++ b/src/account/AccountStorageInitializable.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.19; import {Address} from "@openzeppelin/contracts/utils/Address.sol"; + import {AccountStorage, getAccountStorage} from "../libraries/AccountStorage.sol"; abstract contract AccountStorageInitializable { diff --git a/src/account/BaseModularAccount.sol b/src/account/PluginManagerInternals.sol similarity index 99% rename from src/account/BaseModularAccount.sol rename to src/account/PluginManagerInternals.sol index 9f1bfb5c..0cd6b5fa 100644 --- a/src/account/BaseModularAccount.sol +++ b/src/account/PluginManagerInternals.sol @@ -1,13 +1,11 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.19; +import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol"; import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; -import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; -import {IPluginManager} from "../interfaces/IPluginManager.sol"; import {AccountExecutor} from "./AccountExecutor.sol"; -import {FunctionReference, FunctionReferenceLib} from "../libraries/FunctionReferenceLib.sol"; import { AccountStorage, getAccountStorage, @@ -17,6 +15,8 @@ import { PermittedExternalCallData, StoredInjectedHook } from "../libraries/AccountStorage.sol"; +import {FunctionReference, FunctionReferenceLib} from "../libraries/FunctionReferenceLib.sol"; +import {IPluginManager} from "../interfaces/IPluginManager.sol"; import { IPlugin, ManifestExecutionHook, @@ -27,14 +27,10 @@ import { PluginManifest } from "../interfaces/IPlugin.sol"; -abstract contract BaseModularAccount is IPluginManager, AccountExecutor, IERC165 { +abstract contract PluginManagerInternals is IPluginManager { using EnumerableSet for EnumerableSet.Bytes32Set; using EnumerableSet for EnumerableSet.AddressSet; - // As per the EIP-165 spec, no interface should ever match 0xffffffff - bytes4 internal constant _INTERFACE_ID_INVALID = 0xffffffff; - bytes4 internal constant _IERC165_INTERFACE_ID = 0x01ffc9a7; - error ArrayLengthMismatch(); error ExecuteFromPluginAlreadySet(bytes4 selector, address plugin); error PermittedExecutionSelectorNotInstalled(bytes4 selector, address plugin); diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index b80f5fb8..1467225b 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -1,30 +1,37 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.19; -import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; +import {BaseAccount} from "@eth-infinitism/account-abstraction/core/BaseAccount.sol"; import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; - import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; +import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; import {UserOperation} from "@eth-infinitism/account-abstraction/interfaces/UserOperation.sol"; -import {BaseAccount} from "@eth-infinitism/account-abstraction/core/BaseAccount.sol"; -import {BaseModularAccount} from "./BaseModularAccount.sol"; +import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; + +import {AccountExecutor} from "./AccountExecutor.sol"; import {AccountLoupe} from "./AccountLoupe.sol"; import {IPlugin, PluginManifest} from "../interfaces/IPlugin.sol"; import {IStandardExecutor, Call} from "../interfaces/IStandardExecutor.sol"; import {IPluginExecutor} from "../interfaces/IPluginExecutor.sol"; import {AccountStorage, getAccountStorage, getPermittedCallKey} from "../libraries/AccountStorage.sol"; -import {FunctionReference, FunctionReferenceLib} from "../libraries/FunctionReferenceLib.sol"; import {AccountStorageInitializable} from "./AccountStorageInitializable.sol"; +import {FunctionReference, FunctionReferenceLib} from "../libraries/FunctionReferenceLib.sol"; +import {IPlugin, PluginManifest} from "../interfaces/IPlugin.sol"; +import {IPluginExecutor} from "../interfaces/IPluginExecutor.sol"; import {IPluginManager} from "../interfaces/IPluginManager.sol"; +import {IStandardExecutor} from "../interfaces/IStandardExecutor.sol"; +import {PluginManagerInternals} from "./PluginManagerInternals.sol"; import {_coalescePreValidation, _coalesceValidation} from "../helpers/ValidationDataHelpers.sol"; contract UpgradeableModularAccount is + AccountExecutor, IPluginManager, BaseAccount, - BaseModularAccount, + PluginManagerInternals, AccountLoupe, UUPSUpgradeable, AccountStorageInitializable, + IERC165, IStandardExecutor, IPluginExecutor { @@ -37,6 +44,10 @@ contract UpgradeableModularAccount is IEntryPoint private immutable _ENTRY_POINT; + // As per the EIP-165 spec, no interface should ever match 0xffffffff + bytes4 internal constant _INTERFACE_ID_INVALID = 0xffffffff; + bytes4 internal constant _IERC165_INTERFACE_ID = 0x01ffc9a7; + event ModularAccountInitialized(IEntryPoint indexed entryPoint); error AlwaysDenyRule(); diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index c0fa8edc..6ef81ab0 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.19; import {UserOperation} from "@eth-infinitism/account-abstraction/interfaces/UserOperation.sol"; + import {IPluginManager} from "./IPluginManager.sol"; // Forge formatter will displace the first comment for the enum field out of the enum itself, diff --git a/test/account/ManifestValidity.t.sol b/test/account/ManifestValidity.t.sol index 034e1929..08de7f63 100644 --- a/test/account/ManifestValidity.t.sol +++ b/test/account/ManifestValidity.t.sol @@ -7,7 +7,7 @@ import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.so import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; -import {BaseModularAccount} from "../../src/account/BaseModularAccount.sol"; +import {PluginManagerInternals} from "../../src/account/PluginManagerInternals.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {FunctionReference} from "../../src/libraries/FunctionReferenceLib.sol"; @@ -47,7 +47,7 @@ contract ManifestValidityTest is Test { bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - vm.expectRevert(abi.encodeWithSelector(BaseModularAccount.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); account.installPlugin({ plugin: address(plugin), manifestHash: manifestHash, @@ -65,7 +65,7 @@ contract ManifestValidityTest is Test { bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - vm.expectRevert(abi.encodeWithSelector(BaseModularAccount.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); account.installPlugin({ plugin: address(plugin), manifestHash: manifestHash, @@ -83,7 +83,7 @@ contract ManifestValidityTest is Test { bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - vm.expectRevert(abi.encodeWithSelector(BaseModularAccount.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); account.installPlugin({ plugin: address(plugin), manifestHash: manifestHash, @@ -99,7 +99,7 @@ contract ManifestValidityTest is Test { bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - vm.expectRevert(abi.encodeWithSelector(BaseModularAccount.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); account.installPlugin({ plugin: address(plugin), manifestHash: manifestHash, @@ -115,7 +115,7 @@ contract ManifestValidityTest is Test { bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - vm.expectRevert(abi.encodeWithSelector(BaseModularAccount.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); account.installPlugin({ plugin: address(plugin), manifestHash: manifestHash, @@ -132,7 +132,7 @@ contract ManifestValidityTest is Test { bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - vm.expectRevert(abi.encodeWithSelector(BaseModularAccount.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); account.installPlugin({ plugin: address(plugin), manifestHash: manifestHash, @@ -149,7 +149,7 @@ contract ManifestValidityTest is Test { bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - vm.expectRevert(abi.encodeWithSelector(BaseModularAccount.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); account.installPlugin({ plugin: address(plugin), manifestHash: manifestHash, @@ -165,7 +165,7 @@ contract ManifestValidityTest is Test { bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - vm.expectRevert(abi.encodeWithSelector(BaseModularAccount.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); account.installPlugin({ plugin: address(plugin), manifestHash: manifestHash, diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index 879babc5..9bcdc472 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -7,7 +7,7 @@ import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; import {UserOperation} from "@eth-infinitism/account-abstraction/interfaces/UserOperation.sol"; -import {BaseModularAccount} from "../../src/account/BaseModularAccount.sol"; +import {PluginManagerInternals} from "../../src/account/PluginManagerInternals.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {TokenReceiverPlugin} from "../../src/plugins/TokenReceiverPlugin.sol"; @@ -308,7 +308,7 @@ contract UpgradeableModularAccountTest is Test { vm.expectRevert( abi.encodeWithSelector( - BaseModularAccount.PermittedExecutionSelectorNotInstalled.selector, + PluginManagerInternals.PermittedExecutionSelectorNotInstalled.selector, IPlugin.onInstall.selector, address(mockPluginWithBadPermittedExec) ) @@ -325,7 +325,7 @@ contract UpgradeableModularAccountTest is Test { function test_installPlugin_invalidManifest() public { vm.startPrank(owner2); - vm.expectRevert(abi.encodeWithSelector(BaseModularAccount.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); IPluginManager(account2).installPlugin({ plugin: address(tokenReceiverPlugin), manifestHash: bytes32(0), @@ -340,7 +340,7 @@ contract UpgradeableModularAccountTest is Test { address badPlugin = address(1); vm.expectRevert( - abi.encodeWithSelector(BaseModularAccount.PluginInterfaceNotSupported.selector, address(badPlugin)) + abi.encodeWithSelector(PluginManagerInternals.PluginInterfaceNotSupported.selector, address(badPlugin)) ); IPluginManager(account2).installPlugin({ plugin: address(badPlugin), @@ -365,7 +365,7 @@ contract UpgradeableModularAccountTest is Test { vm.expectRevert( abi.encodeWithSelector( - BaseModularAccount.PluginAlreadyInstalled.selector, address(tokenReceiverPlugin) + PluginManagerInternals.PluginAlreadyInstalled.selector, address(tokenReceiverPlugin) ) ); IPluginManager(account2).installPlugin({ @@ -447,7 +447,7 @@ contract UpgradeableModularAccountTest is Test { // Attempt to uninstall with a blank manifest PluginManifest memory blankManifest; - vm.expectRevert(abi.encodeWithSelector(BaseModularAccount.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); IPluginManager(account2).uninstallPlugin({ plugin: address(plugin), config: abi.encode(blankManifest), @@ -577,7 +577,7 @@ contract UpgradeableModularAccountTest is Test { ); vm.expectRevert( - abi.encodeWithSelector(BaseModularAccount.MissingPluginDependency.selector, address(hooksPlugin)) + abi.encodeWithSelector(PluginManagerInternals.MissingPluginDependency.selector, address(hooksPlugin)) ); vm.prank(owner2); IPluginManager(account2).installPlugin({ @@ -608,7 +608,7 @@ contract UpgradeableModularAccountTest is Test { vm.prank(owner2); vm.expectRevert( - abi.encodeWithSelector(BaseModularAccount.PluginDependencyViolation.selector, address(hooksPlugin)) + abi.encodeWithSelector(PluginManagerInternals.PluginDependencyViolation.selector, address(hooksPlugin)) ); IPluginManager(account2).uninstallPlugin({ plugin: address(hooksPlugin), @@ -644,7 +644,7 @@ contract UpgradeableModularAccountTest is Test { // length != installed hooks length bytes[] memory injectedHooksDatas = new bytes[](2); - vm.expectRevert(BaseModularAccount.ArrayLengthMismatch.selector); + vm.expectRevert(PluginManagerInternals.ArrayLengthMismatch.selector); vm.prank(owner2); IPluginManager(account2).uninstallPlugin({ plugin: address(newPlugin), From 1b2386bdcd4340f4d68479ee2d653abd8c7b03d9 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Tue, 28 Nov 2023 15:53:06 -0800 Subject: [PATCH 07/20] update BaseModularAccount to PluginManagerInternals and clean it up --- src/account/PluginManagerInternals.sol | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 0cd6b5fa..92292bcd 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -3,9 +3,7 @@ pragma solidity ^0.8.19; import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol"; -import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; -import {AccountExecutor} from "./AccountExecutor.sol"; import { AccountStorage, getAccountStorage, From c53849fd7bfa72ee752b7fdd54f29d7becd9b99e Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Tue, 28 Nov 2023 16:49:41 -0800 Subject: [PATCH 08/20] update IPlugin to match spec update 6 --- src/account/PluginManagerInternals.sol | 8 +-- src/interfaces/IPlugin.sol | 32 ++++++--- src/plugins/BasePlugin.sol | 7 +- src/plugins/TokenReceiverPlugin.sol | 27 ++++---- src/plugins/owner/SingleOwnerPlugin.sol | 38 +++++++---- test/account/UpgradeableModularAccount.t.sol | 2 +- test/mocks/MockPlugin.sol | 15 +++-- .../plugins/BadTransferOwnershipPlugin.sol | 19 +++--- test/mocks/plugins/BaseTestPlugin.sol | 12 ++++ test/mocks/plugins/ComprehensivePlugin.sol | 18 +++-- .../ExecFromPluginPermissionsMocks.sol | 67 ++++++++----------- test/mocks/plugins/ManifestValidityMocks.sol | 57 +++++++--------- test/mocks/plugins/ReturnDataPluginMocks.sol | 23 +++---- test/mocks/plugins/ValidationPluginMocks.sol | 19 +++--- 14 files changed, 190 insertions(+), 154 deletions(-) create mode 100644 test/mocks/plugins/BaseTestPlugin.sol diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 92292bcd..d66993fc 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -378,7 +378,7 @@ abstract contract PluginManagerInternals is IPluginManager { // All conflicts should revert. length = manifest.executionFunctions.length; for (uint256 i = 0; i < length;) { - _setExecutionFunction(manifest.executionFunctions[i].selector, plugin); + _setExecutionFunction(manifest.executionFunctions[i], plugin); unchecked { ++i; @@ -396,7 +396,7 @@ abstract contract PluginManagerInternals is IPluginManager { } // Add the permitted external calls to the account. - if (manifest.permitAnyExternalContract) { + if (manifest.permitAnyExternalAddress) { _storage.pluginData[plugin].anyExternalExecPermitted = true; } else { // Only store the specific permitted external calls if "permit any" flag was not set. @@ -767,7 +767,7 @@ abstract contract PluginManagerInternals is IPluginManager { // remove external call permissions - if (manifest.permitAnyExternalContract) { + if (manifest.permitAnyExternalAddress) { // Only clear if it was set during install time _storage.pluginData[plugin].anyExternalExecPermitted = false; } else { @@ -833,7 +833,7 @@ abstract contract PluginManagerInternals is IPluginManager { length = manifest.executionFunctions.length; for (uint256 i = 0; i < length;) { - _removeExecutionFunction(manifest.executionFunctions[i].selector); + _removeExecutionFunction(manifest.executionFunctions[i]); unchecked { ++i; diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index 6ef81ab0..c3e27b0a 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -54,12 +54,13 @@ struct ManifestExternalCallPermission { bytes4[] selectors; } -struct ManifestExecutionFunction { - bytes4 selector; - string[] permissions; +struct SelectorPermission { + bytes4 functionSelector; + string permissionDescription; } -struct PluginManifest { +/// @dev A struct holding fields to describe the plugin in a purely view context. Intended for front end clients. +struct PluginMetadata { // A human-readable name of the plugin. string name; // The version of the plugin, following the semantic versioning scheme. @@ -67,6 +68,13 @@ struct PluginManifest { // 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 + // functions implemented by this plugin. + SelectorPermission[] permissionDescriptors; +} + +/// @dev A struct describing how the plugin should be installed on a modular account. +struct PluginManifest { // List of ERC-165 interfaceIds to add to account to support introspection checks. bytes4[] interfaceIds; // If this plugin depends on other plugins' validation functions and/or hooks, the interface IDs of @@ -74,12 +82,13 @@ struct PluginManifest { // members of `ManifestFunction` structs used in the manifest. bytes4[] dependencyInterfaceIds; // Execution functions defined in this plugin to be installed on the MSCA. - ManifestExecutionFunction[] executionFunctions; - // Native functions or execution functions already installed on the MSCA that this plugin will be - // able to call. + bytes4[] executionFunctions; + // Plugin execution functions already installed on the MSCA that this plugin will be able to call. bytes4[] permittedExecutionSelectors; - // External contract calls that this plugin will be able to make. - bool permitAnyExternalContract; + // Boolean to indicate whether the plugin can call any external contract addresses. + bool permitAnyExternalAddress; + // Boolean to indicate whether the plugin needs access to spend native tokens of the account. + bool canSpendNativeToken; ManifestExternalCallPermission[] permittedExternalCalls; ManifestAssociatedFunction[] userOpValidationFunctions; ManifestAssociatedFunction[] runtimeValidationFunctions; @@ -188,4 +197,9 @@ interface IPlugin { /// @dev This manifest MUST stay constant over time. /// @return A manifest describing the contents and intended configuration of the plugin. function pluginManifest() external pure returns (PluginManifest memory); + + /// @notice Describe the metadata of the plugin. + /// @dev This metadata MUST stay constant over time. + /// @return A metadata struct describing the plugin. + function pluginMetadata() external pure returns (PluginMetadata memory); } diff --git a/src/plugins/BasePlugin.sol b/src/plugins/BasePlugin.sol index e03ba129..a8d610c1 100644 --- a/src/plugins/BasePlugin.sol +++ b/src/plugins/BasePlugin.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.19; import {UserOperation} from "@eth-infinitism/account-abstraction/interfaces/UserOperation.sol"; import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol"; -import {IPlugin, PluginManifest} from "../interfaces/IPlugin.sol"; +import {IPlugin, PluginManifest, PluginMetadata} from "../interfaces/IPlugin.sol"; import {IPluginManager} from "../interfaces/IPluginManager.sol"; /// @title Base contract for plugins @@ -153,6 +153,11 @@ abstract contract BasePlugin is ERC165, IPlugin { revert NotImplemented(); } + /// @notice Describe the metadata of the plugin. + /// @dev This metadata MUST stay constant over time. + /// @return A metadata struct describing the plugin. + function pluginMetadata() external pure virtual returns (PluginMetadata memory); + /// @dev Returns true if this contract implements the interface defined by /// `interfaceId`. See the corresponding /// https://eips.ethereum.org/EIPS/eip-165#how-interfaces-are-identified[EIP section] diff --git a/src/plugins/TokenReceiverPlugin.sol b/src/plugins/TokenReceiverPlugin.sol index 8190a1c9..ac9335fd 100644 --- a/src/plugins/TokenReceiverPlugin.sol +++ b/src/plugins/TokenReceiverPlugin.sol @@ -10,7 +10,7 @@ import { ManifestAssociatedFunctionType, ManifestAssociatedFunction, PluginManifest, - ManifestExecutionFunction + PluginMetadata } from "../interfaces/IPlugin.sol"; import {BasePlugin} from "./BasePlugin.sol"; @@ -72,17 +72,11 @@ contract TokenReceiverPlugin is BasePlugin, IERC721Receiver, IERC777Recipient, I function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.name = NAME; - manifest.version = VERSION; - manifest.author = AUTHOR; - - manifest.executionFunctions = new ManifestExecutionFunction[](4); - manifest.executionFunctions[0] = ManifestExecutionFunction(this.tokensReceived.selector, new string[](0)); - manifest.executionFunctions[1] = ManifestExecutionFunction(this.onERC721Received.selector, new string[](0)); - manifest.executionFunctions[2] = - ManifestExecutionFunction(this.onERC1155Received.selector, new string[](0)); - manifest.executionFunctions[3] = - ManifestExecutionFunction(this.onERC1155BatchReceived.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](4); + manifest.executionFunctions[0] = this.tokensReceived.selector; + manifest.executionFunctions[1] = this.onERC721Received.selector; + manifest.executionFunctions[2] = this.onERC1155Received.selector; + manifest.executionFunctions[3] = this.onERC1155BatchReceived.selector; // Only runtime validationFunction is needed since callbacks come from token contracts only ManifestFunction memory alwaysAllowFunction = ManifestFunction({ @@ -115,4 +109,13 @@ contract TokenReceiverPlugin is BasePlugin, IERC721Receiver, IERC777Recipient, I return manifest; } + + /// @inheritdoc BasePlugin + function pluginMetadata() external pure virtual override returns (PluginMetadata memory) { + PluginMetadata memory metadata; + metadata.name = NAME; + metadata.version = VERSION; + metadata.author = AUTHOR; + return metadata; + } } diff --git a/src/plugins/owner/SingleOwnerPlugin.sol b/src/plugins/owner/SingleOwnerPlugin.sol index 91143eb3..fa8ed177 100644 --- a/src/plugins/owner/SingleOwnerPlugin.sol +++ b/src/plugins/owner/SingleOwnerPlugin.sol @@ -13,7 +13,8 @@ import { ManifestAssociatedFunctionType, ManifestAssociatedFunction, PluginManifest, - ManifestExecutionFunction + PluginMetadata, + SelectorPermission } from "../../interfaces/IPlugin.sol"; import {IStandardExecutor} from "../../interfaces/IStandardExecutor.sol"; import {BasePlugin} from "../BasePlugin.sol"; @@ -139,18 +140,10 @@ contract SingleOwnerPlugin is BasePlugin, ISingleOwnerPlugin, IERC1271 { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.name = NAME; - manifest.version = VERSION; - manifest.author = AUTHOR; - - string[] memory ownerPermissions = new string[](1); - ownerPermissions[0] = "Modify Ownership"; - - manifest.executionFunctions = new ManifestExecutionFunction[](3); - manifest.executionFunctions[0] = - ManifestExecutionFunction(this.transferOwnership.selector, ownerPermissions); - manifest.executionFunctions[1] = ManifestExecutionFunction(this.isValidSignature.selector, new string[](0)); - manifest.executionFunctions[2] = ManifestExecutionFunction(this.owner.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](3); + manifest.executionFunctions[0] = this.transferOwnership.selector; + manifest.executionFunctions[1] = this.isValidSignature.selector; + manifest.executionFunctions[2] = this.owner.selector; ManifestFunction memory ownerUserOpValidationFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, @@ -238,6 +231,25 @@ contract SingleOwnerPlugin is BasePlugin, ISingleOwnerPlugin, IERC1271 { return manifest; } + /// @inheritdoc BasePlugin + function pluginMetadata() external pure virtual override returns (PluginMetadata memory) { + PluginMetadata memory metadata; + metadata.name = NAME; + metadata.version = VERSION; + metadata.author = AUTHOR; + + // Permission strings + string memory modifyOwnershipPermission = "Modify Ownership"; + + // Permission descriptions + metadata.permissionDescriptors = new SelectorPermission[](1); + metadata.permissionDescriptors[0] = SelectorPermission({ + functionSelector: this.transferOwnership.selector, + permissionDescription: modifyOwnershipPermission + }); + + return metadata; + } // ┏━━━━━━━━━━━━━━━┓ // ┃ EIP-165 ┃ // ┗━━━━━━━━━━━━━━━┛ diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index 9bcdc472..421f08da 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -483,7 +483,7 @@ contract UpgradeableModularAccountTest is Test { { hooksPlugin = _installPluginWithExecHooks(); - manifest.permitAnyExternalContract = true; + manifest.permitAnyExternalAddress = true; newPlugin = new MockPlugin(manifest); manifestHash = keccak256(abi.encode(newPlugin.pluginManifest())); diff --git a/test/mocks/MockPlugin.sol b/test/mocks/MockPlugin.sol index 7daa8e5a..273d5335 100644 --- a/test/mocks/MockPlugin.sol +++ b/test/mocks/MockPlugin.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.19; import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol"; -import {PluginManifest, IPlugin} from "../../src/interfaces/IPlugin.sol"; +import {PluginManifest, IPlugin, PluginMetadata} from "../../src/interfaces/IPlugin.sol"; contract MockPlugin is ERC165 { // It's super inefficient to hold the entire abi-encoded manifest in storage, but this is fine since it's @@ -26,10 +26,6 @@ contract MockPlugin is ERC165 { // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ constructor(PluginManifest memory _pluginManifest) { - _pluginManifest.name = NAME; - _pluginManifest.version = VERSION; - _pluginManifest.author = AUTHOR; - _manifest = abi.encode(_pluginManifest); } @@ -52,6 +48,14 @@ contract MockPlugin is ERC165 { return _castToPure(_getManifest)(); } + function pluginMetadata() external pure returns (PluginMetadata memory) { + PluginMetadata memory metadata; + metadata.name = NAME; + metadata.version = VERSION; + metadata.author = AUTHOR; + return metadata; + } + /// @dev Returns true if this contract implements the interface defined by /// `interfaceId`. See the corresponding /// https://eips.ethereum.org/EIPS/eip-165#how-interfaces-are-identified[EIP section] @@ -64,6 +68,7 @@ contract MockPlugin is ERC165 { /// making calls to plugins. /// @param interfaceId The interface ID to check for support. /// @return True if the contract supports `interfaceId`. + function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) { return interfaceId == type(IPlugin).interfaceId || super.supportsInterface(interfaceId); } diff --git a/test/mocks/plugins/BadTransferOwnershipPlugin.sol b/test/mocks/plugins/BadTransferOwnershipPlugin.sol index f7e6a744..0b291c03 100644 --- a/test/mocks/plugins/BadTransferOwnershipPlugin.sol +++ b/test/mocks/plugins/BadTransferOwnershipPlugin.sol @@ -7,7 +7,7 @@ import { ManifestAssociatedFunctionType, ManifestAssociatedFunction, PluginManifest, - ManifestExecutionFunction + PluginMetadata } from "../../../src/interfaces/IPlugin.sol"; import {IPluginManager} from "../../../src/interfaces/IPluginManager.sol"; import {BasePlugin} from "../../../src/plugins/BasePlugin.sol"; @@ -41,13 +41,8 @@ contract BadTransferOwnershipPlugin is BasePlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.name = NAME; - manifest.version = VERSION; - manifest.author = AUTHOR; - - manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = - ManifestExecutionFunction(this.evilTransferOwnership.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.evilTransferOwnership.selector; manifest.permittedExecutionSelectors = new bytes4[](1); manifest.permittedExecutionSelectors[0] = ISingleOwnerPlugin.transferOwnership.selector; @@ -64,4 +59,12 @@ contract BadTransferOwnershipPlugin is BasePlugin { return manifest; } + + function pluginMetadata() external pure virtual override returns (PluginMetadata memory) { + PluginMetadata memory metadata; + metadata.name = NAME; + metadata.version = VERSION; + metadata.author = AUTHOR; + return metadata; + } } diff --git a/test/mocks/plugins/BaseTestPlugin.sol b/test/mocks/plugins/BaseTestPlugin.sol new file mode 100644 index 00000000..508845c1 --- /dev/null +++ b/test/mocks/plugins/BaseTestPlugin.sol @@ -0,0 +1,12 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.19; + +import {BasePlugin} from "../../../src/plugins/BasePlugin.sol"; +import {PluginMetadata} from "../../../src/interfaces/IPlugin.sol"; + +contract BaseTestPlugin is BasePlugin { + // Don't need to implement this in each context + function pluginMetadata() external pure virtual override returns (PluginMetadata memory) { + revert NotImplemented(); + } +} diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index 391b1cee..4be37ff8 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -9,7 +9,7 @@ import { ManifestAssociatedFunctionType, ManifestAssociatedFunction, PluginManifest, - ManifestExecutionFunction + PluginMetadata } from "../../../src/interfaces/IPlugin.sol"; import {IStandardExecutor} from "../../../src/interfaces/IStandardExecutor.sol"; import {BasePlugin} from "../../../src/plugins/BasePlugin.sol"; @@ -118,12 +118,8 @@ contract ComprehensivePlugin is BasePlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.name = NAME; - manifest.version = VERSION; - manifest.author = AUTHOR; - - manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = ManifestExecutionFunction(this.foo.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.foo.selector; ManifestFunction memory fooUserOpValidationFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, @@ -247,4 +243,12 @@ contract ComprehensivePlugin is BasePlugin { return manifest; } + + function pluginMetadata() external pure virtual override returns (PluginMetadata memory) { + PluginMetadata memory metadata; + metadata.name = NAME; + metadata.version = VERSION; + metadata.author = AUTHOR; + return metadata; + } } diff --git a/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol b/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol index d6ed7663..8861012f 100644 --- a/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol +++ b/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol @@ -7,13 +7,12 @@ import { ManifestAssociatedFunction, ManifestExternalCallPermission, ManifestExecutionHook, - PluginManifest, - ManifestExecutionFunction + PluginManifest } from "../../../src/interfaces/IPlugin.sol"; import {IStandardExecutor} from "../../../src/interfaces/IStandardExecutor.sol"; import {IPluginExecutor} from "../../../src/interfaces/IPluginExecutor.sol"; import {IPlugin} from "../../../src/interfaces/IPlugin.sol"; -import {BasePlugin} from "../../../src/plugins/BasePlugin.sol"; +import {BaseTestPlugin} from "./BaseTestPlugin.sol"; import {FunctionReference} from "../../../src/libraries/FunctionReferenceLib.sol"; import {ResultCreatorPlugin} from "./ReturnDataPluginMocks.sol"; @@ -25,7 +24,7 @@ address constant counter1 = 0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f; address constant counter2 = 0x2e234DAe75C793f67A35089C9d99245E1C58470b; address constant counter3 = 0xF62849F9A0B5Bf2913b396098F7c7019b51A820a; -contract EFPCallerPlugin is BasePlugin { +contract EFPCallerPlugin is BaseTestPlugin { function onInstall(bytes calldata) external override {} function onUninstall(bytes calldata) external override {} @@ -33,29 +32,18 @@ contract EFPCallerPlugin is BasePlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new ManifestExecutionFunction[](11); - manifest.executionFunctions[0] = - ManifestExecutionFunction(this.useEFPPermissionAllowed.selector, new string[](0)); - manifest.executionFunctions[1] = - ManifestExecutionFunction(this.useEFPPermissionNotAllowed.selector, new string[](0)); - manifest.executionFunctions[2] = - ManifestExecutionFunction(this.setNumberCounter1.selector, new string[](0)); - manifest.executionFunctions[3] = - ManifestExecutionFunction(this.getNumberCounter1.selector, new string[](0)); - manifest.executionFunctions[4] = - ManifestExecutionFunction(this.incrementCounter1.selector, new string[](0)); - manifest.executionFunctions[5] = - ManifestExecutionFunction(this.setNumberCounter2.selector, new string[](0)); - manifest.executionFunctions[6] = - ManifestExecutionFunction(this.getNumberCounter2.selector, new string[](0)); - manifest.executionFunctions[7] = - ManifestExecutionFunction(this.incrementCounter2.selector, new string[](0)); - manifest.executionFunctions[8] = - ManifestExecutionFunction(this.setNumberCounter3.selector, new string[](0)); - manifest.executionFunctions[9] = - ManifestExecutionFunction(this.getNumberCounter3.selector, new string[](0)); - manifest.executionFunctions[10] = - ManifestExecutionFunction(this.incrementCounter3.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](11); + manifest.executionFunctions[0] = this.useEFPPermissionAllowed.selector; + manifest.executionFunctions[1] = this.useEFPPermissionNotAllowed.selector; + manifest.executionFunctions[2] = this.setNumberCounter1.selector; + manifest.executionFunctions[3] = this.getNumberCounter1.selector; + manifest.executionFunctions[4] = this.incrementCounter1.selector; + manifest.executionFunctions[5] = this.setNumberCounter2.selector; + manifest.executionFunctions[6] = this.getNumberCounter2.selector; + manifest.executionFunctions[7] = this.incrementCounter2.selector; + manifest.executionFunctions[8] = this.setNumberCounter3.selector; + manifest.executionFunctions[9] = this.getNumberCounter3.selector; + manifest.executionFunctions[10] = this.incrementCounter3.selector; manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](11); @@ -67,7 +55,7 @@ contract EFPCallerPlugin is BasePlugin { for (uint256 i = 0; i < manifest.executionFunctions.length; i++) { manifest.runtimeValidationFunctions[i] = ManifestAssociatedFunction({ - executionSelector: manifest.executionFunctions[i].selector, + executionSelector: manifest.executionFunctions[i], associatedFunction: alwaysAllowValidationFunction }); } @@ -182,7 +170,7 @@ contract EFPCallerPlugin is BasePlugin { } } -contract EFPCallerPluginAnyExternal is BasePlugin { +contract EFPCallerPluginAnyExternal is BaseTestPlugin { function onInstall(bytes calldata) external override {} function onUninstall(bytes calldata) external override {} @@ -190,9 +178,8 @@ contract EFPCallerPluginAnyExternal is BasePlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = - ManifestExecutionFunction(this.passthroughExecute.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.passthroughExecute.selector; manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](1); manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ @@ -204,7 +191,7 @@ contract EFPCallerPluginAnyExternal is BasePlugin { }) }); - manifest.permitAnyExternalContract = true; + manifest.permitAnyExternalAddress = true; return manifest; } @@ -219,7 +206,7 @@ contract EFPCallerPluginAnyExternal is BasePlugin { } // Create pre and post permitted call hooks for calling ResultCreatorPlugin.foo via `executeFromPlugin` -contract EFPPermittedCallHookPlugin is BasePlugin { +contract EFPPermittedCallHookPlugin is BaseTestPlugin { bool public preExecHookCalled; bool public postExecHookCalled; @@ -242,8 +229,8 @@ contract EFPPermittedCallHookPlugin is BasePlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = ManifestExecutionFunction(this.performEFPCall.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.performEFPCall.selector; manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](1); manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ @@ -282,7 +269,7 @@ contract EFPPermittedCallHookPlugin is BasePlugin { } // Creates pre and post permitted call hooks for `executeFromPluginExternal` -contract EFPExternalPermittedCallHookPlugin is BasePlugin { +contract EFPExternalPermittedCallHookPlugin is BaseTestPlugin { bool public preExecHookCalled; bool public postExecHookCalled; @@ -305,8 +292,8 @@ contract EFPExternalPermittedCallHookPlugin is BasePlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = ManifestExecutionFunction(this.performIncrement.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.performIncrement.selector; manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](1); manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ @@ -333,7 +320,7 @@ contract EFPExternalPermittedCallHookPlugin is BasePlugin { }) }); - manifest.permitAnyExternalContract = true; + manifest.permitAnyExternalAddress = true; return manifest; } diff --git a/test/mocks/plugins/ManifestValidityMocks.sol b/test/mocks/plugins/ManifestValidityMocks.sol index 4241a4d2..6ec719f4 100644 --- a/test/mocks/plugins/ManifestValidityMocks.sol +++ b/test/mocks/plugins/ManifestValidityMocks.sol @@ -7,16 +7,15 @@ import { ManifestAssociatedFunction, ManifestExecutionHook, ManifestExternalCallPermission, - PluginManifest, - ManifestExecutionFunction + PluginManifest } from "../../../src/interfaces/IPlugin.sol"; import {IStandardExecutor} from "../../../src/interfaces/IStandardExecutor.sol"; import {IPluginExecutor} from "../../../src/interfaces/IPluginExecutor.sol"; import {IPlugin} from "../../../src/interfaces/IPlugin.sol"; -import {BasePlugin} from "../../../src/plugins/BasePlugin.sol"; +import {BaseTestPlugin} from "./BaseTestPlugin.sol"; import {FunctionReference} from "../../../src/libraries/FunctionReferenceLib.sol"; -contract BadValidationMagicValue_UserOp_Plugin is BasePlugin { +contract BadValidationMagicValue_UserOp_Plugin is BaseTestPlugin { function onInstall(bytes calldata) external override {} function onUninstall(bytes calldata) external override {} @@ -28,8 +27,8 @@ contract BadValidationMagicValue_UserOp_Plugin is BasePlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = ManifestExecutionFunction(this.foo.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.foo.selector; manifest.userOpValidationFunctions = new ManifestAssociatedFunction[](1); manifest.userOpValidationFunctions[0] = ManifestAssociatedFunction({ @@ -46,7 +45,7 @@ contract BadValidationMagicValue_UserOp_Plugin is BasePlugin { } } -contract BadValidationMagicValue_PreRuntimeValidationHook_Plugin is BasePlugin { +contract BadValidationMagicValue_PreRuntimeValidationHook_Plugin is BaseTestPlugin { function onInstall(bytes calldata) external override {} function onUninstall(bytes calldata) external override {} @@ -58,8 +57,8 @@ contract BadValidationMagicValue_PreRuntimeValidationHook_Plugin is BasePlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = ManifestExecutionFunction(this.foo.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.foo.selector; manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](1); manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ @@ -72,7 +71,6 @@ contract BadValidationMagicValue_PreRuntimeValidationHook_Plugin is BasePlugin { }); manifest.preRuntimeValidationHooks = new ManifestAssociatedFunction[](1); - // Illegal assignment: validation always allow only usable on runtime validation functions manifest.preRuntimeValidationHooks[0] = ManifestAssociatedFunction({ executionSelector: this.foo.selector, @@ -87,7 +85,7 @@ contract BadValidationMagicValue_PreRuntimeValidationHook_Plugin is BasePlugin { } } -contract BadValidationMagicValue_PreUserOpValidationHook_Plugin is BasePlugin { +contract BadValidationMagicValue_PreUserOpValidationHook_Plugin is BaseTestPlugin { function onInstall(bytes calldata) external override {} function onUninstall(bytes calldata) external override {} @@ -99,8 +97,8 @@ contract BadValidationMagicValue_PreUserOpValidationHook_Plugin is BasePlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = ManifestExecutionFunction(this.foo.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.foo.selector; manifest.userOpValidationFunctions = new ManifestAssociatedFunction[](1); manifest.userOpValidationFunctions[0] = ManifestAssociatedFunction({ @@ -113,7 +111,6 @@ contract BadValidationMagicValue_PreUserOpValidationHook_Plugin is BasePlugin { }); manifest.preUserOpValidationHooks = new ManifestAssociatedFunction[](1); - // Illegal assignment: validation always allow only usable on runtime validation functions manifest.preUserOpValidationHooks[0] = ManifestAssociatedFunction({ executionSelector: this.foo.selector, @@ -128,7 +125,7 @@ contract BadValidationMagicValue_PreUserOpValidationHook_Plugin is BasePlugin { } } -contract BadValidationMagicValue_PreExecHook_Plugin is BasePlugin { +contract BadValidationMagicValue_PreExecHook_Plugin is BaseTestPlugin { function onInstall(bytes calldata) external override {} function onUninstall(bytes calldata) external override {} @@ -140,8 +137,8 @@ contract BadValidationMagicValue_PreExecHook_Plugin is BasePlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = ManifestExecutionFunction(this.foo.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.foo.selector; manifest.executionHooks = new ManifestExecutionHook[](1); @@ -164,7 +161,7 @@ contract BadValidationMagicValue_PreExecHook_Plugin is BasePlugin { } } -contract BadValidationMagicValue_PostExecHook_Plugin is BasePlugin { +contract BadValidationMagicValue_PostExecHook_Plugin is BaseTestPlugin { function onInstall(bytes calldata) external override {} function onUninstall(bytes calldata) external override {} @@ -176,11 +173,10 @@ contract BadValidationMagicValue_PostExecHook_Plugin is BasePlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = ManifestExecutionFunction(this.foo.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.foo.selector; manifest.executionHooks = new ManifestExecutionHook[](1); - // Illegal assignment: validation always allow only usable on runtime validation functions manifest.executionHooks[0] = ManifestExecutionHook({ executionSelector: this.foo.selector, @@ -200,7 +196,7 @@ contract BadValidationMagicValue_PostExecHook_Plugin is BasePlugin { } } -contract BadHookMagicValue_UserOpValidationFunction_Plugin is BasePlugin { +contract BadHookMagicValue_UserOpValidationFunction_Plugin is BaseTestPlugin { function onInstall(bytes calldata) external override {} function onUninstall(bytes calldata) external override {} @@ -212,8 +208,8 @@ contract BadHookMagicValue_UserOpValidationFunction_Plugin is BasePlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = ManifestExecutionFunction(this.foo.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.foo.selector; manifest.userOpValidationFunctions = new ManifestAssociatedFunction[](1); manifest.userOpValidationFunctions[0] = ManifestAssociatedFunction({ @@ -229,7 +225,7 @@ contract BadHookMagicValue_UserOpValidationFunction_Plugin is BasePlugin { } } -contract BadHookMagicValue_RuntimeValidationFunction_Plugin is BasePlugin { +contract BadHookMagicValue_RuntimeValidationFunction_Plugin is BaseTestPlugin { function onInstall(bytes calldata) external override {} function onUninstall(bytes calldata) external override {} @@ -241,8 +237,8 @@ contract BadHookMagicValue_RuntimeValidationFunction_Plugin is BasePlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = ManifestExecutionFunction(this.foo.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.foo.selector; manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](1); manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ @@ -258,7 +254,7 @@ contract BadHookMagicValue_RuntimeValidationFunction_Plugin is BasePlugin { } } -contract BadHookMagicValue_PostExecHook_Plugin is BasePlugin { +contract BadHookMagicValue_PostExecHook_Plugin is BaseTestPlugin { function onInstall(bytes calldata) external override {} function onUninstall(bytes calldata) external override {} @@ -270,11 +266,10 @@ contract BadHookMagicValue_PostExecHook_Plugin is BasePlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = ManifestExecutionFunction(this.foo.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.foo.selector; manifest.executionHooks = new ManifestExecutionHook[](1); - // Illegal assignment: hook always deny only usable on runtime validation functions manifest.executionHooks[0] = ManifestExecutionHook({ executionSelector: this.foo.selector, diff --git a/test/mocks/plugins/ReturnDataPluginMocks.sol b/test/mocks/plugins/ReturnDataPluginMocks.sol index c0dd4d85..e226a329 100644 --- a/test/mocks/plugins/ReturnDataPluginMocks.sol +++ b/test/mocks/plugins/ReturnDataPluginMocks.sol @@ -6,13 +6,12 @@ import { ManifestAssociatedFunctionType, ManifestAssociatedFunction, ManifestExternalCallPermission, - PluginManifest, - ManifestExecutionFunction + PluginManifest } from "../../../src/interfaces/IPlugin.sol"; import {IStandardExecutor} from "../../../src/interfaces/IStandardExecutor.sol"; import {IPluginExecutor} from "../../../src/interfaces/IPluginExecutor.sol"; import {IPlugin} from "../../../src/interfaces/IPlugin.sol"; -import {BasePlugin} from "../../../src/plugins/BasePlugin.sol"; +import {BaseTestPlugin} from "./BaseTestPlugin.sol"; import {FunctionReference} from "../../../src/libraries/FunctionReferenceLib.sol"; contract RegularResultContract { @@ -25,7 +24,7 @@ contract RegularResultContract { } } -contract ResultCreatorPlugin is BasePlugin { +contract ResultCreatorPlugin is BaseTestPlugin { function onInstall(bytes calldata) external override {} function onUninstall(bytes calldata) external override {} @@ -41,9 +40,9 @@ contract ResultCreatorPlugin is BasePlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new ManifestExecutionFunction[](2); - manifest.executionFunctions[0] = ManifestExecutionFunction(this.foo.selector, new string[](0)); - manifest.executionFunctions[1] = ManifestExecutionFunction(this.bar.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](2); + manifest.executionFunctions[0] = this.foo.selector; + manifest.executionFunctions[1] = this.bar.selector; manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](1); manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ @@ -59,7 +58,7 @@ contract ResultCreatorPlugin is BasePlugin { } } -contract ResultConsumerPlugin is BasePlugin { +contract ResultConsumerPlugin is BaseTestPlugin { ResultCreatorPlugin public immutable resultCreator; RegularResultContract public immutable regularResultContract; @@ -117,11 +116,9 @@ contract ResultConsumerPlugin is BasePlugin { function _innerPluginManifest() internal view returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new ManifestExecutionFunction[](2); - manifest.executionFunctions[0] = - ManifestExecutionFunction(this.checkResultEFPFallback.selector, new string[](0)); - manifest.executionFunctions[1] = - ManifestExecutionFunction(this.checkResultEFPExternal.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](2); + manifest.executionFunctions[0] = this.checkResultEFPFallback.selector; + manifest.executionFunctions[1] = this.checkResultEFPExternal.selector; manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](2); manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ diff --git a/test/mocks/plugins/ValidationPluginMocks.sol b/test/mocks/plugins/ValidationPluginMocks.sol index adcc7052..1e6cc941 100644 --- a/test/mocks/plugins/ValidationPluginMocks.sol +++ b/test/mocks/plugins/ValidationPluginMocks.sol @@ -7,12 +7,11 @@ import { ManifestFunction, ManifestAssociatedFunctionType, ManifestAssociatedFunction, - PluginManifest, - ManifestExecutionFunction + PluginManifest } from "../../../src/interfaces/IPlugin.sol"; -import {BasePlugin} from "../../../src/plugins/BasePlugin.sol"; +import {BaseTestPlugin} from "./BaseTestPlugin.sol"; -abstract contract MockBaseUserOpValidationPlugin is BasePlugin { +abstract contract MockBaseUserOpValidationPlugin is BaseTestPlugin { enum FunctionId { USER_OP_VALIDATION, PRE_USER_OP_VALIDATION_HOOK_1, @@ -76,8 +75,8 @@ contract MockUserOpValidationPlugin is MockBaseUserOpValidationPlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = ManifestExecutionFunction(this.foo.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.foo.selector; manifest.userOpValidationFunctions = new ManifestAssociatedFunction[](1); manifest.userOpValidationFunctions[0] = ManifestAssociatedFunction({ @@ -114,8 +113,8 @@ contract MockUserOpValidation1HookPlugin is MockBaseUserOpValidationPlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = ManifestExecutionFunction(this.bar.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.bar.selector; ManifestFunction memory userOpValidationFunctionRef = ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, @@ -166,8 +165,8 @@ contract MockUserOpValidation2HookPlugin is MockBaseUserOpValidationPlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = ManifestExecutionFunction(this.baz.selector, new string[](0)); + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.baz.selector; ManifestFunction memory userOpValidationFunctionRef = ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, From 81c7a6ce9a8c86a5fc9d7d7772351caf2443d228 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Tue, 28 Nov 2023 16:51:41 -0800 Subject: [PATCH 09/20] add canSpendNativeToken support in code --- src/account/PluginManagerInternals.sol | 6 ++++++ src/account/UpgradeableModularAccount.sol | 6 ++++++ src/libraries/AccountStorage.sol | 3 +++ 3 files changed, 15 insertions(+) diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index d66993fc..39050c41 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -376,6 +376,12 @@ abstract contract PluginManagerInternals is IPluginManager { // Update components according to the manifest. // All conflicts should revert. + + // Mark whether or not this plugin may spend native token amounts + if (manifest.canSpendNativeToken) { + _storage.pluginData[plugin].canSpendNativeToken = true; + } + length = manifest.executionFunctions.length; for (uint256 i = 0; i < length;) { _setExecutionFunction(manifest.executionFunctions[i], plugin); diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 1467225b..d4706d6a 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -55,6 +55,7 @@ contract UpgradeableModularAccount is error ExecFromPluginNotPermitted(address plugin, bytes4 selector); 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); @@ -224,6 +225,11 @@ contract UpgradeableModularAccount is bytes4 selector = bytes4(data); AccountStorage storage _storage = getAccountStorage(); + // Make sure plugin is allowed to spend native token. + if (value > 0 && value > msg.value && !_storage.pluginData[msg.sender].canSpendNativeToken) { + revert NativeTokenSpendingNotPermitted(msg.sender); + } + // Check the caller plugin's permission to make this call // Check the target contract permission. diff --git a/src/libraries/AccountStorage.sol b/src/libraries/AccountStorage.sol index 4254e2c2..7439dd56 100644 --- a/src/libraries/AccountStorage.sol +++ b/src/libraries/AccountStorage.sol @@ -10,6 +10,9 @@ bytes32 constant _ACCOUNT_STORAGE_SLOT = 0x9f09680beaa4e5c9f38841db2460c40149916 struct PluginData { bool anyExternalExecPermitted; + // boolean to indicate if the plugin can spend native tokens, if any of the execution function can spend + // native tokens, a plugin is considered to be able to spend native tokens of the accounts + bool canSpendNativeToken; bytes32 manifestHash; FunctionReference[] dependencies; // Tracks the number of times this plugin has been used as a dependency function From 2c8a1241ebc1e5ea3ac8ec3c714e376a9ab142d3 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Wed, 29 Nov 2023 09:54:03 -0800 Subject: [PATCH 10/20] re-arrange imports and inheritance --- src/account/UpgradeableModularAccount.sol | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index d4706d6a..eb9cb5cc 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -10,30 +10,26 @@ import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeab import {AccountExecutor} from "./AccountExecutor.sol"; import {AccountLoupe} from "./AccountLoupe.sol"; -import {IPlugin, PluginManifest} from "../interfaces/IPlugin.sol"; -import {IStandardExecutor, Call} from "../interfaces/IStandardExecutor.sol"; -import {IPluginExecutor} from "../interfaces/IPluginExecutor.sol"; import {AccountStorage, getAccountStorage, getPermittedCallKey} from "../libraries/AccountStorage.sol"; import {AccountStorageInitializable} from "./AccountStorageInitializable.sol"; import {FunctionReference, FunctionReferenceLib} from "../libraries/FunctionReferenceLib.sol"; import {IPlugin, PluginManifest} from "../interfaces/IPlugin.sol"; import {IPluginExecutor} from "../interfaces/IPluginExecutor.sol"; import {IPluginManager} from "../interfaces/IPluginManager.sol"; -import {IStandardExecutor} from "../interfaces/IStandardExecutor.sol"; +import {IStandardExecutor, Call} from "../interfaces/IStandardExecutor.sol"; import {PluginManagerInternals} from "./PluginManagerInternals.sol"; import {_coalescePreValidation, _coalesceValidation} from "../helpers/ValidationDataHelpers.sol"; contract UpgradeableModularAccount is AccountExecutor, - IPluginManager, - BaseAccount, - PluginManagerInternals, AccountLoupe, - UUPSUpgradeable, AccountStorageInitializable, + BaseAccount, IERC165, + IPluginExecutor, IStandardExecutor, - IPluginExecutor + PluginManagerInternals, + UUPSUpgradeable { using EnumerableSet for EnumerableSet.Bytes32Set; From 0e95e25750bdda1c7fa6049beff125f352800ba3 Mon Sep 17 00:00:00 2001 From: adam Date: Wed, 29 Nov 2023 12:22:33 -0500 Subject: [PATCH 11/20] Remove uninstall field checking --- src/account/PluginManagerInternals.sol | 84 ++++---------------- test/account/UpgradeableModularAccount.t.sol | 11 +-- 2 files changed, 16 insertions(+), 79 deletions(-) diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 39050c41..eb3833e8 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -31,14 +31,8 @@ abstract contract PluginManagerInternals is IPluginManager { error ArrayLengthMismatch(); error ExecuteFromPluginAlreadySet(bytes4 selector, address plugin); - error PermittedExecutionSelectorNotInstalled(bytes4 selector, address plugin); - error ExecuteFromPluginNotSet(bytes4 selector, address plugin); error ExecutionFunctionAlreadySet(bytes4 selector); - error ExecutionFunctionNotSet(bytes4 selector); error ExecutionHookAlreadySet(bytes4 selector, FunctionReference hook); - error ExecutionHookNotSet(bytes4 selector, FunctionReference hook); - error InvalidPostExecHook(bytes4 selector, FunctionReference hook); - error InvalidPostPermittedCallHook(bytes4 selector, FunctionReference hook); error InvalidDependenciesProvided(); error InvalidPluginManifest(); error MissingPluginDependency(address dependency); @@ -47,18 +41,13 @@ abstract contract PluginManagerInternals is IPluginManager { error PluginAlreadyInstalled(address plugin); error PluginDependencyViolation(address plugin); error PermittedCallHookAlreadySet(bytes4 selector, address plugin, FunctionReference hook); - error PermittedCallHookNotSet(bytes4 selector, address plugin, FunctionReference hook); error PluginInstallCallbackFailed(address plugin, bytes revertReason); error PluginInterfaceNotSupported(address plugin); error PluginNotInstalled(address plugin); error PreRuntimeValidationHookAlreadySet(bytes4 selector, FunctionReference hook); - error PreRuntimeValidationHookNotSet(bytes4 selector, FunctionReference hook); error PreUserOpValidationHookAlreadySet(bytes4 selector, FunctionReference hook); - error PreUserOpValidationHookNotSet(bytes4 selector, FunctionReference hook); error RuntimeValidationFunctionAlreadySet(bytes4 selector, FunctionReference validationFunction); - error RuntimeValidationFunctionNotSet(bytes4 selector, FunctionReference validationFunction); error UserOpValidationFunctionAlreadySet(bytes4 selector, FunctionReference validationFunction); - error UserOpValidationFunctionNotSet(bytes4 selector, FunctionReference validationFunction); error PluginApplyHookCallbackFailed(address providingPlugin, bytes revertReason); error PluginUnapplyHookCallbackFailed(address providingPlugin, bytes revertReason); @@ -91,10 +80,6 @@ abstract contract PluginManagerInternals is IPluginManager { function _removeExecutionFunction(bytes4 selector) internal { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - if (_selectorData.plugin == address(0)) { - revert ExecutionFunctionNotSet(selector); - } - _selectorData.plugin = address(0); } @@ -117,14 +102,6 @@ abstract contract PluginManagerInternals is IPluginManager { { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - if (_selectorData.userOpValidation != validationFunction) { - // Revert if there's a different validationFunction set than the one the manifest intendes to remove. - // This - // indicates something wrong with the manifest and should not be allowed. In these cases, the original - // manifest should be passed for uninstall. - revert UserOpValidationFunctionNotSet(selector, validationFunction); - } - _selectorData.userOpValidation = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; } @@ -147,14 +124,6 @@ abstract contract PluginManagerInternals is IPluginManager { { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - if (_selectorData.runtimeValidation != validationFunction) { - // Revert if there's a different validationFunction set than the one the manifest intendes to remove. - // This - // indicates something wrong with the manifest and should not be allowed. In these cases, the original - // manifest should be passed for uninstall. - revert RuntimeValidationFunctionNotSet(selector, validationFunction); - } - _selectorData.runtimeValidation = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; } @@ -181,15 +150,9 @@ abstract contract PluginManagerInternals is IPluginManager { { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - // Removal also clears the flags. - if (!_selectorData.preExecHooks.remove(_toSetValue(preExecHook))) { - revert ExecutionHookNotSet(selector, preExecHook); - } + // May ignore return value, as the manifest hash is validated to ensure that the hook exists. + _selectorData.preExecHooks.remove(_toSetValue(preExecHook)); - // Remove the associated post-exec hook, if it is set to the expected value. - if (postExecHook != _selectorData.associatedPostExecHooks[preExecHook]) { - revert InvalidPostExecHook(selector, postExecHook); - } // If the post exec hook is set, clear it. if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { _selectorData.associatedPostExecHooks[preExecHook] = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; @@ -201,13 +164,8 @@ abstract contract PluginManagerInternals is IPluginManager { { bytes24 key = getPermittedCallKey(plugin, selector); - if (accountStorage.selectorData[selector].plugin == address(0)) { - revert PermittedExecutionSelectorNotInstalled(selector, plugin); - } - - if (accountStorage.permittedCalls[key].callPermitted) { - revert ExecuteFromPluginAlreadySet(selector, plugin); - } + // If there are duplicates, this will just enable the flag again. This is not a problem, since the boolean + // will be set to false twice during uninstall, which is fine. accountStorage.permittedCalls[key].callPermitted = true; } @@ -215,9 +173,6 @@ abstract contract PluginManagerInternals is IPluginManager { internal { bytes24 key = getPermittedCallKey(plugin, selector); - if (!accountStorage.permittedCalls[key].callPermitted) { - revert ExecuteFromPluginNotSet(selector, plugin); - } accountStorage.permittedCalls[key].callPermitted = false; } @@ -250,14 +205,9 @@ abstract contract PluginManagerInternals is IPluginManager { bytes24 permittedCallKey = getPermittedCallKey(plugin, selector); PermittedCallData storage _permittedCalldata = getAccountStorage().permittedCalls[permittedCallKey]; - if (!_permittedCalldata.prePermittedCallHooks.remove(_toSetValue(preExecHook))) { - revert PermittedCallHookNotSet(selector, plugin, preExecHook); - } + // May ignore return value, as the manifest hash is validated to ensure that the hook exists. + _permittedCalldata.prePermittedCallHooks.remove(_toSetValue(preExecHook)); - // Remove the associated post-exec hook, if it is set to the expected value. - if (postExecHook != _permittedCalldata.associatedPostPermittedCallHooks[preExecHook]) { - revert InvalidPostPermittedCallHook(selector, postExecHook); - } // If the post permitted call exec hook is set, clear it. if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { _permittedCalldata.associatedPostPermittedCallHooks[preExecHook] = @@ -282,13 +232,10 @@ abstract contract PluginManagerInternals is IPluginManager { internal notNullFunction(preUserOpValidationHook) { - if ( - !getAccountStorage().selectorData[selector].preUserOpValidationHooks.remove( - _toSetValue(preUserOpValidationHook) - ) - ) { - revert PreUserOpValidationHookNotSet(selector, preUserOpValidationHook); - } + // May ignore return value, as the manifest hash is validated to ensure that the hook exists. + getAccountStorage().selectorData[selector].preUserOpValidationHooks.remove( + _toSetValue(preUserOpValidationHook) + ); } function _addPreRuntimeValidationHook(bytes4 selector, FunctionReference preRuntimeValidationHook) @@ -308,13 +255,10 @@ abstract contract PluginManagerInternals is IPluginManager { internal notNullFunction(preRuntimeValidationHook) { - if ( - !getAccountStorage().selectorData[selector].preRuntimeValidationHooks.remove( - _toSetValue(preRuntimeValidationHook) - ) - ) { - revert PreRuntimeValidationHookNotSet(selector, preRuntimeValidationHook); - } + // May ignore return value, as the manifest hash is validated to ensure that the hook exists. + getAccountStorage().selectorData[selector].preRuntimeValidationHooks.remove( + _toSetValue(preRuntimeValidationHook) + ); } function _installPlugin( diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index 421f08da..874b1e98 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -296,7 +296,7 @@ contract UpgradeableModularAccountTest is Test { assertEq(plugins[1], address(tokenReceiverPlugin)); } - function test_installPlugin_ExecuteFromPlugin_BadPermittedExecSelector() public { + function test_installPlugin_ExecuteFromPlugin_PermittedExecSelectorNotInstalled() public { vm.startPrank(owner2); PluginManifest memory m; @@ -306,13 +306,6 @@ contract UpgradeableModularAccountTest is Test { MockPlugin mockPluginWithBadPermittedExec = new MockPlugin(m); bytes32 manifestHash = keccak256(abi.encode(mockPluginWithBadPermittedExec.pluginManifest())); - vm.expectRevert( - abi.encodeWithSelector( - PluginManagerInternals.PermittedExecutionSelectorNotInstalled.selector, - IPlugin.onInstall.selector, - address(mockPluginWithBadPermittedExec) - ) - ); IPluginManager(account2).installPlugin({ plugin: address(mockPluginWithBadPermittedExec), manifestHash: manifestHash, @@ -590,7 +583,7 @@ contract UpgradeableModularAccountTest is Test { } function test_injectHooksUninstall() external { - (, MockPlugin newPlugin, bytes32 manifestHash) = _installWithInjectHooks(); + (, MockPlugin newPlugin,) = _installWithInjectHooks(); vm.expectEmit(true, true, true, true); emit PluginUninstalled(address(newPlugin), true); From 59c482a960eb653656fcdbafeb91527e92a943e0 Mon Sep 17 00:00:00 2001 From: adam Date: Wed, 29 Nov 2023 17:14:19 -0500 Subject: [PATCH 12/20] Move exec hooks and permitted call hooks to hook group --- src/account/AccountLoupe.sol | 9 +++++---- src/account/PluginManagerInternals.sol | 19 ++++++++++--------- src/account/UpgradeableModularAccount.sol | 12 +++++++----- src/libraries/AccountStorage.sol | 18 +++++++++++------- 4 files changed, 33 insertions(+), 25 deletions(-) diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol index 526fe297..6baac4e1 100644 --- a/src/account/AccountLoupe.sol +++ b/src/account/AccountLoupe.sol @@ -50,14 +50,15 @@ abstract contract AccountLoupe is IAccountLoupe { AccountStorage storage _storage = getAccountStorage(); FunctionReference[] memory preExecHooks = - toFunctionReferenceArray(_storage.selectorData[selector].preExecHooks); + toFunctionReferenceArray(_storage.selectorData[selector].executionHooks.preHooks); uint256 numHooks = preExecHooks.length; execHooks = new ExecutionHooks[](numHooks); for (uint256 i = 0; i < numHooks;) { execHooks[i].preExecHook = preExecHooks[i]; - execHooks[i].postExecHook = _storage.selectorData[selector].associatedPostExecHooks[preExecHooks[i]]; + execHooks[i].postExecHook = + _storage.selectorData[selector].executionHooks.associatedPostHooks[preExecHooks[i]]; unchecked { ++i; @@ -76,7 +77,7 @@ abstract contract AccountLoupe is IAccountLoupe { bytes24 key = getPermittedCallKey(callingPlugin, selector); FunctionReference[] memory prePermittedCallHooks = - toFunctionReferenceArray(_storage.permittedCalls[key].prePermittedCallHooks); + toFunctionReferenceArray(_storage.permittedCalls[key].permittedCallHooks.preHooks); uint256 numHooks = prePermittedCallHooks.length; execHooks = new ExecutionHooks[](numHooks); @@ -84,7 +85,7 @@ abstract contract AccountLoupe is IAccountLoupe { for (uint256 i = 0; i < numHooks;) { execHooks[i].preExecHook = prePermittedCallHooks[i]; execHooks[i].postExecHook = - _storage.permittedCalls[key].associatedPostPermittedCallHooks[prePermittedCallHooks[i]]; + _storage.permittedCalls[key].permittedCallHooks.associatedPostHooks[prePermittedCallHooks[i]]; unchecked { ++i; diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index eb3833e8..10494de2 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -133,14 +133,14 @@ abstract contract PluginManagerInternals is IPluginManager { { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - if (!_selectorData.preExecHooks.add(_toSetValue(preExecHook))) { + if (!_selectorData.executionHooks.preHooks.add(_toSetValue(preExecHook))) { // Treat the pre-exec and post-exec hook as a single unit, identified by the pre-exec hook. // If the pre-exec hook exists, revert. revert ExecutionHookAlreadySet(selector, preExecHook); } if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { - _selectorData.associatedPostExecHooks[preExecHook] = postExecHook; + _selectorData.executionHooks.associatedPostHooks[preExecHook] = postExecHook; } } @@ -151,11 +151,12 @@ abstract contract PluginManagerInternals is IPluginManager { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; // May ignore return value, as the manifest hash is validated to ensure that the hook exists. - _selectorData.preExecHooks.remove(_toSetValue(preExecHook)); + _selectorData.executionHooks.preHooks.remove(_toSetValue(preExecHook)); // If the post exec hook is set, clear it. if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { - _selectorData.associatedPostExecHooks[preExecHook] = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; + _selectorData.executionHooks.associatedPostHooks[preExecHook] = + FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; } } @@ -185,14 +186,14 @@ abstract contract PluginManagerInternals is IPluginManager { bytes24 permittedCallKey = getPermittedCallKey(plugin, selector); PermittedCallData storage _permittedCalldata = getAccountStorage().permittedCalls[permittedCallKey]; - if (!_permittedCalldata.prePermittedCallHooks.add(_toSetValue(preExecHook))) { + if (!_permittedCalldata.permittedCallHooks.preHooks.add(_toSetValue(preExecHook))) { // Treat the pre-exec and post-exec hook as a single unit, identified by the pre-exec hook. // If the pre-exec hook exists, revert. revert PermittedCallHookAlreadySet(selector, plugin, preExecHook); } if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { - _permittedCalldata.associatedPostPermittedCallHooks[preExecHook] = postExecHook; + _permittedCalldata.permittedCallHooks.associatedPostHooks[preExecHook] = postExecHook; } } @@ -203,14 +204,14 @@ abstract contract PluginManagerInternals is IPluginManager { FunctionReference postExecHook ) internal notNullPlugin(plugin) notNullFunction(preExecHook) { bytes24 permittedCallKey = getPermittedCallKey(plugin, selector); - PermittedCallData storage _permittedCalldata = getAccountStorage().permittedCalls[permittedCallKey]; + PermittedCallData storage _permittedCallData = getAccountStorage().permittedCalls[permittedCallKey]; // May ignore return value, as the manifest hash is validated to ensure that the hook exists. - _permittedCalldata.prePermittedCallHooks.remove(_toSetValue(preExecHook)); + _permittedCallData.permittedCallHooks.preHooks.remove(_toSetValue(preExecHook)); // If the post permitted call exec hook is set, clear it. if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { - _permittedCalldata.associatedPostPermittedCallHooks[preExecHook] = + _permittedCallData.permittedCallHooks.associatedPostHooks[preExecHook] = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; } } diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index eb9cb5cc..09f8559e 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -477,7 +477,8 @@ contract UpgradeableModularAccount is } function _doPreExecHooks(bytes4 selector) internal returns (PostExecToRun[] memory postHooksToRun) { - EnumerableSet.Bytes32Set storage preExecHooks = getAccountStorage().selectorData[selector].preExecHooks; + EnumerableSet.Bytes32Set storage preExecHooks = + getAccountStorage().selectorData[selector].executionHooks.preHooks; uint256 postExecHooksLength = 0; uint256 preExecHooksLength = preExecHooks.length(); @@ -508,7 +509,7 @@ contract UpgradeableModularAccount is // Check to see if there is a postExec hook set for this preExec hook FunctionReference postExecHook = - getAccountStorage().selectorData[selector].associatedPostExecHooks[preExecHook]; + getAccountStorage().selectorData[selector].executionHooks.associatedPostHooks[preExecHook]; if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { postHooksToRun[postExecHooksLength].postExecHook = postExecHook; postHooksToRun[postExecHooksLength].preExecHookReturnData = preExecHookReturnData; @@ -530,7 +531,7 @@ contract UpgradeableModularAccount is bytes24 permittedCallKey = getPermittedCallKey(callerPlugin, executionSelector); EnumerableSet.Bytes32Set storage preExecHooks = - getAccountStorage().permittedCalls[permittedCallKey].prePermittedCallHooks; + getAccountStorage().permittedCalls[permittedCallKey].permittedCallHooks.preHooks; uint256 postExecHooksLength = 0; uint256 preExecHooksLength = preExecHooks.length(); @@ -559,8 +560,9 @@ contract UpgradeableModularAccount is } // Check to see if there is a postExec hook set for this preExec hook - FunctionReference postExecHook = - getAccountStorage().permittedCalls[permittedCallKey].associatedPostPermittedCallHooks[preExecHook]; + FunctionReference postExecHook = getAccountStorage().permittedCalls[permittedCallKey] + .permittedCallHooks + .associatedPostHooks[preExecHook]; if (FunctionReference.unwrap(postExecHook) != 0) { postHooksToRun[postExecHooksLength].postExecHook = postExecHook; postHooksToRun[postExecHooksLength].preExecHookReturnData = preExecHookReturnData; diff --git a/src/libraries/AccountStorage.sol b/src/libraries/AccountStorage.sol index 7439dd56..3e52e372 100644 --- a/src/libraries/AccountStorage.sol +++ b/src/libraries/AccountStorage.sol @@ -20,7 +20,7 @@ struct PluginData { StoredInjectedHook[] injectedHooks; } -// A version of IPluginManager. InjectedHook used to track injected hooks in storage. +// A version of IPluginManager.InjectedHook used to track injected hooks in storage. // Omits the hookApplyData field, which is not needed for storage, and flattens the struct. struct StoredInjectedHook { // The plugin that provides the hook @@ -37,9 +37,7 @@ struct StoredInjectedHook { // to interact with another plugin installed on the account. struct PermittedCallData { bool callPermitted; - EnumerableSet.Bytes32Set prePermittedCallHooks; - // bytes21 key = pre exec hook function reference - mapping(FunctionReference => FunctionReference) associatedPostPermittedCallHooks; + HookGroup permittedCallHooks; } // Represents data associated with a plugin's permission to use `executeFromPluginExternal` @@ -52,6 +50,14 @@ struct PermittedExternalCallData { mapping(bytes4 => bool) permittedSelectors; } +// Represets a set of pre- and post- hooks. Used to store both execution hooks and permitted call hooks. +struct HookGroup { + EnumerableSet.Bytes32Set preHooks; + // bytes21 key = pre hook function reference + mapping(FunctionReference => FunctionReference) associatedPostHooks; + EnumerableSet.Bytes32Set postOnlyHooks; +} + // Represents data associated with a specifc function selector. struct SelectorData { // The plugin that implements this execution function. @@ -63,9 +69,7 @@ struct SelectorData { EnumerableSet.Bytes32Set preUserOpValidationHooks; EnumerableSet.Bytes32Set preRuntimeValidationHooks; // The execution hooks for this function selector. - EnumerableSet.Bytes32Set preExecHooks; - // bytes21 key = pre exec hook function reference - mapping(FunctionReference => FunctionReference) associatedPostExecHooks; + HookGroup executionHooks; } struct AccountStorage { From cc85a74efdb63cffd16d25d466bb7b0e9d265073 Mon Sep 17 00:00:00 2001 From: Jay Paik Date: Tue, 28 Nov 2023 23:34:30 -0500 Subject: [PATCH 13/20] perf: precompile contracts for faster test runs --- .github/workflows/test.yml | 20 +++---- .gitignore | 5 ++ README.md | 18 +++--- foundry.toml | 32 +++++++--- test/TestUtils.sol | 21 ------- test/account/AccountLoupe.t.sol | 7 +-- test/account/AccountReturnData.t.sol | 7 +-- .../ExecuteFromPluginPermissions.t.sol | 9 ++- test/account/ManifestValidity.t.sol | 7 +-- test/account/UpgradeableModularAccount.t.sol | 9 +-- test/account/ValidationIntersection.t.sol | 7 +-- test/mocks/MSCAFactoryFixture.sol | 6 +- test/plugin/SingleOwnerPlugin.t.sol | 7 +-- test/plugin/TokenReceiverPlugin.t.sol | 10 ++-- test/utils/OptimizedTest.sol | 58 +++++++++++++++++++ 15 files changed, 136 insertions(+), 87 deletions(-) delete mode 100644 test/TestUtils.sol create mode 100644 test/utils/OptimizedTest.sol diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a8527e96..dfa5cf41 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1,4 +1,4 @@ -name: ERC6900 RI Test CI +name: ERC-6900 RI Test CI on: [pull_request, workflow_dispatch] @@ -45,9 +45,9 @@ jobs: - name: "Lint the contracts" run: "pnpm lint" - - test: - name: Run Forge Tests + + test-optimized-test-deep: + name: Run Forge Tests (optimized-test-deep) runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 @@ -63,13 +63,13 @@ jobs: run: forge install - name: Build project - run: forge build + run: FOUNDRY_PROFILE=optimized-build forge build - name: Run tests - run: FOUNDRY_PROFILE=deep forge test -vvv + run: FOUNDRY_PROFILE=optimized-test-deep forge test -vvv - test-lite: - name: Run Forge Tests [lite build] + test-default: + name: Run Forge Tests (default) runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 @@ -85,7 +85,7 @@ jobs: run: forge install - name: Build project - run: FOUNDRY_PROFILE=lite forge build + run: forge build - name: Run tests - run: FOUNDRY_PROFILE=lite forge test -vvv + run: forge test -vvv diff --git a/.gitignore b/.gitignore index 3a109388..3189d156 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,9 @@ # Foundry build and cache directories out/ +out-optimized/ cache/ node_modules/ + +# Coverage +report/ +lcov.info diff --git a/README.md b/README.md index 70657779..fb3e9dbc 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -# ERC-6900 Ref Implementation +# ERC-6900 Reference Implementation Reference implementation for [ERC-6900](https://eips.ethereum.org/EIPS/eip-6900). It is an early draft implementation. @@ -13,20 +13,18 @@ The implementation includes an upgradable modular account with two plugins (`Sin Anyone is welcome to submit feedback and/or PRs to improve code or add Plugins. -### Build +### Testing + +The default Foundry profile can be used to compile (without IR) and test the entire project. The default profile should be used when generating coverage and debugging. ```bash forge build - -# or use the lite profile to reduce compilation time -FOUNDRY_PROFILE=lite forge build +forge test -vvv ``` -### Test +Since IR compilation generates different bytecode, it's useful to test against the contracts compiled via IR. Since compiling the entire project (including the test suite) takes a long time, special profiles can be used to precompile just the source contracts, and have the tests deploy the relevant contracts using those artifacts. ```bash -forge test -vvv - -# or use the lite profile to reduce compilation time -FOUNDRY_PROFILE=lite forge test -vvv +FOUNDRY_PROFILE=optimized-build forge build +FOUNDRY_PROFILE=optimized-test forge test -vvv ``` diff --git a/foundry.toml b/foundry.toml index a5d071fe..2754a5b2 100644 --- a/foundry.toml +++ b/foundry.toml @@ -1,13 +1,16 @@ [profile.default] solc = '0.8.19' -via_ir = true +via_ir = false src = 'src' -out = 'out' test = 'test' libs = ['lib'] +out = 'out' optimizer = true optimizer_runs = 10_000 ignored_error_codes = [3628] +fs_permissions = [ + { access = "read", path = "./out-optimized" } +] [fuzz] runs = 500 @@ -17,12 +20,23 @@ runs=500 fail_on_revert = true depth = 10 -[profile.lite] -solc = '0.8.19' -via_ir = false -optimizer = true -optimizer_runs = 10_000 -ignored_error_codes = [3628] +[profile.optimized-build] +via_ir = true +test = 'src' +out = 'out-optimized' + +[profile.optimized-test] +src = 'test' + +[profile.optimized-test-deep] +src = 'test' + +[profile.optimized-test-deep.fuzz] +runs = 10000 + +[profile.optimized-test-deep.invariant] +runs = 5000 +depth = 32 [profile.deep.fuzz] runs = 10000 @@ -43,4 +57,4 @@ goerli = "${RPC_URL_GOERLI}" mainnet = { key = "${ETHERSCAN_API_KEY}" } goerli = { key = "${ETHERSCAN_API_KEY}" } -# See more config options https://github.com/foundry-rs/foundry/tree/master/config \ No newline at end of file +# See more config options https://github.com/foundry-rs/foundry/tree/master/config diff --git a/test/TestUtils.sol b/test/TestUtils.sol deleted file mode 100644 index 26631b16..00000000 --- a/test/TestUtils.sol +++ /dev/null @@ -1,21 +0,0 @@ -// SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.19; - -import {Test, console} from "forge-std/Test.sol"; - -contract TestUtils is Test { - function printStorageReadsAndWrites(address addr) internal { - (bytes32[] memory accountReads, bytes32[] memory accountWrites) = vm.accesses(addr); - for (uint256 i = 0; i < accountWrites.length; i++) { - bytes32 valWritten = vm.load(addr, accountWrites[i]); - console.log( - string.concat("write loc: ", vm.toString(accountWrites[i]), " val: ", vm.toString(valWritten)) - ); - } - - for (uint256 i = 0; i < accountReads.length; i++) { - bytes32 valRead = vm.load(addr, accountReads[i]); - console.log(string.concat("read: ", vm.toString(accountReads[i]), " val: ", vm.toString(valRead))); - } - } -} diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol index 31d08bb1..0f016895 100644 --- a/test/account/AccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -1,8 +1,6 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; -import {Test} from "forge-std/Test.sol"; - import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; @@ -23,8 +21,9 @@ import {FunctionReference, FunctionReferenceLib} from "../../src/libraries/Funct import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; import {ComprehensivePlugin} from "../mocks/plugins/ComprehensivePlugin.sol"; import {MockPlugin} from "../mocks/MockPlugin.sol"; +import {OptimizedTest} from "../utils/OptimizedTest.sol"; -contract AccountLoupeTest is Test { +contract AccountLoupeTest is OptimizedTest { EntryPoint public entryPoint; SingleOwnerPlugin public singleOwnerPlugin; MSCAFactoryFixture public factory; @@ -40,7 +39,7 @@ contract AccountLoupeTest is Test { function setUp() public { entryPoint = new EntryPoint(); - singleOwnerPlugin = new SingleOwnerPlugin(); + singleOwnerPlugin = _deploySingleOwnerPlugin(); factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); comprehensivePlugin = new ComprehensivePlugin(); diff --git a/test/account/AccountReturnData.t.sol b/test/account/AccountReturnData.t.sol index dc5c350f..d2772a95 100644 --- a/test/account/AccountReturnData.t.sol +++ b/test/account/AccountReturnData.t.sol @@ -1,8 +1,6 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; -import {Test} from "forge-std/Test.sol"; - import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; @@ -17,9 +15,10 @@ import { ResultConsumerPlugin } from "../mocks/plugins/ReturnDataPluginMocks.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; +import {OptimizedTest} from "../utils/OptimizedTest.sol"; // Tests all the different ways that return data can be read from plugins through an account -contract AccountReturnDataTest is Test { +contract AccountReturnDataTest is OptimizedTest { EntryPoint public entryPoint; // Just to be able to construct the factory SingleOwnerPlugin public singleOwnerPlugin; MSCAFactoryFixture public factory; @@ -32,7 +31,7 @@ contract AccountReturnDataTest is Test { function setUp() public { entryPoint = new EntryPoint(); - singleOwnerPlugin = new SingleOwnerPlugin(); + singleOwnerPlugin = _deploySingleOwnerPlugin(); factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); regularResultContract = new RegularResultContract(); diff --git a/test/account/ExecuteFromPluginPermissions.t.sol b/test/account/ExecuteFromPluginPermissions.t.sol index 82e5d027..48b65d98 100644 --- a/test/account/ExecuteFromPluginPermissions.t.sol +++ b/test/account/ExecuteFromPluginPermissions.t.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; -import {Test, console} from "forge-std/Test.sol"; +import {console} from "forge-std/Test.sol"; import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; @@ -11,18 +11,17 @@ import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {FunctionReference} from "../../src/libraries/FunctionReferenceLib.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; - import {Counter} from "../mocks/Counter.sol"; import {ResultCreatorPlugin} from "../mocks/plugins/ReturnDataPluginMocks.sol"; - import { EFPCallerPlugin, EFPCallerPluginAnyExternal, EFPPermittedCallHookPlugin, EFPExternalPermittedCallHookPlugin } from "../mocks/plugins/ExecFromPluginPermissionsMocks.sol"; +import {OptimizedTest} from "../utils/OptimizedTest.sol"; -contract ExecuteFromPluginPermissionsTest is Test { +contract ExecuteFromPluginPermissionsTest is OptimizedTest { Counter public counter1; Counter public counter2; Counter public counter3; @@ -47,7 +46,7 @@ contract ExecuteFromPluginPermissionsTest is Test { // Initialize the contracts needed to use the account. entryPoint = new EntryPoint(); - singleOwnerPlugin = new SingleOwnerPlugin(); + singleOwnerPlugin = _deploySingleOwnerPlugin(); factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); // Initialize the EFP caller plugins, which will attempt to use the permissions system to authorize calls. diff --git a/test/account/ManifestValidity.t.sol b/test/account/ManifestValidity.t.sol index 08de7f63..18972d07 100644 --- a/test/account/ManifestValidity.t.sol +++ b/test/account/ManifestValidity.t.sol @@ -1,8 +1,6 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; -import {Test} from "forge-std/Test.sol"; - import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; @@ -22,8 +20,9 @@ import { BadHookMagicValue_RuntimeValidationFunction_Plugin, BadHookMagicValue_PostExecHook_Plugin } from "../mocks/plugins/ManifestValidityMocks.sol"; +import {OptimizedTest} from "../utils/OptimizedTest.sol"; -contract ManifestValidityTest is Test { +contract ManifestValidityTest is OptimizedTest { EntryPoint public entryPoint; // Just to be able to construct the factory SingleOwnerPlugin public singleOwnerPlugin; MSCAFactoryFixture public factory; @@ -32,7 +31,7 @@ contract ManifestValidityTest is Test { function setUp() public { entryPoint = new EntryPoint(); - singleOwnerPlugin = new SingleOwnerPlugin(); + singleOwnerPlugin = _deploySingleOwnerPlugin(); factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); // Create an account with "this" as the owner, so we can execute along the runtime path with regular diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index 874b1e98..528eedd2 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; -import {Test, console} from "forge-std/Test.sol"; +import {console} from "forge-std/Test.sol"; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; @@ -23,8 +23,9 @@ import {Counter} from "../mocks/Counter.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; import {ComprehensivePlugin} from "../mocks/plugins/ComprehensivePlugin.sol"; import {MockPlugin} from "../mocks/MockPlugin.sol"; +import {OptimizedTest} from "../utils/OptimizedTest.sol"; -contract UpgradeableModularAccountTest is Test { +contract UpgradeableModularAccountTest is OptimizedTest { using ECDSA for bytes32; EntryPoint public entryPoint; @@ -68,8 +69,8 @@ contract UpgradeableModularAccountTest is Test { beneficiary = payable(makeAddr("beneficiary")); vm.deal(beneficiary, 1 wei); - singleOwnerPlugin = new SingleOwnerPlugin(); - tokenReceiverPlugin = new TokenReceiverPlugin(); + singleOwnerPlugin = _deploySingleOwnerPlugin(); + tokenReceiverPlugin = _deployTokenReceiverPlugin(); factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); // Compute counterfactual address diff --git a/test/account/ValidationIntersection.t.sol b/test/account/ValidationIntersection.t.sol index fa2a8390..b33f5187 100644 --- a/test/account/ValidationIntersection.t.sol +++ b/test/account/ValidationIntersection.t.sol @@ -1,8 +1,6 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; -import {Test} from "forge-std/Test.sol"; - import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; import {UserOperation} from "@eth-infinitism/account-abstraction/interfaces/UserOperation.sol"; @@ -18,8 +16,9 @@ import { MockUserOpValidation2HookPlugin, MockUserOpValidationPlugin } from "../mocks/plugins/ValidationPluginMocks.sol"; +import {OptimizedTest} from "../utils/OptimizedTest.sol"; -contract ValidationIntersectionTest is Test { +contract ValidationIntersectionTest is OptimizedTest { uint256 internal constant _SIG_VALIDATION_FAILED = 1; EntryPoint public entryPoint; @@ -35,7 +34,7 @@ contract ValidationIntersectionTest is Test { entryPoint = new EntryPoint(); owner1 = makeAddr("owner1"); - SingleOwnerPlugin singleOwnerPlugin = new SingleOwnerPlugin(); + SingleOwnerPlugin singleOwnerPlugin = _deploySingleOwnerPlugin(); MSCAFactoryFixture factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); account1 = factory.createAccount(owner1, 0); diff --git a/test/mocks/MSCAFactoryFixture.sol b/test/mocks/MSCAFactoryFixture.sol index f920eaea..03b90736 100644 --- a/test/mocks/MSCAFactoryFixture.sol +++ b/test/mocks/MSCAFactoryFixture.sol @@ -8,12 +8,14 @@ import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntry import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; +import {OptimizedTest} from "../utils/OptimizedTest.sol"; + /** * @title MSCAFactoryFixture * @dev a factory that initializes UpgradeableModularAccounts with a single plugin, SingleOwnerPlugin * intended for unit tests and local development, not for production. */ -contract MSCAFactoryFixture { +contract MSCAFactoryFixture is OptimizedTest { UpgradeableModularAccount public accountImplementation; SingleOwnerPlugin public singleOwnerPlugin; bytes32 private immutable _PROXY_BYTECODE_HASH; @@ -28,7 +30,7 @@ contract MSCAFactoryFixture { constructor(IEntryPoint _entryPoint, SingleOwnerPlugin _singleOwnerPlugin) { entryPoint = _entryPoint; - accountImplementation = new UpgradeableModularAccount(_entryPoint); + accountImplementation = _deployUpgradeableModularAccount(_entryPoint); _PROXY_BYTECODE_HASH = keccak256( abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(address(accountImplementation), "")) ); diff --git a/test/plugin/SingleOwnerPlugin.t.sol b/test/plugin/SingleOwnerPlugin.t.sol index d64d4a1d..090e5be1 100644 --- a/test/plugin/SingleOwnerPlugin.t.sol +++ b/test/plugin/SingleOwnerPlugin.t.sol @@ -1,8 +1,6 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; -import {Test} from "forge-std/Test.sol"; - import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; import {UserOperation} from "@eth-infinitism/account-abstraction/interfaces/UserOperation.sol"; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; @@ -10,8 +8,9 @@ import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {ISingleOwnerPlugin} from "../../src/plugins/owner/ISingleOwnerPlugin.sol"; import {ContractOwner} from "../mocks/ContractOwner.sol"; +import {OptimizedTest} from "../utils/OptimizedTest.sol"; -contract SingleOwnerPluginTest is Test { +contract SingleOwnerPluginTest is OptimizedTest { using ECDSA for bytes32; SingleOwnerPlugin public plugin; @@ -29,7 +28,7 @@ contract SingleOwnerPluginTest is Test { event OwnershipTransferred(address indexed account, address indexed previousOwner, address indexed newOwner); function setUp() public { - plugin = new SingleOwnerPlugin(); + plugin = _deploySingleOwnerPlugin(); entryPoint = new EntryPoint(); a = makeAddr("a"); diff --git a/test/plugin/TokenReceiverPlugin.t.sol b/test/plugin/TokenReceiverPlugin.t.sol index e664e68b..009b42e9 100644 --- a/test/plugin/TokenReceiverPlugin.t.sol +++ b/test/plugin/TokenReceiverPlugin.t.sol @@ -1,8 +1,6 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; -import {Test} from "forge-std/Test.sol"; - import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol"; import {ERC721PresetMinterPauserAutoId} from @@ -10,7 +8,6 @@ import {ERC721PresetMinterPauserAutoId} from import {IERC777Recipient} from "@openzeppelin/contracts/token/ERC777/IERC777Recipient.sol"; import {IERC1155Receiver} from "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol"; -import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {TokenReceiverPlugin} from "../../src/plugins/TokenReceiverPlugin.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {FunctionReference} from "../../src/libraries/FunctionReferenceLib.sol"; @@ -19,8 +16,9 @@ import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; import {MockERC777} from "../mocks/MockERC777.sol"; import {MockERC1155} from "../mocks/MockERC1155.sol"; +import {OptimizedTest} from "../utils/OptimizedTest.sol"; -contract TokenReceiverPluginTest is Test, IERC1155Receiver { +contract TokenReceiverPluginTest is OptimizedTest, IERC1155Receiver { UpgradeableModularAccount public acct; TokenReceiverPlugin public plugin; @@ -39,10 +37,10 @@ contract TokenReceiverPluginTest is Test, IERC1155Receiver { uint256 internal constant _BATCH_TOKEN_IDS = 5; function setUp() public { - MSCAFactoryFixture factory = new MSCAFactoryFixture(IEntryPoint(address(0)), new SingleOwnerPlugin()); + MSCAFactoryFixture factory = new MSCAFactoryFixture(IEntryPoint(address(0)), _deploySingleOwnerPlugin()); acct = factory.createAccount(address(this), 0); - plugin = new TokenReceiverPlugin(); + plugin = _deployTokenReceiverPlugin(); t0 = new ERC721PresetMinterPauserAutoId("t0", "t0", ""); t0.mint(address(this)); diff --git a/test/utils/OptimizedTest.sol b/test/utils/OptimizedTest.sol new file mode 100644 index 00000000..f9431acc --- /dev/null +++ b/test/utils/OptimizedTest.sol @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.19; + +import {Test} from "forge-std/Test.sol"; + +import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; + +import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; +import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; +import {TokenReceiverPlugin} from "../../src/plugins/TokenReceiverPlugin.sol"; + +/// @dev This contract provides functions to deploy optimized (via IR) precompiled contracts. By compiling just +/// the source contracts (excluding the test suite) via IR, and using the resulting bytecode within the tests +/// (built without IR), we can avoid the significant overhead of compiling the entire test suite via IR. +/// +/// To use the optimized precompiled contracts, the project must first be built with the "optimized-build" profile +/// to populate the artifacts in the `out-optimized` directory. Then use the "optimized-test" or +/// "optimized-test-deep" profile to run the tests. +/// +/// To bypass this behavior for coverage or debugging, use the "default" profile. +abstract contract OptimizedTest is Test { + function _isOptimizedTest() internal returns (bool) { + string memory profile = vm.envOr("FOUNDRY_PROFILE", string("default")); + return _isStringEq(profile, "optimized-test-deep") || _isStringEq(profile, "optimized-test"); + } + + function _isStringEq(string memory a, string memory b) internal pure returns (bool) { + return keccak256(abi.encodePacked(a)) == keccak256(abi.encodePacked(b)); + } + + function _deployUpgradeableModularAccount(IEntryPoint entryPoint) + internal + returns (UpgradeableModularAccount) + { + return _isOptimizedTest() + ? UpgradeableModularAccount( + payable( + deployCode( + "out-optimized/UpgradeableModularAccount.sol/UpgradeableModularAccount.json", + abi.encode(entryPoint) + ) + ) + ) + : new UpgradeableModularAccount(entryPoint); + } + + function _deploySingleOwnerPlugin() internal returns (SingleOwnerPlugin) { + return _isOptimizedTest() + ? SingleOwnerPlugin(deployCode("out-optimized/SingleOwnerPlugin.sol/SingleOwnerPlugin.json")) + : new SingleOwnerPlugin(); + } + + function _deployTokenReceiverPlugin() internal returns (TokenReceiverPlugin) { + return _isOptimizedTest() + ? TokenReceiverPlugin(deployCode("out-optimized/TokenReceiverPlugin.sol/TokenReceiverPlugin.json")) + : new TokenReceiverPlugin(); + } +} From e49abb733ee6d8089994ca08f18a7886b77f64de Mon Sep 17 00:00:00 2001 From: adam Date: Wed, 29 Nov 2023 19:55:55 -0500 Subject: [PATCH 14/20] Add post-only hooks and related tests --- src/account/PluginManagerInternals.sol | 86 +++--- src/account/UpgradeableModularAccount.sol | 56 ++-- test/account/AccountExecHooks.t.sol | 240 ++++++++++++++++ test/account/AccountPermittedCallHooks.t.sol | 271 +++++++++++++++++++ 4 files changed, 598 insertions(+), 55 deletions(-) create mode 100644 test/account/AccountExecHooks.t.sol create mode 100644 test/account/AccountPermittedCallHooks.t.sol diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 10494de2..3fcf2c30 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -10,6 +10,7 @@ import { SelectorData, PermittedCallData, getPermittedCallKey, + HookGroup, PermittedExternalCallData, StoredInjectedHook } from "../libraries/AccountStorage.sol"; @@ -129,35 +130,18 @@ abstract contract PluginManagerInternals is IPluginManager { function _addExecHooks(bytes4 selector, FunctionReference preExecHook, FunctionReference postExecHook) internal - notNullFunction(preExecHook) { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - if (!_selectorData.executionHooks.preHooks.add(_toSetValue(preExecHook))) { - // Treat the pre-exec and post-exec hook as a single unit, identified by the pre-exec hook. - // If the pre-exec hook exists, revert. - revert ExecutionHookAlreadySet(selector, preExecHook); - } - - if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { - _selectorData.executionHooks.associatedPostHooks[preExecHook] = postExecHook; - } + _addHooks(_selectorData.executionHooks, selector, preExecHook, postExecHook); } function _removeExecHooks(bytes4 selector, FunctionReference preExecHook, FunctionReference postExecHook) internal - notNullFunction(preExecHook) { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - // May ignore return value, as the manifest hash is validated to ensure that the hook exists. - _selectorData.executionHooks.preHooks.remove(_toSetValue(preExecHook)); - - // If the post exec hook is set, clear it. - if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { - _selectorData.executionHooks.associatedPostHooks[preExecHook] = - FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; - } + _removeHooks(_selectorData.executionHooks, preExecHook, postExecHook); } function _enableExecFromPlugin(bytes4 selector, address plugin, AccountStorage storage accountStorage) @@ -182,19 +166,11 @@ abstract contract PluginManagerInternals is IPluginManager { address plugin, FunctionReference preExecHook, FunctionReference postExecHook - ) internal notNullPlugin(plugin) notNullFunction(preExecHook) { + ) internal notNullPlugin(plugin) { bytes24 permittedCallKey = getPermittedCallKey(plugin, selector); PermittedCallData storage _permittedCalldata = getAccountStorage().permittedCalls[permittedCallKey]; - if (!_permittedCalldata.permittedCallHooks.preHooks.add(_toSetValue(preExecHook))) { - // Treat the pre-exec and post-exec hook as a single unit, identified by the pre-exec hook. - // If the pre-exec hook exists, revert. - revert PermittedCallHookAlreadySet(selector, plugin, preExecHook); - } - - if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { - _permittedCalldata.permittedCallHooks.associatedPostHooks[preExecHook] = postExecHook; - } + _addHooks(_permittedCalldata.permittedCallHooks, selector, preExecHook, postExecHook); } function _removePermittedCallHooks( @@ -202,17 +178,55 @@ abstract contract PluginManagerInternals is IPluginManager { address plugin, FunctionReference preExecHook, FunctionReference postExecHook - ) internal notNullPlugin(plugin) notNullFunction(preExecHook) { + ) internal notNullPlugin(plugin) { bytes24 permittedCallKey = getPermittedCallKey(plugin, selector); PermittedCallData storage _permittedCallData = getAccountStorage().permittedCalls[permittedCallKey]; - // May ignore return value, as the manifest hash is validated to ensure that the hook exists. - _permittedCallData.permittedCallHooks.preHooks.remove(_toSetValue(preExecHook)); + _removeHooks(_permittedCallData.permittedCallHooks, preExecHook, postExecHook); + } + + function _addHooks( + HookGroup storage hooks, + bytes4 selector, + FunctionReference preExecHook, + FunctionReference postExecHook + ) internal { + if (preExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { + // add pre or pre/post pair of exec hooks + if (!hooks.preHooks.add(_toSetValue(preExecHook))) { + revert ExecutionHookAlreadySet(selector, preExecHook); + } + + if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { + hooks.associatedPostHooks[preExecHook] = postExecHook; + } + } else { + if (postExecHook == FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { + // both pre and post hooks cannot be null + revert NullFunctionReference(); + } + + hooks.postOnlyHooks.add(_toSetValue(postExecHook)); + } + } + + function _removeHooks(HookGroup storage hooks, FunctionReference preExecHook, FunctionReference postExecHook) + internal + { + if (preExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { + // remove pre or pre/post pair of exec hooks + + // May ignore return value, as the manifest hash is validated to ensure that the hook exists. + hooks.preHooks.remove(_toSetValue(preExecHook)); + + if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { + hooks.associatedPostHooks[preExecHook] = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; + } + } else { + // THe case where both pre and post hooks are null was checked during installation. - // If the post permitted call exec hook is set, clear it. - if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { - _permittedCallData.permittedCallHooks.associatedPostHooks[preExecHook] = - FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; + // May ignore return value, as the manifest hash is validated to ensure that the hook exists. + hooks.postOnlyHooks.remove(_toSetValue(postExecHook)); } } diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 09f8559e..2dd36350 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -10,7 +10,7 @@ import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeab import {AccountExecutor} from "./AccountExecutor.sol"; import {AccountLoupe} from "./AccountLoupe.sol"; -import {AccountStorage, getAccountStorage, getPermittedCallKey} from "../libraries/AccountStorage.sol"; +import {AccountStorage, HookGroup, getAccountStorage, getPermittedCallKey} from "../libraries/AccountStorage.sol"; import {AccountStorageInitializable} from "./AccountStorageInitializable.sol"; import {FunctionReference, FunctionReferenceLib} from "../libraries/FunctionReferenceLib.sol"; import {IPlugin, PluginManifest} from "../interfaces/IPlugin.sol"; @@ -188,7 +188,8 @@ contract UpgradeableModularAccount is revert ExecFromPluginNotPermitted(callingPlugin, selector); } - PostExecToRun[] memory postPermittedCallHooks = _doPrePermittedCallHooks(selector, callingPlugin); + PostExecToRun[] memory postPermittedCallHooks = + _doPrePermittedCallHooks(getPermittedCallKey(callingPlugin, selector), data); address execFunctionPlugin = _storage.selectorData[selector].plugin; @@ -250,8 +251,9 @@ contract UpgradeableModularAccount is // Run any pre plugin exec specific to this caller and the `executeFromPluginExternal` selector - PostExecToRun[] memory postPermittedCallHooks = - _doPrePermittedCallHooks(IPluginExecutor.executeFromPluginExternal.selector, msg.sender); + PostExecToRun[] memory postPermittedCallHooks = _doPrePermittedCallHooks( + getPermittedCallKey(msg.sender, IPluginExecutor.executeFromPluginExternal.selector), msg.data + ); // Run any pre exec hooks for this selector PostExecToRun[] memory postExecHooks = _doPreExecHooks(IPluginExecutor.executeFromPluginExternal.selector); @@ -477,16 +479,15 @@ contract UpgradeableModularAccount is } function _doPreExecHooks(bytes4 selector) internal returns (PostExecToRun[] memory postHooksToRun) { - EnumerableSet.Bytes32Set storage preExecHooks = - getAccountStorage().selectorData[selector].executionHooks.preHooks; + HookGroup storage hooks = getAccountStorage().selectorData[selector].executionHooks; uint256 postExecHooksLength = 0; - uint256 preExecHooksLength = preExecHooks.length(); + uint256 preExecHooksLength = hooks.preHooks.length(); // Over-allocate on length, but not all of this may get filled up. - postHooksToRun = new PostExecToRun[](preExecHooksLength); + postHooksToRun = new PostExecToRun[](preExecHooksLength + hooks.postOnlyHooks.length()); for (uint256 i = 0; i < preExecHooksLength;) { - FunctionReference preExecHook = _toFunctionReference(preExecHooks.at(i)); + FunctionReference preExecHook = _toFunctionReference(hooks.preHooks.at(i)); if (preExecHook.isEmptyOrMagicValue()) { if (preExecHook == FunctionReferenceLib._PRE_HOOK_ALWAYS_DENY) { @@ -522,23 +523,30 @@ contract UpgradeableModularAccount is ++i; } } + + // Add post-only hooks to the end of the array + uint256 postOnlyHooksLength = hooks.postOnlyHooks.length(); + for (uint256 i = 0; i < postOnlyHooksLength;) { + postHooksToRun[postExecHooksLength].postExecHook = _toFunctionReference(hooks.postOnlyHooks.at(i)); + unchecked { + ++postExecHooksLength; + ++i; + } + } } - function _doPrePermittedCallHooks(bytes4 executionSelector, address callerPlugin) + function _doPrePermittedCallHooks(bytes24 permittedCallKey, bytes calldata data) internal returns (PostExecToRun[] memory postHooksToRun) { - bytes24 permittedCallKey = getPermittedCallKey(callerPlugin, executionSelector); - - EnumerableSet.Bytes32Set storage preExecHooks = - getAccountStorage().permittedCalls[permittedCallKey].permittedCallHooks.preHooks; + HookGroup storage hooks = getAccountStorage().permittedCalls[permittedCallKey].permittedCallHooks; uint256 postExecHooksLength = 0; - uint256 preExecHooksLength = preExecHooks.length(); - postHooksToRun = new PostExecToRun[](preExecHooksLength); // Over-allocate on length, but not all of this - // may get filled up. + uint256 preExecHooksLength = hooks.preHooks.length(); + // Over-allocate on length, but not all of this may get filled up. + postHooksToRun = new PostExecToRun[](preExecHooksLength + hooks.postOnlyHooks.length()); for (uint256 i = 0; i < preExecHooksLength;) { - FunctionReference preExecHook = _toFunctionReference(preExecHooks.at(i)); + FunctionReference preExecHook = _toFunctionReference(hooks.preHooks.at(i)); if (preExecHook.isEmptyOrMagicValue()) { if (preExecHook == FunctionReferenceLib._PRE_HOOK_ALWAYS_DENY) { @@ -551,7 +559,7 @@ contract UpgradeableModularAccount is (address plugin, uint8 functionId) = preExecHook.unpack(); bytes memory preExecHookReturnData; - try IPlugin(plugin).preExecutionHook(functionId, msg.sender, msg.value, msg.data) returns ( + try IPlugin(plugin).preExecutionHook(functionId, msg.sender, msg.value, data) returns ( bytes memory returnData ) { preExecHookReturnData = returnData; @@ -575,6 +583,16 @@ contract UpgradeableModularAccount is ++i; } } + + // Copy post-only hooks to the end of the array + uint256 postOnlyHooksLength = hooks.postOnlyHooks.length(); + for (uint256 i = 0; i < postOnlyHooksLength;) { + postHooksToRun[postExecHooksLength].postExecHook = _toFunctionReference(hooks.postOnlyHooks.at(i)); + unchecked { + ++postExecHooksLength; + ++i; + } + } } function _doCachedPostExecHooks(PostExecToRun[] memory postHooksToRun) internal { diff --git a/test/account/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol new file mode 100644 index 00000000..23a53a75 --- /dev/null +++ b/test/account/AccountExecHooks.t.sol @@ -0,0 +1,240 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.19; + +import {Test} from "forge-std/Test.sol"; + +import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; +import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; + +import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; +import { + IPlugin, + ManifestAssociatedFunctionType, + ManifestAssociatedFunction, + ManifestExecutionHook, + ManifestFunction, + PluginManifest +} from "../../src/interfaces/IPlugin.sol"; +import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; +import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; +import {FunctionReference} from "../../src/libraries/FunctionReferenceLib.sol"; +import {Call} from "../../src/interfaces/IStandardExecutor.sol"; + +import {MockPlugin} from "../mocks/MockPlugin.sol"; +import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; + +contract AccountExecHooksTest is Test { + using ECDSA for bytes32; + + EntryPoint public entryPoint; + SingleOwnerPlugin public singleOwnerPlugin; + MSCAFactoryFixture public factory; + + UpgradeableModularAccount public account; + + MockPlugin public mockPlugin1; + bytes32 public manifestHash1; + + 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; + + /// @dev Note that we strip hookApplyData from InjectedHooks in this event for gas savings + event PluginInstalled( + address indexed plugin, + bytes32 manifestHash, + FunctionReference[] dependencies, + IPluginManager.InjectedHook[] injectedHooks + ); + event PluginUninstalled(address indexed plugin, bool indexed callbacksSucceeded); + // emitted by MockPlugin + event ReceivedCall(bytes msgData, uint256 msgValue); + + function setUp() public { + entryPoint = new EntryPoint(); + singleOwnerPlugin = new SingleOwnerPlugin(); + factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); + + // Create an account with "this" as the owner, so we can execute along the runtime path with regular + // solidity semantics + account = factory.createAccount(address(this), 0); + + m1.executionFunctions.push(_EXEC_SELECTOR); + + m1.runtimeValidationFunctions.push( + ManifestAssociatedFunction({ + executionSelector: _EXEC_SELECTOR, + associatedFunction: ManifestFunction({ + functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, + functionId: 0, + dependencyIndex: 0 + }) + }) + ); + } + + function test_preExecHook_install() public { + _installPlugin1WithHooks( + _EXEC_SELECTOR, + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + functionId: _PRE_HOOK_FUNCTION_ID_1, + dependencyIndex: 0 + }), + ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}) + ); + } + + /// @dev Plugin 1 hook pair: [1, null] + /// Expected execution: [1, null] + function test_preExecHook_run() public { + test_preExecHook_install(); + + vm.expectEmit(true, true, true, true); + 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) + ), + 0 // msg value in call to plugin + ); + + (bool success,) = address(account).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + assertTrue(success); + } + + function test_preExecHook_uninstall() public { + test_preExecHook_install(); + + _uninstallPlugin(mockPlugin1); + } + + function test_execHookPair_install() public { + _installPlugin1WithHooks( + _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 + }) + ); + } + + /// @dev Plugin 1 hook pair: [1, 2] + /// Expected execution: [1, 2] + function test_execHookPair_run() public { + test_execHookPair_install(); + + vm.expectEmit(true, true, true, true); + // pre hook call + 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) + ), + 0 // msg value in call to plugin + ); + vm.expectEmit(true, true, true, true); + // exec call + emit ReceivedCall(abi.encodePacked(_EXEC_SELECTOR), 0); + vm.expectEmit(true, true, true, true); + // post hook call + emit ReceivedCall( + abi.encodeCall(IPlugin.postExecutionHook, (_POST_HOOK_FUNCTION_ID_2, "")), + 0 // msg value in call to plugin + ); + + (bool success,) = address(account).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + assertTrue(success); + } + + function test_execHookPair_uninstall() public { + test_execHookPair_install(); + + _uninstallPlugin(mockPlugin1); + } + + function test_postOnlyExecHook_install() public { + _installPlugin1WithHooks( + _EXEC_SELECTOR, + ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}), + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + functionId: _POST_HOOK_FUNCTION_ID_2, + dependencyIndex: 0 + }) + ); + } + + /// @dev Plugin 1 hook pair: [null, 2] + /// Expected execution: [null, 2] + function test_postOnlyExecHook_run() public { + test_postOnlyExecHook_install(); + + vm.expectEmit(true, true, true, true); + emit ReceivedCall( + abi.encodeCall(IPlugin.postExecutionHook, (_POST_HOOK_FUNCTION_ID_2, "")), + 0 // msg value in call to plugin + ); + + (bool success,) = address(account).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + assertTrue(success); + } + + function test_postOnlyExecHook_uninstall() public { + test_postOnlyExecHook_install(); + + _uninstallPlugin(mockPlugin1); + } + + function _installPlugin1WithHooks( + bytes4 selector, + ManifestFunction memory preHook, + ManifestFunction memory postHook + ) internal returns (MockPlugin) { + m1.executionHooks.push(ManifestExecutionHook(selector, preHook, postHook)); + mockPlugin1 = new MockPlugin(m1); + manifestHash1 = keccak256(abi.encode(mockPlugin1.pluginManifest())); + + 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), new IPluginManager.InjectedHook[](0) + ); + + account.installPlugin({ + plugin: address(mockPlugin1), + manifestHash: manifestHash1, + pluginInitData: bytes(""), + dependencies: new FunctionReference[](0), + injectedHooks: new IPluginManager.InjectedHook[](0) + }); + + return mockPlugin1; + } + + function _uninstallPlugin(MockPlugin plugin) internal { + vm.expectEmit(true, true, true, true); + emit ReceivedCall(abi.encodeCall(IPlugin.onUninstall, (bytes(""))), 0); + vm.expectEmit(true, true, true, true); + emit PluginUninstalled(address(plugin), true); + + account.uninstallPlugin(address(plugin), bytes(""), bytes(""), new bytes[](0)); + } +} diff --git a/test/account/AccountPermittedCallHooks.t.sol b/test/account/AccountPermittedCallHooks.t.sol new file mode 100644 index 00000000..aa249ef4 --- /dev/null +++ b/test/account/AccountPermittedCallHooks.t.sol @@ -0,0 +1,271 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.19; + +import {Test} from "forge-std/Test.sol"; + +import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; +import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; + +import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; +import { + IPlugin, + ManifestAssociatedFunctionType, + ManifestAssociatedFunction, + ManifestExecutionHook, + ManifestFunction, + PluginManifest +} from "../../src/interfaces/IPlugin.sol"; +import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; +import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; +import {FunctionReference} from "../../src/libraries/FunctionReferenceLib.sol"; +import {Call} from "../../src/interfaces/IStandardExecutor.sol"; + +import {MockPlugin} from "../mocks/MockPlugin.sol"; +import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; + +contract AccountPermittedCallHooksTest is Test { + using ECDSA for bytes32; + + EntryPoint public entryPoint; + SingleOwnerPlugin public singleOwnerPlugin; + MSCAFactoryFixture public factory; + + UpgradeableModularAccount public account; + + MockPlugin public mockPlugin1; + bytes32 public manifestHash1; + + 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; + + /// @dev Note that we strip hookApplyData from InjectedHooks in this event for gas savings + event PluginInstalled( + address indexed plugin, + bytes32 manifestHash, + FunctionReference[] dependencies, + IPluginManager.InjectedHook[] injectedHooks + ); + event PluginUninstalled(address indexed plugin, bool indexed callbacksSucceeded); + // emitted by MockPlugin + event ReceivedCall(bytes msgData, uint256 msgValue); + + function setUp() public { + entryPoint = new EntryPoint(); + singleOwnerPlugin = new SingleOwnerPlugin(); + factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); + + // Create an account with "this" as the owner, so we can execute along the runtime path with regular + // solidity semantics + account = factory.createAccount(address(this), 0); + + m1.executionFunctions.push(_EXEC_SELECTOR); + + m1.runtimeValidationFunctions.push( + ManifestAssociatedFunction({ + executionSelector: _EXEC_SELECTOR, + associatedFunction: ManifestFunction({ + functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, + functionId: 0, + dependencyIndex: 0 + }) + }) + ); + + m1.permittedExecutionSelectors.push(_EXEC_SELECTOR); + } + + function test_prePermittedCallHook_install() public { + _installPlugin1WithHooks( + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + functionId: _PRE_HOOK_FUNCTION_ID_1, + dependencyIndex: 0 + }), + ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}) + ); + } + + /// @dev Plugin hook pair(s): [1, null] + /// Expected execution: [1, null] + function test_prePermittedCallHook_run() public { + test_prePermittedCallHook_install(); + + vm.startPrank(address(mockPlugin1)); + + vm.expectEmit(true, true, true, true); + emit ReceivedCall( + abi.encodeWithSelector( + IPlugin.preExecutionHook.selector, + _PRE_HOOK_FUNCTION_ID_1, + address(mockPlugin1), // caller + 0, // msg.value in call to account + abi.encodePacked(_EXEC_SELECTOR) + ), + 0 // msg value in call to plugin + ); + + account.executeFromPlugin(abi.encodePacked(_EXEC_SELECTOR)); + + vm.stopPrank(); + } + + function test_prePermittedCallHook_uninstall() public { + test_prePermittedCallHook_install(); + + _uninstallPlugin(mockPlugin1); + } + + function test_permittedCallHookPair_install() public { + _installPlugin1WithHooks( + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + functionId: _PRE_HOOK_FUNCTION_ID_1, + dependencyIndex: 0 + }), + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + functionId: _POST_HOOK_FUNCTION_ID_2, + dependencyIndex: 0 + }) + ); + } + + /// @dev Plugin hook pair(s): [1, 2] + /// Expected execution: [1, 2] + function test_permittedCallHookPair_run() public { + test_permittedCallHookPair_install(); + + vm.startPrank(address(mockPlugin1)); + + vm.expectEmit(true, true, true, true); + // pre hook call + emit ReceivedCall( + abi.encodeWithSelector( + IPlugin.preExecutionHook.selector, + _PRE_HOOK_FUNCTION_ID_1, + address(mockPlugin1), // caller + 0, // msg.value in call to account + abi.encodePacked(_EXEC_SELECTOR) + ), + 0 // msg value in call to plugin + ); + vm.expectEmit(true, true, true, true); + // exec call + emit ReceivedCall(abi.encodePacked(_EXEC_SELECTOR), 0); + vm.expectEmit(true, true, true, true); + // post hook call + emit ReceivedCall( + abi.encodeCall(IPlugin.postExecutionHook, (_POST_HOOK_FUNCTION_ID_2, "")), + 0 // msg value in call to plugin + ); + + account.executeFromPlugin(abi.encodePacked(_EXEC_SELECTOR)); + + vm.stopPrank(); + } + + function test_permittedCallHookPair_uninstall() public { + test_permittedCallHookPair_install(); + + _uninstallPlugin(mockPlugin1); + } + + function test_postOnlyPermittedCallHook_install() public { + _installPlugin1WithHooks( + ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}), + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + functionId: _POST_HOOK_FUNCTION_ID_2, + dependencyIndex: 0 + }) + ); + } + + /// @dev Plugin hook pair(s): [null, 2] + /// Expected execution: [null, 2] + function test_postOnlyPermittedCallHook_run() public { + test_postOnlyPermittedCallHook_install(); + + vm.startPrank(address(mockPlugin1)); + + vm.expectEmit(true, true, true, true); + emit ReceivedCall( + abi.encodeCall(IPlugin.postExecutionHook, (_POST_HOOK_FUNCTION_ID_2, "")), + 0 // msg value in call to plugin + ); + + account.executeFromPlugin(abi.encodePacked(_EXEC_SELECTOR)); + + vm.stopPrank(); + } + + function test_postOnlyPermittedCallHook_uninstall() public { + test_postOnlyPermittedCallHook_install(); + + _uninstallPlugin(mockPlugin1); + } + + function _installPlugin1WithHooks(ManifestFunction memory preHook1, ManifestFunction memory postHook1) + internal + { + m1.permittedCallHooks.push(ManifestExecutionHook(_EXEC_SELECTOR, preHook1, postHook1)); + mockPlugin1 = new MockPlugin(m1); + manifestHash1 = keccak256(abi.encode(mockPlugin1.pluginManifest())); + + 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), new IPluginManager.InjectedHook[](0) + ); + + account.installPlugin({ + plugin: address(mockPlugin1), + manifestHash: manifestHash1, + pluginInitData: bytes(""), + dependencies: new FunctionReference[](0), + injectedHooks: new IPluginManager.InjectedHook[](0) + }); + } + + function _installPlugin1WithHooks( + ManifestFunction memory preHook1, + ManifestFunction memory postHook1, + ManifestFunction memory preHook2, + ManifestFunction memory postHook2 + ) internal { + m1.permittedCallHooks.push(ManifestExecutionHook(_EXEC_SELECTOR, preHook1, postHook1)); + m1.permittedCallHooks.push(ManifestExecutionHook(_EXEC_SELECTOR, preHook2, postHook2)); + mockPlugin1 = new MockPlugin(m1); + manifestHash1 = keccak256(abi.encode(mockPlugin1.pluginManifest())); + + 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), new IPluginManager.InjectedHook[](0) + ); + + account.installPlugin({ + plugin: address(mockPlugin1), + manifestHash: manifestHash1, + pluginInitData: bytes(""), + dependencies: new FunctionReference[](0), + injectedHooks: new IPluginManager.InjectedHook[](0) + }); + } + + function _uninstallPlugin(MockPlugin plugin) internal { + vm.expectEmit(true, true, true, true); + emit ReceivedCall(abi.encodeCall(IPlugin.onUninstall, (bytes(""))), 0); + vm.expectEmit(true, true, true, true); + emit PluginUninstalled(address(plugin), true); + + account.uninstallPlugin(address(plugin), bytes(""), bytes(""), new bytes[](0)); + } +} From 075bc7a710eae18f2a454790e4433ee24e120260 Mon Sep 17 00:00:00 2001 From: adam Date: Wed, 29 Nov 2023 20:03:25 -0500 Subject: [PATCH 15/20] Update to use optimized test --- test/account/AccountExecHooks.t.sol | 5 +++-- test/account/AccountPermittedCallHooks.t.sol | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/test/account/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol index 23a53a75..b7bba4ef 100644 --- a/test/account/AccountExecHooks.t.sol +++ b/test/account/AccountExecHooks.t.sol @@ -22,8 +22,9 @@ import {Call} from "../../src/interfaces/IStandardExecutor.sol"; import {MockPlugin} from "../mocks/MockPlugin.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; +import {OptimizedTest} from "../utils/OptimizedTest.sol"; -contract AccountExecHooksTest is Test { +contract AccountExecHooksTest is OptimizedTest { using ECDSA for bytes32; EntryPoint public entryPoint; @@ -56,7 +57,7 @@ contract AccountExecHooksTest is Test { function setUp() public { entryPoint = new EntryPoint(); - singleOwnerPlugin = new SingleOwnerPlugin(); + singleOwnerPlugin = _deploySingleOwnerPlugin(); factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); // Create an account with "this" as the owner, so we can execute along the runtime path with regular diff --git a/test/account/AccountPermittedCallHooks.t.sol b/test/account/AccountPermittedCallHooks.t.sol index aa249ef4..70735109 100644 --- a/test/account/AccountPermittedCallHooks.t.sol +++ b/test/account/AccountPermittedCallHooks.t.sol @@ -22,8 +22,9 @@ import {Call} from "../../src/interfaces/IStandardExecutor.sol"; import {MockPlugin} from "../mocks/MockPlugin.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; +import {OptimizedTest} from "../utils/OptimizedTest.sol"; -contract AccountPermittedCallHooksTest is Test { +contract AccountPermittedCallHooksTest is OptimizedTest { using ECDSA for bytes32; EntryPoint public entryPoint; @@ -56,7 +57,7 @@ contract AccountPermittedCallHooksTest is Test { function setUp() public { entryPoint = new EntryPoint(); - singleOwnerPlugin = new SingleOwnerPlugin(); + singleOwnerPlugin = _deploySingleOwnerPlugin(); factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); // Create an account with "this" as the owner, so we can execute along the runtime path with regular From 8ed1304ce1aef5d365992546f30192ef505e5a64 Mon Sep 17 00:00:00 2001 From: adam Date: Wed, 29 Nov 2023 20:07:48 -0500 Subject: [PATCH 16/20] Fix pre exec hook data in executeFromPlugin --- src/account/UpgradeableModularAccount.sol | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 2dd36350..32bdaa41 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -66,7 +66,7 @@ contract UpgradeableModularAccount is modifier wrapNativeFunction() { _doRuntimeValidationIfNotFromEP(); - PostExecToRun[] memory postExecHooks = _doPreExecHooks(msg.sig); + PostExecToRun[] memory postExecHooks = _doPreExecHooks(msg.sig, msg.data); _; @@ -127,7 +127,7 @@ contract UpgradeableModularAccount is PostExecToRun[] memory postExecHooks; // Cache post-exec hooks in memory - postExecHooks = _doPreExecHooks(msg.sig); + postExecHooks = _doPreExecHooks(msg.sig, msg.data); // execute the function, bubbling up any reverts (bool execSuccess, bytes memory execReturnData) = execPlugin.call(msg.data); @@ -197,7 +197,7 @@ contract UpgradeableModularAccount is revert UnrecognizedFunction(selector); } - PostExecToRun[] memory postExecHooks = _doPreExecHooks(selector); + PostExecToRun[] memory postExecHooks = _doPreExecHooks(selector, data); (bool success, bytes memory returnData) = execFunctionPlugin.call(data); @@ -256,7 +256,8 @@ contract UpgradeableModularAccount is ); // Run any pre exec hooks for this selector - PostExecToRun[] memory postExecHooks = _doPreExecHooks(IPluginExecutor.executeFromPluginExternal.selector); + PostExecToRun[] memory postExecHooks = + _doPreExecHooks(IPluginExecutor.executeFromPluginExternal.selector, msg.data); // Perform the external call bytes memory returnData = _exec(target, value, data); @@ -478,7 +479,10 @@ contract UpgradeableModularAccount is } } - function _doPreExecHooks(bytes4 selector) internal returns (PostExecToRun[] memory postHooksToRun) { + function _doPreExecHooks(bytes4 selector, bytes calldata data) + internal + returns (PostExecToRun[] memory postHooksToRun) + { HookGroup storage hooks = getAccountStorage().selectorData[selector].executionHooks; uint256 postExecHooksLength = 0; @@ -500,7 +504,7 @@ contract UpgradeableModularAccount is (address plugin, uint8 functionId) = preExecHook.unpack(); bytes memory preExecHookReturnData; - try IPlugin(plugin).preExecutionHook(functionId, msg.sender, msg.value, msg.data) returns ( + try IPlugin(plugin).preExecutionHook(functionId, msg.sender, msg.value, data) returns ( bytes memory returnData ) { preExecHookReturnData = returnData; From 50e6d3e3b5f95e3c1ec8ba64b73964fb841a931b Mon Sep 17 00:00:00 2001 From: adam Date: Wed, 29 Nov 2023 20:54:25 -0500 Subject: [PATCH 17/20] Fix lint --- test/account/AccountExecHooks.t.sol | 3 --- test/account/AccountPermittedCallHooks.t.sol | 3 --- 2 files changed, 6 deletions(-) diff --git a/test/account/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol index b7bba4ef..fbc13f82 100644 --- a/test/account/AccountExecHooks.t.sol +++ b/test/account/AccountExecHooks.t.sol @@ -1,8 +1,6 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; -import {Test} from "forge-std/Test.sol"; - import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; @@ -18,7 +16,6 @@ import { import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {FunctionReference} from "../../src/libraries/FunctionReferenceLib.sol"; -import {Call} from "../../src/interfaces/IStandardExecutor.sol"; import {MockPlugin} from "../mocks/MockPlugin.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; diff --git a/test/account/AccountPermittedCallHooks.t.sol b/test/account/AccountPermittedCallHooks.t.sol index 70735109..841760bf 100644 --- a/test/account/AccountPermittedCallHooks.t.sol +++ b/test/account/AccountPermittedCallHooks.t.sol @@ -1,8 +1,6 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; -import {Test} from "forge-std/Test.sol"; - import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; @@ -18,7 +16,6 @@ import { import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {FunctionReference} from "../../src/libraries/FunctionReferenceLib.sol"; -import {Call} from "../../src/interfaces/IStandardExecutor.sol"; import {MockPlugin} from "../mocks/MockPlugin.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; From ae34bb47e7d34fbaf5b5d541c170e18ca62f1dae Mon Sep 17 00:00:00 2001 From: adam Date: Thu, 30 Nov 2023 11:53:58 -0500 Subject: [PATCH 18/20] Fix via-IR build & refactor --- src/account/UpgradeableModularAccount.sol | 64 ++++------------------- src/libraries/AccountStorage.sol | 2 +- test/mocks/MockPlugin.sol | 5 +- 3 files changed, 13 insertions(+), 58 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 32bdaa41..0e0541af 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -485,58 +485,7 @@ contract UpgradeableModularAccount is { HookGroup storage hooks = getAccountStorage().selectorData[selector].executionHooks; - uint256 postExecHooksLength = 0; - uint256 preExecHooksLength = hooks.preHooks.length(); - - // Over-allocate on length, but not all of this may get filled up. - postHooksToRun = new PostExecToRun[](preExecHooksLength + hooks.postOnlyHooks.length()); - for (uint256 i = 0; i < preExecHooksLength;) { - FunctionReference preExecHook = _toFunctionReference(hooks.preHooks.at(i)); - - if (preExecHook.isEmptyOrMagicValue()) { - if (preExecHook == FunctionReferenceLib._PRE_HOOK_ALWAYS_DENY) { - revert AlwaysDenyRule(); - } - // Function reference cannot be 0. If _RUNTIME_VALIDATION_ALWAYS_ALLOW, revert since it's an - // invalid configuration. - revert InvalidConfiguration(); - } - - (address plugin, uint8 functionId) = preExecHook.unpack(); - bytes memory preExecHookReturnData; - try IPlugin(plugin).preExecutionHook(functionId, msg.sender, msg.value, data) returns ( - bytes memory returnData - ) { - preExecHookReturnData = returnData; - } catch (bytes memory revertReason) { - revert PreExecHookReverted(plugin, functionId, revertReason); - } - - // Check to see if there is a postExec hook set for this preExec hook - FunctionReference postExecHook = - getAccountStorage().selectorData[selector].executionHooks.associatedPostHooks[preExecHook]; - if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { - postHooksToRun[postExecHooksLength].postExecHook = postExecHook; - postHooksToRun[postExecHooksLength].preExecHookReturnData = preExecHookReturnData; - unchecked { - ++postExecHooksLength; - } - } - - unchecked { - ++i; - } - } - - // Add post-only hooks to the end of the array - uint256 postOnlyHooksLength = hooks.postOnlyHooks.length(); - for (uint256 i = 0; i < postOnlyHooksLength;) { - postHooksToRun[postExecHooksLength].postExecHook = _toFunctionReference(hooks.postOnlyHooks.at(i)); - unchecked { - ++postExecHooksLength; - ++i; - } - } + return _doPreHooks(hooks, data); } function _doPrePermittedCallHooks(bytes24 permittedCallKey, bytes calldata data) @@ -545,6 +494,13 @@ contract UpgradeableModularAccount is { HookGroup storage hooks = getAccountStorage().permittedCalls[permittedCallKey].permittedCallHooks; + return _doPreHooks(hooks, data); + } + + function _doPreHooks(HookGroup storage hooks, bytes calldata data) + internal + returns (PostExecToRun[] memory postHooksToRun) + { uint256 postExecHooksLength = 0; uint256 preExecHooksLength = hooks.preHooks.length(); // Over-allocate on length, but not all of this may get filled up. @@ -572,9 +528,7 @@ contract UpgradeableModularAccount is } // Check to see if there is a postExec hook set for this preExec hook - FunctionReference postExecHook = getAccountStorage().permittedCalls[permittedCallKey] - .permittedCallHooks - .associatedPostHooks[preExecHook]; + FunctionReference postExecHook = hooks.associatedPostHooks[preExecHook]; if (FunctionReference.unwrap(postExecHook) != 0) { postHooksToRun[postExecHooksLength].postExecHook = postExecHook; postHooksToRun[postExecHooksLength].preExecHookReturnData = preExecHookReturnData; diff --git a/src/libraries/AccountStorage.sol b/src/libraries/AccountStorage.sol index 3e52e372..070065d0 100644 --- a/src/libraries/AccountStorage.sol +++ b/src/libraries/AccountStorage.sol @@ -90,7 +90,7 @@ struct AccountStorage { } function getAccountStorage() pure returns (AccountStorage storage _storage) { - assembly { + assembly ("memory-safe") { _storage.slot := _ACCOUNT_STORAGE_SLOT } } diff --git a/test/mocks/MockPlugin.sol b/test/mocks/MockPlugin.sol index 273d5335..1a71e999 100644 --- a/test/mocks/MockPlugin.sol +++ b/test/mocks/MockPlugin.sol @@ -39,7 +39,7 @@ contract MockPlugin is ERC165 { pure returns (function() internal pure returns (PluginManifest memory) fnOut) { - assembly { + assembly ("memory-safe") { fnOut := fnIn } } @@ -82,7 +82,8 @@ contract MockPlugin is ERC165 { || msg.sig == IPlugin.preExecutionHook.selector ) { // return 0 for userOp/runtimeVal case, return bytes("") for preExecutionHook case - assembly { + assembly ("memory-safe") { + mstore(0, 0) return(0x00, 0x20) } } From f6bc2172a61aec233f111697a37f8c3a0432f973 Mon Sep 17 00:00:00 2001 From: Jay Paik Date: Thu, 30 Nov 2023 13:09:24 -0500 Subject: [PATCH 19/20] feat: allow overlapping hooks --- lib/forge-std | 2 +- src/account/AccountLoupe.sol | 103 +++++--- src/account/PluginManagerInternals.sol | 104 ++++---- src/account/UpgradeableModularAccount.sol | 113 +++++---- src/libraries/AccountStorage.sol | 27 +- src/libraries/FunctionReferenceLib.sol | 4 + test/account/AccountExecHooks.t.sol | 253 ++++++++++++++++++- test/account/AccountPermittedCallHooks.t.sol | 137 ++++++++++ 8 files changed, 599 insertions(+), 144 deletions(-) diff --git a/lib/forge-std b/lib/forge-std index 066ff16c..2f112697 160000 --- a/lib/forge-std +++ b/lib/forge-std @@ -1 +1 @@ -Subproject commit 066ff16c5c03e6f931cd041fd366bc4be1fae82a +Subproject commit 2f112697506eab12d433a65fdc31a639548fe365 diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol index 6baac4e1..5f68ea32 100644 --- a/src/account/AccountLoupe.sol +++ b/src/account/AccountLoupe.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.19; import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; +import {EnumerableMap} from "@openzeppelin/contracts/utils/structs/EnumerableMap.sol"; import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import {IAccountLoupe} from "../interfaces/IAccountLoupe.sol"; @@ -11,11 +12,13 @@ import { AccountStorage, getAccountStorage, getPermittedCallKey, + HookGroup, toFunctionReferenceArray } from "../libraries/AccountStorage.sol"; import {FunctionReference} from "../libraries/FunctionReferenceLib.sol"; abstract contract AccountLoupe is IAccountLoupe { + using EnumerableMap for EnumerableMap.Bytes32ToUintMap; using EnumerableSet for EnumerableSet.AddressSet; error ManifestDiscrepancy(address plugin); @@ -47,23 +50,7 @@ abstract contract AccountLoupe is IAccountLoupe { /// @inheritdoc IAccountLoupe function getExecutionHooks(bytes4 selector) external view returns (ExecutionHooks[] memory execHooks) { - AccountStorage storage _storage = getAccountStorage(); - - FunctionReference[] memory preExecHooks = - toFunctionReferenceArray(_storage.selectorData[selector].executionHooks.preHooks); - - uint256 numHooks = preExecHooks.length; - execHooks = new ExecutionHooks[](numHooks); - - for (uint256 i = 0; i < numHooks;) { - execHooks[i].preExecHook = preExecHooks[i]; - execHooks[i].postExecHook = - _storage.selectorData[selector].executionHooks.associatedPostHooks[preExecHooks[i]]; - - unchecked { - ++i; - } - } + execHooks = _getHooks(getAccountStorage().selectorData[selector].executionHooks); } /// @inheritdoc IAccountLoupe @@ -72,25 +59,8 @@ abstract contract AccountLoupe is IAccountLoupe { view returns (ExecutionHooks[] memory execHooks) { - AccountStorage storage _storage = getAccountStorage(); - bytes24 key = getPermittedCallKey(callingPlugin, selector); - - FunctionReference[] memory prePermittedCallHooks = - toFunctionReferenceArray(_storage.permittedCalls[key].permittedCallHooks.preHooks); - - uint256 numHooks = prePermittedCallHooks.length; - execHooks = new ExecutionHooks[](numHooks); - - for (uint256 i = 0; i < numHooks;) { - execHooks[i].preExecHook = prePermittedCallHooks[i]; - execHooks[i].postExecHook = - _storage.permittedCalls[key].permittedCallHooks.associatedPostHooks[prePermittedCallHooks[i]]; - - unchecked { - ++i; - } - } + execHooks = _getHooks(getAccountStorage().permittedCalls[key].permittedCallHooks); } /// @inheritdoc IAccountLoupe @@ -112,4 +82,67 @@ abstract contract AccountLoupe is IAccountLoupe { function getInstalledPlugins() external view returns (address[] memory pluginAddresses) { pluginAddresses = getAccountStorage().plugins.values(); } + + function _getHooks(HookGroup storage hooks) internal view returns (ExecutionHooks[] memory execHooks) { + uint256 preExecHooksLength = hooks.preHooks.length(); + uint256 postOnlyExecHooksLength = hooks.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) = hooks.preHooks.at(i); + unchecked { + maxExecHooksLength += (count + 1); + ++i; + } + } + + // Overallocate on length - not all of this may get filled up. We set the correct length later. + execHooks = new ExecutionHooks[](maxExecHooksLength); + uint256 actualExecHooksLength; + + for (uint256 i = 0; i < preExecHooksLength;) { + (bytes32 key,) = hooks.preHooks.at(i); + FunctionReference preExecHook = FunctionReference.wrap(bytes21(key)); + + uint256 associatedPostExecHooksLength = hooks.associatedPostHooks[preExecHook].length(); + if (associatedPostExecHooksLength > 0) { + for (uint256 j = 0; j < associatedPostExecHooksLength;) { + execHooks[actualExecHooksLength].preExecHook = preExecHook; + (key,) = hooks.associatedPostHooks[preExecHook].at(j); + execHooks[actualExecHooksLength].postExecHook = FunctionReference.wrap(bytes21(key)); + + unchecked { + ++actualExecHooksLength; + ++j; + } + } + } else { + execHooks[actualExecHooksLength].preExecHook = preExecHook; + + unchecked { + ++actualExecHooksLength; + } + } + + unchecked { + ++i; + } + } + + for (uint256 i = 0; i < postOnlyExecHooksLength;) { + (bytes32 key,) = hooks.postOnlyHooks.at(i); + execHooks[actualExecHooksLength].postExecHook = FunctionReference.wrap(bytes21(key)); + + unchecked { + ++actualExecHooksLength; + ++i; + } + } + + // Trim the exec hooks array to the actual length, since we may have overallocated. + assembly ("memory-safe") { + mstore(execHooks, actualExecHooksLength) + } + } } diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 3fcf2c30..6f47c55a 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.19; +import {EnumerableMap} from "@openzeppelin/contracts/utils/structs/EnumerableMap.sol"; import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol"; @@ -27,13 +28,11 @@ import { } from "../interfaces/IPlugin.sol"; abstract contract PluginManagerInternals is IPluginManager { - using EnumerableSet for EnumerableSet.Bytes32Set; + using EnumerableMap for EnumerableMap.Bytes32ToUintMap; using EnumerableSet for EnumerableSet.AddressSet; error ArrayLengthMismatch(); - error ExecuteFromPluginAlreadySet(bytes4 selector, address plugin); error ExecutionFunctionAlreadySet(bytes4 selector); - error ExecutionHookAlreadySet(bytes4 selector, FunctionReference hook); error InvalidDependenciesProvided(); error InvalidPluginManifest(); error MissingPluginDependency(address dependency); @@ -41,19 +40,16 @@ abstract contract PluginManagerInternals is IPluginManager { error NullPlugin(); error PluginAlreadyInstalled(address plugin); error PluginDependencyViolation(address plugin); - error PermittedCallHookAlreadySet(bytes4 selector, address plugin, FunctionReference hook); error PluginInstallCallbackFailed(address plugin, bytes revertReason); error PluginInterfaceNotSupported(address plugin); error PluginNotInstalled(address plugin); - error PreRuntimeValidationHookAlreadySet(bytes4 selector, FunctionReference hook); - error PreUserOpValidationHookAlreadySet(bytes4 selector, FunctionReference hook); error RuntimeValidationFunctionAlreadySet(bytes4 selector, FunctionReference validationFunction); error UserOpValidationFunctionAlreadySet(bytes4 selector, FunctionReference validationFunction); error PluginApplyHookCallbackFailed(address providingPlugin, bytes revertReason); error PluginUnapplyHookCallbackFailed(address providingPlugin, bytes revertReason); modifier notNullFunction(FunctionReference functionReference) { - if (functionReference == FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { + if (functionReference.isEmpty()) { revert NullFunctionReference(); } _; @@ -90,7 +86,7 @@ abstract contract PluginManagerInternals is IPluginManager { { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - if (_selectorData.userOpValidation != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { + if (!_selectorData.userOpValidation.isEmpty()) { revert UserOpValidationFunctionAlreadySet(selector, validationFunction); } @@ -112,7 +108,7 @@ abstract contract PluginManagerInternals is IPluginManager { { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - if (_selectorData.runtimeValidation != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { + if (!_selectorData.runtimeValidation.isEmpty()) { revert RuntimeValidationFunctionAlreadySet(selector, validationFunction); } @@ -133,7 +129,7 @@ abstract contract PluginManagerInternals is IPluginManager { { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - _addHooks(_selectorData.executionHooks, selector, preExecHook, postExecHook); + _addHooks(_selectorData.executionHooks, preExecHook, postExecHook); } function _removeExecHooks(bytes4 selector, FunctionReference preExecHook, FunctionReference postExecHook) @@ -170,7 +166,7 @@ abstract contract PluginManagerInternals is IPluginManager { bytes24 permittedCallKey = getPermittedCallKey(plugin, selector); PermittedCallData storage _permittedCalldata = getAccountStorage().permittedCalls[permittedCallKey]; - _addHooks(_permittedCalldata.permittedCallHooks, selector, preExecHook, postExecHook); + _addHooks(_permittedCalldata.permittedCallHooks, preExecHook, postExecHook); } function _removePermittedCallHooks( @@ -185,48 +181,39 @@ abstract contract PluginManagerInternals is IPluginManager { _removeHooks(_permittedCallData.permittedCallHooks, preExecHook, postExecHook); } - function _addHooks( - HookGroup storage hooks, - bytes4 selector, - FunctionReference preExecHook, - FunctionReference postExecHook - ) internal { - if (preExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { - // add pre or pre/post pair of exec hooks - if (!hooks.preHooks.add(_toSetValue(preExecHook))) { - revert ExecutionHookAlreadySet(selector, preExecHook); - } + function _addHooks(HookGroup storage hooks, FunctionReference preExecHook, FunctionReference postExecHook) + internal + { + if (!preExecHook.isEmpty()) { + _addOrIncrement(hooks.preHooks, _toSetValue(preExecHook)); - if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { - hooks.associatedPostHooks[preExecHook] = postExecHook; + if (!postExecHook.isEmpty()) { + _addOrIncrement(hooks.associatedPostHooks[preExecHook], _toSetValue(postExecHook)); } } else { - if (postExecHook == FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { + if (postExecHook.isEmpty()) { // both pre and post hooks cannot be null revert NullFunctionReference(); } - hooks.postOnlyHooks.add(_toSetValue(postExecHook)); + _addOrIncrement(hooks.postOnlyHooks, _toSetValue(postExecHook)); } } function _removeHooks(HookGroup storage hooks, FunctionReference preExecHook, FunctionReference postExecHook) internal { - if (preExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { - // remove pre or pre/post pair of exec hooks - - // May ignore return value, as the manifest hash is validated to ensure that the hook exists. - hooks.preHooks.remove(_toSetValue(preExecHook)); + if (!preExecHook.isEmpty()) { + _removeOrDecrement(hooks.preHooks, _toSetValue(preExecHook)); - if (postExecHook != FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { - hooks.associatedPostHooks[preExecHook] = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; + if (!postExecHook.isEmpty()) { + _removeOrDecrement(hooks.associatedPostHooks[preExecHook], _toSetValue(postExecHook)); } } else { - // THe case where both pre and post hooks are null was checked during installation. + // The case where both pre and post hooks are null was checked during installation. // May ignore return value, as the manifest hash is validated to ensure that the hook exists. - hooks.postOnlyHooks.remove(_toSetValue(postExecHook)); + _removeOrDecrement(hooks.postOnlyHooks, _toSetValue(postExecHook)); } } @@ -234,13 +221,10 @@ abstract contract PluginManagerInternals is IPluginManager { internal notNullFunction(preUserOpValidationHook) { - if ( - !getAccountStorage().selectorData[selector].preUserOpValidationHooks.add( - _toSetValue(preUserOpValidationHook) - ) - ) { - revert PreUserOpValidationHookAlreadySet(selector, preUserOpValidationHook); - } + _addOrIncrement( + getAccountStorage().selectorData[selector].preUserOpValidationHooks, + _toSetValue(preUserOpValidationHook) + ); } function _removePreUserOpValidationHook(bytes4 selector, FunctionReference preUserOpValidationHook) @@ -248,7 +232,8 @@ abstract contract PluginManagerInternals is IPluginManager { notNullFunction(preUserOpValidationHook) { // May ignore return value, as the manifest hash is validated to ensure that the hook exists. - getAccountStorage().selectorData[selector].preUserOpValidationHooks.remove( + _removeOrDecrement( + getAccountStorage().selectorData[selector].preUserOpValidationHooks, _toSetValue(preUserOpValidationHook) ); } @@ -257,13 +242,10 @@ abstract contract PluginManagerInternals is IPluginManager { internal notNullFunction(preRuntimeValidationHook) { - if ( - !getAccountStorage().selectorData[selector].preRuntimeValidationHooks.add( - _toSetValue(preRuntimeValidationHook) - ) - ) { - revert PreRuntimeValidationHookAlreadySet(selector, preRuntimeValidationHook); - } + _addOrIncrement( + getAccountStorage().selectorData[selector].preRuntimeValidationHooks, + _toSetValue(preRuntimeValidationHook) + ); } function _removePreRuntimeValidationHook(bytes4 selector, FunctionReference preRuntimeValidationHook) @@ -271,7 +253,8 @@ abstract contract PluginManagerInternals is IPluginManager { notNullFunction(preRuntimeValidationHook) { // May ignore return value, as the manifest hash is validated to ensure that the hook exists. - getAccountStorage().selectorData[selector].preRuntimeValidationHooks.remove( + _removeOrDecrement( + getAccountStorage().selectorData[selector].preRuntimeValidationHooks, _toSetValue(preRuntimeValidationHook) ); } @@ -855,6 +838,25 @@ abstract contract PluginManagerInternals is IPluginManager { emit PluginUninstalled(plugin, onUninstallSuccess); } + function _addOrIncrement(EnumerableMap.Bytes32ToUintMap storage map, bytes32 key) internal { + (bool success, uint256 value) = map.tryGet(key); + map.set(key, success ? value + 1 : 0); + } + + /// @return True if the key was removed or its value was decremented, false if the key was not found. + function _removeOrDecrement(EnumerableMap.Bytes32ToUintMap storage map, bytes32 key) internal returns (bool) { + (bool success, uint256 value) = map.tryGet(key); + if (!success) { + return false; + } + if (value == 0) { + map.remove(key); + } else { + map.set(key, value - 1); + } + return true; + } + function _toSetValue(FunctionReference functionReference) internal pure returns (bytes32) { return bytes32(FunctionReference.unwrap(functionReference)); } diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 0e0541af..46cd642d 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.19; import {BaseAccount} from "@eth-infinitism/account-abstraction/core/BaseAccount.sol"; +import {EnumerableMap} from "@openzeppelin/contracts/utils/structs/EnumerableMap.sol"; import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; @@ -31,6 +32,7 @@ contract UpgradeableModularAccount is PluginManagerInternals, UUPSUpgradeable { + using EnumerableMap for EnumerableMap.Bytes32ToUintMap; using EnumerableSet for EnumerableSet.Bytes32Set; struct PostExecToRun { @@ -363,22 +365,23 @@ contract UpgradeableModularAccount is UserOperation calldata userOp, bytes32 userOpHash ) internal returns (uint256 validationData) { - if (userOpValidationFunction == FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { + if (userOpValidationFunction.isEmpty()) { revert UserOpValidationFunctionMissing(selector); } uint256 currentValidationData; // Do preUserOpValidation hooks - EnumerableSet.Bytes32Set storage preUserOpValidationHooks = + EnumerableMap.Bytes32ToUintMap storage preUserOpValidationHooks = getAccountStorage().selectorData[selector].preUserOpValidationHooks; uint256 preUserOpValidationHooksLength = preUserOpValidationHooks.length(); for (uint256 i = 0; i < preUserOpValidationHooksLength;) { - // FunctionReference preUserOpValidationHook = preUserOpValidationHooks[i]; + (bytes32 key,) = preUserOpValidationHooks.at(i); + FunctionReference preUserOpValidationHook = _toFunctionReference(key); - if (!_toFunctionReference(preUserOpValidationHooks.at(i)).isEmptyOrMagicValue()) { - (address plugin, uint8 functionId) = _toFunctionReference(preUserOpValidationHooks.at(i)).unpack(); + if (!preUserOpValidationHook.isEmptyOrMagicValue()) { + (address plugin, uint8 functionId) = preUserOpValidationHook.unpack(); try IPlugin(plugin).preUserOpValidationHook(functionId, userOp, userOpHash) returns ( uint256 returnData ) { @@ -432,12 +435,13 @@ contract UpgradeableModularAccount is AccountStorage storage _storage = getAccountStorage(); FunctionReference runtimeValidationFunction = _storage.selectorData[msg.sig].runtimeValidation; // run all preRuntimeValidation hooks - EnumerableSet.Bytes32Set storage preRuntimeValidationHooks = + EnumerableMap.Bytes32ToUintMap storage preRuntimeValidationHooks = getAccountStorage().selectorData[msg.sig].preRuntimeValidationHooks; uint256 preRuntimeValidationHooksLength = preRuntimeValidationHooks.length(); for (uint256 i = 0; i < preRuntimeValidationHooksLength;) { - FunctionReference preRuntimeValidationHook = _toFunctionReference(preRuntimeValidationHooks.at(i)); + (bytes32 key,) = preRuntimeValidationHooks.at(i); + FunctionReference preRuntimeValidationHook = _toFunctionReference(key); if (!preRuntimeValidationHook.isEmptyOrMagicValue()) { (address plugin, uint8 functionId) = preRuntimeValidationHook.unpack(); @@ -469,7 +473,7 @@ contract UpgradeableModularAccount is revert RuntimeValidationFunctionReverted(plugin, functionId, revertReason); } } else { - if (runtimeValidationFunction == FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { + if (runtimeValidationFunction.isEmpty()) { revert RuntimeValidationFunctionMissing(msg.sig); } else if (runtimeValidationFunction == FunctionReferenceLib._PRE_HOOK_ALWAYS_DENY) { revert InvalidConfiguration(); @@ -501,40 +505,48 @@ contract UpgradeableModularAccount is internal returns (PostExecToRun[] memory postHooksToRun) { - uint256 postExecHooksLength = 0; uint256 preExecHooksLength = hooks.preHooks.length(); - // Over-allocate on length, but not all of this may get filled up. - postHooksToRun = new PostExecToRun[](preExecHooksLength + hooks.postOnlyHooks.length()); - for (uint256 i = 0; i < preExecHooksLength;) { - FunctionReference preExecHook = _toFunctionReference(hooks.preHooks.at(i)); + uint256 postOnlyHooksLength = hooks.postOnlyHooks.length(); + uint256 maxPostExecHooksLength = postOnlyHooksLength; - if (preExecHook.isEmptyOrMagicValue()) { - if (preExecHook == FunctionReferenceLib._PRE_HOOK_ALWAYS_DENY) { - revert AlwaysDenyRule(); - } - // Function reference cannot be 0. If RUNTIME_VALIDATION_BYPASS, revert since it's an invalid - // configuration. - revert InvalidConfiguration(); + // There can only be as many associated post hooks to run as there are pre hooks. + for (uint256 i = 0; i < preExecHooksLength;) { + (, uint256 count) = hooks.preHooks.at(i); + unchecked { + maxPostExecHooksLength += (count + 1); + ++i; } + } + + // Overallocate on length - not all of this may get filled up. We set the correct length later. + postHooksToRun = new PostExecToRun[](maxPostExecHooksLength); + uint256 actualPostHooksToRunLength; + + for (uint256 i = 0; i < preExecHooksLength;) { + (bytes32 key,) = hooks.preHooks.at(i); + FunctionReference preExecHook = _toFunctionReference(key); - (address plugin, uint8 functionId) = preExecHook.unpack(); - bytes memory preExecHookReturnData; - try IPlugin(plugin).preExecutionHook(functionId, msg.sender, msg.value, data) returns ( - bytes memory returnData - ) { - preExecHookReturnData = returnData; - } catch (bytes memory revertReason) { - revert PreExecHookReverted(plugin, functionId, revertReason); + if (preExecHook.isEmptyOrMagicValue()) { + // The function reference must be PRE_HOOK_ALWAYS_DENY in this case, because zero and any other + // magic value is unassignable here. + revert AlwaysDenyRule(); } - // Check to see if there is a postExec hook set for this preExec hook - FunctionReference postExecHook = hooks.associatedPostHooks[preExecHook]; - if (FunctionReference.unwrap(postExecHook) != 0) { - postHooksToRun[postExecHooksLength].postExecHook = postExecHook; - postHooksToRun[postExecHooksLength].preExecHookReturnData = preExecHookReturnData; - unchecked { - ++postExecHooksLength; + uint256 associatedPostExecHooksLength = hooks.associatedPostHooks[preExecHook].length(); + if (associatedPostExecHooksLength > 0) { + for (uint256 j = 0; j < associatedPostExecHooksLength;) { + (key,) = hooks.associatedPostHooks[preExecHook].at(j); + postHooksToRun[actualPostHooksToRunLength].postExecHook = _toFunctionReference(key); + postHooksToRun[actualPostHooksToRunLength].preExecHookReturnData = + _runPreExecHook(preExecHook, data); + + unchecked { + ++actualPostHooksToRunLength; + ++j; + } } + } else { + _runPreExecHook(preExecHook, data); } unchecked { @@ -543,26 +555,39 @@ contract UpgradeableModularAccount is } // Copy post-only hooks to the end of the array - uint256 postOnlyHooksLength = hooks.postOnlyHooks.length(); for (uint256 i = 0; i < postOnlyHooksLength;) { - postHooksToRun[postExecHooksLength].postExecHook = _toFunctionReference(hooks.postOnlyHooks.at(i)); + (bytes32 key,) = hooks.postOnlyHooks.at(i); + postHooksToRun[actualPostHooksToRunLength].postExecHook = _toFunctionReference(key); unchecked { - ++postExecHooksLength; + ++actualPostHooksToRunLength; ++i; } } + + // Trim the post hook array to the actual length, since we may have overallocated. + assembly ("memory-safe") { + mstore(postHooksToRun, actualPostHooksToRunLength) + } + } + + function _runPreExecHook(FunctionReference preExecHook, 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 ( + bytes memory returnData + ) { + preExecHookReturnData = returnData; + } catch (bytes memory revertReason) { + revert PreExecHookReverted(plugin, functionId, revertReason); + } } function _doCachedPostExecHooks(PostExecToRun[] memory postHooksToRun) internal { uint256 postHooksToRunLength = postHooksToRun.length; for (uint256 i = 0; i < postHooksToRunLength;) { PostExecToRun memory postHookToRun = postHooksToRun[i]; - FunctionReference postExecHook = postHookToRun.postExecHook; - if (postExecHook == FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE) { - // Reached the end of runnable postExec hooks, stop. - // Array may be over-allocated. - return; - } (address plugin, uint8 functionId) = postHookToRun.postExecHook.unpack(); // solhint-disable-next-line no-empty-blocks try IPlugin(plugin).postExecutionHook(functionId, postHookToRun.preExecHookReturnData) {} diff --git a/src/libraries/AccountStorage.sol b/src/libraries/AccountStorage.sol index 070065d0..36d2845d 100644 --- a/src/libraries/AccountStorage.sol +++ b/src/libraries/AccountStorage.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.19; import {IPlugin} from "../interfaces/IPlugin.sol"; +import {EnumerableMap} from "@openzeppelin/contracts/utils/structs/EnumerableMap.sol"; import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import {FunctionReference} from "../libraries/FunctionReferenceLib.sol"; @@ -52,10 +53,10 @@ struct PermittedExternalCallData { // Represets a set of pre- and post- hooks. Used to store both execution hooks and permitted call hooks. struct HookGroup { - EnumerableSet.Bytes32Set preHooks; + EnumerableMap.Bytes32ToUintMap preHooks; // bytes21 key = pre hook function reference - mapping(FunctionReference => FunctionReference) associatedPostHooks; - EnumerableSet.Bytes32Set postOnlyHooks; + mapping(FunctionReference => EnumerableMap.Bytes32ToUintMap) associatedPostHooks; + EnumerableMap.Bytes32ToUintMap postOnlyHooks; } // Represents data associated with a specifc function selector. @@ -66,8 +67,8 @@ struct SelectorData { FunctionReference userOpValidation; FunctionReference runtimeValidation; // The pre validation hooks for this function selector. - EnumerableSet.Bytes32Set preUserOpValidationHooks; - EnumerableSet.Bytes32Set preRuntimeValidationHooks; + EnumerableMap.Bytes32ToUintMap preUserOpValidationHooks; + EnumerableMap.Bytes32ToUintMap preRuntimeValidationHooks; // The execution hooks for this function selector. HookGroup executionHooks; } @@ -100,15 +101,21 @@ function getPermittedCallKey(address addr, bytes4 selector) pure returns (bytes2 } // Helper function to get all elements of a set into memory. -using EnumerableSet for EnumerableSet.Bytes32Set; +using EnumerableMap for EnumerableMap.Bytes32ToUintMap; -function toFunctionReferenceArray(EnumerableSet.Bytes32Set storage set) +function toFunctionReferenceArray(EnumerableMap.Bytes32ToUintMap storage map) view returns (FunctionReference[] memory) { - FunctionReference[] memory result = new FunctionReference[](set.length()); - for (uint256 i = 0; i < set.length(); i++) { - result[i] = FunctionReference.wrap(bytes21(set.at(i))); + uint256 length = map.length(); + FunctionReference[] memory result = new FunctionReference[](length); + for (uint256 i = 0; i < length;) { + (bytes32 key,) = map.at(i); + result[i] = FunctionReference.wrap(bytes21(key)); + + unchecked { + ++i; + } } return result; } diff --git a/src/libraries/FunctionReferenceLib.sol b/src/libraries/FunctionReferenceLib.sol index 0bef591c..897f5bd1 100644 --- a/src/libraries/FunctionReferenceLib.sol +++ b/src/libraries/FunctionReferenceLib.sol @@ -25,6 +25,10 @@ library FunctionReferenceLib { functionId = uint8(bytes1(underlying << 160)); } + function isEmpty(FunctionReference fr) internal pure returns (bool) { + return fr == _EMPTY_FUNCTION_REFERENCE; + } + function isEmptyOrMagicValue(FunctionReference fr) internal pure returns (bool) { return FunctionReference.unwrap(fr) <= bytes21(uint168(2)); } diff --git a/test/account/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol index fbc13f82..cd10df41 100644 --- a/test/account/AccountExecHooks.t.sol +++ b/test/account/AccountExecHooks.t.sol @@ -15,7 +15,7 @@ import { } from "../../src/interfaces/IPlugin.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; -import {FunctionReference} from "../../src/libraries/FunctionReferenceLib.sol"; +import {FunctionReference, FunctionReferenceLib} from "../../src/libraries/FunctionReferenceLib.sol"; import {MockPlugin} from "../mocks/MockPlugin.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; @@ -31,7 +31,9 @@ contract AccountExecHooksTest is OptimizedTest { UpgradeableModularAccount public account; MockPlugin public mockPlugin1; + MockPlugin public mockPlugin2; bytes32 public manifestHash1; + bytes32 public manifestHash2; bytes4 internal constant _EXEC_SELECTOR = bytes4(uint32(1)); uint8 internal constant _PRE_HOOK_FUNCTION_ID_1 = 1; @@ -40,6 +42,7 @@ contract AccountExecHooksTest is OptimizedTest { uint8 internal constant _POST_HOOK_FUNCTION_ID_4 = 4; PluginManifest public m1; + PluginManifest public m2; /// @dev Note that we strip hookApplyData from InjectedHooks in this event for gas savings event PluginInstalled( @@ -200,11 +203,223 @@ contract AccountExecHooksTest is OptimizedTest { _uninstallPlugin(mockPlugin1); } + function test_overlappingExecHookPairs_install() public { + // Install the first plugin. + _installPlugin1WithHooks( + _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 + }) + ); + + // Install a second plugin that applies the first plugin's hook pair to the same selector. + 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); + _installPlugin2WithHooks( + _EXEC_SELECTOR, + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.DEPENDENCY, + functionId: 0, + dependencyIndex: 0 + }), + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.DEPENDENCY, + functionId: 0, + dependencyIndex: 1 + }), + dependencies + ); + + vm.stopPrank(); + } + + /// @dev Plugin 1 hook pair: [1, 2] + /// Plugin 2 hook pair: [1, 2] + /// Expected execution: [1, 2] + function test_overlappingExecHookPairs_run() public { + test_overlappingExecHookPairs_install(); + + // Expect the pre hook to be called just once. + vm.expectCall( + address(mockPlugin1), + abi.encodeWithSelector( + IPlugin.preExecutionHook.selector, + _PRE_HOOK_FUNCTION_ID_1, + address(this), // caller + 0, // msg.value in call to account + abi.encodeWithSelector(_EXEC_SELECTOR) + ), + 1 + ); + + // Expect the post hook to be called just once, with the expected data. + vm.expectCall( + address(mockPlugin1), + abi.encodeWithSelector(IPlugin.postExecutionHook.selector, _POST_HOOK_FUNCTION_ID_2, ""), + 1 + ); + + (bool success,) = address(account).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + assertTrue(success); + } + + function test_overlappingExecHookPairs_uninstall() public { + test_overlappingExecHookPairs_install(); + + // Uninstall the second plugin. + _uninstallPlugin(mockPlugin2); + + // Expect the pre/post hooks to still exist after uninstalling a plugin with a duplicate hook. + vm.expectCall( + address(mockPlugin1), + abi.encodeWithSelector( + IPlugin.preExecutionHook.selector, + _PRE_HOOK_FUNCTION_ID_1, + address(this), // caller + 0, // msg.value in call to account + abi.encodeWithSelector(_EXEC_SELECTOR) + ), + 1 + ); + vm.expectCall( + address(mockPlugin1), + abi.encodeWithSelector(IPlugin.postExecutionHook.selector, _POST_HOOK_FUNCTION_ID_2, ""), + 1 + ); + (bool success,) = address(account).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + assertTrue(success); + + // Uninstall the first plugin. + _uninstallPlugin(mockPlugin1); + + // Execution selector should no longer exist. + (success,) = address(account).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + assertFalse(success); + } + + function test_overlappingExecHookPairsOnPost_install() public { + // Install the first plugin. + _installPlugin1WithHooks( + _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 + }) + ); + + // Install the second plugin. + FunctionReference[] memory dependencies = new FunctionReference[](1); + dependencies[0] = FunctionReferenceLib.pack(address(mockPlugin1), _POST_HOOK_FUNCTION_ID_2); + _installPlugin2WithHooks( + _EXEC_SELECTOR, + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + functionId: _PRE_HOOK_FUNCTION_ID_3, + dependencyIndex: 0 + }), + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.DEPENDENCY, + functionId: 0, + dependencyIndex: 0 + }), + dependencies + ); + } + + /// @dev Plugin 1 hook pair: [1, 2] + /// Plugin 2 hook pair: [3, 2] + /// Expected execution: [1, 2], [3, 2] + function test_overlappingExecHookPairsOnPost_run() public { + test_overlappingExecHookPairsOnPost_install(); + + // Expect each pre hook to be called once. + vm.expectCall( + address(mockPlugin1), + abi.encodeWithSelector( + IPlugin.preExecutionHook.selector, + _PRE_HOOK_FUNCTION_ID_1, + address(this), // caller + 0, // msg.value in call to account + abi.encodeWithSelector(_EXEC_SELECTOR) + ), + 1 + ); + vm.expectCall( + address(mockPlugin2), + abi.encodeWithSelector( + IPlugin.preExecutionHook.selector, + _PRE_HOOK_FUNCTION_ID_3, + address(this), // caller + 0, // msg.value in call to account + abi.encodeWithSelector(_EXEC_SELECTOR) + ), + 1 + ); + + // Expect the post hook to be called twice, with the expected data. + vm.expectCall( + address(mockPlugin1), + abi.encodeWithSelector(IPlugin.postExecutionHook.selector, _POST_HOOK_FUNCTION_ID_2, ""), + 2 + ); + + (bool success,) = address(account).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + assertTrue(success); + } + + function test_overlappingExecHookPairsOnPost_uninstall() public { + test_overlappingExecHookPairsOnPost_install(); + + // Uninstall the second plugin. + _uninstallPlugin(mockPlugin2); + + // Expect the pre/post hooks to still exist after uninstalling a plugin with a duplicate hook. + vm.expectCall( + address(mockPlugin1), + abi.encodeWithSelector( + IPlugin.preExecutionHook.selector, + _PRE_HOOK_FUNCTION_ID_1, + address(this), // caller + 0, // msg.value in call to account + abi.encodeWithSelector(_EXEC_SELECTOR) + ), + 1 + ); + vm.expectCall( + address(mockPlugin1), + abi.encodeWithSelector(IPlugin.postExecutionHook.selector, _POST_HOOK_FUNCTION_ID_2, ""), + 1 + ); + (bool success,) = address(account).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + assertTrue(success); + + // Uninstall the first plugin. + _uninstallPlugin(mockPlugin1); + + // Execution selector should no longer exist. + (success,) = address(account).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + assertFalse(success); + } + function _installPlugin1WithHooks( bytes4 selector, ManifestFunction memory preHook, ManifestFunction memory postHook - ) internal returns (MockPlugin) { + ) internal { m1.executionHooks.push(ManifestExecutionHook(selector, preHook, postHook)); mockPlugin1 = new MockPlugin(m1); manifestHash1 = keccak256(abi.encode(mockPlugin1.pluginManifest())); @@ -223,8 +438,40 @@ contract AccountExecHooksTest is OptimizedTest { dependencies: new FunctionReference[](0), injectedHooks: new IPluginManager.InjectedHook[](0) }); + } + + function _installPlugin2WithHooks( + bytes4 selector, + ManifestFunction memory preHook, + ManifestFunction memory postHook, + FunctionReference[] memory dependencies + ) internal { + if (preHook.functionType == ManifestAssociatedFunctionType.DEPENDENCY) { + m2.dependencyInterfaceIds.push(type(IPlugin).interfaceId); + } + if (postHook.functionType == ManifestAssociatedFunctionType.DEPENDENCY) { + m2.dependencyInterfaceIds.push(type(IPlugin).interfaceId); + } - return mockPlugin1; + m2.executionHooks.push(ManifestExecutionHook(selector, preHook, postHook)); + + mockPlugin2 = new MockPlugin(m2); + manifestHash2 = keccak256(abi.encode(mockPlugin2.pluginManifest())); + + vm.expectEmit(true, true, true, true); + emit ReceivedCall(abi.encodeCall(IPlugin.onInstall, (bytes(""))), 0); + vm.expectEmit(true, true, true, true); + emit PluginInstalled( + address(mockPlugin2), manifestHash2, dependencies, new IPluginManager.InjectedHook[](0) + ); + + account.installPlugin({ + plugin: address(mockPlugin2), + manifestHash: manifestHash2, + pluginInitData: bytes(""), + dependencies: dependencies, + injectedHooks: new IPluginManager.InjectedHook[](0) + }); } function _uninstallPlugin(MockPlugin plugin) internal { diff --git a/test/account/AccountPermittedCallHooks.t.sol b/test/account/AccountPermittedCallHooks.t.sol index 841760bf..a90eaae0 100644 --- a/test/account/AccountPermittedCallHooks.t.sol +++ b/test/account/AccountPermittedCallHooks.t.sol @@ -208,6 +208,143 @@ contract AccountPermittedCallHooksTest is OptimizedTest { _uninstallPlugin(mockPlugin1); } + function test_overlappingPermittedCallHookPairs_install() public { + _installPlugin1WithHooks( + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + functionId: _PRE_HOOK_FUNCTION_ID_1, + dependencyIndex: 0 + }), + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + functionId: _POST_HOOK_FUNCTION_ID_2, + dependencyIndex: 0 + }), + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + functionId: _PRE_HOOK_FUNCTION_ID_1, + dependencyIndex: 0 + }), + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + functionId: _POST_HOOK_FUNCTION_ID_2, + dependencyIndex: 0 + }) + ); + } + + /// @dev Plugin hook pair(s): [1, 2], [1, 2] + /// Expected execution: [1, 2] + function test_overlappingPermittedCallHookPairs_run() public { + test_overlappingPermittedCallHookPairs_install(); + + vm.startPrank(address(mockPlugin1)); + + // Expect the pre hook to be called just once. + vm.expectCall( + address(mockPlugin1), + abi.encodeWithSelector( + IPlugin.preExecutionHook.selector, + _PRE_HOOK_FUNCTION_ID_1, + address(mockPlugin1), // caller + 0, // msg.value in call to account + abi.encodePacked(_EXEC_SELECTOR) + ), + 1 + ); + + // Expect the post hook to be called just once, with the expected data. + vm.expectCall( + address(mockPlugin1), + abi.encodeWithSelector(IPlugin.postExecutionHook.selector, _POST_HOOK_FUNCTION_ID_2, ""), + 1 + ); + + account.executeFromPlugin(abi.encodePacked(_EXEC_SELECTOR)); + + vm.stopPrank(); + } + + function test_overlappingPermittedCallHookPairs_uninstall() public { + test_overlappingPermittedCallHookPairs_install(); + + _uninstallPlugin(mockPlugin1); + } + + function test_overlappingPermittedCallHookPairsOnPost_install() public { + _installPlugin1WithHooks( + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + functionId: _PRE_HOOK_FUNCTION_ID_1, + dependencyIndex: 0 + }), + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + functionId: _POST_HOOK_FUNCTION_ID_2, + dependencyIndex: 0 + }), + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + functionId: _PRE_HOOK_FUNCTION_ID_3, + dependencyIndex: 0 + }), + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.SELF, + functionId: _POST_HOOK_FUNCTION_ID_2, + dependencyIndex: 0 + }) + ); + } + + /// @dev Plugin hook pair(s): [1, 2], [3, 2] + /// Expected execution: [1, 2], [3, 2] + function test_overlappingPermittedCallHookPairsOnPost_run() public { + test_overlappingPermittedCallHookPairsOnPost_install(); + + vm.startPrank(address(mockPlugin1)); + + // Expect each pre hook to be called once. + vm.expectCall( + address(mockPlugin1), + abi.encodeWithSelector( + IPlugin.preExecutionHook.selector, + _PRE_HOOK_FUNCTION_ID_3, + address(mockPlugin1), // caller + 0, // msg.value in call to account + abi.encodePacked(_EXEC_SELECTOR) + ), + 1 + ); + vm.expectCall( + address(mockPlugin1), + abi.encodeWithSelector( + IPlugin.preExecutionHook.selector, + _PRE_HOOK_FUNCTION_ID_1, + address(mockPlugin1), // caller + 0, // msg.value in call to account + abi.encodePacked(_EXEC_SELECTOR) + ), + 1 + ); + + // Expect the post hook to be called twice, with the expected data. + vm.expectCall( + address(mockPlugin1), + abi.encodeWithSelector(IPlugin.postExecutionHook.selector, _POST_HOOK_FUNCTION_ID_2, ""), + 2 + ); + + account.executeFromPlugin(abi.encodePacked(_EXEC_SELECTOR)); + + vm.stopPrank(); + } + + function test_overlappingPermittedCallHookPairsOnPost_uninstall() public { + test_overlappingPermittedCallHookPairsOnPost_install(); + + _uninstallPlugin(mockPlugin1); + } + function _installPlugin1WithHooks(ManifestFunction memory preHook1, ManifestFunction memory postHook1) internal { From c57b48630b3e91c6611b0951c42d66edc02fbedd Mon Sep 17 00:00:00 2001 From: Jay Paik Date: Fri, 1 Dec 2023 12:17:44 -0500 Subject: [PATCH 20/20] feat: update ordering of associated post hook executions --- src/account/UpgradeableModularAccount.sol | 33 ++++++++++++----------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 46cd642d..0caf3b13 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -522,6 +522,18 @@ contract UpgradeableModularAccount is postHooksToRun = new PostExecToRun[](maxPostExecHooksLength); uint256 actualPostHooksToRunLength; + // Copy post-only hooks to the array. + for (uint256 i = 0; i < postOnlyHooksLength;) { + (bytes32 key,) = hooks.postOnlyHooks.at(i); + postHooksToRun[actualPostHooksToRunLength].postExecHook = _toFunctionReference(key); + unchecked { + ++actualPostHooksToRunLength; + ++i; + } + } + + // Then run the pre hooks and copy the associated post hooks (along with their pre hook's return data) to + // the array. for (uint256 i = 0; i < preExecHooksLength;) { (bytes32 key,) = hooks.preHooks.at(i); FunctionReference preExecHook = _toFunctionReference(key); @@ -554,16 +566,6 @@ contract UpgradeableModularAccount is } } - // Copy post-only hooks to the end of the array - for (uint256 i = 0; i < postOnlyHooksLength;) { - (bytes32 key,) = hooks.postOnlyHooks.at(i); - postHooksToRun[actualPostHooksToRunLength].postExecHook = _toFunctionReference(key); - unchecked { - ++actualPostHooksToRunLength; - ++i; - } - } - // Trim the post hook array to the actual length, since we may have overallocated. assembly ("memory-safe") { mstore(postHooksToRun, actualPostHooksToRunLength) @@ -584,9 +586,14 @@ contract UpgradeableModularAccount is } } + /// @dev Associated post hooks are run in reverse order of their pre hooks. function _doCachedPostExecHooks(PostExecToRun[] memory postHooksToRun) internal { uint256 postHooksToRunLength = postHooksToRun.length; - for (uint256 i = 0; i < postHooksToRunLength;) { + for (uint256 i = postHooksToRunLength; i > 0;) { + unchecked { + --i; + } + PostExecToRun memory postHookToRun = postHooksToRun[i]; (address plugin, uint8 functionId) = postHookToRun.postExecHook.unpack(); // solhint-disable-next-line no-empty-blocks @@ -594,10 +601,6 @@ contract UpgradeableModularAccount is catch (bytes memory revertReason) { revert PostExecHookReverted(plugin, functionId, revertReason); } - - unchecked { - ++i; - } } }