-
Notifications
You must be signed in to change notification settings - Fork 627
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-216 Proposal: Packing UserOperation and paymaster gas limits #363
Changes from 10 commits
3181436
1e2fe50
bef7e4d
41bad54
f27b2d6
287fe60
b2a9e61
46fb3ba
7494872
0cc1d67
6ffd042
56a5710
2e3ceff
d95187b
415ef69
ffdb104
7b6949a
47a57fa
a54d10b
08ef38e
052a8ad
87a8eac
277c379
effa64f
8ff7490
24cf39e
b9504f4
6252387
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 |
---|---|---|
|
@@ -225,8 +225,10 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, | |
struct MemoryUserOp { | ||
address sender; | ||
uint256 nonce; | ||
uint256 callGasLimit; | ||
uint256 verificationGasLimit; | ||
uint128 verificationGasLimit; | ||
uint128 callGasLimit; | ||
uint128 paymasterVerificationGasLimit; | ||
uint128 paymasterPostOpGasLimit; | ||
uint256 preVerificationGas; | ||
address paymaster; | ||
uint256 maxFeePerGas; | ||
|
@@ -261,7 +263,10 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, | |
unchecked { | ||
// handleOps was called with gas limit too low. abort entire bundle. | ||
if ( | ||
gasleft() < callGasLimit + mUserOp.verificationGasLimit + 5000 | ||
gasleft() < | ||
callGasLimit + | ||
mUserOp.paymasterPostOpGasLimit + | ||
5000 | ||
) { | ||
assembly { | ||
mstore(0, INNER_OUT_OF_GAS) | ||
|
@@ -313,8 +318,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, | |
) internal pure { | ||
mUserOp.sender = userOp.sender; | ||
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. Why not have the entire 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. This copy is an internal optimization of EntryPoint. we might even remove copying some fields if we find they are not needed (e.g. used exactly once) |
||
mUserOp.nonce = userOp.nonce; | ||
mUserOp.callGasLimit = userOp.callGasLimit; | ||
mUserOp.verificationGasLimit = userOp.verificationGasLimit; | ||
(mUserOp.verificationGasLimit, mUserOp.callGasLimit) = UserOperationLib.unpackAccountGasLimits(userOp.accountGasLimits); | ||
mUserOp.preVerificationGas = userOp.preVerificationGas; | ||
mUserOp.maxFeePerGas = userOp.maxFeePerGas; | ||
mUserOp.maxPriorityFeePerGas = userOp.maxPriorityFeePerGas; | ||
|
@@ -325,8 +329,12 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, | |
"AA93 invalid paymasterAndData" | ||
); | ||
mUserOp.paymaster = address(bytes20(paymasterAndData[:20])); | ||
mUserOp.paymasterVerificationGasLimit = uint128(bytes16(paymasterAndData[20:36])); | ||
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.
|
||
mUserOp.paymasterPostOpGasLimit = uint128(bytes16(paymasterAndData[36:52])); | ||
} else { | ||
mUserOp.paymaster = address(0); | ||
mUserOp.paymasterVerificationGasLimit = 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. Solidity structs are pre-initialized to all-zeros. 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. Except when one does crazy stuff like recycling memory by using the free-pointer back - which we do :) So we can't assume zeros anywhere. 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. Yes, we can assume zeros 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 paymaster initialization to address(0) was there, I only added the other fields. Do you want to remove all of them? 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.
Interesting, I wonder why they do that. Solidity always allocates new memory (containing zeros) so why do they need to explicitly write zeros? (We do, due to our asm code, but normal solidity doesn't) |
||
mUserOp.paymasterPostOpGasLimit = 0; | ||
} | ||
} | ||
|
||
|
@@ -338,12 +346,10 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, | |
MemoryUserOp memory mUserOp | ||
) internal pure returns (uint256 requiredPrefund) { | ||
unchecked { | ||
// When using a Paymaster, the verificationGasLimit is used also to as a limit for the postOp call. | ||
// Our security model might call postOp eventually twice. | ||
uint256 mul = mUserOp.paymaster != address(0) ? 2 : 1; | ||
uint256 requiredGas = mUserOp.callGasLimit + | ||
mUserOp.verificationGasLimit * | ||
mul + | ||
uint256 requiredGas = mUserOp.verificationGasLimit + | ||
mUserOp.callGasLimit + | ||
mUserOp.paymasterVerificationGasLimit + | ||
mUserOp.paymasterPostOpGasLimit + | ||
mUserOp.preVerificationGas; | ||
|
||
requiredPrefund = requiredGas * mUserOp.maxFeePerGas; | ||
|
@@ -472,13 +478,12 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, | |
) internal returns (bytes memory context, uint256 validationData) { | ||
unchecked { | ||
MemoryUserOp memory mUserOp = opInfo.mUserOp; | ||
// TODO: Do we actually need that still? | ||
uint256 verificationGasLimit = mUserOp.verificationGasLimit; | ||
shahafn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
require( | ||
verificationGasLimit > gasUsedByValidateAccountPrepayment, | ||
"AA41 too little verificationGas" | ||
); | ||
uint256 gas = verificationGasLimit - | ||
gasUsedByValidateAccountPrepayment; | ||
|
||
address paymaster = mUserOp.paymaster; | ||
DepositInfo storage paymasterInfo = deposits[paymaster]; | ||
|
@@ -488,7 +493,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, | |
} | ||
paymasterInfo.deposit = uint112(deposit - requiredPreFund); | ||
try | ||
IPaymaster(paymaster).validatePaymasterUserOp{gas: gas}( | ||
IPaymaster(paymaster).validatePaymasterUserOp{gas: mUserOp.paymasterVerificationGasLimit}( | ||
shahafn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
op, | ||
opInfo.userOpHash, | ||
requiredPreFund | ||
|
@@ -584,6 +589,8 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, | |
uint256 maxGasValues = mUserOp.preVerificationGas | | ||
mUserOp.verificationGasLimit | | ||
mUserOp.callGasLimit | | ||
mUserOp.paymasterVerificationGasLimit | | ||
mUserOp.paymasterPostOpGasLimit | | ||
userOp.maxFeePerGas | | ||
userOp.maxPriorityFeePerGas; | ||
require(maxGasValues <= type(uint120).max, "AA94 gas values overflow"); | ||
|
@@ -621,7 +628,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, | |
unchecked { | ||
uint256 gasUsed = preGas - gasleft(); | ||
|
||
if (userOp.verificationGasLimit < gasUsed) { | ||
if (mUserOp.verificationGasLimit + mUserOp.paymasterVerificationGasLimit < gasUsed) { | ||
revert FailedOp(opIndex, "AA40 over verificationGasLimit"); | ||
} | ||
outOpInfo.prefund = requiredPreFund; | ||
|
@@ -662,7 +669,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard, | |
actualGasCost = actualGas * gasPrice; | ||
if (mode != IPaymaster.PostOpMode.postOpReverted) { | ||
IPaymaster(paymaster).postOp{ | ||
gas: mUserOp.verificationGasLimit | ||
gas: mUserOp.paymasterPostOpGasLimit | ||
}(mode, context, actualGasCost); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,8 +55,7 @@ library UserOperationLib { | |
uint256 nonce = userOp.nonce; | ||
bytes32 hashInitCode = calldataKeccak(userOp.initCode); | ||
bytes32 hashCallData = calldataKeccak(userOp.callData); | ||
uint256 callGasLimit = userOp.callGasLimit; | ||
uint256 verificationGasLimit = userOp.verificationGasLimit; | ||
bytes32 accountGasLimits = userOp.accountGasLimits; | ||
uint256 preVerificationGas = userOp.preVerificationGas; | ||
uint256 maxFeePerGas = userOp.maxFeePerGas; | ||
uint256 maxPriorityFeePerGas = userOp.maxPriorityFeePerGas; | ||
|
@@ -65,12 +64,24 @@ library UserOperationLib { | |
return abi.encode( | ||
sender, nonce, | ||
hashInitCode, hashCallData, | ||
callGasLimit, verificationGasLimit, preVerificationGas, | ||
accountGasLimits, preVerificationGas, | ||
maxFeePerGas, maxPriorityFeePerGas, | ||
hashPaymasterAndData | ||
); | ||
} | ||
|
||
function unpackAccountGasLimits( | ||
bytes32 accountGasLimits | ||
) internal pure returns(uint128 validationGasLimit, uint128 callGasLimit) { | ||
return (uint128(bytes16(accountGasLimits)), uint128(uint256(accountGasLimits))); | ||
} | ||
|
||
function unpackPaymasterStaticFields( | ||
shahafn marked this conversation as resolved.
Show resolved
Hide resolved
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. I don't think we ever decode the paymaster itself, only paymasterGaslimits 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. We do decode it.. |
||
bytes calldata paymasterAndData | ||
) internal pure returns(address paymaster, uint128 validationGasLimit, uint128 postOp) { | ||
return (address(bytes20(paymasterAndData[:20])), uint128(bytes16(paymasterAndData[20:36])), uint128(bytes16(paymasterAndData[36:52]))); | ||
shahafn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/** | ||
* Hash the user operation data. | ||
* @param userOp - The user operation data. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,15 +123,15 @@ contract TokenPaymaster is BasePaymaster, UniswapHelper, OracleHelper { | |
override | ||
returns (bytes memory context, uint256 validationResult) {unchecked { | ||
uint256 priceMarkup = tokenPaymasterConfig.priceMarkup; | ||
uint256 paymasterAndDataLength = userOp.paymasterAndData.length - 20; | ||
uint256 paymasterAndDataLength = userOp.paymasterAndData.length - 52; | ||
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. we need to define these as constants, in UserOpLib. its not good to have so many explicit numerics spread in our code. 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. Done |
||
require(paymasterAndDataLength == 0 || paymasterAndDataLength == 32, | ||
"TPM: invalid data length" | ||
); | ||
uint256 preChargeNative = requiredPreFund + (tokenPaymasterConfig.refundPostopCost * userOp.maxFeePerGas); | ||
// note: as price is in ether-per-token and we want more tokens increasing it means dividing it by markup | ||
uint256 cachedPriceWithMarkup = cachedPrice * PRICE_DENOMINATOR / priceMarkup; | ||
if (paymasterAndDataLength == 32) { | ||
uint256 clientSuppliedPrice = uint256(bytes32(userOp.paymasterAndData[20 : 52])); | ||
uint256 clientSuppliedPrice = uint256(bytes32(userOp.paymasterAndData[52 : 84])); | ||
if (clientSuppliedPrice < cachedPriceWithMarkup) { | ||
// note: smaller number means 'more ether per token' | ||
cachedPriceWithMarkup = clientSuppliedPrice; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,9 +23,9 @@ contract VerifyingPaymaster is BasePaymaster { | |
|
||
address public immutable verifyingSigner; | ||
|
||
uint256 private constant VALID_TIMESTAMP_OFFSET = 20; | ||
uint256 private constant VALID_TIMESTAMP_OFFSET = 52; | ||
shahafn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
uint256 private constant SIGNATURE_OFFSET = 84; | ||
uint256 private constant SIGNATURE_OFFSET = 116; | ||
|
||
constructor(IEntryPoint _entryPoint, address _verifyingSigner) BasePaymaster(_entryPoint) { | ||
verifyingSigner = _verifyingSigner; | ||
|
@@ -50,8 +50,8 @@ contract VerifyingPaymaster is BasePaymaster { | |
userOp.nonce, | ||
keccak256(userOp.initCode), | ||
keccak256(userOp.callData), | ||
userOp.callGasLimit, | ||
userOp.verificationGasLimit, | ||
userOp.accountGasLimits, | ||
uint256(bytes32(userOp.paymasterAndData[20:52])), | ||
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. afaik, bytes32 is enough ( |
||
userOp.preVerificationGas, | ||
userOp.maxFeePerGas, | ||
userOp.maxPriorityFeePerGas, | ||
|
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.
At this point we already have a
context
, so if it is empty thepostOp
will not be called. I think it may make sense to allow the transaction to go on in this case, instead of always addingpaymasterPostOpGasLimit
to the required gas left?Say, the UserOp have requested 1'000'000 gas for a
postOp
but it does not need it any more, why should this check fail?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.
why should we (entrypoint) waste gas to check it? it is the paymaster's job to put a valid postOpGasLimit, even if context is empty.
There is another edge case:
validatePaymasterUserOp() returns a
context, and the
postOpGasLimit` is zero: in this case, we still execute the callData and revert the postOp (on OOG) - again, because it is not the EntryPoint's job to validate the userop fields.