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 execute user op #57

Closed
wants to merge 3 commits into from
Closed

Conversation

howydev
Copy link
Collaborator

@howydev howydev commented May 15, 2024

This is an implementation of adding executeUserOp to ERC6900. This means all ERC6900 accounts would pass context from validation to execution via calldata. Accounts can still opt to do TSTORE or SSTORE

Changes:

  1. preExecutionHook now passes bytes memory senderContext instead of address sender. This is to account for an r1 curves that have 64 byte public keys
  2. userOpValidationFunction now returns an additional field, bytes memory senderContext. The additional context added in user ops need to validated, and this has to be done in the account or the plugin. This implementation does it in the account. Doing it this way allows flexibility for ERC6900 accounts to pass context from validation to execution with any user op field (userop.sig, userop.nonce etc), and since the plugin is already doing signature format parsing (which can vary from plugin to plugin), we should continue relying on the plugin for this. Note: we could opt to compress bytes memory senderContext into bytes32 by hashing it, at a spec level
  3. Plugins signal that they want UO context by setting 128 < functionId < 256. The better way to do it is probably to add an additional byte to FunctionReference for execution functions to denote if the plugin needs UO context or not. This implementation was chosen to speed up the PR since the better way of doing it would require changes in 20+ files

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.

Something I'm a bit confused about - why do we need to add the senderContext parameter to preExecutionHook and userOpValidation? It seems a bit like we're embedding a specific use case of "who is the sender" as the use case for executeUserOp, as opposed to some wider set of potential uses.

In the examples provided where the SingleOwnerPlugin returns the ECDSA/contract owner address, couldn't this be fetched by the execution and/or hook plugin? For the ECDSA owner case, if it gets the fields from executeUserOp, it should be able to pull out the signature from there and (hypothetically) ecrecover it, or if it doesn't want to do that it should be able to just read from the plugin storage to find out who the owner is.

I think it'd be useful to find a way for the actual PackedUserOp contents to be accessible to the plugins. This would allow for extra data in userOp.signature to be read, for example, or for stateful gas limits to be updated in the execution phase.

// if executeUserOp is used, parse out the right calldata and verify senderContext
bytes memory validationContextData;
// userOp.calldata = bytes4(executeUserOp.selector) || bytes callData || bytes senderContext
// TODO: need to modify userOp.calldata here, it should point to `bytes callData` within
Copy link
Contributor

Choose a reason for hiding this comment

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

You can't modify the variable of type PackedUserOperation calldata because calldata references are immutable, but you can do a calldata -> memory copy, then reassign the .callData portion of it. Here's an example: https://github.com/erc6900/reference-implementation/pull/62/files#diff-4218469ba4b89c8a36e355bbf535a3d32fbe4f6846506a6dc961b5ec5cd2f1d7R410-R415

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, realized it was nontrivial so I left it as a TODO. Thanks for the example impl!

@howydev
Copy link
Collaborator Author

howydev commented Jun 24, 2024

Closing in favor of the impl in the permissions stack

@howydev howydev closed this Jun 24, 2024
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.

2 participants