AA-543: Refactor post execution #552
Conversation
drortirosh
force-pushed
the
refactor-post-execution
branch
from
d261501 to
19be0d2
Compare
drortirosh
force-pushed
the
refactor-post-execution
branch
from
a65d294 to
5075d38
Compare
contracts/core/EntryPoint.sol
Outdated
| } | ||
| bool callSuccess; | ||
| if (success) { | ||
| callSuccess = returnData != 0; |
There was a problem hiding this comment.
I would suggest executionSuccess or something like that, more explicit.
contracts/core/EntryPoint.sol
Outdated
| emitUserOperationEvent(opInfo, false, actualGasCost, actualGas); | ||
| collected = actualGasCost; | ||
| // innerCall reverted on prefund too low. | ||
| // below we re-calculate the cast cost and refund. |
There was a problem hiding this comment.
Where? Put function name, lines or some other meaningful reference to what you mean here.
Also, this entire if is empty, could we maybe "invert" it to be else if (!innerRevertCode == INNER_REVERT_LOW_PREFUND) { ... } ??
contracts/core/EntryPoint.sol
Outdated
| * call after innerCall executed to call the account's callData and paymaster's postOp. | ||
| * - calculate the overall gas used and unuased penalty. | ||
| * - refund payer if needed | ||
| * - emit UserOperationEvent |
There was a problem hiding this comment.
Rewrite this comment. Use ChatGPT.
contracts/core/EntryPoint.sol
Outdated
| unchecked { | ||
| uint256 prefund = opInfo.prefund; | ||
| uint256 actualGasCost = actualGas * getUserOpGasPrice(opInfo.mUserOp); | ||
| uint256 refund; |
There was a problem hiding this comment.
Are you seriously creating two variables - refund and prefund here? Absolutely not. These two variables MUST have two or three words names that don't sound anything alike.
There was a problem hiding this comment.
suggest better names.
There was a problem hiding this comment.
I don't know, originalPrefund and remainingGasRefund?
@shahafn could you help me out here? 😄
contracts/core/EntryPoint.sol
Outdated
| refund = prefund - actualGasCost; | ||
| } else { | ||
| actualGasCost = prefund; | ||
| //depending where the over-gas-used was found, we either reverted innerCall or not. |
There was a problem hiding this comment.
Instead of writing this comment add a boolean parameter to the emitPrefundTooLow - we don't always need it but it will help make the code flow more clear.
| uint256 gasUsed = preGas - gasleft(); | ||
| assembly ("memory-safe") { | ||
| mstore(0, INNER_OUT_OF_GAS) | ||
| revert(0, 32) | ||
| mstore(32, gasUsed) | ||
| revert(0, 64) |
There was a problem hiding this comment.
If we are making a big refactoring now, would you consider extracting this entire unchecked { ... } block to some _verifyEnoughGasForInner function? This is a very distracting code.
| uint256 postOpGasUsed = postOpPreGas - gasleft(); | ||
| postOpUnusedGasPenalty = _getUnusedGasPenalty(postOpGasUsed, mUserOp.paymasterPostOpGasLimit); | ||
| actualGas = executionGas + _getUnusedGasPenalty(executionGas, mUserOp.callGasLimit) + opInfo.preOpGas; | ||
| if (context.length > 0) { |
There was a problem hiding this comment.
Also for the entire if (context.length > 0) { ... } block - it would probably cost nothing to make it an internal function.
contracts/core/EntryPoint.sol
Outdated
| if (context.length > 0) { | ||
| uint256 postOpPreGas = gasleft(); | ||
| uint256 actualGasCostForPostOp = actualGas * gasPrice; | ||
| uint256 paymasterPostOpGasLimit = mUserOp.paymasterPostOpGasLimit; |
There was a problem hiding this comment.
Why create a variable if mUserOp is in memory anyways? MLOAD costs 3 gas, no?
There was a problem hiding this comment.
removed.
mUserOp is "double-reference", and in the past, it was better to copy a struct member to a local (stack) one.
but apparently, the compiler now behaves better if we don't.
(btw: its not good optimization of the compiler: it should be agnostic to the way we read again the same memory var)
contracts/core/EntryPoint.sol
Outdated
| uint256 postOpPreGas = gasleft(); | ||
| uint256 actualGasCostForPostOp = actualGas * gasPrice; | ||
| uint256 paymasterPostOpGasLimit = mUserOp.paymasterPostOpGasLimit; | ||
| // todo: no real need to check if paymaster has code. we already called validatePaymasdterUserOp |
There was a problem hiding this comment.
Now is not a time to add new TODOs. Either go full assembly (not recommened) or ignore the unnecessary "has code" checks you get from solidity (recommended).
contracts/core/EntryPoint.sol
Outdated
| // todo: no real need to check if paymaster has code. we already called validatePaymasdterUserOp | ||
| try IPaymaster(paymaster).postOp{ | ||
| gas: paymasterPostOpGasLimit | ||
| }(mode, context, actualGasCostForPostOp, gasPrice) |
There was a problem hiding this comment.
We can totally remove the postOpReverted "mode" from the codebase by the way.
note: most of the gas reduction are due to change of context to "calldata" from "memory", not from the method extract...
|
A remider to self to remove |
No description provided.