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

AA-245 Proposal: add executeUserOp() #380

Merged
merged 3 commits into from
Jan 3, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 28 additions & 6 deletions contracts/core/EntryPoint.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ pragma solidity ^0.8.12;
/* solhint-disable no-inline-assembly */

import "../interfaces/IAccount.sol";
import "../interfaces/IAccountExecute.sol";
import "../interfaces/IPaymaster.sol";
import "../interfaces/IEntryPoint.sol";

Expand Down Expand Up @@ -79,12 +80,33 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
(uint256 collected) {
uint256 preGas = gasleft();
bytes memory context = getMemoryBytesFromOffset(opInfo.contextOffset);

try this.innerHandleOp(userOp.callData, opInfo, context) returns (
uint256 _actualGasCost
) {
collected = _actualGasCost;
} catch {
uint saveFreePtr;
assembly {
saveFreePtr := mload(0x40)
}
bytes calldata callData = userOp.callData;
bytes memory innerCall;
bytes4 methodSig;
assembly {
let len := callData.length
if gt(len,3) {
methodSig := calldataload(callData.offset)
}
}
if (methodSig == IAccountExecute.executeUserOp.selector) {
bytes memory executeUserOp = abi.encodeCall(IAccountExecute.executeUserOp, (userOp, opInfo.userOpHash));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

innerCall = abi.encodeCall(this.innerHandleOp, (executeUserOp, opInfo, context));
} else
{
innerCall = abi.encodeCall(this.innerHandleOp, (callData, opInfo, context));
}
bool success;
assembly {
success := call(gas(), address(), 0, add(innerCall, 0x20), mload(innerCall), 0, 32)
collected := mload(0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the meaning of mload(0)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

mstore(0x40, saveFreePtr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
if (!success) {
bytes32 innerRevertCode;
assembly {
let len := returndatasize()
Expand Down
20 changes: 20 additions & 0 deletions contracts/interfaces/IAccountExecute.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.12;

import "./UserOperation.sol";

interface IAccountExecute {
/**
* Account may implement this execute method.
* passing this methodSig at the beginning of callData will cause the entryPoint to pass the full UserOp (and hash)
* to the account.
* The account should skip the methodSig, and use the callData (and optionally, other UserOp fields)
*
* @param userOp - The operation that was just validated.
* @param userOpHash - Hash of the user's request data.
*/
function executeUserOp(
UserOperation calldata userOp,
bytes32 userOpHash
) external;
}
75 changes: 75 additions & 0 deletions contracts/test/TestExecAccount.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
// SPDX-License-Identifier: GPL-3.0

/* solhint-disable one-contract-per-file */
/* solhint-disable avoid-low-level-calls */
pragma solidity ^0.8.15;

import "@openzeppelin/contracts/utils/Create2.sol";
import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";

import "../samples/SimpleAccount.sol";
import "../interfaces/IAccountExecute.sol";

/**
* a sample account with execUserOp.
* Note that this account does nothing special with the userop, just extract
* call to execute. In theory, such account can reference the signature, the hash, etc.
*/
contract TestExecAccount is SimpleAccount, IAccountExecute {

constructor(IEntryPoint anEntryPoint) SimpleAccount(anEntryPoint){
}

event Executed(UserOperation userOp, bytes innerCallRet);

function executeUserOp(UserOperation calldata userOp, bytes32 /*userOpHash*/) external {
_requireFromEntryPointOrOwner();

// read from the userOp.callData, but skip the "magic" prefix (executeUserOp sig),
// which caused it to call this method.
bytes calldata innerCall = userOp.callData[4 :];

bytes memory innerCallRet;
if (innerCall.length > 0) {
(address target, bytes memory data) = abi.decode(innerCall, (address, bytes));
bool success;
(success, innerCallRet) = target.call(data);
require(success, "inner call failed");
}

emit Executed(userOp, innerCallRet);
}
}

contract TestExecAccountFactory {
TestExecAccount public immutable accountImplementation;

constructor(IEntryPoint _entryPoint) {
accountImplementation = new TestExecAccount(_entryPoint);
}

function createAccount(address owner, uint256 salt) public returns (address ret) {
address addr = getAddress(owner, salt);
uint codeSize = addr.code.length;
if (codeSize > 0) {
return addr;
}
ret = address(new ERC1967Proxy{salt: bytes32(salt)}(
address(accountImplementation),
abi.encodeCall(SimpleAccount.initialize, (owner))
));
}

/**
* calculate the counterfactual address of this account as it would be returned by createAccount()
*/
function getAddress(address owner, uint256 salt) public view returns (address) {
return Create2.computeAddress(bytes32(salt), keccak256(abi.encodePacked(
type(ERC1967Proxy).creationCode,
abi.encode(
address(accountImplementation),
abi.encodeCall(SimpleAccount.initialize, (owner))
)
)));
}
}
30 changes: 15 additions & 15 deletions reports/gas-checker.txt
Original file line number Diff line number Diff line change
Expand Up @@ -12,35 +12,35 @@
║ │ │ │ (delta for │ (compared to ║
║ │ │ │ one UserOp) │ account.exec()) ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple │ 1 │ 81918 │ │ ║
║ simple │ 1 │ 81961 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple - diff from previous │ 2 │ │ 4418715173
║ simple - diff from previous │ 2 │ │ 4420615192
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple │ 10 │ 479730 │ │ ║
║ simple │ 10 │ 480110 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple - diff from previous │ 11 │ │ 4424715233
║ simple - diff from previous │ 11 │ │ 4424915235
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster │ 1 │ 89813 │ │ ║
║ simple paymaster │ 1 │ 89871 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster with diff │ 2 │ │ 4479615782
║ simple paymaster with diff │ 2 │ │ 4481215798
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster │ 10 │ 493254 │ │ ║
║ simple paymaster │ 10 │ 493672 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster with diff │ 11 │ │ 4482015806
║ simple paymaster with diff │ 11 │ │ 4483315819
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ big tx 5k │ 1 │ 182975 │ │ ║
║ big tx 5k │ 1 │ 183018 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ big tx - diff from previous │ 2 │ │ 14469819438
║ big tx - diff from previous │ 2 │ │ 14472919469
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ big tx 5k │ 10 │ 1485374 │ │ ║
║ big tx 5k │ 10 │ 1485774 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ big tx - diff from previous │ 11 │ │ 14475919499
║ big tx - diff from previous │ 11 │ │ 14480919549
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ token paymaster │ 1 │ 148244 │ │ ║
║ token paymaster │ 1 │ 148291 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ token paymaster with diff │ 2 │ │ 7292043906
║ token paymaster with diff │ 2 │ │ 7294743933
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ token paymaster │ 10 │ 804795 │ │ ║
║ token paymaster │ 10 │ 805221 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ token paymaster with diff │ 11 │ │ 73015 │ 44001 ║
╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝
Expand Down
56 changes: 56 additions & 0 deletions test/testExecAccount.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import { before } from 'mocha'
import {
EntryPoint,
TestExecAccount,
TestExecAccount__factory,
TestExecAccountFactory__factory
} from '../typechain'
import { createAccountOwner, deployEntryPoint, fund, objdump } from './testutils'
import { fillAndSign } from './UserOp'
import { Signer, Wallet } from 'ethers'
import { ethers } from 'hardhat'
import { defaultAbiCoder, hexConcat, hexStripZeros } from 'ethers/lib/utils'
import { expect } from 'chai'

describe('IAccountExecute', () => {
let ethersSigner: Signer
let entryPoint: EntryPoint
let account: TestExecAccount
let owner: Wallet
before(async () => {
const provider = ethers.provider
ethersSigner = provider.getSigner()
entryPoint = await deployEntryPoint()
const factory = await new TestExecAccountFactory__factory(ethersSigner).deploy(entryPoint.address)
owner = createAccountOwner()
await factory.createAccount(owner.getAddress(), 0)
const accountAddress = await factory.callStatic.createAccount(owner.getAddress(), 0)
account = TestExecAccount__factory.connect(accountAddress, provider)
await fund(accountAddress)
})

it('should execute ', async () => {
const execSig = account.interface.getSighash('executeUserOp')
// innerCall, as TestExecAccount.executeUserOp will try to decode it:
const innerCall = defaultAbiCoder.encode(['address', 'bytes'], [
account.address,
account.interface.encodeFunctionData('entryPoint')
])

const userOp = await fillAndSign({
sender: account.address,
callGasLimit: 100000, // normal estimate also chokes on this callData
callData: hexConcat([execSig, innerCall])
}, owner, entryPoint)

await entryPoint.handleOps([userOp], ethersSigner.getAddress())

const e =
await account.queryFilter(account.filters.Executed())

expect(e.length).to.eq(1, "didn't call inner execUserOp (no Executed event)")
console.log(e[0].event, objdump(e[0].args))
// validate we retrieved the return value of the called "entryPoint()" function:
expect(hexStripZeros(e[0].args.innerCallRet)).to.eq(hexStripZeros(entryPoint.address))
})
})
Loading