Skip to content
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.

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

[2/n permissions] feat: add permission hooks #68

Merged
merged 6 commits into from
Jun 19, 2024

Conversation

howydev
Copy link
Collaborator

@howydev howydev commented Jun 7, 2024

(For the entire PR stack)
Permission hooks are hooks associated with a validation function. The proposed model of permissions is:

  1. A validation function, global or associated with a selector
  2. A set of pre validation functions associated with the validation function that will all run, and must all pass (AND logic)
  3. A set of permission hooks (pre and post execution) that applies to the validation. If the permission hook requires the UO context, the top level execution call must be made to executeUserOp, and the full PackedUserOp struct would be passed to the hook

Changelog in this PR:

  1. Add a boolean requireUOContext to ExecutionHook for plugins to signal that the hook needs PackedUserOperation
  2. Add execution hooks associated with a validator
  3. Add getters in the loupe for these hooks
  4. Add string[] to PluginMetadata for permission plugins to declare ERC-7715 permission strings
  5. Use a variable uint8 requireUOHookCount to track the number of permission hooks to allow a cheap check during validation

2 issues that will be addressed in future PRs:

  1. As part of the change, plugins can specify permission hooks to install in the manifest, associated with any validation function. Long term, permission configuration should be user-defined on install time as opposed to plugin-defined on plugin compile time. This change would be done in a separate PR, in the 6900 plugin install configs revamp.
  2. Pre execution/permission hooks now pass in abi.encodePacked(msg.sender, msg.value, msg.data) or abi.encode(PackedUserOperation). As we'll likely transition to composable validation with "entity ids", the final shape of execution hooks relies on that. At the same time, instead of passing in msg.value, we should consider passing in an int256 "outgoing value minus incoming value"

@howydev howydev force-pushed the howy/remove-permissions branch 2 times, most recently from e069b7a to 73e57f3 Compare June 7, 2024 20:56
@howydev howydev force-pushed the howy/add-permission-hooks branch 7 times, most recently from 53edb51 to b183d5a Compare June 10, 2024 22:30
function preExecutionHook(uint8 functionId, address sender, uint256 value, bytes calldata data)
external
returns (bytes memory);
function preExecutionHook(uint8 functionId, bytes calldata data) external returns (bytes memory);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you expand on why we need this format change? The old format is capable of encoding a call to executeUserOp, too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The hook needs to send both address, uint256, bytes and PackedUserOp. With the alternative of having 2 formats for exec hooks, to be safe the plugin would have to implement both hook formats and revert in the other format they're not expecting, but in this case they get reverts for free from failing abi decoding

No strong preference here in format. Overall, would prefer if we did separation by call path - all UOs have to send UO context and exec hooks always have PackedUserOperation, and all runtime exec hooks send msg.sender msg.data. Think its a simplification that is a step in the right direction for plugin devs

Copy link
Contributor

Choose a reason for hiding this comment

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

The original format also benefits from the "free" abi-decoding revert. This format is also missing the user op hash provided by the EntryPoint to the account, and introduces a new, custom format for both calls. I think the original format of "caller, value, data" can just as easily handle the calls to executeUserOp and to the regular function, so I don't think we need a format change here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I'm misunderstanding something, can you expand? Would we send (msg.sender, msg.value, requiresUOContext ? abi.encode(PackedUserOperation) : msg.data)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need any conditional at all - if it is a call to executeUserOp, then msg.data will contain the packed user op.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see, thanks for explaining! In the UO case, msg.sender and msg.value are known, its not clear to me how useful this information is. If there aren't uses, I'd prefer to leave it out of the standard

Copy link
Contributor

Choose a reason for hiding this comment

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

But this would add extra behavior into the standard to filter it out. It seems like it's a net increase in complexity to have branching cases, rather than provide the data as-is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does it make sense to have a UO exec hook vs runtime exec hook? I think theres a lot of branching cases here already, would love to simplify if possible

As a plugin dev, if they specify that they don't need the UO context, they still need to check and conditionally filter for executeUserOp.selector

Copy link
Contributor

@adamegyed adamegyed left a comment

Choose a reason for hiding this comment

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

Looks good overall! This makes sense as a way to create exec hooks that are associated with a validation, rather than an exec selector.

I think there is 1 missing case that should be implemented here - it is possible to create a validation associated exec hook that does not require UO context, and I think those hooks aren't run currently in executeWithAuthorization for the runtime case.

@@ -47,15 +41,35 @@ abstract contract AccountLoupe is IAccountLoupe {
override
returns (ExecutionHook[] memory execHooks)
{
SelectorData storage selectorData = getAccountStorage().selectorData[selector];
uint256 executionHooksLength = selectorData.executionHooks.length();
EnumerableSet.Bytes32Set storage hooks = getAccountStorage().selectorData[selector].executionHooks;
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch on simplifying here

}

/// @inheritdoc IAccountLoupe
function getPermissionHooks(FunctionReference validationFunction)
Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense to also surface these hooks in the loupe. We should consider naming them something other than permission hooks, since some permissions may be implemented elsewhere like pre-validation hooks. OK with this for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

another one in the list of stuff to rename lol

src/account/UpgradeableModularAccount.sol Outdated Show resolved Hide resolved
test/account/AccountPermissionHooks.t.sol Outdated Show resolved Hide resolved
@howydev howydev force-pushed the howy/add-permission-hooks branch from b183d5a to f3ad732 Compare June 17, 2024 21:15
@howydev howydev force-pushed the howy/remove-permissions branch 2 times, most recently from 8f55c1b to a1c31ce Compare June 17, 2024 21:22
@howydev howydev force-pushed the howy/add-permission-hooks branch 2 times, most recently from 9164020 to dbcf555 Compare June 17, 2024 21:33
@howydev howydev force-pushed the howy/remove-permissions branch from a1c31ce to 3941060 Compare June 17, 2024 21:36
@howydev howydev force-pushed the howy/add-permission-hooks branch from 7d9f003 to 3c65bc6 Compare June 17, 2024 22:35
@howydev howydev marked this pull request as ready for review June 18, 2024 17:51
@howydev howydev changed the title [DRAFT] [2/n permissions] feat: add permission hooks [2/n permissions] feat: add permission hooks Jun 18, 2024
@howydev howydev force-pushed the howy/add-permission-hooks branch from 4f8c2dc to 5d2c4ff Compare June 19, 2024 19:49
@howydev howydev force-pushed the howy/remove-permissions branch from 3941060 to ee0a657 Compare June 19, 2024 19:49
@howydev howydev merged commit 5d2c4ff into howy/remove-permissions Jun 19, 2024
@howydev howydev deleted the howy/add-permission-hooks branch June 19, 2024 20:01
@howydev
Copy link
Collaborator Author

howydev commented Jul 23, 2024

addresses: erc6900/resources#50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants