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: add call flow spec for validation and execution #166

Merged
merged 3 commits into from
Aug 29, 2024

Conversation

adamegyed
Copy link
Contributor

Motivation

We want to formally specify the requirements for modular accounts on ERC-6900 v0.8 with respect to call flows for validation and execution.

Solution

Remove the old spec for validation and execution, which was needless restrictive and verbose.

Add a spec for ERC-6900 v0.8. Includes:

  • Validation types: user op, runtime, signature
  • Implementation-defined (aka non-standardized) selection of validation functions.
  • Validation applicability: global or selector-based
  • Hook execution ordering
  • Self-call validation restrictions for execute and executeBatch.

Also adds ERC-1271 as a mandatory interface for modular accounts.

This PR does not yet update the spec for the install flows in v0.8.

@adamegyed adamegyed requested a review from a team August 27, 2024 21:19

Within the implementation of each type of validation function, the modular account MUST check that the provided validation function applies to the given function selector intended to be run. Then, the account MUST execute all pre validation hooks of the corresponding type associated with the validation function in use. These hooks MUST be executed in the order specified during installation. After the execution of pre validation hooks, the account MUST invoke the validation function of the corresponding type. If any pre validation hooks or the validation function revert, the account MUST revert. It SHOULD include the module's revert data within its revert data.

The account MUST define a way to pass data separately for each validation hook and the validation function itself. This data MUST be sent as the `userOp.signature` field for user op validation, the `authorization` field for runtime validation, and the `signature` field for signature validation.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The account MUST define a way to pass data separately for each validation hook and the validation function itself. This data MUST be sent as the `userOp.signature` field for user op validation, the `authorization` field for runtime validation, and the `signature` field for signature validation.
The account MUST define a way to pass data separately for each pre-validation hook and the validation function itself. This data MUST be sent as the `userOp.signature` field for user op validation, the `authorization` field for runtime validation, and the `signature` field for signature validation.

Is the second sentence necessary?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use pre validation hook(s) for all validation hook(s) references to be precise and consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the second sentence necessary?

Yes, if that is not standardized, then the modules may be written to expect the data in different locations, which would result in modules that are compatible with some 6900-compliant accounts, but not all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use pre validation hook(s) for all validation hook(s) references to be precise and consistent.

I thought about this, but I think the fact that there are not post validation hook(s) means that the "pre" wording doesn't differentiate it any further. It gives some insight into when to expect it to be run, but that's not really necessary for this section of the spec.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely get what you mean. However, I think there are benefits to have pre as it makes it a bit easier to understand how it works for new comers to (4337 and 6900) who does not know a ton how exactly validation works yet.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaning towards just calling them validation hooks (adding "pre" might signal that there exists a "post", similar to execution hooks). But I see that we do reference "pre validation hooks" in the prior paragraph. We should be consistent everywhere.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point on signaling "post". Happy to go with "validation hooks" with consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to be consistent


The result of user op validation SHOULD be the intersection of time bounds returned by the pre validation hooks and the validation function. If any validation hooks or the validation functions returns a value of `1` for the authorizer field, indicating a signature verification failure by the ERC-4337 standard, the account MUST return a value of `1` for the authorizer portion of the validation data.

The set of validation hooks run MUST be the hooks specified by account state at the start of validation. In other words, if the set of applicable hooks changes during validation, the original set of hooks MUST still run, and only future invocations of the same validation should reflect the changed set of hooks.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The set of validation hooks run MUST be the hooks specified by account state at the start of validation. In other words, if the set of applicable hooks changes during validation, the original set of hooks MUST still run, and only future invocations of the same validation should reflect the changed set of hooks.
The set of pre validation hooks run MUST be the hooks specified by account state at the start of validation. In other words, if the set of applicable hooks changes during validation, the original set of hooks MUST still run, and only future invocations of the same validation should reflect the changed set of hooks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's been decided to just use "validation hooks" as the consistent term yeah?


The `executeFromModuleExternal` function MUST allow modules to call external addresses as specified by its parameters on behalf of the modular account. If the calling module's manifest did not explicitly allow the external call within `permittedExternalCalls` at the time of installation, execution MUST revert.
Module execution functions where the field `isPublic` is set to true, or native functions without access control, SHOULD omit the runtime validation step, including any runtime validation hooks. Native functions without access control MAY also omit running execution hooks.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should state the risk where if the isPublic function changes states, omitting validation can be dangerous.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need that extra detail? I think that is obvious from the spec stating that validation can be skipped if the function is marked public.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code comment on isPublic is Whether or not the function needs runtime validation, or can be called by anyone. .
Why didn't we name it skipRuntimeValidation flag?

My concern of the name isPublic is that it does not indicate that it can potentially be state changing. We only skip runtime by using the old ALWAYS_ALLOW.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why didn't we name it skipRuntimeValidation flag?

That does seem like a good idea, makes it more literal. Maybe less obvious to connect it to view functions, but it would match the naming of other flags.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe less obvious to connect it to view functions

They don't have to be view functions is the whole point (to allow state changing though I am not sure what the use cases are, maybe exec timelock tx after trigger time or sth), right?
Otherwise, it can just be isPublic or isPublicView or isView, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Let's update that in a different PR, because it also requires code changes.

@adamegyed adamegyed force-pushed the adam/spec-call-flows branch from 80f1074 to ad364dc Compare August 28, 2024 19:59
@adamegyed adamegyed force-pushed the adam/spec-call-flows branch from 011c0f9 to ca981c8 Compare August 29, 2024 15:57

The account MUST define a way to pass data separately for each validation hook and the validation function itself. This data MUST be sent as the `userOp.signature` field for user op validation, the `authorization` field for runtime validation, and the `signature` field for signature validation.

The result of user op validation SHOULD be the intersection of time bounds returned by the validation hooks and the validation function. If any validation hooks or the validation functions returns a value of `1` for the authorizer field, indicating a signature verification failure by the ERC-4337 standard, the account MUST return a value of `1` for the authorizer portion of the validation data.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This, in other words, means that the result of UO validation should always be the longest possible time bound that satisfies all time bounds form hooks , right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Technically I think it is accurate to describe it as the "longest time bound that satisfies hooks", but intuitively, the operation of intersecting them is to take the min. E.g. if you have ascending timestamps $t_1$, $t_2$, $t_3$, $t_4$, and hook 1 returns the range $(t_1, t_3)$, and the validation function returns $(t_2, t_4)$, the result of validation should be the range $(t_2, t_3)$.

@adamegyed adamegyed merged commit d883e04 into develop Aug 29, 2024
3 checks passed
@adamegyed adamegyed deleted the adam/spec-call-flows branch August 29, 2024 22:11
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.

4 participants