Skip to content

feat: [v0.8-develop] remove execute from plugin #65

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 3 commits into from
Jun 20, 2024
Merged

feat: [v0.8-develop] remove execute from plugin #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
18eac4f
remove executeFromPlugin & efp external
adamegyed Jun 4, 2024
c9fcabf
remove remaining storage vars
adamegyed Jun 10, 2024
84487b6
rollback test styling
adamegyed Jun 20, 2024
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
16 changes: 0 additions & 16 deletions src/account/AccountStorage.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,33 +3,19 @@ pragma solidity ^0.8.25;

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

import {IPlugin} from "../interfaces/IPlugin.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 {
bool anyExternalExecPermitted;
// boolean to indicate if the plugin can spend native tokens from the account.
bool canSpendNativeToken;
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 plugin's permission to use `executeFromPluginExternal`
// to interact with contracts and addresses external to the account and its plugins.
struct PermittedExternalCallData {
// Is this address on the permitted addresses list? If it is, we either have a
// list of allowed selectors, or the flag that allows any selector.
bool addressPermitted;
bool anySelectorPermitted;
mapping(bytes4 => bool) permittedSelectors;
}

// Represents data associated with a specifc function selector.
struct SelectorData {
// The plugin that implements this execution function.
Expand Down Expand Up @@ -67,8 +53,6 @@ struct AccountStorage {
mapping(bytes4 => SelectorData) selectorData;
mapping(FunctionReference validationFunction => ValidationData) validationData;
mapping(address caller => mapping(bytes4 selector => bool)) callPermitted;
// key = address(calling plugin) || target address
mapping(IPlugin => mapping(address => PermittedExternalCallData)) permittedExternalCalls;
// For ERC165 introspection
mapping(bytes4 => uint256) supportedIfaces;
}
Expand Down
67 changes: 1 addition & 66 deletions src/account/PluginManagerInternals.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,12 @@ import {
ManifestFunction,
ManifestAssociatedFunctionType,
ManifestAssociatedFunction,
ManifestExternalCallPermission,
PluginManifest
} from "../interfaces/IPlugin.sol";
import {ExecutionHook} from "../interfaces/IAccountLoupe.sol";
import {FunctionReference, IPluginManager} from "../interfaces/IPluginManager.sol";
import {
AccountStorage,
getAccountStorage,
SelectorData,
toSetValue,
PermittedExternalCallData
} from "./AccountStorage.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;
Expand Down Expand Up @@ -211,11 +204,6 @@ abstract contract PluginManagerInternals is IPluginManager {

// Update components according to the manifest.

// Mark whether or not this plugin may spend native token amounts
if (manifest.canSpendNativeToken) {
_storage.pluginData[plugin].canSpendNativeToken = true;
}

length = manifest.executionFunctions.length;
for (uint256 i = 0; i < length; ++i) {
bytes4 selector = manifest.executionFunctions[i].executionSelector;
Expand All @@ -232,31 +220,6 @@ abstract contract PluginManagerInternals is IPluginManager {
_storage.callPermitted[plugin][manifest.permittedExecutionSelectors[i]] = true;
}

// Add the permitted external calls to the account.
if (manifest.permitAnyExternalAddress) {
_storage.pluginData[plugin].anyExternalExecPermitted = true;
} else {
// Only store the specific permitted external calls if "permit any" flag was not set.
length = manifest.permittedExternalCalls.length;
for (uint256 i = 0; i < length; ++i) {
ManifestExternalCallPermission memory externalCallPermission = manifest.permittedExternalCalls[i];

PermittedExternalCallData storage permittedExternalCallData =
_storage.permittedExternalCalls[IPlugin(plugin)][externalCallPermission.externalAddress];

permittedExternalCallData.addressPermitted = true;

if (externalCallPermission.permitAnySelector) {
permittedExternalCallData.anySelectorPermitted = true;
} else {
uint256 externalContractSelectorsLength = externalCallPermission.selectors.length;
for (uint256 j = 0; j < externalContractSelectorsLength; ++j) {
permittedExternalCallData.permittedSelectors[externalCallPermission.selectors[j]] = true;
}
}
}
}

length = manifest.validationFunctions.length;
for (uint256 i = 0; i < length; ++i) {
ManifestAssociatedFunction memory mv = manifest.validationFunctions[i];
Expand Down Expand Up @@ -350,34 +313,6 @@ abstract contract PluginManagerInternals is IPluginManager {
);
}

// remove external call permissions

if (manifest.permitAnyExternalAddress) {
// Only clear if it was set during install time
_storage.pluginData[plugin].anyExternalExecPermitted = false;
} else {
// Only clear the specific permitted external calls if "permit any" flag was not set.
length = manifest.permittedExternalCalls.length;
for (uint256 i = 0; i < length; ++i) {
ManifestExternalCallPermission memory externalCallPermission = manifest.permittedExternalCalls[i];

PermittedExternalCallData storage permittedExternalCallData =
_storage.permittedExternalCalls[IPlugin(plugin)][externalCallPermission.externalAddress];

permittedExternalCallData.addressPermitted = false;

// Only clear this flag if it was set in the constructor.
if (externalCallPermission.permitAnySelector) {
permittedExternalCallData.anySelectorPermitted = false;
} else {
uint256 externalContractSelectorsLength = externalCallPermission.selectors.length;
for (uint256 j = 0; j < externalContractSelectorsLength; ++j) {
permittedExternalCallData.permittedSelectors[externalCallPermission.selectors[j]] = false;
}
}
}
}

length = manifest.permittedExecutionSelectors.length;
for (uint256 i = 0; i < length; ++i) {
_storage.callPermitted[plugin][manifest.permittedExecutionSelectors[i]] = false;
Expand Down
85 changes: 1 addition & 84 deletions src/account/UpgradeableModularAccount.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {IPlugin, PluginManifest} from "../interfaces/IPlugin.sol";
import {IValidation} from "../interfaces/IValidation.sol";
import {IValidationHook} from "../interfaces/IValidationHook.sol";
import {IExecutionHook} from "../interfaces/IExecutionHook.sol";
import {IPluginExecutor} from "../interfaces/IPluginExecutor.sol";
import {FunctionReference, IPluginManager} from "../interfaces/IPluginManager.sol";
import {IStandardExecutor, Call} from "../interfaces/IStandardExecutor.sol";
import {AccountExecutor} from "./AccountExecutor.sol";
Expand All @@ -39,7 +38,6 @@ contract UpgradeableModularAccount is
BaseAccount,
IERC165,
IERC1271,
IPluginExecutor,
IStandardExecutor,
PluginManagerInternals,
PluginManager2,
Expand Down Expand Up @@ -185,88 +183,7 @@ contract UpgradeableModularAccount is
}
}

/// @inheritdoc IPluginExecutor
function executeFromPlugin(bytes calldata data) external payable override returns (bytes memory) {
bytes4 selector = bytes4(data[:4]);
address callingPlugin = msg.sender;

AccountStorage storage _storage = getAccountStorage();

if (!_storage.callPermitted[callingPlugin][selector]) {
revert ExecFromPluginNotPermitted(callingPlugin, selector);
}

address execFunctionPlugin = _storage.selectorData[selector].plugin;

if (execFunctionPlugin == address(0)) {
revert UnrecognizedFunction(selector);
}

PostExecToRun[] memory postExecHooks = _doPreExecHooks(selector, data);

(bool success, bytes memory returnData) = execFunctionPlugin.call(data);

if (!success) {
assembly ("memory-safe") {
revert(add(returnData, 32), mload(returnData))
}
}

_doCachedPostExecHooks(postExecHooks);

return returnData;
}

/// @inheritdoc IPluginExecutor
function executeFromPluginExternal(address target, uint256 value, bytes calldata data)
external
payable
returns (bytes memory)
{
bytes4 selector = bytes4(data);
AccountStorage storage _storage = getAccountStorage();

// Make sure plugin is allowed to spend native token.
if (value > 0 && value > msg.value && !_storage.pluginData[msg.sender].canSpendNativeToken) {
revert NativeTokenSpendingNotPermitted(msg.sender);
}

// Check the caller plugin's permission to make this call

// Check the target contract permission.
// This first checks that the intended target is permitted at all. If it is, then it checks if any selector
// is permitted. If any selector is permitted, then it skips the selector-level permission check.
// If only a subset of selectors are permitted, then it also checks the selector-level permission.
// By checking in the order of [address specified with any selector allowed], [any address allowed],
// [address specified and selector specified], along with the extra bool `permittedCall`, we can
// reduce the number of `sload`s in the worst-case from 3 down to 2.
bool targetContractPermittedCall = _storage.permittedExternalCalls[IPlugin(msg.sender)][target]
.addressPermitted
&& (
_storage.permittedExternalCalls[IPlugin(msg.sender)][target].anySelectorPermitted
|| _storage.permittedExternalCalls[IPlugin(msg.sender)][target].permittedSelectors[selector]
);

// If the target contract is not permitted, check if the caller plugin is permitted to make any external
// calls.
if (!(targetContractPermittedCall || _storage.pluginData[msg.sender].anyExternalExecPermitted)) {
revert ExecFromPluginExternalNotPermitted(msg.sender, target, value, data);
}

// Run any pre exec hooks for this selector
PostExecToRun[] memory postExecHooks =
_doPreExecHooks(IPluginExecutor.executeFromPluginExternal.selector, msg.data);

// Perform the external call
bytes memory returnData = _exec(target, value, data);

// Run any post exec hooks for this selector
_doCachedPostExecHooks(postExecHooks);

return returnData;
}

/// @inheritdoc IPluginExecutor
/// @inheritdoc IStandardExecutor
function executeWithAuthorization(bytes calldata data, bytes calldata authorization)
external
payable
Expand Down
6 changes: 1 addition & 5 deletions src/helpers/KnownSelectors.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {IPaymaster} from "@eth-infinitism/account-abstraction/interfaces/IPaymas
import {IAccountLoupe} from "../interfaces/IAccountLoupe.sol";
import {IExecutionHook} from "../interfaces/IExecutionHook.sol";
import {IPlugin} from "../interfaces/IPlugin.sol";
import {IPluginExecutor} from "../interfaces/IPluginExecutor.sol";
import {IPluginManager} from "../interfaces/IPluginManager.sol";
import {IStandardExecutor} from "../interfaces/IStandardExecutor.sol";
import {IValidation} from "../interfaces/IValidation.sol";
Expand All @@ -32,10 +31,7 @@ library KnownSelectors {
|| selector == UUPSUpgradeable.upgradeToAndCall.selector
// check against IStandardExecutor methods
|| selector == IStandardExecutor.execute.selector || selector == IStandardExecutor.executeBatch.selector
// check against IPluginExecutor methods
|| selector == IPluginExecutor.executeFromPlugin.selector
|| selector == IPluginExecutor.executeFromPluginExternal.selector
|| selector == IPluginExecutor.executeWithAuthorization.selector
|| selector == IStandardExecutor.executeWithAuthorization.selector
// check against IAccountLoupe methods
|| selector == IAccountLoupe.getExecutionFunctionHandler.selector
|| selector == IAccountLoupe.getValidations.selector
Expand Down
12 changes: 0 additions & 12 deletions src/interfaces/IPlugin.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,6 @@ struct ManifestExecutionHook {
bool isPostHook;
}

struct ManifestExternalCallPermission {
address externalAddress;
bool permitAnySelector;
bytes4[] selectors;
}

struct SelectorPermission {
bytes4 functionSelector;
string permissionDescription;
Expand Down Expand Up @@ -81,12 +75,6 @@ struct PluginManifest {
uint8[] signatureValidationFunctions;
// Plugin execution functions already installed on the MSCA that this plugin will be able to call.
bytes4[] permittedExecutionSelectors;
// Boolean to indicate whether the plugin can call any external address.
bool permitAnyExternalAddress;
// Boolean to indicate whether the plugin needs access to spend native tokens of the account. If false, the
// plugin MUST still be able to spend up to the balance that it sends to the account in the same call.
bool canSpendNativeToken;
ManifestExternalCallPermission[] permittedExternalCalls;
// List of ERC-165 interface IDs to add to account to support introspection checks. This MUST NOT include
// IPlugin's interface ID.
bytes4[] interfaceIds;
Expand Down
33 changes: 0 additions & 33 deletions src/interfaces/IPluginExecutor.sol
View file
Open in desktop

This file was deleted.

10 changes: 10 additions & 0 deletions src/interfaces/IStandardExecutor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,14 @@ interface IStandardExecutor {
/// @param calls The array of calls.
/// @return An array containing the return data from the calls.
function executeBatch(Call[] calldata calls) external payable returns (bytes[] memory);

/// @notice Execute a call using a specified runtime validation, as given in the first 21 bytes of
/// `authorization`.
/// @param data The calldata to send to the account.
/// @param authorization The authorization data to use for the call. The first 21 bytes specifies which runtime
/// validation to use, and the rest is sent as a parameter to runtime validation.
function executeWithAuthorization(bytes calldata data, bytes calldata authorization)
Copy link
Contributor

@huaweigu huaweigu Jun 7, 2024

Choose a reason for hiding this comment

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

I'm curious to hear your thoughts on why you prefer the new function to be part of the standard executor rather than an extension executor :)

Copy link
Contributor Author

@adamegyed adamegyed Jun 10, 2024

Choose a reason for hiding this comment

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

To be honest I don't have a strict reasoning - I think we could split it out, or we could try to bring together all of the required account functions into one interface for simplicity. I think I'm leaning more towards the latter to make it easier to read the spec, but open to discussing.

Copy link
Collaborator

@fangting-alchemy fangting-alchemy Jun 18, 2024

Choose a reason for hiding this comment

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

I am more lean towards to split it out.
StandardExecutor indicates the functions here are following a certain standard (ERC-4337).

Copy link
Contributor Author

@adamegyed adamegyed Jun 19, 2024

Choose a reason for hiding this comment

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

Technically the functions in IStandardExecutor are only standardized in ERC-6900, but 6900 also requires the plugin installation functions, which are in a separate interface IPluginManager.

As for executeWithAuthorization, it's something that all 6900 compliant accounts will need to implement after this spec change. Do y'all have any suggestions for what to name an interface that only holds this one function? Or how to rename IStandardExecutor or other existing account interfaces to hold it?

Copy link
Collaborator

@fangting-alchemy fangting-alchemy Jun 19, 2024

Choose a reason for hiding this comment

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

Actually to rethink on this, especially since executeWithAuthorization is something that all 6900 compliant accounts will need to implement. We can just keep it in there.

external
payable
returns (bytes memory);
}
10 changes: 5 additions & 5 deletions test/account/AccountExecHooks.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ contract AccountExecHooksTest is AccountTestBase {
uint8 internal constant _POST_HOOK_FUNCTION_ID_2 = 2;
uint8 internal constant _BOTH_HOOKS_FUNCTION_ID_3 = 3;

PluginManifest public m1;
PluginManifest public m2;
PluginManifest internal _m1;
PluginManifest internal _m2;

event PluginInstalled(address indexed plugin, bytes32 manifestHash, FunctionReference[] dependencies);
event PluginUninstalled(address indexed plugin, bool indexed callbacksSucceeded);
Expand All @@ -35,7 +35,7 @@ contract AccountExecHooksTest is AccountTestBase {
function setUp() public {
_transferOwnershipToTest();

m1.executionFunctions.push(
_m1.executionFunctions.push(
ManifestExecutionFunction({
executionSelector: _EXEC_SELECTOR,
isPublic: true,
Expand Down Expand Up @@ -163,8 +163,8 @@ contract AccountExecHooksTest is AccountTestBase {
}

function _installPlugin1WithHooks(ManifestExecutionHook memory execHooks) internal {
m1.executionHooks.push(execHooks);
mockPlugin1 = new MockPlugin(m1);
_m1.executionHooks.push(execHooks);
mockPlugin1 = new MockPlugin(_m1);
manifestHash1 = keccak256(abi.encode(mockPlugin1.pluginManifest()));

vm.expectEmit(true, true, true, true);
Expand Down
10 changes: 5 additions & 5 deletions test/account/AccountReturnData.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,16 +99,16 @@ contract AccountReturnDataTest is AccountTestBase {
assertEq(result2, keccak256("foo"));
}

// Tests the ability to read data via executeFromPlugin routing to fallback functions
// Tests the ability to read data via routing to fallback functions
function test_returnData_execFromPlugin_fallback() public {
bool result = ResultConsumerPlugin(address(account1)).checkResultEFPFallback(keccak256("bar"));
bool result = ResultConsumerPlugin(address(account1)).checkResultFallback(keccak256("bar"));

assertTrue(result);
}

// Tests the ability to read data via executeFromPluginExternal
function test_returnData_execFromPlugin_execute() public {
bool result = ResultConsumerPlugin(address(account1)).checkResultEFPExternal(
// Tests the ability to read data via executeWithAuthorization
function test_returnData_authorized_exec() public {
bool result = ResultConsumerPlugin(address(account1)).checkResultExecuteWithAuthorization(
address(regularResultContract), keccak256("bar")
);

Expand Down
Loading