AA-245 Proposal: add executeUserOp()

[drortirosh]

Proposal: process the entire UserOp during account execution.

The account MAY implement a method:

function executeUserOp(UserOperation userOp, bytes32 userOpHash)

if it needs parts of the UserOp during its execution.

(note that account is not required to use it: it may continue to use standard "execute" methods, and even support both calling conventions, for different userops)

To signal the EntryPoint to call this method, the wallet should start the userOp.callData with the "magic" value - executeUserOp.selector

Note that the calldata is no longer directly callable - since obviously the rest of the callData is NOT the parameters of executeUserOp.

The account should parse the callData to remove this 4-byte prefix and process the rest of the callData as it sees fit.
See the TestExecAccount for a sample that uses this callData.

The calling wallet should properly encode this call data.

Passing information between validation and execution.

If the account needs to transfer information from the validateUserOp to the executeUserOp, it should use storage (or, when available, transient storage) based on the userOpHash, which is the same value for both validate and execute.

[adam-alchemy]

Curious for the reasoning behind adding this method. By using storage or transient storage, it is already possible to pass information from validation to execution, and the amount passed may be limited to just what is needed. It may be more expensive, however.

Between validateUserOp, RIP-7560's validateTransaction, and the proposed executeUserOp, the number of possible combinations of account functions is going up. And to make use of this function, the account would either have to re-implement a function dispatcher, or copy calldata again and perform an inner call like the example test case provided.

[yoavw]

Thanks for the feedback @adam-alchemy

Context: the purpose of this PR is to collect feedback from the community before deciding whether to implement this. We'll publish it and seek feedback before merging. Keep in mind that this method is optional and does not affect accounts that don't use it.

Curious for the reasoning behind adding this method.

Several projects asked to access parts of the UserOp or to know the UserOpHash during execution. And other forms of native AA (zksync and starknet) make it available during execution. In 4337 and 7560 we couldn't do it because we didn't want to mandate a specific execution function and break some projects' compatibility. We figured we could accommodate this request by adding this optional method. Accounts that don't need it will keep working the usual way, and ones who do - can use this method.

In fact it was also requested by one of the other ERC-6900 authors at some point, so we thought it would simplify some ERC-6900 flows as well.

Another benefit is that it'll help wallets that run on zksync support 4337 and 7560.

By using storage or transient storage, it is already possible to pass information from validation to execution, and the amount passed may be limited to just what is needed. It may be more expensive, however.

