New TokenPaymaster #286

forshtat · 2023-05-18T14:35:06Z

No description provided.

huaweigu · 2023-05-22T19:22:04Z

contracts/samples/TokenPaymaster.sol

+                }
+            }
+            uint256 tokenAmount = weiToToken(preChargeNative, cachedPriceWithMarkup, false);
+            SafeERC20.safeTransferFrom(token, userOp.sender, address(this), tokenAmount);


minor, IIRC from our past debug experience, SafeERC20 uses banned selfbalance opcode

Thanks, I did not know that! Created a small PR in the OpenZeppelin repo, hope this is all there is to this issue:
OpenZeppelin/openzeppelin-contracts#4271

I hope to avoid having to copy SafeERC20.sol library into the account-abstraction repo, but maybe we will have no choice.

maybe we can use solady in that case https://github.com/Vectorized/solady/blob/f9a6cc652dc8b37387590588a78644abfa4be3ac/src/utils/SafeTransferLib.sol#L238

huaweigu · 2023-05-22T19:28:38Z

contracts/samples/TokenPaymaster.sol

+                );
+            } else {
+                // If the token amount is not greater than the actual amount needed, revert to remove incentive to cheat
+                revert("TPM: preCharge was too low");


I wonder if it's worth of attempting to transfer the owed token (preCharge - actualTokenNeeded) from sender here

I think this may actually work, will bring this up with the team. So far I did add an attempt to withdraw the owed amount (actualTokenNeeded - preCharge) from the UserOp sender to the Paymaster.

huaweigu · 2023-05-22T19:47:16Z

contracts/samples/TokenPaymaster.sol

+    function setTokenPaymasterConfig(
+        TokenPaymasterConfig memory _tokenPaymasterConfig
+    ) public onlyOwner {
+        require(_tokenPaymasterConfig.priceMarkup <= 1900000, "TPM: price markup too high");


Out of curiosity, how do we pick the upper bound (1900000)?

None of these hard-coded parameters are selected yet, but in my opinion the maximum possible value for the markup should just be there to prevent human error or malicious configuration.

For example, Pimlico Paymaster used 120e4 which sounds to me like it may be a bit too limiting:
https://github.com/pimlicolabs/erc20-paymaster-contracts/blob/c9911402a49928398441b07fb9caa9f4b4d0352f/src/PimlicoERC20Paymaster.sol#L68

maybe 2e6 is good upper limit

@livingrockrises Agreed. Will set for 200% for now. Will also make it explicitly related to the PRICE_DENOMINATOR value.

contracts/samples/TokenPaymaster.sol

…erdraft')

livingrockrises · 2023-05-25T04:29:43Z

contracts/samples/TokenPaymaster.sol

+            }
+
+            emit UserOperationSponsored(userOpSender, actualTokenNeeded, actualGasCost, cachedPrice);
+            refillEntryPointDeposit(_cachedPrice);


Does this mean router swapping cost is dumped on to the users?
since it is conditional postOp cost would change
if not user who is paying for this computation - paymaster's gas deposit?

A single user should not get stuck with the bill for the Paymaster's swap operation on its own.
I am not sure but I think what we should do is to set the POSTOP_COST parameter such that it kind of spreads the cost of these periodic swaps over all other transactions.
Of course this way it may end up accumulating some loss or profit over time if left completely unattended.

If paymaster is always charging premium, POSTOP_COST can just be everything unaccounted on-chain on entry point and postOp - swap executionGas.

@livingrockrises
A paymaster is expected to set refundPostopCost to actual cost of postOp, with faction of the cost of the uniswap, since the swap is expected to happen only every "N" transaction.

…ator'

leekt · 2023-05-30T17:21:38Z

contracts/samples/TokenPaymaster.sol

+            uint256 preCharge = uint256(bytes32(context[0 : 32]));
+            uint256 maxFeePerGas = uint256(bytes32(context[32 : 64]));
+            address userOpSender = address(bytes20(context[64 : 84]));
+            if (mode == PostOpMode.postOpReverted) {


This branch cannot be reached since there is a immediate return

Thanks. Fixed.

livingrockrises · 2023-06-01T06:49:56Z

contracts/samples/TokenPaymaster.sol

+        // 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;
+        // Refund tokens based on actual gas cost
+            uint256 actualChargeNative = actualGasCost + REFUND_POSTOP_COST * maxFeePerGas;


instead of using maxFeePerGas, does it make sense to use tx.gasprice ?

You are right, thanks! However, we cannot just use tx.gasprice as the bundler can manipulate this value. I think we should use userOp.gasPrice() here. Updated.

userop.gasPrice() touches BASEFEE which forbidden opcode. this would be fine in postOp(), but you shouldn't calculate it in validatePaymasterUserOp and pass it in context.
Instead pass maxPriorityFee and maxFee in context and calculate like below in the postOp

uint256 opGasPrice; unchecked { if (maxFeePerGas == maxPriorityFeePerGas) { opGasPrice = maxFeePerGas; } else { opGasPrice = Math.min(maxFeePerGas, maxPriorityFeePerGas + block.basefee); } }

livingrockrises · 2023-06-03T19:25:52Z

contracts/samples/TokenPaymaster.sol

+    event Received(address indexed sender, uint256 value);
+
+    /// @notice All 'price' variables are multiplied by this value to avoid rounding up
+    uint256 private constant PRICE_DENOMINATOR = 1e26;


what is the rationale behind using 1e26?

At this point constants are actually pretty random and will be finalised after we make some gas usage optimisations. Do you have suggestions on what value should be used and why?

…e/refund

livingrockrises · 2023-06-04T20:01:43Z

contracts/samples/TokenPaymaster.sol

+            require(paymasterAndDataLength == 0 || paymasterAndDataLength == 32,
+                "TPM: invalid data length"
+            );
+            uint256 gasPrice = userOp.gasPrice();


this will revert on the bundler
cc @leekt

yup this will revert for sure

…rice' This is the behaviour of the EntryPoint contract as well.

leekt · 2023-06-06T11:30:08Z

contracts/samples/TokenPaymaster.sol

+                    preCharge - actualTokenNeeded
+                );
+            } else if (preCharge < actualTokenNeeded) {
+                // Attempt to cover Paymaster's gas expenses by withdrawing the 'overdraft' from the client


Not sure about this part. Won't this make clientSuppliedPrice invalid? clientSuppliedPrice should be the guard for how much token user is willing to pay but this part will end up forcing user to pay the full slippage

Well, yes, with this implementation the user-supplied price can only increase the pre-charge for the user, but does not set an upper limit of the token price, so once the users approve this paymaster they basically agree to pay for gas at any current market price.

@drortirosh what do you think about this feature?

CC: @huaweigu

drortirosh · 2023-06-06T17:02:17Z

contracts/samples/TokenPaymaster.sol

+    uint256 private constant PRICE_DENOMINATOR = 1e26;
+
+    /// @notice Estimated gas cost for refunding tokens after the transaction is completed
+    uint256 public constant REFUND_POSTOP_COST = 40000;


postop_cost depends on price of token transfer, which varies with different token implementation.
to be accurate, this is better be a constructor parameter

drortirosh · 2023-06-06T17:03:38Z

contracts/samples/TokenPaymaster.sol

    }

