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

fix: Expand and clarify hook state behavior in the spec #34

Merged
merged 2 commits into from
Jan 24, 2024

Conversation

adam-alchemy
Copy link
Contributor

Motivation

The ERC-6900 spec has a (somewhat) ambiguous comment about which hooks should be run, because it implies a special casing for uninstallPlugin specifically. The behavior described there should apply to all execution functions.

Solution

Expand that note into two sentences and clarify that it applies to all execution functions, along with some rationale.

Note that this does not fully extend the requirement to load all account state that is used, it specifically only requires it for hooks, as it was before.

We should consider extending this requirement to the validation and execution functions themselves.

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.

lgtm!

Base automatically changed from 01-22-feat_update_spec_for_v0.7.0 to main January 24, 2024 22:33
@jaypaik jaypaik force-pushed the adam/expand-hook-spec branch from 7272c0c to fc9de69 Compare January 24, 2024 22:41
@jaypaik jaypaik force-pushed the adam/expand-hook-spec branch from fc9de69 to 3b85fa1 Compare January 24, 2024 22:49
@jaypaik jaypaik merged commit a178bd4 into main Jan 24, 2024
3 checks passed
@jaypaik jaypaik deleted the adam/expand-hook-spec branch January 24, 2024 22: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.

3 participants