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.

Sign up for GitHub

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

Refactor _callInnerHandleOp #556

Open
wants to merge 2 commits into
base: refactor-post-execution
Choose a base branch
from
Open

Refactor _callInnerHandleOp #556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6905c57
refactor _callinnerHandleOp
drortirosh Mar 2, 2025
1b63bac
reordered, fir simpler diff.
drortirosh Mar 2, 2025
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
2 changes: 1 addition & 1 deletion contracts/accounts/Simple7702Account.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ contract Simple7702Account is BaseAccount, IERC165, IERC1271, ERC1155Holder, ERC

// temporary address of entryPoint v0.8
function entryPoint() public pure override returns (IEntryPoint) {
return IEntryPoint(0x8195d7440D459fb0D36c95FC9D7B09CeBf3D54b2);
return IEntryPoint(0x3C965AC44c8C3946de6aBdc65b1Df5FeDCaeb06B);
}

/**
Expand Down
89 changes: 47 additions & 42 deletions contracts/core/EntryPoint.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,44 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT
require(success, "AA91 failed send to beneficiary");
}

/**
* call innerHandleOp
* call innerHandleOp either with userOp.callData or (if it starts with executeUserOp selector) build an executeUserOp call
* @return returnData - 0 - execution success
* 1 - execution call reverted
* anything else - revert forwarded from innerHandleOp
*/
function _callInnerHandleOp(
PackedUserOperation calldata userOp,
UserOpInfo memory opInfo
) internal virtual returns (uint256 returnData, uint256 actualGas) {
uint256 saveFreePtr = getFreePtr();
bytes memory context = getMemoryBytesFromOffset(opInfo.contextOffset);
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));
}
assembly ("memory-safe") {
pop(call(gas(), address(), 0, add(innerCall, 0x20), mload(innerCall), 0, 64))
returnData := mload(0)
// returned by either INNER_REVERT_LOW_PREFUND or successful return.
actualGas := mload(32)
}
restoreFreePtr(saveFreePtr);
}

