-
Notifications
You must be signed in to change notification settings - Fork 2
feat: spec simplification and corrections #10
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
Conversation
| ``` | ||
|
|
||
| ### Validation Functions and Their Installation/Uninstallation | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to include the following points under the validation installation section?
- An account can have multiple validation modules/functions installed.
- An account can have the same validation module installed multiple times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite following why those aspects were removed. I think this one is also worth keeping:
- The entity ID of a validation function installed on an account MUST be unique.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An account can have multiple validation modules/functions installed.
An account can have the same validation module installed multiple times.
These are implicitly true from the spec's interfaces - I thought we didn't need to call them out.
The entity ID of a validation function installed on an account MUST be unique.
I'm not sure how this got into the spec, since this isn't the case for the RI right now. We should remove this.
|
|
||
| ### Execution Functions and Their Installation/Uninstallation | ||
|
|
||
| - An account can install any number of execution functions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also need to keep this?
- An account can install any number of execution functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this was implicitly true from the interfaces and the installation flow - as long as there isn't a selector collision, you can keep installing new execution functions.
| /// @param data The calldata sent. For `executeUserOp` calls of validation-associated hooks, hook modules | ||
| /// should receive the full calldata. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this as a part of ethereum#754
jaypaik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed in working group.
44eb68b to
e80c144
Compare
|
Thank you all for the review. Closing this PR as it has been merged here: ethereum#771 |
Fixes:
IExecutionHook.preExecutionHookto only sendexecuteUserOpdata for validation-associated hooks.msg.data, and clarifies that this is available asmsg.datawithin solidity.Simplification: