From b31dfd0d1dac36e65f2ac057809b8a6ea7d2f421 Mon Sep 17 00:00:00 2001 From: Jean Regisser Date: Tue, 7 Nov 2023 18:20:25 +0100 Subject: [PATCH] fix(fees): set tip on Celo from `eth_maxPriorityFeePerGas` (#4444) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### 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](https://github.com/valora-inc/wallet/pull/4335/commits/ec11ae7f4c2d50e93eee7122a407d58b08c08386) 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 --- src/viem/estimateFeesPerGas.test.ts | 6 ++-- src/viem/estimateFeesPerGas.ts | 22 ++++++++---- src/viem/prepareTransactions.test.ts | 51 ++++++++++++++-------------- 3 files changed, 45 insertions(+), 34 deletions(-) diff --git a/src/viem/estimateFeesPerGas.test.ts b/src/viem/estimateFeesPerGas.test.ts index e51ba902c1e..355bb3c5833 100644 --- a/src/viem/estimateFeesPerGas.test.ts +++ b/src/viem/estimateFeesPerGas.test.ts @@ -6,11 +6,12 @@ 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 () => { @@ -18,10 +19,11 @@ describe(estimateFeesPerGas, () => { 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) }) }) }) diff --git a/src/viem/estimateFeesPerGas.ts b/src/viem/estimateFeesPerGas.ts index da130da1994..8336ad5bb17 100644 --- a/src/viem/estimateFeesPerGas.ts +++ b/src/viem/estimateFeesPerGas.ts @@ -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 @@ -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) +} diff --git a/src/viem/prepareTransactions.test.ts b/src/viem/prepareTransactions.test.ts index 4a8a48a3491..2ef8204cdae 100644 --- a/src/viem/prepareTransactions.test.ts +++ b/src/viem/prepareTransactions.test.ts @@ -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' @@ -15,6 +16,7 @@ import { tryEstimateTransaction, tryEstimateTransactions, } from 'src/viem/prepareTransactions' +import { mockCeloTokenBalance } from 'test/values' import { Address, BaseError, @@ -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', () => ({ @@ -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)) @@ -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) @@ -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) @@ -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) @@ -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({ @@ -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({ @@ -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)) @@ -318,7 +318,7 @@ describe('prepareTransactions module', () => { type: 'cip42', gas: BigInt(500), maxFeePerGas: BigInt(1), - maxPriorityFeePerGas: undefined, + maxPriorityFeePerGas: BigInt(2), }, { from: '0xfrom', @@ -327,7 +327,7 @@ describe('prepareTransactions module', () => { type: 'cip42', gas: BigInt(100), maxFeePerGas: BigInt(1), - maxPriorityFeePerGas: undefined, + maxPriorityFeePerGas: BigInt(2), }, ], feeCurrency: mockFeeCurrencies[0], @@ -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)) @@ -374,7 +374,7 @@ describe('prepareTransactions module', () => { type: 'cip42', gas: BigInt(500), maxFeePerGas: BigInt(1), - maxPriorityFeePerGas: undefined, + maxPriorityFeePerGas: BigInt(2), feeCurrency: mockFeeCurrencies[1].address, }, { @@ -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, }, ], @@ -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)) @@ -431,7 +431,7 @@ describe('prepareTransactions module', () => { type: 'cip42', gas: BigInt(500), maxFeePerGas: BigInt(1), - maxPriorityFeePerGas: undefined, + maxPriorityFeePerGas: BigInt(2), }, { from: '0xfrom', @@ -440,7 +440,7 @@ describe('prepareTransactions module', () => { type: 'cip42', gas: BigInt(100), maxFeePerGas: BigInt(1), - maxPriorityFeePerGas: undefined, + maxPriorityFeePerGas: BigInt(2), }, ], feeCurrency: mockFeeCurrencies[0], @@ -448,12 +448,13 @@ describe('prepareTransactions module', () => { }) }) 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) @@ -461,7 +462,7 @@ describe('prepareTransactions module', () => { from: '0x123', gas: BigInt(123), maxFeePerGas: BigInt(456), - maxPriorityFeePerGas: undefined, + maxPriorityFeePerGas: BigInt(2), }) }) it('includes feeCurrency if address is given', async () => { @@ -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( @@ -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( @@ -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), }, ]) }) @@ -680,7 +681,7 @@ describe('prepareTransactions module', () => { type: 'cip42', gas: BigInt(500), maxFeePerGas: BigInt(1), - maxPriorityFeePerGas: undefined, + maxPriorityFeePerGas: BigInt(2), }, { from: '0xfrom', @@ -689,7 +690,7 @@ describe('prepareTransactions module', () => { type: 'cip42', gas: BigInt(100), maxFeePerGas: BigInt(1), - maxPriorityFeePerGas: undefined, + maxPriorityFeePerGas: BigInt(2), }, ], feeCurrency: mockFeeCurrencies[0],