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, experimental] Merge validation function assignments #40

Merged
merged 6 commits into from
Apr 15, 2024

Conversation

adam-alchemy
Copy link
Contributor

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

  • Replace the manifest fields userOpValidationFunctions and runtimeValidationFunctions with validationFunctions.
  • Replace the storage fields userOpValidation and runtimeValidation within SelectorData with just validation.
  • Use either userOpValidationFunction or runtimeValidationFunction with the same function id assigned from UpgradeableModularAccount.
  • Merge validation function id's in the provided sample and mock plugins.
  • Update the manifests used in sample and mock plugins to only specify validation functions once.
  • Update the dependencies used by plugins to only specify one dependent validation function, rather than two.

@dlim-circle
Copy link

Curious what's the impact to existing plugins (eg. Session Key plugin) given the manifest hash has changed. Does the account need to uninstall and reinstall on new plugin or is there an upgrade path

cc @huaweigu @walkerq

@adam-alchemy
Copy link
Contributor Author

@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.

Copy link

@huaweigu huaweigu left a 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?

@adam-alchemy
Copy link
Contributor Author

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.

@jaypaik
Copy link
Collaborator

jaypaik commented Mar 11, 2024

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.

@jaypaik jaypaik requested a review from a team April 15, 2024 20:41
Copy link
Collaborator

@howydev howydev left a 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

@adam-alchemy adam-alchemy merged commit f6e335c into v0.8-develop Apr 15, 2024
3 checks passed
@adam-alchemy adam-alchemy deleted the adam/validation-merge branch April 15, 2024 22:29
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.

None yet

6 participants