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

adamegyed · 2024-07-17T19:35:57Z

Motivation

A point of contention in the ERC-6900 v0.7 design was the fixed nature of the manifest making plugin design inflexible. This caused issues in two areas:

It was difficult to reuse plugins in different settings, because there was only 1 possible way to install them.
Plugin-limiting permissions were difficult to configure: the account would always grant permissions requested in the manifest, and there wasn’t a clean way to attach restricting or blocking hooks to these permission requests because the hook plugins were inflexible.

These concerns were captured in a 6900 standards improvement issue here: erc6900/resources#9

As shown in our goals of composable and reusable validation functions, it is in our interest to allow plugins to be more flexibly installed than what we currently support. In the process of implementing v0.8 features, we introduced installValidation, a function to allow customizable installation that only follows user-supplied instructions for installation, rather than decoding and following what a manifest says. This allows us to install validation plugins multiple times with different IDs and hooks attached to them, and for the user to control what functions on the account a validation may use. This solves the issues of static, manifest-based installs, but only for validation functions.

Now, we have to answer the question of what to do with the existing workflow in installPlugin / uninstallPlugin, where the plugin manifest is read, and individual plugin functions are added according to the manifest.

Solution

This PR makes the manifest a fully user-controlled install sequence. There is still the view function pluginManifest() that allows a caller to retrieve the manifest, which describes what execution functions, execution hooks, validation functions, and ERC-165 interface IDs should be added to the account. However, the installPlugin and uninstallPlugin functions no longer read the manifest in their implementation and work off of that, instead having the manifest passed in as a parameter by the user.

This allows the caller to inspect and modify the manifest before installing the functions within it. It also removes the necessity for the account to:

retrieve the manifest during install
compare the retrieved manifest with a user-provided hash
store the manifest hash for the future
provide an escape hatch for the manifest to be passed in during uninstall, if the plugin changes the return value of the manifest in the future

It does, however, mean that the caller (user, or the wallet software around it) will need to be aware of what functions were installed, in order to have a clean uninstall. Doing so efficiently may require us to change the types emitted in events, but that is deferred here.

Future work

This PR does not address the issue of applying limitations to validations installed via the manifest format. I.e. if there is some validation function defined in the PluginManifest provided to install plugin, it is not yet possible to apply pre-validation hooks or permission hooks to it.

fangting-alchemy · 2024-07-18T21:43:34Z

src/account/PluginManagerInternals.sol

        AccountStorage storage _storage = getAccountStorage();

        if (plugin == address(0)) {
            revert NullPlugin();
        }

-        // Check if the plugin exists.
-        if (_storage.pluginManifestHashes.contains(plugin)) {
-            revert PluginAlreadyInstalled(plugin);


How is duplicated installation checked now? How do we prevent duplicated installation?

Followup on this, I'm not sure if we actually really need to prevent duplicate installation, but from what I can tell the only place this would revert is if the plugin is installing an already installed execution function.

Otherwise, validation functions would just override storage (or add selectors) to the passed validation, and execution hooks would silently fail on set add.

It's not checking on the plugin address level, it's checked within each component. Installs should still fail if they overlap at all.

Zer0dot · 2024-07-19T12:30:22Z

src/account/PluginManagerInternals.sol

-            revert PluginAlreadyInstalled(plugin);
-        }
-
+        // TODO: do we need this check? Or switch to a non-165 checking function?


IMO it's kinda useless, it's maybe a good footgun to prevent some accidents, but in the bad actor case, anyone can just support the interfaces this checks.

I'm not sure we even need this check at all, what are some potential issues we could be preventing?

It's just a footgun check, to validate that the address you're trying to install is in fact a module. We've been using ERC-165, alternatives have created custom, module specific view functions for this purpose.

I guess evading undefined behaviour is a good reason, but ultimately, I'm not sure how the risk VS gas tradeoff is. Off the top of my head the only case where this would be helpful is when you're using an app that thinks your account is compatible with some other form of module-- but we do have forced uninstalls which can help here.

Security-wise, any contract can support this check, even an account with added execution functions I guess, so I don't think that's a good reason to keep it. It would prevent accidentally installing an EOA though.

What are your general thoughts? Do we need to explicitly prevent installing non-modules? Or are the cases where this would happen quite rare?

Zer0dot · 2024-07-19T12:46:36Z

src/account/PluginManagerInternals.sol

@@ -138,32 +133,21 @@ abstract contract PluginManagerInternals is IPluginManager {
        );
    }

