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: allow overlapping hooks #17

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

jaypaik
Copy link
Collaborator

@jaypaik jaypaik commented Dec 1, 2023

Motivation

The spec is not explicit about how the account should treat overlapping hooks (identical hooks that are applied on the same execution function selector by one or more plugins). Although reuse of hook functions across plugins may be uncommon, allowing overlaps helps in the case where multiple plugins opt to use the short-circuit magic value FunctionReference._PRE_HOOK_ALWAYS_DENY as a way to protect a particular function.

This is also a step toward potentially allowing multiple validation functions to be applied to a given selector.

Solution

Use an EnumerableMap to keep track of the number of overlaps, to be able to keep the account in a consistent state when overlapping hooks are uninstalled.

When running the hooks, the following strategy is applied.

  1. Coalesce all exact duplicate pairs of hooks, and execute them only once.
  2. If there are two hook pairs that have the same pre-exec hook but have different post-exec hooks, the pre-exec should be executed twice - once for each of the corresponding post-exec hook - and the return data from each should be passed to them. Same applies for two pairs of hooks that have the same post-exec hook but different pre-exec hooks.

Additional changes

  • I've bumped up the forge-std version to be able to use vm.expectCall with the count parameter.
  • Added isEmpty() to FunctionReferenceLib and updated code to use it over direct comparisons.
  • Order of associated post hook executions have been reversed to follow the middleware pattern of executing pre/post processing steps in layers.

@jaypaik jaypaik force-pushed the 11-30-feat_allow_overlapping_hooks branch from c38b642 to f6bc217 Compare December 1, 2023 02:20
@jaypaik jaypaik marked this pull request as ready for review December 1, 2023 02:21
Copy link
Contributor

@adam-alchemy adam-alchemy 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!

@jaypaik jaypaik merged commit 42c6056 into spec-update-6 Dec 1, 2023
3 checks passed
@jaypaik jaypaik deleted the 11-30-feat_allow_overlapping_hooks branch December 1, 2023 18:28
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