-
Notifications
You must be signed in to change notification settings - Fork 26
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, experimental] Merge validation function assignments #40
Conversation
@dlim-circle Given that this change affects the plugin manifest structure, both the plugins and the account would need an update to support this. It affects the install flow (how an account parses a manifest) and the manifest data format a plugin uses to share information with accounts & frontends. For these reasons, we should gate this to a full version change, along with other important items still in the backlog. For plugins currently installed on accounts, they will keep the same manifest because they are not deployed behind proxies, so install / uninstall workflows should work normally. |
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.
The simplification makes sense to me. Will preUserOpValidationHooks
and preRuntimeValidationHooks
behave differently in real life use cases?
Do you think if we can merge them?
I think it should be possible to merge the two pre-validation hook types together, but I haven't tried it out yet. That merge might also overlap somewhat with the multi-validation support, where the distinction between a validation hook and a validation "step" might overlap. |
On this specifically, I left some thoughts here: erc6900/resources#8 I came to the conclusion that the current separation makes sense, and any attempt to separate the two will still need some level of differentiation to note which one is allowed to return aggregator addresses. But this was just a cursory look at it. |
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.
1 small nit otherwise LGTM
Motivation
Under the current design of ERC-6900, the account is responsible for tracking the assigned validation plugin for user op validation and runtime validation separately. In practice, it rarely makes sense for these to be assigned differently, and may actually cause ambiguity around whether or not a plugin should leave one validator unassigned or set to a magic value if the plugin doesn't implement it.
Therefore, it might make sense to merge the assignment of these functions, and expect plugins to implement the functions (or revert if unsupported). This is an experimental change, looking for feedback.
The merge of these two assignments also simplifies the work needed to support multiple validation schemes per function (erc6900/resources#4).
Solution
userOpValidationFunctions
andruntimeValidationFunctions
withvalidationFunctions
.userOpValidation
andruntimeValidation
withinSelectorData
with justvalidation
.userOpValidationFunction
orruntimeValidationFunction
with the same function id assigned fromUpgradeableModularAccount
.