1. Using transient storage for that purpose without having userOpHash is error prone and may lead to vulnerabilities. The account dev may not consider the fact that a bundler may include two UserOps of the same account in the same bundle (as it's not enforced on-chain, just bundlers). In that case the 2nd validation will overwrite the transient storage set by the 1st validation, and both executions will see the 2nd one. A proper implementation would have to use an array in transient storage, and executions would keep a transient pointer to the array in order to use the correct context for each UserOp. But with executeUserOp you can use a transient mapping instead, so you don't need to worry about this case.
2. Transient storage is indeed more expensive, so unless you already performed some expensive processing during validation and wish to use the result in execution, it might make more sense to access the UserOp in execution instead of incurring the cost of TSTORE+TLOAD.

Between validateUserOp, RIP-7560's validateTransaction, and the proposed executeUserOp, the number of possible combinations of account functions is going up.

The former two are mandatory (one in 4337 and one in 7560). The latter one is not. It's just an execution function. Any account may have any number of execution functions, we're only suggesting assigning a special meaning to one and reserving it to support this use case. If you're not using it, you don't need to implement it or even know about it. It might still make sense for SDKs to implement an empty/reverting one, just to make sure no one implements a methodsig that collides with it and behaves unexpectedly.

And to make use of this function, the account would either have to re-implement a function dispatcher, or copy calldata again and perform an inner call like the example test case provided.

Most accounts won't make use of it and can just ignore it. You should only use it if you need to access parts of UserOp (other than UserOp.calldata). Until now this use case was not supported, so accounts don't need to reimplement anything (since they didn't have that functionality before). Now they can use a dispatcher (like the one in the example) to call their existing execution functions after extracting what they need from the UserOp.

There are two separate questions we seek feedback on:

1. Will this functionality be useful for some of the account devs, standards like ERC-6900, etc.?
2. If you're not going to use it yourself, do you see a reason not to include this functionality for those who do? As far as we can tell, it has no impact on other accounts, except for a very minor gas cost (~40 gas).

[adam-alchemy]

Thank you for the very detailed description and reasoning. Having access to the UO hash does seem useful and brings feature parity with other AA implementations, and like you said makes the process of sharing context between phases more straightforward to implement.

I think from the perspective of an account developer, this is relatively straightforward: you use it if it is mandatory for some account feature, and ignore it otherwise. That is the benefit of making this optional. It gets a little bit trickier if you approach this from the perspective of a plugin/module developer: you have to consider whether it is necessary for your specific feature, find a way to enforce that it is called, then perform your feature-specific processing of the user operation's data before the actual action specified in userOp.callData happens.

From the perspective of 6900 & modular accounts, supporting multiple validation conditions & execution configurations gets a bit more complicated, as does enforcing protections preventing incorrect call flows. But it could also be an unlock for easier context-aware features, like using gas values, the signature, or the paymaster and data. It seems like this is a net positive for accounts. I just need to think a bit more on how this can fit into a flexible account framework, and if there are any changes that could be made to the proposal to make it easier.

[yoavw]

Thanks @adam-alchemy

I think for plugins/modules it can be abstracted away, and they'll just get the information they need. Let's consider how ERC-6900 would use it:

• It has no effect on validation hooks since it only matters during execution.
• In execution, it matters to pre-execution and post-execution hooks. We could add two new types of these hooks, that take UserOp as an arg. If a plugin developer needs access to the UserOp, just implement the hook that has the UserOp arg instead of the one that doesn't.

In the core account:

• Implement a executeUserOp that calls these hooks and executes UserOp.calldata[4:].
• During validation, if you see that you're going to use these hooks, require that UserOp.calldata[0:4] is executeUserOp.

The core account contract makes things transparent to plugins. They just choose whether they need this extra info by specifying the right kind of hooks in their manifest.

[NIC619]

Thanks @drortirosh and @yoavw ! We are very much looking forward to be able to get userOpHash during execution phase to help us provide a more granular safety check as mentioned in #337.

What do you think about the approach of using transient storage to store userOpHash and have a public getUserOpHash view function for account to query it during its execution phase?

bytes32 private transient _userOpHash;

function getUserOpHash() view returns (bytes32) {
    return _userOpHash;
}

function _executeUserOp(...) {
    ...

    _userOpHash = userOpHash;
    try this.innerHandleOp(userOp.callData, opInfo, context) returns (...) {
        ...
    }
    _userOpHash = bytes32(0);

    ...
}

[drortirosh]

What do you think about the approach of using transient storage to store userOpHash and have a public getUserOpHash view function for account to query it during its execution phase?

We thought of it, but feel uneasy adding userop-specific state to EntryPoint.

[forshtat]

Is there no reasonable way to implement this without manually resetting the free pointer? This code has two assembly chunks and looks a bit complicated.

[drortirosh]

it is very expensive to expand memory and never use it.

[forshtat]

What is the meaning of mload(0)?

[drortirosh]

the return value of innerCall is saved into address 0 (the last 2 params of call are "offset", "length")
mload load that return value

[forshtat]

I think this is actually a bit too awkward. When the wallet app wants to pass some params to the Smart Account (say it wants to know what is 2+2) so it will need to create a calldata that is abi.encodeWithSelector("executeUserOp", "add", 2, 2).
However this (string, uint,uint) is not the correct method signature for the executeUserOp method.
I think that developers will (rightfully) feel like they need to abi.encode the correct method call, which is executeUserOp(UserOperation,bytes32), and will be confused.

[drortirosh]

the executeUserOp is not a real methodsig, but instead a MAGIC value at the beginning of the callData which is expected not to collide with any valid methodSig.
The executeUserOp method is expected to read more than just the calldata to execute (otherwise, it wouldn't use this mechanism, but normal execute()...

if all you want is an executeUserOp that performs just what "execute" does, then the encoding should probably be:

abi.encodePacked( executeUserOp.selector, abi.encodeCall( execute, (target,0,data)));

and executeUserOp would call the inner method as:
address(this).call(userOp.callData[4:])
but again, this is a very heavy way to achieve just what "execute" does.

[jayden-sudo]

you don't need to implement it or even know about it. It might still make sense for SDKs to implement an empty/reverting one, just to make sure no one implements a methodsig that collides with it and behaves unexpectedly.

Thanks so much for enabling access to userOp during the execution phase, which will unlock many new use cases!

However, I would like to point out that even without considering 'just to make sure no one implements a methodsig that collides with it and behaves unexpectedly,' it may still be necessary to have an empty function.

Reason: In fact, contract wallets often have the ability to dynamically change the execution process of fallback() function. The wallet client may determine whether the calldata is executed successfully based on the success field in the UserOperationEvent. However, if the executeUserOp() function is not implemented in the contract, it will enter the fallback() function, which may eventually enter some functions that never revert, causing the client to not handle the execution status of calldata correctly.

Therefore, I suggest that even if the executeUserOp() function is not used, as long as the internal execution process of fallback() can be modified, it is necessary to implement the executeUserOp() function, and the function must contain a revert.

And my implementation