Skip to content

Commit

Permalink
fix(fees): set tip on Celo from eth_maxPriorityFeePerGas (#4444)
Browse files Browse the repository at this point in the history 
### Description

We initially got the
[recommendation](https://valora-app.slack.com/archives/CNJ7KTHQU/p1697718182062359?thread_ts=1697647756.662059&cid=CNJ7KTHQU)
to only set `maxFeePerGas` to the result of `eth_gasPrice` and that the
difference between it and the base fee would be the tip distributed to
the validator.

However after testing, we noticed that if we leave
`maxPriorityFeePerGas` to `undefined` (or if we don't specify it), the
final serialized tx has is set to `0`.
Resulting to `0` tip being effectively distributed to the validator.
Which doesn't look right for validators 😄

So here we bring back the original implementation I was originally
[proposing](ec11ae7)
where we set it to the result of `eth_maxPriorityFeePerGas`. Similar to
what [viem does on
Ethereum](https://github.com/wagmi-dev/viem/blob/291e9ba2abc1d298d77e5a66f91f85bc54a3d31b/src/actions/public/estimateFeesPerGas.ts#L144-L162).

Note: we'll wait for the confirmation from cLabs before merging this
though.

### Test plan

- Updated tests

### Related issues

- N/A

### Backwards compatibility

Yes
  • Loading branch information
@jeanregisser
jeanregisser authored Nov 7, 2023
1 parent b06f1e3 commit b31dfd0
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 34 deletions.
6 changes: 4 additions & 2 deletions src/viem/estimateFeesPerGas.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,24 @@ describe(estimateFeesPerGas, () => {
request: jest.fn(async ({ method, params }) => {
expect(params).toBeUndefined()
if (method === 'eth_gasPrice') return '0x64' // 100 in hex
if (method === 'eth_maxPriorityFeePerGas') return '0xa' // 10 in hex
throw new Error(`Unknown method ${method}`)
}),
}
const fees = await estimateFeesPerGas(client as any)
expect(fees).toEqual({ maxFeePerGas: BigInt(100), maxPriorityFeePerGas: undefined })
expect(fees).toEqual({ maxFeePerGas: BigInt(110), maxPriorityFeePerGas: BigInt(10) })
})

it('should return the correct fees per gas when fee currency is specified', async () => {
const client = {
request: jest.fn(async ({ method, params }) => {
expect(params).toEqual(['0x123'])
if (method === 'eth_gasPrice') return '0x64' // 100 in hex
if (method === 'eth_maxPriorityFeePerGas') return '0xa' // 10 in hex
throw new Error(`Unknown method ${method}`)
}),
}
const fees = await estimateFeesPerGas(client as any, '0x123')
expect(fees).toEqual({ maxFeePerGas: BigInt(100), maxPriorityFeePerGas: undefined })
expect(fees).toEqual({ maxFeePerGas: BigInt(110), maxPriorityFeePerGas: BigInt(10) })
})
})
22 changes: 15 additions & 7 deletions src/viem/estimateFeesPerGas.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,14 @@ import { Address, Client, hexToBigInt } from 'viem'
// See https://github.com/wagmi-dev/viem/discussions/914
export async function estimateFeesPerGas(client: Client, feeCurrency?: Address) {
// The gasPrice returned on Celo is already roughly 2x baseFeePerGas
// so we can just use that as maxFeePerGas, no need for an explicit maxPriorityFeePerGas
// See this interesting thread for more details:
// https://valora-app.slack.com/archives/CNJ7KTHQU/p1697717100995909?thread_ts=1697647756.662059&cid=CNJ7KTHQU
const gasPrice = await getGasPrice(client, feeCurrency)
const maxFeePerGas = gasPrice
// Here we still return maxPriorityFeePerGas as undefined to be consistent with viem
// and so consumer TXs are correctly serialized as CIP-42 TXs
// See https://github.com/wagmi-dev/viem/blob/5c95fafceffe7f399b5b5ee32119e2d78a0c8acd/src/chains/celo/serializers.ts#L105-L106
return { maxFeePerGas, maxPriorityFeePerGas: undefined }
const [gasPrice, maxPriorityFeePerGas] = await Promise.all([
getGasPrice(client, feeCurrency),
getMaxPriorityFeePerGas(client, feeCurrency),
])
const maxFeePerGas = gasPrice + maxPriorityFeePerGas
return { maxFeePerGas, maxPriorityFeePerGas }
}

// Get gas price with optional fee currency, this is Celo specific
Expand All @@ -24,3 +23,12 @@ export async function getGasPrice(client: Client, feeCurrency: Address | undefin
})
return hexToBigInt(priceHex)
}

