From 67b7583bfb934d46809a30ab2fc682efcbcaf5e4 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Tue, 28 Nov 2023 15:44:41 -0800 Subject: [PATCH] update BaseModularAccount to PluginManagerInternals and clean it up --- src/account/AccountStorageInitializable.sol | 1 + ...Account.sol => PluginManagerInternals.sol} | 12 +++------ src/account/UpgradeableModularAccount.sol | 26 ++++++++++++------- src/interfaces/IPlugin.sol | 1 + test/account/ManifestValidity.t.sol | 18 ++++++------- test/account/UpgradeableModularAccount.t.sol | 18 ++++++------- 6 files changed, 41 insertions(+), 35 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 bc6a45d7..9590feff 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -1,31 +1,35 @@ // 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} from "../interfaces/IStandardExecutor.sol"; -import {IPluginExecutor} from "../interfaces/IPluginExecutor.sol"; import {AccountStorage, getAccountStorage, getPermittedCallKey} from "../libraries/AccountStorage.sol"; +import {AccountStorageInitializable} from "./AccountStorageInitializable.sol"; import {Execution} from "../libraries/ERC6900TypeUtils.sol"; import {FunctionReference, FunctionReferenceLib} from "../libraries/FunctionReferenceLib.sol"; -import {AccountStorageInitializable} from "./AccountStorageInitializable.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 { @@ -38,6 +42,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 1a95c2d1..f30d70f7 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"; @@ -316,7 +316,7 @@ contract UpgradeableModularAccountTest is Test { vm.expectRevert( abi.encodeWithSelector( - BaseModularAccount.PermittedExecutionSelectorNotInstalled.selector, + PluginManagerInternals.PermittedExecutionSelectorNotInstalled.selector, IPlugin.onInstall.selector, address(mockPluginWithBadPermittedExec) ) @@ -333,7 +333,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), @@ -348,7 +348,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), @@ -373,7 +373,7 @@ contract UpgradeableModularAccountTest is Test { vm.expectRevert( abi.encodeWithSelector( - BaseModularAccount.PluginAlreadyInstalled.selector, address(tokenReceiverPlugin) + PluginManagerInternals.PluginAlreadyInstalled.selector, address(tokenReceiverPlugin) ) ); IPluginManager(account2).installPlugin({ @@ -455,7 +455,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), @@ -585,7 +585,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({ @@ -616,7 +616,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), @@ -652,7 +652,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),