+    function setOracleConfiguration(


do we need setters? if the data is immutable, it the logic much cheaper.
There is no harm at re-deploy the paymaster with different configuration.

This paymaster will accumulate client base by getting 'approval' in ERC-20 token used. Deploying a new paymaster just to switch to a different price oracle will lose all existing clients until they migrate their approval.
Do you want me to make the fields immutable and make the paymaster an upgradeable proxy instead?

@drortirosh
Bumping this one. Should we?

As a user, I trust the config values the paymaster have - but I really can't fully trust it, since it can change its configuration at will (e.g. increase the postOp cost, or switch to a malicious oracle)

I want to trust an address given by some authority, and know this address can't change.
Better yet - I want to know this address define all parameters values.

Also other parameters (like minEPbalance) are not expected to be modified much.

at Biconomy we are returning a fee quote to the sender and allow them to update userop.calldata with the approval transaction with necessary amount each time. then partialUserOp is filled with paymasterAndData and signature. SDK account APIs can facilitate this for decoding calldata and append approval transaction in a batch and have new callData, callGasLimit

drortirosh · 2023-06-06T17:06:32Z

contracts/samples/TokenPaymaster.sol

+            uint256 tokenAmount = weiToToken(preChargeNative, cachedPriceWithMarkup);
+            SafeERC20.safeTransferFrom(token, userOp.sender, address(this), tokenAmount);
+            context = abi.encodePacked(tokenAmount, userOp.maxFeePerGas, userOp.maxPriorityFeePerGas, userOp.sender);
+            validationResult = 0;


Better return validUntil as cachedPriceTimestamp + maxCacheAge

drortirosh · 2023-06-06T17:08:09Z

contracts/samples/TokenPaymaster.sol

+    function _postOp(PostOpMode mode, bytes calldata context, uint256 actualGasCost) internal override {
+        unchecked {
+            uint256 priceMarkup = tokenPaymasterConfig.priceMarkup;
+            uint256 preCharge = uint256(bytes32(context[0 : 32]));


abi.decode() is that bad?
it surely much clearer to understand.

Fixed. I was trying to same some bytes by doing abi.encodePacked.

drortirosh · 2023-06-06T17:15:59Z

contracts/samples/utils/OracleHelper.sol

+        IOracle nativeOracle = oracleHelperConfig.nativeOracle;
+
+        uint256 cacheAge = block.timestamp - cachedPriceTimestamp;
+        if (!force && cacheAge <= cacheTimeToLive) {


better be moved up, and avoid loading storage fields before the quick bail-out.

…price

drortirosh

Should also add gas-check test in gas-calc/

drortirosh · 2023-06-18T13:02:22Z

contracts/samples/TokenPaymaster.sol

+            context = abi.encode(tokenAmount, userOp.maxFeePerGas, userOp.maxPriorityFeePerGas, userOp.sender);
+            validationResult = _packValidationData(
+                false,
+                uint48(cachedPriceTimestamp + tokenPaymasterConfig.priceMaxAge),


storage optimization: instead referencing 2 storage slots on each call, use
cachedPriceExpirationTimestamp = timestamp + priceMaxAge

Please elaborate where does the timestamp variable come from?

drortirosh · 2023-06-18T13:06:43Z

test/samples/OracleHelper.test.ts

@@ -102,6 +102,8 @@ describe.only('OracleHelper', function () {
    const owner = await ethersSigner.getAddress()

    const tokenPaymasterConfig: TokenPaymaster.TokenPaymasterConfigStruct = {
+      priceMaxAge: 86400,


^^ remove "only" from describe.only('OracleHelper')

drortirosh · 2023-06-18T13:08:37Z

contracts/index.ts

@@ -1,2 +0,0 @@
-export { UserOperationStruct } from './types/EntryPoint'


true, should be removed (along with the code to use it during "yarn publish"), but unrelated to TokenPaymaster.

This was messing up the environment for me though.

drortirosh · 2023-06-18T13:14:33Z

contracts/test/TestUniswap.sol

+
+    /// @notice Simplified code copied from here:
+    /// https://github.com/Uniswap/v3-periphery/blob/main/contracts/base/PeripheryPayments.sol#L19
+    function unwrapWETH9(uint256 amountMinimum, address recipient) public payable {


Why not? This is used by the UniswapHelper.sol file:

function unwrapWeth(uint256 amount) internal { IPeripheryPayments(address(uniswap)).unwrapWETH9(amount, address(this)); }

drortirosh · 2023-06-18T13:23:29Z

contracts/samples/TokenPaymaster.sol

+                );
+            }
+
+            emit UserOperationSponsored(userOpSender, actualTokenNeeded, actualGasCost, cachedPrice);


I think this is _cachedPrice, not cachedPrice

drortirosh · 2023-06-18T13:27:03Z

contracts/samples/TokenPaymaster.sol

+            }
+
+            emit UserOperationSponsored(userOpSender, actualTokenNeeded, actualGasCost, cachedPrice);
+            refillEntryPointDeposit(_cachedPrice);


@livingrockrises
A paymaster is expected to set refundPostopCost to actual cost of postOp, with faction of the cost of the uniswap, since the swap is expected to happen only every "N" transaction.

drortirosh · 2023-06-18T13:32:36Z

contracts/samples/TokenPaymaster.sol

    }

+    function setOracleConfiguration(


As a user, I trust the config values the paymaster have - but I really can't fully trust it, since it can change its configuration at will (e.g. increase the postOp cost, or switch to a malicious oracle)

I want to trust an address given by some authority, and know this address can't change.
Better yet - I want to know this address define all parameters values.

Also other parameters (like minEPbalance) are not expected to be modified much.

1. Remove 'setOracleConfiguration' - only constructor can set oracles 2. Use '_cachedPrice' instead of 'cachedPrice' to avoid extra state read 3. Remove 'only' from TokenPaymaster related tests

drortirosh · 2023-06-19T11:55:17Z

contracts/samples/utils/OracleHelper.sol

@@ -67,7 +67,7 @@ abstract contract OracleHelper {

    function _setOracleConfiguration(
        OracleHelperConfig memory _oracleHelperConfig
-    ) internal {
+    ) private {


leaving this method (called only from constructor) means that we still pay the SLOAD when using these parameters.
(and you can't make a member "immutable" if it is set from a private method)

Currently, the paymaster uses 15 slots (14, if we exclude "owner")
(checked with solc --storage-layout)
So it uses 14 storage slots on each operation, which is at minimum 14*2100 = 29400 gas .
But I suggest adding a "gascalc" test before optimizing it.

forshtat added 14 commits May 18, 2023 17:34

Bring over PimlicoERC20Paymaster as new TokenPaymaster

eabd560

Pass local lint and original tests

e814e97

Fix bug in denominators calculation; streamline tests; use OZ SafeERC20

70ce950

Reuse 'updatePrice' logic and test 'TokenPriceUpdated'

82bf877

Extract oracle-specific logic to the OracleHelper library

d78b712

Bring over necessary UniswapHelper code - solidity compiles

664e65f

Some cleanup

e39e9a3

Deploy TokenPaymaster witout Create2 factory for the unit tests

23312c6

REPRO: out of gas in 'handleOps'

06c130c

Add 'TestUniswap' to silence errors; fix price-related errors

5e0cca2

Add token price override and test for it

03efff5

WIP: Implemented TestUniswap stub logic and deposit to EntryPoint logic

5a4f340

Remove all 'console.log'

dd896a0

Fix accidental change in EP

aae7c9a

huaweigu reviewed May 22, 2023

View reviewed changes

forshtat mentioned this pull request May 23, 2023

Avoid accessing SELFBALANCE opcode for zero-value function calls OpenZeppelin/openzeppelin-contracts#4271

Closed

3 tasks

TBD: charge the sender account in postOp if 'refund' is negative ('ov…

63cf750

…erdraft')

livingrockrises reviewed May 25, 2023

View reviewed changes

WIP: Check new price calculation function; use 1e26 as 'price denomin…

39a1e41

…ator'

leekt reviewed May 30, 2023

View reviewed changes

livingrockrises reviewed Jun 1, 2023

View reviewed changes

livingrockrises reviewed Jun 3, 2023

View reviewed changes

forshtat added 2 commits June 4, 2023 21:54

Use 'userOp.gasPrice' instead of 'maxFeePerGas' to calculate precharg…

a047984

…e/refund

Avoid duplicate return from '_postOp'

6ac12fe

livingrockrises reviewed Jun 4, 2023

View reviewed changes

forshtat added 2 commits June 5, 2023 00:58

Correction: pre-charge based on 'maxFeePerGas', refund based on 'gasP…

c2c775d

…rice' This is the behaviour of the EntryPoint contract as well.

Add support for legacy chains (avoid 'basefee' opcode)

41ee2ec

leekt reviewed Jun 6, 2023

View reviewed changes

drortirosh reviewed Jun 6, 2023

View reviewed changes

Switch from 'abi.encodePacked' to 'abi.encode' for better readability

28916ad

forshtat added 3 commits June 16, 2023 16:37

Make 'refundPostopCost' a part of 'TokenPaymasterConfig'

821ada7

Return price invalidation time to prevent ops using against outdated …

d461522

…price

Reorder lines of code to make the fast way out even more faster

36633a5

drortirosh reviewed Jun 18, 2023

View reviewed changes

Address PR comments (see list)

eeaf42a

1. Remove 'setOracleConfiguration' - only constructor can set oracles 2. Use '_cachedPrice' instead of 'cachedPrice' to avoid extra state read 3. Remove 'only' from TokenPaymaster related tests

drortirosh approved these changes Jun 19, 2023

View reviewed changes

forshtat merged commit 05691c0 into develop Jun 19, 2023

forshtat deleted the new_token_paymaster branch June 19, 2023 14:33

mmv08 mentioned this pull request Jan 25, 2024

Evaluate 4337 Module Changes For EntryPoint 0.7.0 safe-global/safe-modules#215

Closed

		@@ -1,2 +0,0 @@
		export { UserOperationStruct } from './types/EntryPoint'

New TokenPaymaster #286

New TokenPaymaster #286

Conversation

forshtat commented May 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forshtat May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forshtat Jun 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drortirosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forshtat May 23, 2023 •

edited

Loading

forshtat Jun 4, 2023 •

edited

Loading