From 8b0ddf34392ab92f2bd2e50b9672d43cec2d85a3 Mon Sep 17 00:00:00 2001 From: adam Date: Wed, 10 Jan 2024 11:52:17 -0500 Subject: [PATCH 1/2] Revert "Split up base account to fit under size limit (#19)" This reverts commit e1a78822a4d114a5ca65121febd298172d812d5d. --- foundry.toml | 1 + ...Manager.sol => PluginManagerInternals.sol} | 427 +++++++++--------- src/account/UpgradeableModularAccount.sol | 43 +- test/account/ManifestValidity.t.sol | 18 +- test/account/UpgradeableModularAccount.t.sol | 18 +- test/mocks/MockPlugin.sol | 2 - test/utils/OptimizedTest.sol | 17 +- 7 files changed, 231 insertions(+), 295 deletions(-) rename src/account/{PluginManager.sol => PluginManagerInternals.sol} (97%) diff --git a/foundry.toml b/foundry.toml index 23b1cb58..2754a5b2 100644 --- a/foundry.toml +++ b/foundry.toml @@ -7,6 +7,7 @@ libs = ['lib'] out = 'out' optimizer = true optimizer_runs = 10_000 +ignored_error_codes = [3628] fs_permissions = [ { access = "read", path = "./out-optimized" } ] diff --git a/src/account/PluginManager.sol b/src/account/PluginManagerInternals.sol similarity index 97% rename from src/account/PluginManager.sol rename to src/account/PluginManagerInternals.sol index 0f5b209a..6f47c55a 100644 --- a/src/account/PluginManager.sol +++ b/src/account/PluginManagerInternals.sol @@ -27,12 +27,10 @@ import { PluginManifest } from "../interfaces/IPlugin.sol"; -contract PluginManager { +abstract contract PluginManagerInternals is IPluginManager { using EnumerableMap for EnumerableMap.Bytes32ToUintMap; using EnumerableSet for EnumerableSet.AddressSet; - address private immutable _SELF; - error ArrayLengthMismatch(); error ExecutionFunctionAlreadySet(bytes4 selector); error InvalidDependenciesProvided(); @@ -40,7 +38,6 @@ contract PluginManager { error MissingPluginDependency(address dependency); error NullFunctionReference(); error NullPlugin(); - error OnlyDelegate(); error PluginAlreadyInstalled(address plugin); error PluginDependencyViolation(address plugin); error PluginInstallCallbackFailed(address plugin, bytes revertReason); @@ -51,17 +48,6 @@ contract PluginManager { error PluginApplyHookCallbackFailed(address providingPlugin, bytes revertReason); error PluginUnapplyHookCallbackFailed(address providingPlugin, bytes revertReason); - // Re-declare events from IPluginManager, since solidity doesn't support importing events from interfaces. - - event PluginInstalled( - address indexed plugin, - bytes32 manifestHash, - FunctionReference[] dependencies, - IPluginManager.InjectedHook[] injectedHooks - ); - - event PluginUninstalled(address indexed plugin, bool indexed callbacksSucceeded); - modifier notNullFunction(FunctionReference functionReference) { if (functionReference.isEmpty()) { revert NullFunctionReference(); @@ -76,24 +62,210 @@ contract PluginManager { _; } - modifier onlyDelegate() { - if (address(this) == _SELF) { - revert OnlyDelegate(); + // Storage update operations + + function _setExecutionFunction(bytes4 selector, address plugin) internal notNullPlugin(plugin) { + SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; + + if (_selectorData.plugin != address(0)) { + revert ExecutionFunctionAlreadySet(selector); + } + + _selectorData.plugin = plugin; + } + + function _removeExecutionFunction(bytes4 selector) internal { + SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; + + _selectorData.plugin = address(0); + } + + function _addUserOpValidationFunction(bytes4 selector, FunctionReference validationFunction) + internal + notNullFunction(validationFunction) + { + SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; + + if (!_selectorData.userOpValidation.isEmpty()) { + revert UserOpValidationFunctionAlreadySet(selector, validationFunction); + } + + _selectorData.userOpValidation = validationFunction; + } + + function _removeUserOpValidationFunction(bytes4 selector, FunctionReference validationFunction) + internal + notNullFunction(validationFunction) + { + SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; + + _selectorData.userOpValidation = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; + } + + function _addRuntimeValidationFunction(bytes4 selector, FunctionReference validationFunction) + internal + notNullFunction(validationFunction) + { + SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; + + if (!_selectorData.runtimeValidation.isEmpty()) { + revert RuntimeValidationFunctionAlreadySet(selector, validationFunction); + } + + _selectorData.runtimeValidation = validationFunction; + } + + function _removeRuntimeValidationFunction(bytes4 selector, FunctionReference validationFunction) + internal + notNullFunction(validationFunction) + { + SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; + + _selectorData.runtimeValidation = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; + } + + function _addExecHooks(bytes4 selector, FunctionReference preExecHook, FunctionReference postExecHook) + internal + { + SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; + + _addHooks(_selectorData.executionHooks, preExecHook, postExecHook); + } + + function _removeExecHooks(bytes4 selector, FunctionReference preExecHook, FunctionReference postExecHook) + internal + { + SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; + + _removeHooks(_selectorData.executionHooks, preExecHook, postExecHook); + } + + function _enableExecFromPlugin(bytes4 selector, address plugin, AccountStorage storage accountStorage) + internal + { + bytes24 key = getPermittedCallKey(plugin, selector); + + // 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; + } + + function _disableExecFromPlugin(bytes4 selector, address plugin, AccountStorage storage accountStorage) + internal + { + bytes24 key = getPermittedCallKey(plugin, selector); + accountStorage.permittedCalls[key].callPermitted = false; + } + + function _addPermittedCallHooks( + bytes4 selector, + address plugin, + FunctionReference preExecHook, + FunctionReference postExecHook + ) internal notNullPlugin(plugin) { + bytes24 permittedCallKey = getPermittedCallKey(plugin, selector); + PermittedCallData storage _permittedCalldata = getAccountStorage().permittedCalls[permittedCallKey]; + + _addHooks(_permittedCalldata.permittedCallHooks, preExecHook, postExecHook); + } + + function _removePermittedCallHooks( + bytes4 selector, + address plugin, + FunctionReference preExecHook, + FunctionReference postExecHook + ) internal notNullPlugin(plugin) { + bytes24 permittedCallKey = getPermittedCallKey(plugin, selector); + PermittedCallData storage _permittedCallData = getAccountStorage().permittedCalls[permittedCallKey]; + + _removeHooks(_permittedCallData.permittedCallHooks, preExecHook, postExecHook); + } + + function _addHooks(HookGroup storage hooks, FunctionReference preExecHook, FunctionReference postExecHook) + internal + { + if (!preExecHook.isEmpty()) { + _addOrIncrement(hooks.preHooks, _toSetValue(preExecHook)); + + if (!postExecHook.isEmpty()) { + _addOrIncrement(hooks.associatedPostHooks[preExecHook], _toSetValue(postExecHook)); + } + } else { + if (postExecHook.isEmpty()) { + // both pre and post hooks cannot be null + revert NullFunctionReference(); + } + + _addOrIncrement(hooks.postOnlyHooks, _toSetValue(postExecHook)); } - _; } - constructor() { - _SELF = address(this); + function _removeHooks(HookGroup storage hooks, FunctionReference preExecHook, FunctionReference postExecHook) + internal + { + if (!preExecHook.isEmpty()) { + _removeOrDecrement(hooks.preHooks, _toSetValue(preExecHook)); + + if (!postExecHook.isEmpty()) { + _removeOrDecrement(hooks.associatedPostHooks[preExecHook], _toSetValue(postExecHook)); + } + } else { + // 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. + _removeOrDecrement(hooks.postOnlyHooks, _toSetValue(postExecHook)); + } + } + + function _addPreUserOpValidationHook(bytes4 selector, FunctionReference preUserOpValidationHook) + internal + notNullFunction(preUserOpValidationHook) + { + _addOrIncrement( + getAccountStorage().selectorData[selector].preUserOpValidationHooks, + _toSetValue(preUserOpValidationHook) + ); + } + + function _removePreUserOpValidationHook(bytes4 selector, FunctionReference preUserOpValidationHook) + internal + notNullFunction(preUserOpValidationHook) + { + // May ignore return value, as the manifest hash is validated to ensure that the hook exists. + _removeOrDecrement( + getAccountStorage().selectorData[selector].preUserOpValidationHooks, + _toSetValue(preUserOpValidationHook) + ); + } + + function _addPreRuntimeValidationHook(bytes4 selector, FunctionReference preRuntimeValidationHook) + internal + notNullFunction(preRuntimeValidationHook) + { + _addOrIncrement( + getAccountStorage().selectorData[selector].preRuntimeValidationHooks, + _toSetValue(preRuntimeValidationHook) + ); + } + + function _removePreRuntimeValidationHook(bytes4 selector, FunctionReference preRuntimeValidationHook) + internal + notNullFunction(preRuntimeValidationHook) + { + // May ignore return value, as the manifest hash is validated to ensure that the hook exists. + _removeOrDecrement( + getAccountStorage().selectorData[selector].preRuntimeValidationHooks, + _toSetValue(preRuntimeValidationHook) + ); } - function installPlugin( + function _installPlugin( address plugin, bytes32 manifestHash, bytes memory pluginInitData, FunctionReference[] memory dependencies, - IPluginManager.InjectedHook[] memory injectedHooks - ) external onlyDelegate { + InjectedHook[] memory injectedHooks + ) internal { AccountStorage storage _storage = getAccountStorage(); // Check if the plugin exists. @@ -212,7 +384,7 @@ contract PluginManager { } for (uint256 i = 0; i < length;) { - IPluginManager.InjectedHook memory hook = injectedHooks[i]; + InjectedHook memory hook = injectedHooks[i]; _storage.pluginData[plugin].injectedHooks[i] = StoredInjectedHook({ providingPlugin: hook.providingPlugin, selector: hook.selector, @@ -364,7 +536,7 @@ contract PluginManager { length = injectedHooks.length; for (uint256 i = 0; i < length;) { - IPluginManager.InjectedHook memory hook = injectedHooks[i]; + InjectedHook memory hook = injectedHooks[i]; // not inlined in function call to avoid stack too deep error bytes memory onHookApplyData = injectedHooks[i].hookApplyData; /* solhint-disable no-empty-blocks */ @@ -388,12 +560,12 @@ contract PluginManager { emit PluginInstalled(plugin, manifestHash, dependencies, injectedHooks); } - function uninstallPlugin( + function _uninstallPlugin( address plugin, PluginManifest memory manifest, bytes memory uninstallData, bytes[] calldata hookUnapplyData - ) external onlyDelegate { + ) internal { AccountStorage storage _storage = getAccountStorage(); // Check if the plugin exists. @@ -636,7 +808,7 @@ contract PluginManager { /* solhint-disable no-empty-blocks */ try IPlugin(hook.providingPlugin).onHookUnapply( plugin, - IPluginManager.InjectedHooksInfo({ + InjectedHooksInfo({ preExecHookFunctionId: hook.preExecHookFunctionId, isPostHookUsed: hook.isPostHookUsed, postExecHookFunctionId: hook.postExecHookFunctionId @@ -666,203 +838,6 @@ contract PluginManager { emit PluginUninstalled(plugin, onUninstallSuccess); } - // Storage update operations - - function _setExecutionFunction(bytes4 selector, address plugin) internal notNullPlugin(plugin) { - SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - - if (_selectorData.plugin != address(0)) { - revert ExecutionFunctionAlreadySet(selector); - } - - _selectorData.plugin = plugin; - } - - function _removeExecutionFunction(bytes4 selector) internal { - SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - - _selectorData.plugin = address(0); - } - - function _addUserOpValidationFunction(bytes4 selector, FunctionReference validationFunction) - internal - notNullFunction(validationFunction) - { - SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - - if (!_selectorData.userOpValidation.isEmpty()) { - revert UserOpValidationFunctionAlreadySet(selector, validationFunction); - } - - _selectorData.userOpValidation = validationFunction; - } - - function _removeUserOpValidationFunction(bytes4 selector, FunctionReference validationFunction) - internal - notNullFunction(validationFunction) - { - SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - - _selectorData.userOpValidation = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; - } - - function _addRuntimeValidationFunction(bytes4 selector, FunctionReference validationFunction) - internal - notNullFunction(validationFunction) - { - SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - - if (!_selectorData.runtimeValidation.isEmpty()) { - revert RuntimeValidationFunctionAlreadySet(selector, validationFunction); - } - - _selectorData.runtimeValidation = validationFunction; - } - - function _removeRuntimeValidationFunction(bytes4 selector, FunctionReference validationFunction) - internal - notNullFunction(validationFunction) - { - SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - - _selectorData.runtimeValidation = FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; - } - - function _addExecHooks(bytes4 selector, FunctionReference preExecHook, FunctionReference postExecHook) - internal - { - SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - - _addHooks(_selectorData.executionHooks, preExecHook, postExecHook); - } - - function _removeExecHooks(bytes4 selector, FunctionReference preExecHook, FunctionReference postExecHook) - internal - { - SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - - _removeHooks(_selectorData.executionHooks, preExecHook, postExecHook); - } - - function _enableExecFromPlugin(bytes4 selector, address plugin, AccountStorage storage accountStorage) - internal - { - bytes24 key = getPermittedCallKey(plugin, selector); - - // 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; - } - - function _disableExecFromPlugin(bytes4 selector, address plugin, AccountStorage storage accountStorage) - internal - { - bytes24 key = getPermittedCallKey(plugin, selector); - accountStorage.permittedCalls[key].callPermitted = false; - } - - function _addPermittedCallHooks( - bytes4 selector, - address plugin, - FunctionReference preExecHook, - FunctionReference postExecHook - ) internal notNullPlugin(plugin) { - bytes24 permittedCallKey = getPermittedCallKey(plugin, selector); - PermittedCallData storage _permittedCalldata = getAccountStorage().permittedCalls[permittedCallKey]; - - _addHooks(_permittedCalldata.permittedCallHooks, preExecHook, postExecHook); - } - - function _removePermittedCallHooks( - bytes4 selector, - address plugin, - FunctionReference preExecHook, - FunctionReference postExecHook - ) internal notNullPlugin(plugin) { - bytes24 permittedCallKey = getPermittedCallKey(plugin, selector); - PermittedCallData storage _permittedCallData = getAccountStorage().permittedCalls[permittedCallKey]; - - _removeHooks(_permittedCallData.permittedCallHooks, preExecHook, postExecHook); - } - - function _addHooks(HookGroup storage hooks, FunctionReference preExecHook, FunctionReference postExecHook) - internal - { - if (!preExecHook.isEmpty()) { - _addOrIncrement(hooks.preHooks, _toSetValue(preExecHook)); - - if (!postExecHook.isEmpty()) { - _addOrIncrement(hooks.associatedPostHooks[preExecHook], _toSetValue(postExecHook)); - } - } else { - if (postExecHook.isEmpty()) { - // both pre and post hooks cannot be null - revert NullFunctionReference(); - } - - _addOrIncrement(hooks.postOnlyHooks, _toSetValue(postExecHook)); - } - } - - function _removeHooks(HookGroup storage hooks, FunctionReference preExecHook, FunctionReference postExecHook) - internal - { - if (!preExecHook.isEmpty()) { - _removeOrDecrement(hooks.preHooks, _toSetValue(preExecHook)); - - if (!postExecHook.isEmpty()) { - _removeOrDecrement(hooks.associatedPostHooks[preExecHook], _toSetValue(postExecHook)); - } - } else { - // 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. - _removeOrDecrement(hooks.postOnlyHooks, _toSetValue(postExecHook)); - } - } - - function _addPreUserOpValidationHook(bytes4 selector, FunctionReference preUserOpValidationHook) - internal - notNullFunction(preUserOpValidationHook) - { - _addOrIncrement( - getAccountStorage().selectorData[selector].preUserOpValidationHooks, - _toSetValue(preUserOpValidationHook) - ); - } - - function _removePreUserOpValidationHook(bytes4 selector, FunctionReference preUserOpValidationHook) - internal - notNullFunction(preUserOpValidationHook) - { - // May ignore return value, as the manifest hash is validated to ensure that the hook exists. - _removeOrDecrement( - getAccountStorage().selectorData[selector].preUserOpValidationHooks, - _toSetValue(preUserOpValidationHook) - ); - } - - function _addPreRuntimeValidationHook(bytes4 selector, FunctionReference preRuntimeValidationHook) - internal - notNullFunction(preRuntimeValidationHook) - { - _addOrIncrement( - getAccountStorage().selectorData[selector].preRuntimeValidationHooks, - _toSetValue(preRuntimeValidationHook) - ); - } - - function _removePreRuntimeValidationHook(bytes4 selector, FunctionReference preRuntimeValidationHook) - internal - notNullFunction(preRuntimeValidationHook) - { - // May ignore return value, as the manifest hash is validated to ensure that the hook exists. - _removeOrDecrement( - getAccountStorage().selectorData[selector].preRuntimeValidationHooks, - _toSetValue(preRuntimeValidationHook) - ); - } - function _addOrIncrement(EnumerableMap.Bytes32ToUintMap storage map, bytes32 key) internal { (bool success, uint256 value) = map.tryGet(key); map.set(key, success ? value + 1 : 0); diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 8d624ba0..0caf3b13 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -18,7 +18,7 @@ import {IPlugin, PluginManifest} from "../interfaces/IPlugin.sol"; import {IPluginExecutor} from "../interfaces/IPluginExecutor.sol"; import {IPluginManager} from "../interfaces/IPluginManager.sol"; import {IStandardExecutor, Call} from "../interfaces/IStandardExecutor.sol"; -import {PluginManager} from "./PluginManager.sol"; +import {PluginManagerInternals} from "./PluginManagerInternals.sol"; import {_coalescePreValidation, _coalesceValidation} from "../helpers/ValidationDataHelpers.sol"; contract UpgradeableModularAccount is @@ -29,7 +29,7 @@ contract UpgradeableModularAccount is IERC165, IPluginExecutor, IStandardExecutor, - IPluginManager, + PluginManagerInternals, UUPSUpgradeable { using EnumerableMap for EnumerableMap.Bytes32ToUintMap; @@ -41,7 +41,6 @@ contract UpgradeableModularAccount is } IEntryPoint private immutable _ENTRY_POINT; - PluginManager private immutable _PLUGIN_MANAGER; // As per the EIP-165 spec, no interface should ever match 0xffffffff bytes4 internal constant _INTERFACE_ID_INVALID = 0xffffffff; @@ -50,7 +49,6 @@ contract UpgradeableModularAccount is event ModularAccountInitialized(IEntryPoint indexed entryPoint); error AlwaysDenyRule(); - error ArrayLengthMismatch(); error AuthorizeUpgradeReverted(bytes revertReason); error ExecFromPluginNotPermitted(address plugin, bytes4 selector); error ExecFromPluginExternalNotPermitted(address plugin, address target, uint256 value, bytes data); @@ -77,9 +75,8 @@ contract UpgradeableModularAccount is _doCachedPostExecHooks(postExecHooks); } - constructor(IEntryPoint anEntryPoint, PluginManager aPluginManager) { + constructor(IEntryPoint anEntryPoint) { _ENTRY_POINT = anEntryPoint; - _PLUGIN_MANAGER = aPluginManager; _disableInitializers(); } @@ -282,7 +279,7 @@ contract UpgradeableModularAccount is bytes32 manifestHash, bytes calldata pluginInitData, FunctionReference[] calldata dependencies, - IPluginManager.InjectedHook[] calldata injectedHooks + InjectedHook[] calldata injectedHooks ) external override wrapNativeFunction { _installPlugin(plugin, manifestHash, pluginInitData, dependencies, injectedHooks); } @@ -302,15 +299,7 @@ contract UpgradeableModularAccount is manifest = IPlugin(plugin).pluginManifest(); } - // Perform the action through the plugin manager contract. - bytes memory data = - abi.encodeCall(PluginManager.uninstallPlugin, (plugin, manifest, pluginUninstallData, hookUnapplyData)); - (bool success, bytes memory returndata) = address(_PLUGIN_MANAGER).delegatecall(data); - if (!success) { - assembly ("memory-safe") { - revert(add(returndata, 32), mload(returndata)) - } - } + _uninstallPlugin(plugin, manifest, pluginUninstallData, hookUnapplyData); } /// @notice ERC165 introspection @@ -617,26 +606,4 @@ contract UpgradeableModularAccount is // solhint-disable-next-line no-empty-blocks function _authorizeUpgrade(address newImplementation) internal override {} - - function _toFunctionReference(bytes32 setValue) internal pure returns (FunctionReference) { - return FunctionReference.wrap(bytes21(setValue)); - } - - function _installPlugin( - address plugin, - bytes32 manifestHash, - bytes memory pluginInitData, - FunctionReference[] memory dependencies, - InjectedHook[] memory injectedHooks - ) internal { - bytes memory data = abi.encodeCall( - PluginManager.installPlugin, (plugin, manifestHash, pluginInitData, dependencies, injectedHooks) - ); - (bool success, bytes memory returndata) = address(_PLUGIN_MANAGER).delegatecall(data); - if (!success) { - assembly ("memory-safe") { - revert(add(returndata, 32), mload(returndata)) - } - } - } } diff --git a/test/account/ManifestValidity.t.sol b/test/account/ManifestValidity.t.sol index 5c705a3f..18972d07 100644 --- a/test/account/ManifestValidity.t.sol +++ b/test/account/ManifestValidity.t.sol @@ -5,7 +5,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 {PluginManager} from "../../src/account/PluginManager.sol"; +import {PluginManagerInternals} from "../../src/account/PluginManagerInternals.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {FunctionReference} from "../../src/libraries/FunctionReferenceLib.sol"; @@ -46,7 +46,7 @@ contract ManifestValidityTest is OptimizedTest { bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - vm.expectRevert(abi.encodeWithSelector(PluginManager.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); account.installPlugin({ plugin: address(plugin), manifestHash: manifestHash, @@ -64,7 +64,7 @@ contract ManifestValidityTest is OptimizedTest { bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - vm.expectRevert(abi.encodeWithSelector(PluginManager.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); account.installPlugin({ plugin: address(plugin), manifestHash: manifestHash, @@ -82,7 +82,7 @@ contract ManifestValidityTest is OptimizedTest { bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - vm.expectRevert(abi.encodeWithSelector(PluginManager.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); account.installPlugin({ plugin: address(plugin), manifestHash: manifestHash, @@ -98,7 +98,7 @@ contract ManifestValidityTest is OptimizedTest { bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - vm.expectRevert(abi.encodeWithSelector(PluginManager.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); account.installPlugin({ plugin: address(plugin), manifestHash: manifestHash, @@ -114,7 +114,7 @@ contract ManifestValidityTest is OptimizedTest { bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - vm.expectRevert(abi.encodeWithSelector(PluginManager.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); account.installPlugin({ plugin: address(plugin), manifestHash: manifestHash, @@ -131,7 +131,7 @@ contract ManifestValidityTest is OptimizedTest { bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - vm.expectRevert(abi.encodeWithSelector(PluginManager.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); account.installPlugin({ plugin: address(plugin), manifestHash: manifestHash, @@ -148,7 +148,7 @@ contract ManifestValidityTest is OptimizedTest { bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - vm.expectRevert(abi.encodeWithSelector(PluginManager.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); account.installPlugin({ plugin: address(plugin), manifestHash: manifestHash, @@ -164,7 +164,7 @@ contract ManifestValidityTest is OptimizedTest { bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - vm.expectRevert(abi.encodeWithSelector(PluginManager.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 bbe7d0e7..528eedd2 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 {PluginManager} from "../../src/account/PluginManager.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"; @@ -319,7 +319,7 @@ contract UpgradeableModularAccountTest is OptimizedTest { function test_installPlugin_invalidManifest() public { vm.startPrank(owner2); - vm.expectRevert(abi.encodeWithSelector(PluginManager.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); IPluginManager(account2).installPlugin({ plugin: address(tokenReceiverPlugin), manifestHash: bytes32(0), @@ -334,7 +334,7 @@ contract UpgradeableModularAccountTest is OptimizedTest { address badPlugin = address(1); vm.expectRevert( - abi.encodeWithSelector(PluginManager.PluginInterfaceNotSupported.selector, address(badPlugin)) + abi.encodeWithSelector(PluginManagerInternals.PluginInterfaceNotSupported.selector, address(badPlugin)) ); IPluginManager(account2).installPlugin({ plugin: address(badPlugin), @@ -358,7 +358,9 @@ contract UpgradeableModularAccountTest is OptimizedTest { }); vm.expectRevert( - abi.encodeWithSelector(PluginManager.PluginAlreadyInstalled.selector, address(tokenReceiverPlugin)) + abi.encodeWithSelector( + PluginManagerInternals.PluginAlreadyInstalled.selector, address(tokenReceiverPlugin) + ) ); IPluginManager(account2).installPlugin({ plugin: address(tokenReceiverPlugin), @@ -439,7 +441,7 @@ contract UpgradeableModularAccountTest is OptimizedTest { // Attempt to uninstall with a blank manifest PluginManifest memory blankManifest; - vm.expectRevert(abi.encodeWithSelector(PluginManager.InvalidPluginManifest.selector)); + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); IPluginManager(account2).uninstallPlugin({ plugin: address(plugin), config: abi.encode(blankManifest), @@ -569,7 +571,7 @@ contract UpgradeableModularAccountTest is OptimizedTest { ); vm.expectRevert( - abi.encodeWithSelector(PluginManager.MissingPluginDependency.selector, address(hooksPlugin)) + abi.encodeWithSelector(PluginManagerInternals.MissingPluginDependency.selector, address(hooksPlugin)) ); vm.prank(owner2); IPluginManager(account2).installPlugin({ @@ -600,7 +602,7 @@ contract UpgradeableModularAccountTest is OptimizedTest { vm.prank(owner2); vm.expectRevert( - abi.encodeWithSelector(PluginManager.PluginDependencyViolation.selector, address(hooksPlugin)) + abi.encodeWithSelector(PluginManagerInternals.PluginDependencyViolation.selector, address(hooksPlugin)) ); IPluginManager(account2).uninstallPlugin({ plugin: address(hooksPlugin), @@ -636,7 +638,7 @@ contract UpgradeableModularAccountTest is OptimizedTest { // length != installed hooks length bytes[] memory injectedHooksDatas = new bytes[](2); - vm.expectRevert(PluginManager.ArrayLengthMismatch.selector); + vm.expectRevert(PluginManagerInternals.ArrayLengthMismatch.selector); vm.prank(owner2); IPluginManager(account2).uninstallPlugin({ plugin: address(newPlugin), diff --git a/test/mocks/MockPlugin.sol b/test/mocks/MockPlugin.sol index 67891502..1a71e999 100644 --- a/test/mocks/MockPlugin.sol +++ b/test/mocks/MockPlugin.sol @@ -73,8 +73,6 @@ contract MockPlugin is ERC165 { return interfaceId == type(IPlugin).interfaceId || super.supportsInterface(interfaceId); } - receive() external payable {} - // solhint-disable-next-line no-complex-fallback fallback() external payable { emit ReceivedCall(msg.data, msg.value); diff --git a/test/utils/OptimizedTest.sol b/test/utils/OptimizedTest.sol index c0336708..f9431acc 100644 --- a/test/utils/OptimizedTest.sol +++ b/test/utils/OptimizedTest.sol @@ -6,7 +6,6 @@ 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 {PluginManager} from "../../src/account/PluginManager.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {TokenReceiverPlugin} from "../../src/plugins/TokenReceiverPlugin.sol"; @@ -33,22 +32,16 @@ abstract contract OptimizedTest is Test { internal returns (UpgradeableModularAccount) { - if (_isOptimizedTest()) { - PluginManager pluginManager = - PluginManager(deployCode("out-optimized/PluginManager.sol/PluginManager.json")); - - return UpgradeableModularAccount( + return _isOptimizedTest() + ? UpgradeableModularAccount( payable( deployCode( "out-optimized/UpgradeableModularAccount.sol/UpgradeableModularAccount.json", - abi.encode(entryPoint, pluginManager) + abi.encode(entryPoint) ) ) - ); - } else { - PluginManager pluginManager = new PluginManager(); - return new UpgradeableModularAccount(entryPoint, pluginManager); - } + ) + : new UpgradeableModularAccount(entryPoint); } function _deploySingleOwnerPlugin() internal returns (SingleOwnerPlugin) { From f12421f1b6b7b97f365f6e283e9124f2ee3e73ca Mon Sep 17 00:00:00 2001 From: adam Date: Wed, 10 Jan 2024 11:59:14 -0500 Subject: [PATCH 2/2] Codesize drop and remove compiler warning suppression --- foundry.toml | 4 ++-- test/mocks/MockPlugin.sol | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/foundry.toml b/foundry.toml index 2754a5b2..8d882d47 100644 --- a/foundry.toml +++ b/foundry.toml @@ -6,8 +6,8 @@ test = 'test' libs = ['lib'] out = 'out' optimizer = true -optimizer_runs = 10_000 -ignored_error_codes = [3628] +optimizer_runs = 200 +ignored_error_codes = [] fs_permissions = [ { access = "read", path = "./out-optimized" } ] diff --git a/test/mocks/MockPlugin.sol b/test/mocks/MockPlugin.sol index 1a71e999..67891502 100644 --- a/test/mocks/MockPlugin.sol +++ b/test/mocks/MockPlugin.sol @@ -73,6 +73,8 @@ contract MockPlugin is ERC165 { return interfaceId == type(IPlugin).interfaceId || super.supportsInterface(interfaceId); } + receive() external payable {} + // solhint-disable-next-line no-complex-fallback fallback() external payable { emit ReceivedCall(msg.data, msg.value);