From a208af5fd83c0b3bf7a8812015bb986b17d694b2 Mon Sep 17 00:00:00 2001 From: Alexandre ABRIOUX Date: Fri, 30 Jun 2023 12:13:33 +0200 Subject: [PATCH 1/3] fix(gas-fee): catch estimation errors --- .../ethereum-storage/src/gas-fee-definer.ts | 12 +++++- .../contract-setup/adminTasks.ts | 2 +- .../scripts-create2/xdeployer.ts | 2 +- packages/utils/src/estimate-gas-fees.ts | 38 ++++++++++++------- packages/utils/test/estimate-gas-fee.test.ts | 8 ++-- 5 files changed, 40 insertions(+), 22 deletions(-) diff --git a/packages/ethereum-storage/src/gas-fee-definer.ts b/packages/ethereum-storage/src/gas-fee-definer.ts index 09afa7c8d7..957ab5133b 100644 --- a/packages/ethereum-storage/src/gas-fee-definer.ts +++ b/packages/ethereum-storage/src/gas-fee-definer.ts @@ -1,15 +1,19 @@ import { BigNumber, providers, constants } from 'ethers'; import { GasDefinerProps } from './ethereum-storage-ethers'; import { estimateGasFees } from '@requestnetwork/utils'; +import { LogTypes } from '@requestnetwork/types'; export class GasFeeDefiner { + private readonly logger: LogTypes.ILogger; private readonly provider: providers.JsonRpcProvider; private readonly gasPriceMin: BigNumber; constructor({ + logger, provider, gasPriceMin, - }: GasDefinerProps & { provider: providers.JsonRpcProvider }) { + }: GasDefinerProps & { logger?: LogTypes.ILogger; provider: providers.JsonRpcProvider }) { + this.logger = logger || console; this.provider = provider; this.gasPriceMin = gasPriceMin || constants.Zero; } @@ -18,6 +22,10 @@ export class GasFeeDefiner { maxFeePerGas?: BigNumber; maxPriorityFeePerGas?: BigNumber; }> { - return estimateGasFees({ provider: this.provider, gasPriceMin: this.gasPriceMin }); + return estimateGasFees({ + logger: this.logger, + provider: this.provider, + gasPriceMin: this.gasPriceMin, + }); } } diff --git a/packages/smart-contracts/scripts-create2/contract-setup/adminTasks.ts b/packages/smart-contracts/scripts-create2/contract-setup/adminTasks.ts index cc74a3682a..e934c15c36 100644 --- a/packages/smart-contracts/scripts-create2/contract-setup/adminTasks.ts +++ b/packages/smart-contracts/scripts-create2/contract-setup/adminTasks.ts @@ -283,7 +283,7 @@ export const getSignerAndGasFees = async ( const signer = new hre.ethers.Wallet(hre.config.xdeploy.signer).connect(provider); const txOverrides = (await isEip1559Supported(provider)) - ? await estimateGasFees({ provider }) + ? await estimateGasFees({ logger: console, provider }) : {}; return { diff --git a/packages/smart-contracts/scripts-create2/xdeployer.ts b/packages/smart-contracts/scripts-create2/xdeployer.ts index c7861c0d70..a7bc3eda54 100644 --- a/packages/smart-contracts/scripts-create2/xdeployer.ts +++ b/packages/smart-contracts/scripts-create2/xdeployer.ts @@ -79,7 +79,7 @@ export const xdeploy = async ( let txOverrides: Overrides = {}; if (await isEip1559Supported(provider, console)) { - txOverrides = await estimateGasFees({ provider }); + txOverrides = await estimateGasFees({ logger: console, provider }); } txOverrides.gasLimit = hre.config.xdeploy.gasLimit; diff --git a/packages/utils/src/estimate-gas-fees.ts b/packages/utils/src/estimate-gas-fees.ts index 18c4e4d95a..2f26a191a9 100644 --- a/packages/utils/src/estimate-gas-fees.ts +++ b/packages/utils/src/estimate-gas-fees.ts @@ -1,6 +1,7 @@ import { BigNumber, constants, providers } from 'ethers'; import { suggestFees } from '@rainbow-me/fee-suggestions'; import { maxBigNumber } from './index'; +import { LogTypes } from '@requestnetwork/types'; /** * The function estimates gas fee with EIP-1559. @@ -11,36 +12,45 @@ import { maxBigNumber } from './index'; * maxFeePerGas = baseFeePerGas + maxPriorityFeePerGas * The baseFeePerGas depends on how full the previous blocks were. * - maxPriorityFeePerGas: The maximum priority fee per unit of gas for this transaction. - * - gasPrice: Optional fallback: the gas price for this transaction. */ async function estimateGasFees({ + logger, provider, gasPriceMin, }: { + logger: LogTypes.ILogger; provider: providers.Provider | providers.JsonRpcProvider; gasPriceMin?: BigNumber; }): Promise<{ maxFeePerGas?: BigNumber; maxPriorityFeePerGas?: BigNumber; }> { - const suggestedFee = await suggestFees(provider as providers.JsonRpcProvider); + try { + const suggestedFee = await suggestFees(provider as providers.JsonRpcProvider); - const baseFee = maxBigNumber(suggestedFee.baseFeeSuggestion, gasPriceMin || constants.Zero); + const baseFee = maxBigNumber(suggestedFee.baseFeeSuggestion, gasPriceMin || constants.Zero); - const maxPriorityFeePerGas = maxBigNumber( - suggestedFee.maxPriorityFeeSuggestions.urgent, - gasPriceMin || constants.Zero, - ); - const maxFeePerGas = baseFee.add(maxPriorityFeePerGas); + const maxPriorityFeePerGas = maxBigNumber( + suggestedFee.maxPriorityFeeSuggestions.urgent, + gasPriceMin || constants.Zero, + ); + const maxFeePerGas = baseFee.add(maxPriorityFeePerGas); - if (maxPriorityFeePerGas.eq(0) || maxFeePerGas.eq(0)) { + if (maxPriorityFeePerGas.eq(0) || maxFeePerGas.eq(0)) { + logger.warn( + `estimateGasFees: maxPriorityFeePerGas or maxFeePerGas too low (maxPriorityFeePerGas: ${maxPriorityFeePerGas.toString()} / maxFeePerGas: ${maxFeePerGas.toString()})`, + ); + return {}; + } + + return { + maxPriorityFeePerGas, + maxFeePerGas, + }; + } catch (e) { + logger.error(`estimateGasFees error: ${e}`); return {}; } - - return { - maxPriorityFeePerGas, - maxFeePerGas, - }; } export { estimateGasFees }; diff --git a/packages/utils/test/estimate-gas-fee.test.ts b/packages/utils/test/estimate-gas-fee.test.ts index 3bb7d3b96a..2e4e41620d 100644 --- a/packages/utils/test/estimate-gas-fee.test.ts +++ b/packages/utils/test/estimate-gas-fee.test.ts @@ -21,15 +21,15 @@ const checkEstimation = ( describe('Gas fee estimation', () => { it('Should not be undefined', async () => { - const estimation = await estimateGasFees({ provider }); + const estimation = await estimateGasFees({ logger: console, provider }); expect(estimation.maxFeePerGas).toBeDefined(); expect(estimation.maxPriorityFeePerGas).toBeDefined(); }); it('Should return a lower estimation when the previous block is empty', async () => { - const firstEstimation = await estimateGasFees({ provider }); + const firstEstimation = await estimateGasFees({ logger: console, provider }); await provider.send('evm_mine', []); - const secondEstimation = await estimateGasFees({ provider }); + const secondEstimation = await estimateGasFees({ logger: console, provider }); expect( firstEstimation.maxFeePerGas?.sub(secondEstimation.maxFeePerGas || 0).toNumber(), @@ -45,7 +45,7 @@ describe('Gas fee estimation', () => { }); } - const estimation = await estimateGasFees({ provider }); + const estimation = await estimateGasFees({ logger: console, provider }); const tx = await wallet.sendTransaction({ to: dummyAddress, value: BigNumber.from(1), From eab23b6e3d96a807677e2ec9e9e087e5c091f87c Mon Sep 17 00:00:00 2001 From: Alexandre ABRIOUX Date: Fri, 30 Jun 2023 13:04:33 +0200 Subject: [PATCH 2/3] added tests --- packages/utils/test/estimate-gas-fee.test.ts | 51 +++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/packages/utils/test/estimate-gas-fee.test.ts b/packages/utils/test/estimate-gas-fee.test.ts index 2e4e41620d..bc698786d7 100644 --- a/packages/utils/test/estimate-gas-fee.test.ts +++ b/packages/utils/test/estimate-gas-fee.test.ts @@ -19,6 +19,10 @@ const checkEstimation = ( expect(absRatio).toBeLessThan(ratioMax); }; +afterEach(() => { + jest.clearAllMocks(); +}); + describe('Gas fee estimation', () => { it('Should not be undefined', async () => { const estimation = await estimateGasFees({ logger: console, provider }); @@ -51,6 +55,51 @@ describe('Gas fee estimation', () => { value: BigNumber.from(1), }); checkEstimation(estimation.maxFeePerGas as BigNumber, tx.maxFeePerGas as BigNumber, 0.1); - checkEstimation(estimation.maxFeePerGas as BigNumber, tx.maxFeePerGas as BigNumber, 0.1); + }); + + it('Should handle estimation errors properly', async () => { + jest.spyOn(provider, 'send').mockImplementation((command) => { + if (command !== 'eth_feeHistory') throw new Error('unhandled command'); + return Promise.resolve({ + // random data, found on https://docs.infura.io/networks/ethereum/json-rpc-methods/eth_feehistory#body + // not important in this test + baseFeePerGas: [ + '0x3da8e7618', + '0x3e1ba3b1b', + '0x3dfd72b90', + '0x3d64eee76', + '0x3d4da2da0', + '0x3ccbcac6b', + ], + gasUsedRatio: [ + 0.5290747666666666, 0.49240453333333334, 0.4615576, 0.49407083333333335, 0.4669053, + ], + oldestBlock: '0xfab8ac', + // here return only rewards > 5 Gwei + // thus all blocks would be considered as outlier blocks in https://github.com/rainbow-me/fee-suggestions/blob/main/src/utils.ts#L123C11-L123C22 + // thus triggering an error + reward: [ + // 6000000000 wei + ['x165A0BC00'], + // 7000000000 wei + ['x1A13B8600'], + // 8000000000 wei + ['x1DCD65000'], + ], + }); + }); + + const loggerMock = { + debug: jest.fn(), + error: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + }; + + const estimation = await estimateGasFees({ logger: loggerMock, provider }); + expect(estimation).toStrictEqual({}); + expect(loggerMock.error).toHaveBeenCalledWith( + 'estimateGasFees error: Error: Error: ema was undefined', + ); }); }); From 768148982740e3b4efceb4055a6d64fc550df516 Mon Sep 17 00:00:00 2001 From: Alexandre ABRIOUX Date: Fri, 30 Jun 2023 13:11:31 +0200 Subject: [PATCH 3/3] better comment --- packages/utils/test/estimate-gas-fee.test.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/utils/test/estimate-gas-fee.test.ts b/packages/utils/test/estimate-gas-fee.test.ts index bc698786d7..0817c74e21 100644 --- a/packages/utils/test/estimate-gas-fee.test.ts +++ b/packages/utils/test/estimate-gas-fee.test.ts @@ -75,9 +75,10 @@ describe('Gas fee estimation', () => { 0.5290747666666666, 0.49240453333333334, 0.4615576, 0.49407083333333335, 0.4669053, ], oldestBlock: '0xfab8ac', - // here return only rewards > 5 Gwei - // thus all blocks would be considered as outlier blocks in https://github.com/rainbow-me/fee-suggestions/blob/main/src/utils.ts#L123C11-L123C22 - // thus triggering an error + // Here, return all rewards as > 5 Gwei. + // This is so that all blocks would be considered as outlier blocks in + // https://github.com/rainbow-me/fee-suggestions/blob/76b9fe14d3740c9df7cedf40b2f85cd8871ff9c2/src/utils.ts#L123C11-L123C22 + // thus triggering an error. reward: [ // 6000000000 wei ['x165A0BC00'],