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

chore: add requirements for IAccountExecute #180

Merged
merged 5 commits into from
Sep 5, 2024
Merged

Conversation

howydev
Copy link
Collaborator

@howydev howydev commented Sep 3, 2024

No description provided.

@jaypaik jaypaik requested a review from a team September 4, 2024 21:17
jaypaik
jaypaik previously approved these changes Sep 4, 2024
@adamegyed
Copy link
Contributor

Could you expand a bit on why we want to loosen this?

And also, I remember we previously discussed how validation-associated exec hooks could be done without executeUserOp via transient storage or similar, but I just realized how doing so would result in different data provided to the hook (call to executeUserOp vs just the inner userOp.callData). Is there any way for the account user to specify which form the hook receives?

@jaypaik
Copy link
Collaborator

jaypaik commented Sep 4, 2024

Oh hmm. Do we absolutely need accounts to implement executeUserOp to be compatible with the spec currently?

@jaypaik jaypaik self-requested a review September 4, 2024 21:20
@jaypaik jaypaik dismissed their stale review September 4, 2024 21:20

discussion needed

@howydev
Copy link
Collaborator Author

howydev commented Sep 5, 2024

Discussed async, summary:

  1. In light of 6900 being a spec for accounts <> modules, theres a risk of accounts not being able to use some hooks correctly if executeUserOp is not implemented as currently done in ref impl.
  2. Instead of loosening the reqs, we should keep it the same, and then add a requirement s.t. permission hooks for that validator should be called, and the full msg.data should be sent as the hook data

@@ -540,7 +540,7 @@ If the selector being checked is `execute` or `executeBatch`, the modular accoun

Installed validations have two flag variables indicating what they may be used for. If a validation is attempted to be used for user op validation and the flag `isUserOpValidation` is set to false, validation MUST revert. If the validation is attempted to be used for signature validation and the flag `isSignatureValidation` is set to false, validation MUST revert.

#### Direct Call Validation.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fix - other titles do not contain periods at the end

@howydev howydev changed the title chore: loosen requirement for IAccountExecute chore: add requirements for IAccountExecute Sep 5, 2024
Copy link
Contributor

@adamegyed adamegyed left a comment

Choose a reason for hiding this comment

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

Looks good, two small nits

standard/ERCs/erc-6900.md Outdated Show resolved Hide resolved
standard/ERCs/erc-6900.md Outdated Show resolved Hide resolved
@howydev howydev merged commit cc5ac05 into develop Sep 5, 2024
3 checks passed
@howydev howydev deleted the howy/update-spec-2 branch September 5, 2024 21:48
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