Skip to content

feat: [v0.8-develop] remove plugin dependencies #86

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 10, 2024
Merged

feat: [v0.8-develop] remove plugin dependencies #86

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: 3 additions & 2 deletions src/account/AccountLoupe.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ pragma solidity ^0.8.25;

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

import {IAccountLoupe, ExecutionHook} from "../interfaces/IAccountLoupe.sol";
import {FunctionReference, IPluginManager} from "../interfaces/IPluginManager.sol";
Expand All @@ -11,7 +12,7 @@ import {getAccountStorage, toExecutionHook, toSelector} from "./AccountStorage.s

abstract contract AccountLoupe is IAccountLoupe {
using EnumerableSet for EnumerableSet.Bytes32Set;
using EnumerableSet for EnumerableSet.AddressSet;
using EnumerableMap for EnumerableMap.AddressToUintMap;

/// @inheritdoc IAccountLoupe
function getExecutionFunctionHandler(bytes4 selector) external view override returns (address plugin) {
Expand Down Expand Up @@ -89,6 +90,6 @@ abstract contract AccountLoupe is IAccountLoupe {

/// @inheritdoc IAccountLoupe
function getInstalledPlugins() external view override returns (address[] memory pluginAddresses) {
pluginAddresses = getAccountStorage().plugins.values();
pluginAddresses = getAccountStorage().pluginManifestHashes.keys();
}
}
11 changes: 2 additions & 9 deletions src/account/AccountStorage.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,14 @@
pragma solidity ^0.8.25;

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

import {ExecutionHook} from "../interfaces/IAccountLoupe.sol";
import {FunctionReference} from "../interfaces/IPluginManager.sol";

// bytes = keccak256("ERC6900.UpgradeableModularAccount.Storage")
bytes32 constant _ACCOUNT_STORAGE_SLOT = 0x9f09680beaa4e5c9f38841db2460c401499164f368baef687948c315d9073e40;

struct PluginData {
bytes32 manifestHash;
FunctionReference[] dependencies;
// Tracks the number of times this plugin has been used as a dependency function
uint256 dependentCount;
}

// Represents data associated with a specifc function selector.
struct SelectorData {
// The plugin that implements this execution function.
Expand Down Expand Up @@ -51,8 +45,7 @@ struct AccountStorage {
uint8 initialized;
bool initializing;
// Plugin metadata storage
EnumerableSet.AddressSet plugins;
mapping(address => PluginData) pluginData;
EnumerableMap.AddressToUintMap pluginManifestHashes;
Copy link
Collaborator

@fangting-alchemy fangting-alchemy Jun 27, 2024

Choose a reason for hiding this comment

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

Why not AddressToBytes32Map?

Copy link
Contributor Author

@adamegyed adamegyed Jun 27, 2024

Choose a reason for hiding this comment

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

Unfortunately, it doesn't exist: https://docs.openzeppelin.com/contracts/5.x/api/utils#EnumerableMap

This is the only map that has address as a key type.

Copy link
Collaborator

@fangting-alchemy fangting-alchemy Jul 8, 2024

Choose a reason for hiding this comment

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

We need to use a newer version!

Copy link
Contributor Author

@adamegyed adamegyed Jul 10, 2024

Choose a reason for hiding this comment

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

I think it is the newest version, haha

Copy link
Collaborator

@fangting-alchemy fangting-alchemy Jul 10, 2024

Choose a reason for hiding this comment

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

Cannot be, it is in v5.1.

Copy link
Contributor Author

@adamegyed adamegyed Jul 10, 2024

Choose a reason for hiding this comment

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

Ah that's not released yet, check under the releases tab. I'd prefer to avoid using pre-release content, but we should update this when 5.1 is released.

// Execution functions and their associated functions
mapping(bytes4 => SelectorData) selectorData;
mapping(FunctionReference validationFunction => ValidationData) validationData;
Expand Down
189 changes: 52 additions & 137 deletions src/account/PluginManagerInternals.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,62 +3,38 @@ pragma solidity ^0.8.25;

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

import {FunctionReferenceLib} from "../helpers/FunctionReferenceLib.sol";
import {
IPlugin,
ManifestExecutionHook,
ManifestFunction,
ManifestAssociatedFunctionType,
ManifestAssociatedFunction,
PluginManifest
} from "../interfaces/IPlugin.sol";
import {IPlugin, ManifestExecutionHook, ManifestValidation, PluginManifest} from "../interfaces/IPlugin.sol";
import {ExecutionHook} from "../interfaces/IAccountLoupe.sol";
import {FunctionReference, IPluginManager} from "../interfaces/IPluginManager.sol";
import {KnownSelectors} from "../helpers/KnownSelectors.sol";
import {AccountStorage, getAccountStorage, SelectorData, toSetValue} from "./AccountStorage.sol";

abstract contract PluginManagerInternals is IPluginManager {
using EnumerableSet for EnumerableSet.Bytes32Set;
using EnumerableSet for EnumerableSet.AddressSet;
using EnumerableMap for EnumerableMap.AddressToUintMap;
using FunctionReferenceLib for FunctionReference;

error ArrayLengthMismatch();
error Erc4337FunctionNotAllowed(bytes4 selector);
error ExecutionFunctionAlreadySet(bytes4 selector);
error InvalidDependenciesProvided();
error InvalidPluginManifest();
error IPluginFunctionNotAllowed(bytes4 selector);
error MissingPluginDependency(address dependency);
error NativeFunctionNotAllowed(bytes4 selector);
error NullFunctionReference();
error NullPlugin();
error PluginAlreadyInstalled(address plugin);
error PluginDependencyViolation(address plugin);
error PluginInstallCallbackFailed(address plugin, bytes revertReason);
error PluginInterfaceNotSupported(address plugin);
error PluginNotInstalled(address plugin);
error ValidationFunctionAlreadySet(bytes4 selector, FunctionReference validationFunction);

modifier notNullFunction(FunctionReference functionReference) {
if (functionReference.isEmpty()) {
revert NullFunctionReference();
}
_;
}

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

// Storage update operations

function _setExecutionFunction(bytes4 selector, bool isPublic, bool allowDefaultValidation, address plugin)
internal
notNullPlugin(plugin)
{
SelectorData storage _selectorData = getAccountStorage().selectorData[selector];

Expand Down Expand Up @@ -99,25 +75,40 @@ abstract contract PluginManagerInternals is IPluginManager {
_selectorData.allowDefaultValidation = false;
}

function _addValidationFunction(bytes4 selector, FunctionReference validationFunction)
internal
notNullFunction(validationFunction)
{
// Fail on duplicate validation functions. Otherwise, dependency validation functions could shadow
// non-depdency validation functions. Then, if a either plugin is uninstalled, it would cause a partial
// uninstall of the other.
if (!getAccountStorage().validationData[validationFunction].selectors.add(toSetValue(selector))) {
revert ValidationFunctionAlreadySet(selector, validationFunction);
function _addValidationFunction(address plugin, ManifestValidation memory mv) internal {
AccountStorage storage _storage = getAccountStorage();

FunctionReference validationFunction = FunctionReferenceLib.pack(plugin, mv.functionId);

if (mv.isDefault) {
_storage.validationData[validationFunction].isDefault = true;
}

if (mv.isSignatureValidation) {
Comment on lines +83 to +87
Copy link
Collaborator

@fangting-alchemy fangting-alchemy Jun 27, 2024

Choose a reason for hiding this comment

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

Do we want to keep validation installation in plugin installation? If not, not worth adding new code in.

We talked about only keeping it in one place. While experimenting with validationId, validation installation within plugin installation is almost impossible.

  • It require unique onInstall data (an extra validationId) that is not required by other plugin installation.
  • We might generate false positive validations due to unable to match the validationIds for selectors actually just using the same validation.

Copy link
Contributor Author

@adamegyed adamegyed Jun 28, 2024

Choose a reason for hiding this comment

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

I think it's worth considering removing validation install via the manifest completely - this PR doesn't do that yet though, it just removes dependencies.

Copy link
Collaborator

@fangting-alchemy fangting-alchemy Jul 8, 2024

Choose a reason for hiding this comment

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

I meant that we don't need to add more code in here like checking isSignatureValidation since we plan to remove validation install here altogether. No big deal either way.

_storage.validationData[validationFunction].isSignatureValidation = true;
}

// Add the validation function to the selectors.
uint256 length = mv.selectors.length;
for (uint256 i = 0; i < length; ++i) {
bytes4 selector = mv.selectors[i];
_storage.validationData[validationFunction].selectors.add(toSetValue(selector));
}
}

function _removeValidationFunction(bytes4 selector, FunctionReference validationFunction)
internal
notNullFunction(validationFunction)
{
// May ignore return value, as the manifest hash is validated to ensure that the validation function
// exists.
getAccountStorage().validationData[validationFunction].selectors.remove(toSetValue(selector));
function _removeValidationFunction(address plugin, ManifestValidation memory mv) internal {
AccountStorage storage _storage = getAccountStorage();

FunctionReference validationFunction = FunctionReferenceLib.pack(plugin, mv.functionId);

_storage.validationData[validationFunction].isDefault = false;
_storage.validationData[validationFunction].isSignatureValidation = false;

// Clear the selectors
while (_storage.validationData[validationFunction].selectors.length() > 0) {
bytes32 selector = _storage.validationData[validationFunction].selectors.at(0);
_storage.validationData[validationFunction].selectors.remove(selector);
}
}

function _addExecHooks(
Expand Down Expand Up @@ -146,16 +137,15 @@ abstract contract PluginManagerInternals is IPluginManager {
);
}

function _installPlugin(
address plugin,
bytes32 manifestHash,
bytes memory pluginInstallData,
FunctionReference[] memory dependencies
) internal {
function _installPlugin(address plugin, bytes32 manifestHash, bytes memory pluginInstallData) internal {
AccountStorage storage _storage = getAccountStorage();

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

// Check if the plugin exists.
if (!_storage.plugins.add(plugin)) {
if (_storage.pluginManifestHashes.contains(plugin)) {
revert PluginAlreadyInstalled(plugin);
}

Expand All @@ -170,37 +160,11 @@ abstract contract PluginManagerInternals is IPluginManager {
revert InvalidPluginManifest();
}

// Check that the dependencies match the manifest.
if (dependencies.length != manifest.dependencyInterfaceIds.length) {
revert InvalidDependenciesProvided();
}

uint256 length = dependencies.length;
for (uint256 i = 0; i < length; ++i) {
// Check the dependency interface id over the address of the dependency.
(address dependencyAddr,) = dependencies[i].unpack();

// Check that the dependency is installed.
if (_storage.pluginData[dependencyAddr].manifestHash == bytes32(0)) {
revert MissingPluginDependency(dependencyAddr);
}

// Check that the dependency supports the expected interface.
if (!ERC165Checker.supportsInterface(dependencyAddr, manifest.dependencyInterfaceIds[i])) {
revert InvalidDependenciesProvided();
}

// Increment the dependency's dependents counter.
_storage.pluginData[dependencyAddr].dependentCount += 1;
}

// Add the plugin metadata to the account
_storage.pluginData[plugin].manifestHash = manifestHash;
_storage.pluginData[plugin].dependencies = dependencies;
_storage.pluginManifestHashes.set(plugin, uint256(manifestHash));

// Update components according to the manifest.

length = manifest.executionFunctions.length;
uint256 length = manifest.executionFunctions.length;
for (uint256 i = 0; i < length; ++i) {
bytes4 selector = manifest.executionFunctions[i].executionSelector;
bool isPublic = manifest.executionFunctions[i].isPublic;
Expand All @@ -210,17 +174,9 @@ abstract contract PluginManagerInternals is IPluginManager {

length = manifest.validationFunctions.length;
for (uint256 i = 0; i < length; ++i) {
ManifestAssociatedFunction memory mv = manifest.validationFunctions[i];
_addValidationFunction(
mv.executionSelector, _resolveManifestFunction(mv.associatedFunction, plugin, dependencies)
);
}

length = manifest.signatureValidationFunctions.length;
for (uint256 i = 0; i < length; ++i) {
FunctionReference signatureValidationFunction =
FunctionReferenceLib.pack(plugin, manifest.signatureValidationFunctions[i]);
_storage.validationData[signatureValidationFunction].isSignatureValidation = true;
// Todo: limit this to only "direct runtime call" validation path (old EFP),
// and add a way for the user to specify permission/pre-val hooks here.
_addValidationFunction(plugin, manifest.validationFunctions[i]);
}

length = manifest.executionHooks.length;
Expand All @@ -243,7 +199,7 @@ abstract contract PluginManagerInternals is IPluginManager {
revert PluginInstallCallbackFailed(plugin, revertReason);
}

emit PluginInstalled(plugin, manifestHash, dependencies);
emit PluginInstalled(plugin, manifestHash);
}

function _uninstallPlugin(address plugin, PluginManifest memory manifest, bytes memory uninstallData)
Expand All @@ -252,54 +208,29 @@ abstract contract PluginManagerInternals is IPluginManager {
AccountStorage storage _storage = getAccountStorage();

// Check if the plugin exists.
if (!_storage.plugins.remove(plugin)) {
if (!_storage.pluginManifestHashes.contains(plugin)) {
revert PluginNotInstalled(plugin);
}

// Check manifest hash.
bytes32 manifestHash = _storage.pluginData[plugin].manifestHash;
bytes32 manifestHash = bytes32(_storage.pluginManifestHashes.get(plugin));
if (!_isValidPluginManifest(manifest, manifestHash)) {
revert InvalidPluginManifest();
}

// Ensure that there are no dependent plugins.
if (_storage.pluginData[plugin].dependentCount != 0) {
revert PluginDependencyViolation(plugin);
}

// Remove this plugin as a dependent from its dependencies.
FunctionReference[] memory dependencies = _storage.pluginData[plugin].dependencies;
uint256 length = dependencies.length;
for (uint256 i = 0; i < length; ++i) {
FunctionReference dependency = dependencies[i];
(address dependencyAddr,) = dependency.unpack();

// Decrement the dependent count for the dependency function.
_storage.pluginData[dependencyAddr].dependentCount -= 1;
}

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

uint256 length = manifest.executionHooks.length;
for (uint256 i = 0; i < length; ++i) {
ManifestExecutionHook memory mh = manifest.executionHooks[i];
FunctionReference hookFunction = FunctionReferenceLib.pack(plugin, mh.functionId);
EnumerableSet.Bytes32Set storage execHooks = _storage.selectorData[mh.executionSelector].executionHooks;
_removeExecHooks(execHooks, hookFunction, mh.isPreHook, mh.isPostHook);
}

length = manifest.signatureValidationFunctions.length;
for (uint256 i = 0; i < length; ++i) {
FunctionReference signatureValidationFunction =
FunctionReferenceLib.pack(plugin, manifest.signatureValidationFunctions[i]);
_storage.validationData[signatureValidationFunction].isSignatureValidation = false;
}

length = manifest.validationFunctions.length;
for (uint256 i = 0; i < length; ++i) {
ManifestAssociatedFunction memory mv = manifest.validationFunctions[i];
_removeValidationFunction(
mv.executionSelector, _resolveManifestFunction(mv.associatedFunction, plugin, dependencies)
);
_removeValidationFunction(plugin, manifest.validationFunctions[i]);
}

length = manifest.executionFunctions.length;
Expand All @@ -314,7 +245,7 @@ abstract contract PluginManagerInternals is IPluginManager {
}

// Remove the plugin metadata from the account.
delete _storage.pluginData[plugin];
_storage.pluginManifestHashes.remove(plugin);

// Clear the plugin storage for the account.
bool onUninstallSuccess = true;
Expand All @@ -334,20 +265,4 @@ abstract contract PluginManagerInternals is IPluginManager {
{
return manifestHash == keccak256(abi.encode(manifest));
}

function _resolveManifestFunction(
ManifestFunction memory manifestFunction,
address plugin,
FunctionReference[] memory dependencies
) internal pure returns (FunctionReference) {
if (manifestFunction.functionType == ManifestAssociatedFunctionType.SELF) {
return FunctionReferenceLib.pack(plugin, manifestFunction.functionId);
} else if (manifestFunction.functionType == ManifestAssociatedFunctionType.DEPENDENCY) {
if (manifestFunction.dependencyIndex >= dependencies.length) {
revert InvalidPluginManifest();
}
return dependencies[manifestFunction.dependencyIndex];
}
return FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; // Empty checks are done elsewhere
}
}
Loading