// Get max priority fee per gas with optional fee currency, this is Celo specific
export async function getMaxPriorityFeePerGas(client: Client, feeCurrency?: Address) {
const maxPriorityFeePerGasHex = await client.request({
method: 'eth_maxPriorityFeePerGas',
...((feeCurrency ? { params: [feeCurrency] } : {}) as object),
})
return hexToBigInt(maxPriorityFeePerGasHex)
}
51 changes: 26 additions & 25 deletions src/viem/prepareTransactions.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import BigNumber from 'bignumber.js'
import { TransactionRequestCIP42 } from 'node_modules/viem/_types/chains/celo/types'
import erc20 from 'src/abis/IERC20'
import stableToken from 'src/abis/StableToken'
import { TokenBalanceWithAddress } from 'src/tokens/slice'
import { Network, NetworkId } from 'src/transactions/types'
import { estimateFeesPerGas } from 'src/viem/estimateFeesPerGas'
Expand All @@ -15,6 +16,7 @@ import {
tryEstimateTransaction,
tryEstimateTransactions,
} from 'src/viem/prepareTransactions'
import { mockCeloTokenBalance } from 'test/values'
import {
Address,
BaseError,
Expand All @@ -24,8 +26,6 @@ import {
encodeFunctionData,
} from 'viem'
import mocked = jest.mocked
import { mockCeloTokenBalance } from 'test/values'
import stableToken from 'src/abis/StableToken'

jest.mock('src/viem/estimateFeesPerGas')
jest.mock('viem', () => ({
Expand Down Expand Up @@ -119,7 +119,7 @@ describe('prepareTransactions module', () => {
it("returns a 'not-enough-balance-for-gas' result when the balances for feeCurrencies are too low to cover the fee", async () => {
mocked(estimateFeesPerGas).mockResolvedValue({
maxFeePerGas: BigInt(100),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
})
mockPublicClient.estimateGas.mockResolvedValue(BigInt(1_000))

Expand Down Expand Up @@ -147,7 +147,7 @@ describe('prepareTransactions module', () => {
it("returns a 'not-enough-balance-for-gas' result when gas estimation throws error due to insufficient funds", async () => {
mocked(estimateFeesPerGas).mockResolvedValue({
maxFeePerGas: BigInt(100),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
})
mockPublicClient.estimateGas.mockRejectedValue(mockInsufficientFundsError)

Expand All @@ -173,7 +173,7 @@ describe('prepareTransactions module', () => {
it("returns a 'not-enough-balance-for-gas' result when gas estimation throws error due to value exceeded balance", async () => {
mocked(estimateFeesPerGas).mockResolvedValue({
maxFeePerGas: BigInt(100),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
})
mockPublicClient.estimateGas.mockRejectedValue(mockValueExceededBalanceError)

Expand All @@ -199,7 +199,7 @@ describe('prepareTransactions module', () => {
it('throws if gas estimation throws error for some other reason besides insufficient funds', async () => {
mocked(estimateFeesPerGas).mockResolvedValue({
maxFeePerGas: BigInt(100),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
})
mockPublicClient.estimateGas.mockRejectedValue(mockExceededAllowanceError)

Expand All @@ -223,7 +223,7 @@ describe('prepareTransactions module', () => {
it("returns a 'need-decrease-spend-amount-for-gas' result when spending the exact max amount of a feeCurrency, and no other feeCurrency has enough balance to pay for the fee", async () => {
mocked(estimateFeesPerGas).mockResolvedValue({
maxFeePerGas: BigInt(1),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
})

const result = await prepareTransactions({
Expand Down Expand Up @@ -251,7 +251,7 @@ describe('prepareTransactions module', () => {
it("returns a 'need-decrease-spend-amount-for-gas' result when spending close to the max amount of a feeCurrency, and no other feeCurrency has enough balance to pay for the fee", async () => {
mocked(estimateFeesPerGas).mockResolvedValue({
maxFeePerGas: BigInt(1),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
})

const result = await prepareTransactions({
Expand Down Expand Up @@ -281,7 +281,7 @@ describe('prepareTransactions module', () => {
it("returns a 'possible' result when spending a feeCurrency, when there's enough balance to cover for the fee", async () => {
mocked(estimateFeesPerGas).mockResolvedValue({
maxFeePerGas: BigInt(1),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
})
mockPublicClient.estimateGas.mockResolvedValue(BigInt(500))

Expand Down Expand Up @@ -318,7 +318,7 @@ describe('prepareTransactions module', () => {
type: 'cip42',
gas: BigInt(500),
maxFeePerGas: BigInt(1),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
},
{
from: '0xfrom',
Expand All @@ -327,7 +327,7 @@ describe('prepareTransactions module', () => {
type: 'cip42',
gas: BigInt(100),
maxFeePerGas: BigInt(1),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
},
],
feeCurrency: mockFeeCurrencies[0],
Expand All @@ -336,7 +336,7 @@ describe('prepareTransactions module', () => {
it("returns a 'possible' result when spending the max balance of a feeCurrency when there's another feeCurrency to pay for the fee", async () => {
mocked(estimateFeesPerGas).mockResolvedValue({
maxFeePerGas: BigInt(1),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
})
mockPublicClient.estimateGas.mockResolvedValue(BigInt(500))

Expand Down Expand Up @@ -374,7 +374,7 @@ describe('prepareTransactions module', () => {
type: 'cip42',
gas: BigInt(500),
maxFeePerGas: BigInt(1),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
feeCurrency: mockFeeCurrencies[1].address,
},
{
Expand All @@ -384,7 +384,7 @@ describe('prepareTransactions module', () => {
type: 'cip42',
gas: BigInt(50_100),
maxFeePerGas: BigInt(1),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
feeCurrency: mockFeeCurrencies[1].address,
},
],
Expand All @@ -394,7 +394,7 @@ describe('prepareTransactions module', () => {
it("returns a 'possible' result when spending the max balance of a token that isn't a feeCurrency when there's another feeCurrency to pay for the fee", async () => {
mocked(estimateFeesPerGas).mockResolvedValue({
maxFeePerGas: BigInt(1),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
})
mockPublicClient.estimateGas.mockResolvedValue(BigInt(500))

Expand Down Expand Up @@ -431,7 +431,7 @@ describe('prepareTransactions module', () => {
type: 'cip42',
gas: BigInt(500),
maxFeePerGas: BigInt(1),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
},
{
from: '0xfrom',
Expand All @@ -440,28 +440,29 @@ describe('prepareTransactions module', () => {
type: 'cip42',
gas: BigInt(100),
maxFeePerGas: BigInt(1),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
},
],
feeCurrency: mockFeeCurrencies[0],
})
})
})
describe('tryEstimateTransaction', () => {
it('does not include feeCurrency if not address undefined', async () => {
it('does not include feeCurrency if address is undefined', async () => {
mockPublicClient.estimateGas.mockResolvedValue(BigInt(123))
const baseTransaction: TransactionRequestCIP42 = { from: '0x123' }
const estimateTransactionOutput = await tryEstimateTransaction({
baseTransaction,
maxFeePerGas: BigInt(456),
maxPriorityFeePerGas: BigInt(2),
feeCurrencySymbol: 'FEE',
})
expect(estimateTransactionOutput && 'feeCurrency' in estimateTransactionOutput).toEqual(false)
expect(estimateTransactionOutput).toStrictEqual({
from: '0x123',
gas: BigInt(123),
maxFeePerGas: BigInt(456),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
})
})
it('includes feeCurrency if address is given', async () => {
Expand Down Expand Up @@ -512,7 +513,7 @@ describe('prepareTransactions module', () => {
it('returns null if estimateGas throws error due to insufficient funds', async () => {
mocked(estimateFeesPerGas).mockResolvedValue({
maxFeePerGas: BigInt(10),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
})
mockPublicClient.estimateGas.mockRejectedValue(mockInsufficientFundsError)
const estimateTransactionsOutput = await tryEstimateTransactions(
Expand All @@ -524,7 +525,7 @@ describe('prepareTransactions module', () => {
it('estimates gas only for transactions missing a gas field', async () => {
mocked(estimateFeesPerGas).mockResolvedValue({
maxFeePerGas: BigInt(10),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
})
mockPublicClient.estimateGas.mockResolvedValue(BigInt(123))
const estimateTransactionsOutput = await tryEstimateTransactions(
Expand All @@ -536,13 +537,13 @@ describe('prepareTransactions module', () => {
from: '0x123',
gas: BigInt(123),
maxFeePerGas: BigInt(10),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
},
{
from: '0x123',
gas: BigInt(456),
maxFeePerGas: BigInt(10),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
},
])
})
Expand Down Expand Up @@ -680,7 +681,7 @@ describe('prepareTransactions module', () => {
type: 'cip42',
gas: BigInt(500),
maxFeePerGas: BigInt(1),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
},
{
from: '0xfrom',
Expand All @@ -689,7 +690,7 @@ describe('prepareTransactions module', () => {
type: 'cip42',
gas: BigInt(100),
maxFeePerGas: BigInt(1),
maxPriorityFeePerGas: undefined,
maxPriorityFeePerGas: BigInt(2),
},
],
feeCurrency: mockFeeCurrencies[0],
Expand Down

0 comments on commit b31dfd0

Please sign in to comment.