-    function _installPlugin(address plugin, bytes32 manifestHash, bytes memory pluginInstallData) internal {
+    function _installPlugin(address plugin, PluginManifest calldata manifest, bytes memory pluginInstallData)


It feels like we're converging on installing components rather than entire modules (prev: plugins), maybe we can gain some efficiency by directly installing those individual components (kind of like how we did with the validations).

One area of efficiency gain is in how we're obliged to pass an entire, even if pretty empty, manifest to install even a single exec hook, idk how legit this is, but it might be worth exploring.

Yeah, that is the direction we're converging towards. Right now, the only thing the manifest does as an install parameter is being a container for a few sub-lists of each component type.

Gotcha, I don't think we need to change anything here for now, but it's probably worth keeping in mind for future updates, maybe we can simplify some installation paths.

Zer0dot · 2024-07-19T12:47:56Z

src/account/UpgradeableModularAccount.sol

    /// @param pluginInstallDatas The plugin install datas of the plugins to install
    function initialize(
        address[] memory plugins,
-        bytes32[] memory manifestHashes,
+        PluginManifest[] calldata manifests,


Minor nitpick, but I think we could rename it to a simpler word than manifest now-- it's no longer a request from the plugin, so maybe something like pluginInstallConfig?

I like this idea. But tbh I thought manifest ~= install config? Thought I guess manifest has connotations of also holding other metadata.

Yeah definitely a super-small thing, I think it's just a less frequently used word that a bunch of people might not really understand at first (maybe relevant for non-native speakers, I guess?) although it's fairly straightforward if you look at the data structure.

Zer0dot · 2024-07-19T12:48:40Z

src/account/UpgradeableModularAccount.sol

        bytes[] memory pluginInstallDatas
    ) external initializer {
        uint256 length = plugins.length;

-        if (length != manifestHashes.length || length != pluginInstallDatas.length) {
+        if (length != manifests.length || length != pluginInstallDatas.length) {


I guess we could pack pluginInstallDatas into the same struct and skip this check altogether since we're no longer just passing an array of raw hashes

Yeah, this initialization sequence is also not used anymore, since there's a validation-specific initialization now. Possibly worth removing.

Sounds good to me! Are there any potential uses where this would be necessary (in an atomic context, deploy & init w/ plugin) that you can think of?

Anyway this could be done using a UserOp if push comes to shove, yeah? But besides that it wouldn't be possible to do so atomically without a custom deployer that then executes stuff (correct me if I'm wrong).

Some factories may wish to have some more heavyweight initialization, like installing multiple plugins. Though currently, as soon as one initializer runs, the other is disabled, so this doesn't address that. A clumsy workaround is to add the factory as an owner mid-deploy, then give up ownership by the end of the deploy.

Either way, it's for a different workflow than what this PR is working on. I'll remove this initializer in #104, because that's the PR where validation is removed as an installable component via the manifest.

test/account/AccountExecHooks.t.sol

howydev · 2024-07-19T22:13:12Z

src/account/UpgradeableModularAccount.sol

@@ -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(


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

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!

@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.

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

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.

fangting-alchemy

Looks good

0xrubes · 2024-07-23T11:30:55Z

It probably makes sense to still have some mechanism to query an account about installed modules without aggregating events? It is however not as straight forward anymore with an EnumerableMap because the 1:1 relationship between manifest hashes and modules is broken and we can have multiple installs of the same module 🤔

adamegyed · 2024-07-23T15:34:20Z

It probably makes sense to still have some mechanism to query an account about installed modules without aggregating events?

I think a realization I had was that the module address is insufficient on its own to describe the install state of some feature, specifically because it may be installed multiple times. For validation functions, pre-validation hooks, and "permission hooks" (execution hooks associated with a validation function), the best query path to determine if they're installed is based on the validation function itself - it's what everything is associated with. For things that are purely execution-function based, the best thing to query by is either the function selector (if you're looking up the implementation of a function, or any exec hooks associated with it), or an interface ID (if you're checking to see whether an account supports a set of execution functions).

adamegyed marked this pull request as draft July 17, 2024 19:36

adamegyed marked this pull request as ready for review July 18, 2024 19:32

adamegyed requested a review from a team July 18, 2024 19:32

adamegyed changed the title ~~[draft] feat: [v0.8-develop] User controlled install 1/N~~ feat: [v0.8-develop] User controlled install 1/N Jul 18, 2024

fangting-alchemy reviewed Jul 18, 2024

View reviewed changes

Zer0dot reviewed Jul 19, 2024

View reviewed changes

test/account/AccountExecHooks.t.sol Show resolved Hide resolved

Pass in plugin manifest for install & uninstall

044492f

adamegyed force-pushed the adam/user-controlled-install branch from 9b36599 to 044492f Compare July 19, 2024 18:29

howydev reviewed Jul 19, 2024

View reviewed changes

fangting-alchemy approved these changes Jul 22, 2024

View reviewed changes

adamegyed merged commit ce001a8 into v0.8-develop Jul 23, 2024
3 checks passed

adamegyed deleted the adam/user-controlled-install branch July 23, 2024 16:59

adamegyed added a commit that referenced this pull request Jul 26, 2024

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

5cb215f

adamegyed mentioned this pull request Aug 1, 2024

feat: update install execution events #125

Merged

adamegyed added a commit that referenced this pull request Aug 5, 2024

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

560b18b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

adamegyed commented Jul 17, 2024 •

edited

Loading

fangting-alchemy Jul 18, 2024 •

edited

Loading

Zer0dot Jul 19, 2024

adamegyed Jul 19, 2024

Zer0dot Jul 19, 2024

adamegyed Jul 19, 2024

Zer0dot Jul 22, 2024

Zer0dot Jul 19, 2024

adamegyed Jul 19, 2024

Zer0dot Jul 22, 2024

Zer0dot Jul 19, 2024

adamegyed Jul 19, 2024

Zer0dot Jul 22, 2024

Zer0dot Jul 19, 2024

adamegyed Jul 19, 2024

Zer0dot Jul 22, 2024

adamegyed Jul 22, 2024

howydev Jul 19, 2024 •

edited

Loading

Zer0dot Jul 22, 2024

adamegyed Jul 22, 2024

0xrubes Jul 23, 2024

adamegyed Jul 23, 2024

fangting-alchemy left a comment

0xrubes commented Jul 23, 2024

adamegyed commented Jul 23, 2024

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

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

Conversation

adamegyed commented Jul 17, 2024 • edited Loading

Motivation

Solution

Future work

fangting-alchemy Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howydev Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fangting-alchemy left a comment

Choose a reason for hiding this comment

0xrubes commented Jul 23, 2024

adamegyed commented Jul 23, 2024

adamegyed commented Jul 17, 2024 •

edited

Loading

fangting-alchemy Jul 18, 2024 •

edited

Loading

howydev Jul 19, 2024 •

edited

Loading