-
Notifications
You must be signed in to change notification settings - Fork 670
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ pragma solidity ^0.8.23; | |
/* solhint-disable no-inline-assembly */ | ||
|
||
import "../interfaces/IAccount.sol"; | ||
import "../interfaces/IAccountExecute.sol"; | ||
import "../interfaces/IPaymaster.sol"; | ||
import "../interfaces/IEntryPoint.sol"; | ||
|
||
|
@@ -78,12 +79,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)); | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the meaning of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") |
||
mstore(0x40, saveFreePtr) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
|
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; | ||
} |
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)) | ||
) | ||
))); | ||
} | ||
} |
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)) | ||
}) | ||
}) |
There was a problem hiding this comment.
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 theexecuteUserOp
method.I think that developers will (rightfully) feel like they need to
abi.encode
the correct method call, which isexecuteUserOp(UserOperation,bytes32)
, and will be confused.There was a problem hiding this comment.
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.