Skip to content

Commit

Permalink
Pass in plugin manifest for install & uninstall
Browse files Browse the repository at this point in the history
  • Loading branch information
adamegyed committed Jul 19, 2024
1 parent 6abe02c commit 044492f
Show file tree
Hide file tree
Showing 15 changed files with 75 additions and 203 deletions.
5 changes: 0 additions & 5 deletions src/account/AccountLoupe.sol
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,4 @@ abstract contract AccountLoupe is IAccountLoupe {
{
preValidationHooks = getAccountStorage().validationData[validationFunction].preValidationHooks;
}

/// @inheritdoc IAccountLoupe
function getInstalledModules() external view override returns (address[] memory moduleAddresses) {
moduleAddresses = getAccountStorage().moduleManifestHashes.keys();
}
}
2 changes: 0 additions & 2 deletions src/account/AccountStorage.sol
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.25;

import {EnumerableMap} from "@openzeppelin/contracts/utils/structs/EnumerableMap.sol";
import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";

import {ExecutionHook} from "../interfaces/IAccountLoupe.sol";
Expand Down Expand Up @@ -43,7 +42,6 @@ struct AccountStorage {
// AccountStorageInitializable variables
uint8 initialized;
bool initializing;
EnumerableMap.AddressToUintMap moduleManifestHashes;
// Execution functions and their associated functions
mapping(bytes4 => SelectorData) selectorData;
mapping(ModuleEntity validationFunction => ValidationData) validationData;
Expand Down
50 changes: 6 additions & 44 deletions src/account/ModuleManagerInternals.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ pragma solidity ^0.8.25;

import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol";

import {EnumerableMap} from "@openzeppelin/contracts/utils/structs/EnumerableMap.sol";
import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";

import {KnownSelectors} from "../helpers/KnownSelectors.sol";
Expand All @@ -15,18 +14,14 @@ import {AccountStorage, SelectorData, getAccountStorage, toSetValue} from "./Acc

abstract contract ModuleManagerInternals is IModuleManager {
using EnumerableSet for EnumerableSet.Bytes32Set;
using EnumerableMap for EnumerableMap.AddressToUintMap;
using ModuleEntityLib for ModuleEntity;

error ArrayLengthMismatch();
error Erc4337FunctionNotAllowed(bytes4 selector);
error ExecutionFunctionAlreadySet(bytes4 selector);
error InvalidModuleManifest();
error IModuleFunctionNotAllowed(bytes4 selector);
error NativeFunctionNotAllowed(bytes4 selector);
error NullModuleEntity();
error NullModule();
error ModuleAlreadyInstalled(address module);
error ModuleInstallCallbackFailed(address module, bytes revertReason);
error ModuleInterfaceNotSupported(address module);
error ModuleNotInstalled(address module);
Expand Down Expand Up @@ -138,32 +133,21 @@ abstract contract ModuleManagerInternals is IModuleManager {
);
}

function _installModule(address module, bytes32 manifestHash, bytes memory moduleInstallData) internal {
function _installModule(address module, ModuleManifest calldata manifest, bytes memory moduleInstallData)
internal
{
AccountStorage storage _storage = getAccountStorage();

if (module == address(0)) {
revert NullModule();
}

// Check if the module exists.
if (_storage.moduleManifestHashes.contains(module)) {
revert ModuleAlreadyInstalled(module);
}

// TODO: do we need this check? Or switch to a non-165 checking function?
// Check that the module supports the IModule interface.
if (!ERC165Checker.supportsInterface(module, type(IModule).interfaceId)) {
revert ModuleInterfaceNotSupported(module);
}

// Check manifest hash.
ModuleManifest memory manifest = IModule(module).moduleManifest();
if (!_isValidModuleManifest(manifest, manifestHash)) {
revert InvalidModuleManifest();
}

// Add the module metadata to the account
_storage.moduleManifestHashes.set(module, uint256(manifestHash));

// Update components according to the manifest.
uint256 length = manifest.executionFunctions.length;
for (uint256 i = 0; i < length; ++i) {
Expand Down Expand Up @@ -200,25 +184,14 @@ abstract contract ModuleManagerInternals is IModuleManager {
revert ModuleInstallCallbackFailed(module, revertReason);
}

emit ModuleInstalled(module, manifestHash);
emit ModuleInstalled(module);
}

function _uninstallModule(address module, ModuleManifest memory manifest, bytes memory uninstallData)
function _uninstallModule(address module, ModuleManifest calldata manifest, bytes memory uninstallData)
internal
{
AccountStorage storage _storage = getAccountStorage();

// Check if the module exists.
if (!_storage.moduleManifestHashes.contains(module)) {
revert ModuleNotInstalled(module);
}

// Check manifest hash.
bytes32 manifestHash = bytes32(_storage.moduleManifestHashes.get(module));
if (!_isValidModuleManifest(manifest, manifestHash)) {
revert InvalidModuleManifest();
}

// Remove components according to the manifest, in reverse order (by component type) of their installation.

uint256 length = manifest.executionHooks.length;
Expand All @@ -245,9 +218,6 @@ abstract contract ModuleManagerInternals is IModuleManager {
_storage.supportedIfaces[manifest.interfaceIds[i]] -= 1;
}

// Remove the module metadata from the account.
_storage.moduleManifestHashes.remove(module);

// Clear the module storage for the account.
bool onUninstallSuccess = true;
// solhint-disable-next-line no-empty-blocks
Expand All @@ -258,12 +228,4 @@ abstract contract ModuleManagerInternals is IModuleManager {

emit ModuleUninstalled(module, onUninstallSuccess);
}

function _isValidModuleManifest(ModuleManifest memory manifest, bytes32 manifestHash)
internal
pure
returns (bool)
{
return manifestHash == keccak256(abi.encode(manifest));
}
}
24 changes: 8 additions & 16 deletions src/account/UpgradeableModularAccount.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {ValidationConfigLib} from "../helpers/ValidationConfigLib.sol";
import {_coalescePreValidation, _coalesceValidation} from "../helpers/ValidationResHelpers.sol";

import {IExecutionHook} from "../interfaces/IExecutionHook.sol";
import {IModule, ModuleManifest} from "../interfaces/IModule.sol";
import {ModuleManifest} from "../interfaces/IModule.sol";
import {IModuleManager, ModuleEntity, ValidationConfig} from "../interfaces/IModuleManager.sol";
import {Call, IStandardExecutor} from "../interfaces/IStandardExecutor.sol";
import {IValidation} from "../interfaces/IValidation.sol";
Expand Down Expand Up @@ -109,21 +109,21 @@ contract UpgradeableModularAccount is

/// @notice Initializes the account with a set of modules
/// @param modules The modules to install
/// @param manifestHashes The manifest hashes of the modules to install
/// @param manifests The manifests of the modules to install
/// @param moduleInstallDatas The module install datas of the modules to install
function initialize(
address[] memory modules,
bytes32[] memory manifestHashes,
ModuleManifest[] calldata manifests,
bytes[] memory moduleInstallDatas
) external initializer {
uint256 length = modules.length;

if (length != manifestHashes.length || length != moduleInstallDatas.length) {
if (length != manifests.length || length != moduleInstallDatas.length) {
revert ArrayLengthMismatch();
}

for (uint256 i = 0; i < length; ++i) {
_installModule(modules[i], manifestHashes[i], moduleInstallDatas[i]);
_installModule(modules[i], manifests[i], moduleInstallDatas[i]);
}

emit ModularAccountInitialized(_ENTRY_POINT);
Expand Down Expand Up @@ -249,29 +249,21 @@ contract UpgradeableModularAccount is

/// @inheritdoc IModuleManager
/// @notice May be validated by a global validation.
function installModule(address module, bytes32 manifestHash, bytes calldata moduleInstallData)
function installModule(address module, ModuleManifest calldata manifest, bytes calldata moduleInstallData)
external
override
wrapNativeFunction
{
_installModule(module, manifestHash, moduleInstallData);
_installModule(module, manifest, moduleInstallData);
}

/// @inheritdoc IModuleManager
/// @notice May be validated by a global validation.
function uninstallModule(address module, bytes calldata config, bytes calldata moduleUninstallData)
function uninstallModule(address module, ModuleManifest calldata manifest, bytes calldata moduleUninstallData)
external
override
wrapNativeFunction
{
ModuleManifest memory manifest;

if (config.length > 0) {
manifest = abi.decode(config, (ModuleManifest));
} else {
manifest = IModule(module).moduleManifest();
}

_uninstallModule(module, manifest, moduleUninstallData);
}

Expand Down
3 changes: 1 addition & 2 deletions src/helpers/KnownSelectors.sol
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@ library KnownSelectors {
// check against IAccountLoupe methods
|| selector == IAccountLoupe.getExecutionFunctionHandler.selector
|| selector == IAccountLoupe.getSelectors.selector || selector == IAccountLoupe.getExecutionHooks.selector
|| selector == IAccountLoupe.getPreValidationHooks.selector
|| selector == IAccountLoupe.getInstalledModules.selector;
|| selector == IAccountLoupe.getPreValidationHooks.selector;
}

function isErc4337Function(bytes4 selector) internal pure returns (bool) {
Expand Down
4 changes: 0 additions & 4 deletions src/interfaces/IAccountLoupe.sol
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,4 @@ interface IAccountLoupe {
external
view
returns (ModuleEntity[] memory preValidationHooks);

/// @notice Get an array of all installed modules.
/// @return The addresses of all installed modules.
function getInstalledModules() external view returns (address[] memory);
}
15 changes: 9 additions & 6 deletions src/interfaces/IModuleManager.sol
Original file line number Diff line number Diff line change
@@ -1,21 +1,24 @@
// SPDX-License-Identifier: CC0-1.0
pragma solidity ^0.8.25;

import {ModuleManifest} from "./IModule.sol";

type ModuleEntity is bytes24;

type ValidationConfig is bytes26;

interface IModuleManager {
event ModuleInstalled(address indexed module, bytes32 manifestHash);
event ModuleInstalled(address indexed module);

event ModuleUninstalled(address indexed module, bool indexed onUninstallSucceeded);

/// @notice Install a module to the modular account.
/// @param module The module to install.
/// @param manifestHash The hash of the module manifest.
/// @param manifest the manifest describing functions to install
/// @param moduleInstallData Optional data to be decoded and used by the module to setup initial module data
/// for the modular account.
function installModule(address module, bytes32 manifestHash, bytes calldata moduleInstallData) external;
function installModule(address module, ModuleManifest calldata manifest, bytes calldata moduleInstallData)
external;

/// @notice Temporary install function - pending a different user-supplied install config & manifest validation
/// path.
Expand Down Expand Up @@ -53,9 +56,9 @@ interface IModuleManager {

/// @notice Uninstall a module from the modular account.
/// @param module The module to uninstall.
/// @param config An optional, implementation-specific field that accounts may use to ensure consistency
/// guarantees.
/// @param manifest the manifest describing functions to uninstall.
/// @param moduleUninstallData Optional data to be decoded and used by the module to clear module data for the
/// modular account.
function uninstallModule(address module, bytes calldata config, bytes calldata moduleUninstallData) external;
function uninstallModule(address module, ModuleManifest calldata manifest, bytes calldata moduleUninstallData)
external;
}
17 changes: 8 additions & 9 deletions test/account/AccountExecHooks.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ import {AccountTestBase} from "../utils/AccountTestBase.sol";

contract AccountExecHooksTest is AccountTestBase {
MockModule public mockModule1;
bytes32 public manifestHash1;
bytes32 public manifestHash2;

bytes4 internal constant _EXEC_SELECTOR = bytes4(uint32(1));
uint32 internal constant _PRE_HOOK_FUNCTION_ID_1 = 1;
Expand All @@ -24,7 +22,7 @@ contract AccountExecHooksTest is AccountTestBase {

ModuleManifest internal _m1;

event ModuleInstalled(address indexed module, bytes32 manifestHash);
event ModuleInstalled(address indexed module);
event ModuleUninstalled(address indexed module, bool indexed callbacksSucceeded);
// emitted by MockModule
event ReceivedCall(bytes msgData, uint256 msgValue);
Expand Down Expand Up @@ -162,19 +160,19 @@ contract AccountExecHooksTest is AccountTestBase {
function _installModule1WithHooks(ManifestExecutionHook memory execHooks) internal {
_m1.executionHooks.push(execHooks);
mockModule1 = new MockModule(_m1);
manifestHash1 = keccak256(abi.encode(mockModule1.moduleManifest()));

vm.expectEmit(true, true, true, true);
emit ReceivedCall(abi.encodeCall(IModule.onInstall, (bytes(""))), 0);
vm.expectEmit(true, true, true, true);
emit ModuleInstalled(address(mockModule1), manifestHash1);
emit ModuleInstalled(address(mockModule1));

vm.prank(address(entryPoint));
vm.startPrank(address(entryPoint));
account1.installModule({
module: address(mockModule1),
manifestHash: manifestHash1,
manifest: mockModule1.moduleManifest(),
moduleInstallData: bytes("")
});
vm.stopPrank();
}

function _uninstallModule(MockModule module) internal {
Expand All @@ -183,7 +181,8 @@ contract AccountExecHooksTest is AccountTestBase {
vm.expectEmit(true, true, true, true);
emit ModuleUninstalled(address(module), true);

vm.prank(address(entryPoint));
account1.uninstallModule(address(module), bytes(""), bytes(""));
vm.startPrank(address(entryPoint));
account1.uninstallModule(address(module), module.moduleManifest(), bytes(""));
vm.stopPrank();
}
}
14 changes: 3 additions & 11 deletions test/account/AccountLoupe.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,9 @@ contract AccountLoupeTest is CustomValidationTestBase {

_customValidationSetup();

bytes32 manifestHash = keccak256(abi.encode(comprehensiveModule.moduleManifest()));
vm.prank(address(entryPoint));
account1.installModule(address(comprehensiveModule), manifestHash, "");
}

function test_moduleLoupe_getInstalledModules_initial() public {
address[] memory modules = account1.getInstalledModules();

assertEq(modules.length, 1);

assertEq(modules[0], address(comprehensiveModule));
vm.startPrank(address(entryPoint));
account1.installModule(address(comprehensiveModule), comprehensiveModule.moduleManifest(), "");
vm.stopPrank();
}

function test_moduleLoupe_getExecutionFunctionHandler_native() public {
Expand Down
10 changes: 4 additions & 6 deletions test/account/AccountReturnData.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -26,21 +26,19 @@ contract AccountReturnDataTest is AccountTestBase {
resultConsumerModule = new ResultConsumerModule(resultCreatorModule, regularResultContract);

// Add the result creator module to the account
bytes32 resultCreatorManifestHash = keccak256(abi.encode(resultCreatorModule.moduleManifest()));
vm.prank(address(entryPoint));
vm.startPrank(address(entryPoint));
account1.installModule({
module: address(resultCreatorModule),
manifestHash: resultCreatorManifestHash,
manifest: resultCreatorModule.moduleManifest(),
moduleInstallData: ""
});
// Add the result consumer module to the account
bytes32 resultConsumerManifestHash = keccak256(abi.encode(resultConsumerModule.moduleManifest()));
vm.prank(address(entryPoint));
account1.installModule({
module: address(resultConsumerModule),
manifestHash: resultConsumerManifestHash,
manifest: resultConsumerModule.moduleManifest(),
moduleInstallData: ""
});
vm.stopPrank();
}

// Tests the ability to read the result of module execution functions via the account's fallback
Expand Down
10 changes: 4 additions & 6 deletions test/account/PermittedCallPermissions.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,19 @@ contract PermittedCallPermissionsTest is AccountTestBase {
permittedCallerModule = new PermittedCallerModule();

// Add the result creator module to the account
bytes32 resultCreatorManifestHash = keccak256(abi.encode(resultCreatorModule.moduleManifest()));
vm.prank(address(entryPoint));
vm.startPrank(address(entryPoint));
account1.installModule({
module: address(resultCreatorModule),
manifestHash: resultCreatorManifestHash,
manifest: resultCreatorModule.moduleManifest(),
moduleInstallData: ""
});
// Add the permitted caller module to the account
bytes32 permittedCallerManifestHash = keccak256(abi.encode(permittedCallerModule.moduleManifest()));
vm.prank(address(entryPoint));
account1.installModule({
module: address(permittedCallerModule),
manifestHash: permittedCallerManifestHash,
manifest: permittedCallerModule.moduleManifest(),
moduleInstallData: ""
});
vm.stopPrank();
}

function test_permittedCall_Allowed() public {
Expand Down
Loading

0 comments on commit 044492f

Please sign in to comment.