/**
* Execute a user operation.
* @param opIndex - Index into the opInfo array.
Expand All @@ -93,47 +131,14 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT
internal virtual
returns (uint256 actualGasCost) {
uint256 preGas = gasleft();
bytes memory context = getMemoryBytesFromOffset(opInfo.contextOffset);
bool innerSuccess;
bytes32 returnData;
uint256 actualGas;
{
uint256 saveFreePtr = getFreePtr();
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));
}
assembly ("memory-safe") {
innerSuccess := call(gas(), address(), 0, add(innerCall, 0x20), mload(innerCall), 0, 64)
returnData := mload(0)
// returned by either INNER_REVERT_LOW_PREFUND or successful return.
actualGas := mload(32)
}
restoreFreePtr(saveFreePtr);
}
bool executionSuccess;
if (innerSuccess) {
executionSuccess = returnData != 0;
} else {
bytes32 innerRevertCode = returnData;
if (innerRevertCode == INNER_OUT_OF_GAS) {
(uint256 returnData, uint256 actualGas) = _callInnerHandleOp(userOp, opInfo);
if (returnData > 1) {
if (bytes32(returnData) == INNER_OUT_OF_GAS) {
// handleOps was called with gas limit too low. abort entire bundle.
// can only be caused by bundler (leaving not enough gas for inner call)
revert FailedOp(opIndex, "AA95 out of gas");
}
if (innerRevertCode != INNER_REVERT_LOW_PREFUND) {
if (bytes32(returnData) != INNER_REVERT_LOW_PREFUND) {
actualGas = preGas - gasleft();
actualGas += _getUnusedGasPenalty(actualGas, opInfo.mUserOp.callGasLimit + opInfo.mUserOp.paymasterPostOpGasLimit);
emit PostOpRevertReason(
Expand All @@ -147,6 +152,8 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT
// and postInnerCall below will call _emitPrefundTooLow
}

bool executionSuccess = returnData == 0;
bool innerSuccess = returnData <= 1;
return _postInnerCall(
opInfo,
executionSuccess,
Expand All @@ -164,7 +171,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT
* @param opInfo - UserOp fields and info collected during validation.
* @param executionSuccess - Whether account execution was successful.
* @param actualGas - actual gas used for execution and postOp
* @param innerSuccess - Whether inner call succeeded or reverted
* @param innerSuccess - Whether inner call succeeded or reverted (it can only revert on postOp revert or low prefund)
*/
function _postInnerCall(
UserOpInfo memory opInfo,
Expand Down Expand Up @@ -368,14 +375,12 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT
* @param callData - The callData to execute.
* @param opInfo - The UserOpInfo struct.
* @param context - The context bytes.
* @return callSuccess - return status of sender callData
* @return actualGasUsed - gas used by this call, including unused gas penalty
*/
function innerHandleOp(
bytes memory callData,
UserOpInfo memory opInfo,
bytes calldata context
) external returns (bool callSuccess, uint256 actualGasUsed) {
) external returns (bool executionReverted, uint256 actualGasUsed) {
uint256 preGas = gasleft();
require(msg.sender == address(this), "AA92 internal call only");
MemoryUserOp memory mUserOp = opInfo.mUserOp;
Expand Down Expand Up @@ -418,7 +423,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuardT
unchecked {
uint256 executionGas = preGas - gasleft();
uint256 actualGas = _postExecution(mode, opInfo, context, executionGas);
return (mode == IPaymaster.PostOpMode.opSucceeded, actualGas);
return (mode != IPaymaster.PostOpMode.opSucceeded, actualGas);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why reversing the condition? That means that the assembly code that you extracted to an internal function not just moved, but also changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I wanted 0=success, 1=exec-revert, other=inner-call+reverted

}
}

Expand Down
32 changes: 16 additions & 16 deletions reports/gas-checker.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,36 +12,36 @@
║ │ │ │ (delta for │ (compared to ║
║ │ │ │ one UserOp) │ account.exec()) ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple │ 1 │ 77235 │ │ ║
║ simple │ 1 │ 77219 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple - diff from previous │ 2 │ │ 41292 │ 12293 ║
║ simple - diff from previous │ 2 │ │ 41288 │ 12289 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple │ 10 │ 448970 │ │ ║
║ simple │ 10 │ 448930 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple - diff from previous │ 11 │ │ 41400 │ 12401 ║
║ simple - diff from previous │ 11 │ │ 41420 │ 12421 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster │ 1 │ 83095 │ │ ║
║ simple paymaster │ 1 │ 83139 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster with diff │ 2 │ │ 39889 │ 10890 ║
║ simple paymaster with diff │ 2 │ │ 39897 │ 10898 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster │ 10 │ 442210 │ │ ║
║ simple paymaster │ 10 │ 442422 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster with diff │ 11 │ │ 39897 │ 10898 ║
║ simple paymaster with diff │ 11 │ │ 39953 │ 10954 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ big tx 5k │ 1 │ 167018 │ │ ║
║ big tx 5k │ 1 │ 167014 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ big tx - diff from previous │ 2 │ │ 130513 │ 16073 ║
║ big tx - diff from previous │ 2 │ │ 130521 │ 16081 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ big tx 5k │ 10 │ 1341879 │ │ ║
║ big tx 5k │ 10 │ 1341767 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ big tx - diff from previous │ 11 │ │ 130560 │ 16120 ║
║ big tx - diff from previous │ 11 │ │ 130544 │ 16104 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ paymaster+postOp │ 1 │ 84549 │ │ ║
║ paymaster+postOp │ 1 │ 84533 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ paymaster+postOp with diff │ 2 │ │ 41319 │ 12320 ║
║ paymaster+postOp with diff │ 2 │ │ 41327 │ 12328 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ paymaster+postOp │ 10 │ 456540 │ │ ║
║ paymaster+postOp │ 10 │ 456488 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ paymaster+postOp with diff │ 11 │ │ 41283 │ 12284 ║
║ paymaster+postOp with diff │ 11 │ │ 41339 │ 12340 ║
╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