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

add postOp param (gasPrice) #395

Merged
merged 4 commits into from
Jan 9, 2024
Merged

add postOp param (gasPrice) #395

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1a18ece
add postOp param (gasPrice)
drortirosh Jan 6, 2024
49e9917
update erc
drortirosh Jan 8, 2024
b7902f0
PR review rename field
drortirosh Jan 8, 2024
8faf84a
Merge branch 'develop' into postOp-gas-price
drortirosh Jan 8, 2024
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
10 changes: 6 additions & 4 deletions contracts/core/BasePaymaster.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,11 @@ abstract contract BasePaymaster is IPaymaster, Ownable {
function postOp(
PostOpMode mode,
bytes calldata context,
uint256 actualGasCost
uint256 actualGasCost,
uint gasPrice
Copy link
Collaborator

Choose a reason for hiding this comment

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

This name is highly confusing. The postOp method can actually call tx.gasprice and developers will be confused with the fact they are not the same thing. We should name it actualUserOpFeePerGas or something like that, so that the name "screams" that this is not tx.gasprice.
Also, please, use uint256 for both params - makes no sense to use two different ways in one line.

) external override {
_requireFromEntryPoint();
_postOp(mode, context, actualGasCost);
_postOp(mode, context, actualGasCost, gasPrice);
}

/**
Expand All @@ -76,9 +77,10 @@ abstract contract BasePaymaster is IPaymaster, Ownable {
function _postOp(
PostOpMode mode,
bytes calldata context,
uint256 actualGasCost
uint256 actualGasCost,
uint gasPrice
) internal virtual {
(mode, context, actualGasCost); // unused params
(mode, context, actualGasCost, gasPrice); // unused params
// subclass must override this method if validatePaymasterUserOp returns a context
revert("must override");
}
Expand Down
2 changes: 1 addition & 1 deletion contracts/core/EntryPoint.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,7 +677,7 @@ contract EntryPoint is IEntryPoint, StakeManager, NonceManager, ReentrancyGuard,
if (mode != IPaymaster.PostOpMode.postOpReverted) {
try IPaymaster(paymaster).postOp{
gas: mUserOp.verificationGasLimit
}(mode, context, actualGasCost)
}(mode, context, actualGasCost, gasPrice)
// solhint-disable-next-line no-empty-blocks
{} catch {
bytes memory reason = Exec.getReturnData(REVERT_REASON_MAX_LEN);
Expand Down
3 changes: 2 additions & 1 deletion contracts/interfaces/IPaymaster.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ interface IPaymaster {
function postOp(
PostOpMode mode,
bytes calldata context,
uint256 actualGasCost
uint256 actualGasCost,
uint gasPrice
) external;
}
4 changes: 2 additions & 2 deletions contracts/samples/LegacyTokenPaymaster.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,11 @@ contract LegacyTokenPaymaster is BasePaymaster, ERC20 {
* the user's TX , back to the state it was before the transaction started (before the validatePaymasterUserOp),
* and the transaction should succeed there.
*/
function _postOp(PostOpMode mode, bytes calldata context, uint256 actualGasCost) internal override {
function _postOp(PostOpMode mode, bytes calldata context, uint256 actualGasCost, uint gasPrice) internal override {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated - why do we keep this old code around? Can we remove it?

//we don't really care about the mode, we just pay the gas with the user's tokens.
(mode);
address sender = abi.decode(context, (address));
uint256 charge = getTokenValueOfEth(actualGasCost + COST_OF_POST);
uint256 charge = getTokenValueOfEth(actualGasCost + COST_OF_POST * gasPrice);
//actualGasCost is known to be no larger than the above requiredPreFund, so the transfer should succeed.
_transfer(sender, address(this), charge);
}
Expand Down
9 changes: 3 additions & 6 deletions contracts/samples/TokenPaymaster.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ contract TokenPaymaster is BasePaymaster, UniswapHelper, OracleHelper {
}
uint256 tokenAmount = weiToToken(preChargeNative, cachedPriceWithMarkup);
SafeERC20.safeTransferFrom(token, userOp.sender, address(this), tokenAmount);
context = abi.encode(tokenAmount, userOp.maxFeePerGas, userOp.maxPriorityFeePerGas, userOp.sender);
context = abi.encode(tokenAmount, userOp.sender);
validationResult = _packValidationData(
false,
uint48(cachedPriceTimestamp + tokenPaymasterConfig.priceMaxAge),
Expand All @@ -149,16 +149,13 @@ contract TokenPaymaster is BasePaymaster, UniswapHelper, OracleHelper {
/// @dev This function is called after a user operation has been executed or reverted.
/// @param context The context containing the token amount and user sender address.
/// @param actualGasCost The actual gas cost of the transaction.
function _postOp(PostOpMode, bytes calldata context, uint256 actualGasCost) internal override {
function _postOp(PostOpMode, bytes calldata context, uint256 actualGasCost, uint gasPrice) internal override {
unchecked {
uint256 priceMarkup = tokenPaymasterConfig.priceMarkup;
(
uint256 preCharge,
uint256 maxFeePerGas,
uint256 maxPriorityFeePerGas,
address userOpSender
) = abi.decode(context, (uint256, uint256, uint256, address));
uint256 gasPrice = getGasPrice(maxFeePerGas, maxPriorityFeePerGas);
) = abi.decode(context, (uint256, address));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This means the entire getGasPrice function is no longer needed - please remove.

uint256 _cachedPrice = updateCachedPrice(false);
// 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;
Expand Down
2 changes: 1 addition & 1 deletion contracts/test/TestPaymasterRevertCustomError.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ contract TestPaymasterRevertCustomError is BasePaymaster {
revertType = _revertType;
}

function _postOp(PostOpMode, bytes calldata, uint256) internal view override {
function _postOp(PostOpMode, bytes calldata, uint256, uint256) internal view override {
if (revertType == RevertType.customError){
revert CustomError("this is a long revert reason string we are looking for");
}
Expand Down
3 changes: 2 additions & 1 deletion contracts/test/TestPaymasterWithPostOp.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ contract TestPaymasterWithPostOp is TestPaymasterAcceptAll {
function _postOp(
PostOpMode mode,
bytes calldata context,
uint256 actualGasCost
uint256 actualGasCost,
uint gasPrice
) internal override {
}
}
26 changes: 13 additions & 13 deletions reports/gas-checker.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,17 @@
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple │ 1 │ 81570 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple - diff from previous │ 2 │ │ 43842 │ 14863 ║
║ simple - diff from previous │ 2 │ │ 43830 │ 14851 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple │ 10 │ 476227 │ │ ║
║ simple │ 10 │ 476275 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple - diff from previous │ 11 │ │ 43873 │ 14894 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster │ 1 │ 87750 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster with diff │ 2 │ │ 42716 │ 13737 ║
║ simple paymaster with diff │ 2 │ │ 42728 │ 13749 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster │ 10 │ 472423 │ │ ║
║ simple paymaster │ 10 │ 472435 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ simple paymaster with diff │ 11 │ │ 42781 │ 13802 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
Expand All @@ -34,22 +34,22 @@
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ big tx 5k │ 10 │ 1481819 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ big tx - diff from previous │ 11 │ │ 144445 │ 19221 ║
║ big tx - diff from previous │ 11 │ │ 144421 │ 19197 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ paymaster+postOp │ 1 │ 89483 │ │ ║
║ paymaster+postOp │ 1 │ 89507 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ paymaster+postOp with diff │ 2 │ │ 44463 │ 15484 ║
║ paymaster+postOp with diff │ 2 │ │ 44487 │ 15508 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ paymaster+postOp │ 10 │ 489783 │ │ ║
║ paymaster+postOp │ 10 │ 490047 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ paymaster+postOp with diff │ 11 │ │ 44508 │ 15529 ║
║ paymaster+postOp with diff │ 11 │ │ 44532 │ 15553 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ token paymaster │ 1 │ 147976 │ │ ║
║ token paymaster │ 1 │ 147245 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ token paymaster with diff │ 2 │ │ 72659 │ 43680 ║
║ token paymaster with diff │ 2 │ │ 71921 │ 42942 ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ token paymaster │ 10 │ 802242 │ │ ║
║ token paymaster │ 10 │ 794781 │ │ ║
╟────────────────────────────────┼───────┼───────────────┼────────────────┼─────────────────────╢
║ token paymaster with diff │ 11 │ │ 72727 │ 43748 ║
║ token paymaster with diff │ 11 │ │ 71953 │ 42974 ║
╚════════════════════════════════╧═══════╧═══════════════╧════════════════╧═════════════════════╝

Loading