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

feat: [v0.8-develop] remove hook overlap support #47

Merged
merged 6 commits into from
May 3, 2024

Conversation

adam-alchemy
Copy link
Contributor

@adam-alchemy adam-alchemy commented Apr 18, 2024

Motivation

Overlapping hooks were an edge case introduced to handle certain behavior when using hooks as dependencies. However, hook dependencies as a whole were disabled in v0.7 due to risks around inconsistent plugin state configuration, meaning overlapping hooks became impossible except when using the "always deny" magic value.

This has led to the large amounts of code in the reference implementation handling multiple hooks assignments overlapping going unused, since this case only happens under one condition and never results in hooks actually being executed. This worsens readability & slows development when changing how account state is stored.

Solution

  • Revert the usage of EnumerableMap.Bytes32ToUintMap to EnumerableSet.Bytes32Set for holding plugin functions.
  • Remove internal helpers like _addOrIncrement and _removeOrDecrement intended to handle hook overlaps.
  • Remove the hook "unrolling" behavior in _doPreExecHooks and in the getExecutionHooks loupe function.

How this change handles overlapping PRE_HOOK_ALWAYS_DENY

Rather than assigning a "count" to a FunctionReference type, and incrementing the count for the number of overlapping deny hooks, this change moves the count of overlapping denies out to a variable within SelectorData. The "magic value" FunctionReference indicating PRE_HOOK_ALWAYS_DENY is now no longer added to the actual hooks set, and instead this counter reflects how many times it has been applied.

Note that previously, the PRE_HOOK_ALWAYS_DENY value could be applied over any of pre validation hooks (previously split across user op and runtime, but now merged) or execution hooks. However, this would cause an annoying split given the current architecture - the hook could apply in one of two places, meaning we would need to track two counters rather than one.

To simplify, this PR changes the spec to only allow the PRE_HOOK_ALWAYS_DENY magic value to be assigned to pre validation hooks. However, if any are assigned (count > 0), then the account behavior will be to revert during any validation or execution path for that selector.

To discuss: this handles all previous workflows except one, where PRE_HOOK_ALWAYS_DENY is applied to pre validation hooks but not pre execution. In that case, any regular plugin execution would be denied, but it would still be possible to call via executeFromPlugin. Given the number of validation-path changes coming in v0.8, this seems fine, but flagging here in case anyone has thoughts about that workflow.

@adam-alchemy adam-alchemy requested review from jaypaik and howydev April 18, 2024 20:59
@jaypaik jaypaik requested a review from a team April 22, 2024 23:41
Copy link
Collaborator

@jaypaik jaypaik left a comment

Choose a reason for hiding this comment

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

On:

To discuss: this handles all previous workflows except one, where PRE_HOOK_ALWAYS_DENY is applied to pre validation hooks but not pre execution. In that case, any regular plugin execution would be denied, but it would still be possible to call via executeFromPlugin. Given the number of validation-path changes coming in v0.8, this seems fine, but flagging here in case anyone has thoughts about that workflow.

I can't think of any use cases right now. This does seem like a convenient simplification, but I'm still deciding how I feel about it, since this blurs the line between pre exec and pre validation hooks a bit.

I almost want to just get rid of magic values. 😅

src/account/AccountLoupe.sol Outdated Show resolved Hide resolved
src/account/AccountStorage.sol Show resolved Hide resolved
src/account/AccountStorage.sol Outdated Show resolved Hide resolved
src/account/PluginManagerInternals.sol Outdated Show resolved Hide resolved
src/account/PluginManagerInternals.sol Outdated Show resolved Hide resolved
src/account/UpgradeableModularAccount.sol Show resolved Hide resolved
src/account/UpgradeableModularAccount.sol Outdated Show resolved Hide resolved
src/account/UpgradeableModularAccount.sol Outdated Show resolved Hide resolved
src/account/UpgradeableModularAccount.sol Outdated Show resolved Hide resolved
Base automatically changed from adam/merge-pre-validation to v0.8-develop April 26, 2024 16:19
@adam-alchemy adam-alchemy force-pushed the adam/remove-hook-overlap branch from f75f9b6 to 619793b Compare April 26, 2024 17:03
@adam-alchemy adam-alchemy force-pushed the adam/remove-hook-overlap branch from 3002b9d to ded7df1 Compare May 2, 2024 15:47
@jaypaik jaypaik requested a review from a team May 2, 2024 17:19
Copy link
Collaborator

@jaypaik jaypaik left a comment

Choose a reason for hiding this comment

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

I still can't decide how I feel about the pre exec hook magic value impacting validation, but it does make sense to do, since it'll just fail during execution anyway. Accepting to unblock, but would be curious to hear the working group's thoughts next week.

src/account/PluginManagerInternals.sol Outdated Show resolved Hide resolved
@adam-alchemy
Copy link
Contributor Author

Hmm, I was thinking about this a bit more, and I'm actually starting to think that it makes more sense to specify this value only within a pre-validation hook, but keep the current behavior where it reverts during validation & execution (if the execution is triggered via executeFromPlugin, which is a pseudo-validation path).

@jaypaik
Copy link
Collaborator

jaypaik commented May 2, 2024

Hmm, I was thinking about this a bit more, and I'm actually starting to think that it makes more sense to specify this value only within a pre-validation hook, but keep the current behavior where it reverts during validation & execution (if the execution is triggered via executeFromPlugin, which is a pseudo-validation path).

Sure, that does seem more appropriate.

@adam-alchemy adam-alchemy merged commit f00dba4 into v0.8-develop May 3, 2024
3 checks passed
@adam-alchemy adam-alchemy deleted the adam/remove-hook-overlap branch May 3, 2024 19:41
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.

3 participants