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

New TokenPaymaster #286

Merged
merged 25 commits into from
Jun 19, 2023
Merged

New TokenPaymaster #286

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
eabd560
Bring over PimlicoERC20Paymaster as new TokenPaymaster
forshtat May 18, 2023
e814e97
Pass local lint and original tests
forshtat May 18, 2023
70ce950
Fix bug in denominators calculation; streamline tests; use OZ SafeERC20
forshtat May 18, 2023
82bf877
Reuse 'updatePrice' logic and test 'TokenPriceUpdated'
forshtat May 19, 2023
d78b712
Extract oracle-specific logic to the OracleHelper library
forshtat May 19, 2023
664e65f
Bring over necessary UniswapHelper code - solidity compiles
forshtat May 20, 2023
e39e9a3
Some cleanup
forshtat May 20, 2023
23312c6
Deploy TokenPaymaster witout Create2 factory for the unit tests
forshtat May 20, 2023
06c130c
REPRO: out of gas in 'handleOps'
forshtat May 21, 2023
5e0cca2
Add 'TestUniswap' to silence errors; fix price-related errors
forshtat May 21, 2023
03efff5
Add token price override and test for it
forshtat May 21, 2023
5a4f340
WIP: Implemented TestUniswap stub logic and deposit to EntryPoint logic
forshtat May 22, 2023
dd896a0
Remove all 'console.log'
forshtat May 22, 2023
aae7c9a
Fix accidental change in EP
forshtat May 22, 2023
63cf750
TBD: charge the sender account in postOp if 'refund' is negative ('ov…
forshtat May 23, 2023
39a1e41
WIP: Check new price calculation function; use 1e26 as 'price denomin…
forshtat May 25, 2023
a047984
Use 'userOp.gasPrice' instead of 'maxFeePerGas' to calculate precharg…
forshtat Jun 4, 2023
6ac12fe
Avoid duplicate return from '_postOp'
forshtat Jun 4, 2023
c2c775d
Correction: pre-charge based on 'maxFeePerGas', refund based on 'gasP…
forshtat Jun 4, 2023
41ee2ec
Add support for legacy chains (avoid 'basefee' opcode)
forshtat Jun 4, 2023
28916ad
Switch from 'abi.encodePacked' to 'abi.encode' for better readability
forshtat Jun 16, 2023
821ada7
Make 'refundPostopCost' a part of 'TokenPaymasterConfig'
forshtat Jun 16, 2023
d461522
Return price invalidation time to prevent ops using against outdated …
forshtat Jun 16, 2023
36633a5
Reorder lines of code to make the fast way out even more faster
forshtat Jun 16, 2023
eeaf42a
Address PR comments (see list)
forshtat Jun 18, 2023
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
8 changes: 1 addition & 7 deletions contracts/samples/TokenPaymaster.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,6 @@ contract TokenPaymaster is BasePaymaster, UniswapHelper, OracleHelper {
emit ConfigUpdated(_tokenPaymasterConfig);
}

function setOracleConfiguration(
OracleHelperConfig memory _oracleHelperConfig
) external onlyOwner {
_setOracleConfiguration(_oracleHelperConfig);
}

function setUniswapConfiguration(
UniswapHelperConfig memory _uniswapHelperConfig
) external onlyOwner {
Expand Down Expand Up @@ -198,7 +192,7 @@ contract TokenPaymaster is BasePaymaster, UniswapHelper, OracleHelper {
);
}

emit UserOperationSponsored(userOpSender, actualTokenNeeded, actualGasCost, cachedPrice);
emit UserOperationSponsored(userOpSender, actualTokenNeeded, actualGasCost, _cachedPrice);
refillEntryPointDeposit(_cachedPrice);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

}
}
Expand Down
2 changes: 1 addition & 1 deletion contracts/samples/utils/OracleHelper.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ abstract contract OracleHelper {

function _setOracleConfiguration(
OracleHelperConfig memory _oracleHelperConfig
) internal {
) private {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

oracleHelperConfig = _oracleHelperConfig;
require(_oracleHelperConfig.priceUpdateThreshold <= 1e6, "TPM: update threshold too high");
tokenOracleDecimalPower = 10 ** oracleHelperConfig.tokenOracle.decimals();
Expand Down
86 changes: 60 additions & 26 deletions test/samples/OracleHelper.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { ethers } from 'hardhat'
import { AddressZero } from '../testutils'

import {
TestERC20,
TestERC20__factory,
TestOracle2,
TestOracle2__factory,
Expand Down Expand Up @@ -55,7 +56,7 @@ const sampleResponses = {
}

// note: direct or reverse designations are quite arbitrary
describe.only('OracleHelper', function () {
describe('OracleHelper', function () {
function testOracleFiguredPriceOut (): void {
it('should figure out the correct price', async function () {
await testEnv.paymaster.updateCachedPrice(true)
Expand Down Expand Up @@ -87,8 +88,12 @@ describe.only('OracleHelper', function () {
}

interface TestEnv {
owner: string
expectedPrice: string
expectedTokensPerEtherCalculated: string
tokenPaymasterConfig: TokenPaymaster.TokenPaymasterConfigStruct
uniswapHelperConfig: UniswapHelperNamespace.UniswapHelperConfigStruct
token: TestERC20
paymaster: TokenPaymaster
tokenOracle: TestOracle2
nativeAssetOracle: TestOracle2
Expand All @@ -99,15 +104,15 @@ describe.only('OracleHelper', function () {

before(async function () {
const ethersSigner = ethers.provider.getSigner()
const owner = await ethersSigner.getAddress()
testEnv.owner = await ethersSigner.getAddress()

const tokenPaymasterConfig: TokenPaymaster.TokenPaymasterConfigStruct = {
testEnv.tokenPaymasterConfig = {
priceMaxAge: 86400,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

refundPostopCost: 40000,
minEntryPointBalance: 0,
priceMarkup: priceDenominator.mul(19).div(10) // 190%
}
const uniswapHelperConfig: UniswapHelperNamespace.UniswapHelperConfigStruct = {
testEnv.uniswapHelperConfig = {
minSwapAmount: 1,
slippage: 5,
uniswapPoolFee: 3
Expand All @@ -117,21 +122,21 @@ describe.only('OracleHelper', function () {
testEnv.tokenOracle = await new TestOracle2__factory(ethersSigner).deploy(1, 0)
testEnv.nativeAssetOracle = await new TestOracle2__factory(ethersSigner).deploy(1, 0)

const token = await new TestERC20__factory(ethersSigner).deploy(18)
testEnv.token = await new TestERC20__factory(ethersSigner).deploy(18)

testEnv.paymaster = await new TokenPaymaster__factory(ethersSigner).deploy(
token.address,
testEnv.token.address,
AddressZero,
AddressZero,
owner, // cannot approve to AddressZero
tokenPaymasterConfig,
testEnv.owner, // cannot approve to AddressZero
testEnv.tokenPaymasterConfig,
getOracleConfig({
nativeOracleReverse: false,
tokenOracleReverse: false,
tokenToNativeOracle: false
}),
uniswapHelperConfig,
owner
testEnv.uniswapHelperConfig,
testEnv.owner
)
})

Expand All @@ -156,11 +161,21 @@ describe.only('OracleHelper', function () {
.div(res.answer)
.toString()

await testEnv.paymaster.setOracleConfiguration(getOracleConfig({
tokenToNativeOracle: true,
tokenOracleReverse: false,
nativeOracleReverse: false
}))
const ethersSigner = ethers.provider.getSigner()
testEnv.paymaster = await new TokenPaymaster__factory(ethersSigner).deploy(
testEnv.token.address,
AddressZero,
AddressZero,
testEnv.owner, // cannot approve to AddressZero
testEnv.tokenPaymasterConfig,
getOracleConfig({
tokenToNativeOracle: true,
tokenOracleReverse: false,
nativeOracleReverse: false
}),
testEnv.uniswapHelperConfig,
testEnv.owner
)
})

testOracleFiguredPriceOut()
Expand Down Expand Up @@ -197,11 +212,21 @@ describe.only('OracleHelper', function () {
// sanity check for the price calculation - use direct price and cached-like reverse price
assert.equal(expectedTokensPerEtherCalculated.toString(), testEnv.expectedTokensPerEtherCalculated.toString())

await testEnv.paymaster.setOracleConfiguration(getOracleConfig({
tokenToNativeOracle: true,
tokenOracleReverse: true,
nativeOracleReverse: false
}))
const ethersSigner = ethers.provider.getSigner()
testEnv.paymaster = await new TokenPaymaster__factory(ethersSigner).deploy(
testEnv.token.address,
AddressZero,
AddressZero,
testEnv.owner, // cannot approve to AddressZero
testEnv.tokenPaymasterConfig,
getOracleConfig({
tokenToNativeOracle: true,
tokenOracleReverse: true,
nativeOracleReverse: false
}),
testEnv.uniswapHelperConfig,
testEnv.owner
)
})
testOracleFiguredPriceOut()
})
Expand All @@ -217,12 +242,21 @@ describe.only('OracleHelper', function () {
await testEnv.nativeAssetOracle.setPrice(resNative.answer) // $1,817.65
await testEnv.nativeAssetOracle.setDecimals(resNative.decimals)

await testEnv.paymaster.setOracleConfiguration(getOracleConfig({
tokenToNativeOracle: false,
tokenOracleReverse: false,
nativeOracleReverse: false
}))

const ethersSigner = ethers.provider.getSigner()
testEnv.paymaster = await new TokenPaymaster__factory(ethersSigner).deploy(
testEnv.token.address,
AddressZero,
AddressZero,
testEnv.owner, // cannot approve to AddressZero
testEnv.tokenPaymasterConfig,
getOracleConfig({
tokenToNativeOracle: false,
tokenOracleReverse: false,
nativeOracleReverse: false
}),
testEnv.uniswapHelperConfig,
testEnv.owner
)
// note: oracle decimals are same and cancel each other out
testEnv.expectedPrice =
priceDenominator
Expand Down
2 changes: 1 addition & 1 deletion test/samples/TokenPaymaster.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ function generatePaymasterAndData (pm: string, tokenPrice?: BigNumberish): strin

const priceDenominator = BigNumber.from(10).pow(26)

describe.only('TokenPaymaster', function () {
describe('TokenPaymaster', function () {
const minEntryPointBalance = 1e17.toString()
const initialPriceToken = 100000000 // USD per TOK
const initialPriceEther = 500000000 // USD per ETH
Expand Down