Skip to content

feat: [v0.8-develop] User controlled install 1/N #101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adamegyed merged 1 commit into from
Jul 23, 2024
Merged

feat: [v0.8-develop] User controlled install 1/N #101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions src/account/AccountLoupe.sol
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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(
Copy link
Collaborator

@howydev howydev Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One part I really liked about the previous PluginManifest design is that doing plugin.pluginManifest() is the cheapest possible option (compared to calldata because of potential L1 DA costs, or contract storage) to pass data into the installation flow. Curious to hear your thoughts on whether its worth including an option to "install plugin with it's recommended config, as-is" that would pull the manifest from the plugin.

In general, I'm not a fan of one-offs, but this should be a huge calldata savings in the average case and would require alterations to the spec (changing the initialize call format, changing the installModule call format) and think it might be worth including in the spec and in RI

Copy link
Contributor

@Zer0dot Zer0dot Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it's possible to make it an optional part of the spec for modules to implement? "Modules MAY implement a pluginManifest() function..." (maybe we should rename it to recommendedManifest to signal that it isn't actually a guarantee to be respected by accounts, and is optional)

This way, common plugins can still provide the configs needed to install efficiently.

And then the rest can be achieved (h/t @adamegyed came up with this) via a plugin with permission to call install functions.

Down the line we could explore splitting and simplifying installation though!

Copy link
Contributor Author

@adamegyed adamegyed Jul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fangting-alchemy came up with the idea of making the manifest only required for plugins that define execution functions, so we could move the definition to an new interface IExecution. I like this idea because it will remove the unused empty manifests from most validation & hook plugins.

The IExecution interface would also fill the gap left by the existing interfaces: IValidation, IValidationHook, and IExecutionHook.

Copy link

@0xrubes 0xrubes Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree there should be some mechanism like recommendedManifest or defaultManifest, as there will inevitably be install configurations that result in a malfunctioning module. Auditors could then review modules based on configurations defined in the given configurations

Copy link
Contributor Author

@adamegyed adamegyed Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I still think the plugins should report manifests as their intended configuration, if they define functions that use manifests for install (like execution functions, hooks associated with an exec selector, or an interface ID representing multiple execution functions).

doing plugin.pluginManifest() is the cheapest possible option

I don't think this is the case – performing the call, the ABI encode/decode, and the hash verification, is necessarily more execution gas than just performing the ABI decode from calldata. The only case where the on-chain call could be cheaper is if the calldata cost is very high, at which point implementing the manifest reading mechanism on-chain makes sense, because both storage and execution gas would be relatively cheaper than the calldata cost.

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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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