From b57a12e55535a1e969f664b4dbfac8e2b33611e0 Mon Sep 17 00:00:00 2001 From: Brad Decker Date: Thu, 1 Jul 2021 13:51:12 -0500 Subject: [PATCH 1/4] normalize and update api --- src/gas/GasFeeController.test.ts | 55 +++++++++++--- src/gas/GasFeeController.ts | 118 ++++++++++++++++++++++++------- src/gas/gas-util.test.ts | 28 ++++++++ src/gas/gas-util.ts | 36 ++++++++-- 4 files changed, 197 insertions(+), 40 deletions(-) create mode 100644 src/gas/gas-util.test.ts diff --git a/src/gas/GasFeeController.test.ts b/src/gas/GasFeeController.test.ts index c88997a4cba..9946f49d7ee 100644 --- a/src/gas/GasFeeController.test.ts +++ b/src/gas/GasFeeController.test.ts @@ -5,7 +5,9 @@ import { GasFeeController, GetGasFeeState, GasFeeStateChange, + LegacyGasPriceEstimate, } from './GasFeeController'; +import { EXTERNAL_GAS_PRICES_API_URL } from './gas-util'; const GAS_FEE_API = 'https://mock-gas-server.herokuapp.com/'; @@ -28,6 +30,8 @@ function getRestrictedMessenger() { describe('GasFeeController', () => { let gasFeeController: GasFeeController; + let getIsMainnet: jest.Mock; + let getIsEIP1559Compatible: jest.Mock>; beforeAll(() => { nock.disableNetConnect(); @@ -38,8 +42,12 @@ describe('GasFeeController', () => { }); beforeEach(() => { + getIsMainnet = jest.fn().mockImplementation(() => false); + getIsEIP1559Compatible = jest + .fn() + .mockImplementation(() => Promise.resolve(true)); nock(GAS_FEE_API) - .get('/') + .get(/.+/u) .reply(200, { low: { minWaitTimeEstimate: 60000, @@ -60,14 +68,25 @@ describe('GasFeeController', () => { suggestedMaxFeePerGas: '50', }, estimatedBaseFee: '28', - }); + }) + .persist(); + + nock(EXTERNAL_GAS_PRICES_API_URL) + .get(/.+/u) + .reply(200, { + SafeGasPrice: '22', + ProposeGasPrice: '25', + FastGasPrice: '30', + }) + .persist(); gasFeeController = new GasFeeController({ interval: 10000, messenger: getRestrictedMessenger(), getProvider: () => stub(), onNetworkStateChange: () => stub(), - getCurrentNetworkEIP1559Compatibility: () => Promise.resolve(true), // change this for networkController.state.properties.isEIP1559Compatible ??? + getIsMainnet, + getCurrentNetworkEIP1559Compatibility: getIsEIP1559Compatible, // change this for networkController.state.properties.isEIP1559Compatible ??? }); }); @@ -94,12 +113,28 @@ describe('GasFeeController', () => { ); }); - it('should _fetchGasFeeEstimateData', async () => { - expect(gasFeeController.state.gasFeeEstimates).toStrictEqual({}); - const estimates = await gasFeeController._fetchGasFeeEstimateData(); - expect(estimates).toHaveProperty('gasFeeEstimates'); - expect(gasFeeController.state.gasFeeEstimates).toHaveProperty( - 'estimatedBaseFee', - ); + describe('when on mainnet before london', () => { + it('should _fetchGasFeeEstimateData', async () => { + getIsMainnet.mockImplementation(() => true); + getIsEIP1559Compatible.mockImplementation(() => Promise.resolve(false)); + expect(gasFeeController.state.gasFeeEstimates).toStrictEqual({}); + const estimates = await gasFeeController._fetchGasFeeEstimateData(); + expect(estimates).toHaveProperty('gasFeeEstimates'); + expect( + (gasFeeController.state.gasFeeEstimates as LegacyGasPriceEstimate).high, + ).toBe('30'); + }); + }); + + describe('when on any network supporting EIP-1559', () => { + it('should _fetchGasFeeEstimateData', async () => { + getIsMainnet.mockImplementation(() => true); + expect(gasFeeController.state.gasFeeEstimates).toStrictEqual({}); + const estimates = await gasFeeController._fetchGasFeeEstimateData(); + expect(estimates).toHaveProperty('gasFeeEstimates'); + expect(gasFeeController.state.gasFeeEstimates).toHaveProperty( + 'estimatedBaseFee', + ); + }); }); }); diff --git a/src/gas/GasFeeController.ts b/src/gas/GasFeeController.ts index 4c4139bfcc8..309dfa08db0 100644 --- a/src/gas/GasFeeController.ts +++ b/src/gas/GasFeeController.ts @@ -11,29 +11,64 @@ import type { } from '../network/NetworkController'; import { fetchGasEstimates as defaultFetchGasEstimates, - fetchLegacyGasPriceEstimate as defaultFetchLegacyGasPriceEstimate, + fetchEthGasPriceEstimate as defaultFetchEthGasPriceEstimate, + fetchLegacyGasPriceEstimates as defaultFetchLegacyGasPriceEstimates, calculateTimeEstimate, } from './gas-util'; export type unknownString = 'unknown'; +/** + * Indicates which type of gasEstimate the controller is currently returning. + * This is useful as a way of asserting that the shape of gasEstimates matches + * expectations. NONE is a special case indicating that no previous gasEstimate + * has been fetched. + */ +export const GAS_ESTIMATE_TYPES = { + FEE_MARKET: 'fee-market' as const, + LEGACY: 'legacy' as const, + ETH_GASPRICE: 'eth_gasPrice' as const, + NONE: 'none' as const, +}; + +export type GasEstimateType = typeof GAS_ESTIMATE_TYPES[keyof typeof GAS_ESTIMATE_TYPES]; + export interface EstimatedGasFeeTimeBounds { lowerTimeBound: number | null; upperTimeBound: number | unknownString; } /** - * @type LegacyGasPriceEstimate + * @type EthGasPriceEstimate * * A single gas price estimate for networks and accounts that don't support EIP-1559 + * This estimate comes from eth_gasPrice but is converted to dec gwei to match other + * return values * - * @property gasPrice - A GWEI hex number, the result of a call to eth_gasPrice + * @property gasPrice - A GWEI dec string */ -export interface LegacyGasPriceEstimate { +export interface EthGasPriceEstimate { gasPrice: string; } +/** + * @type LegacyGasPriceEstimate + * + * A set of gas price estimates for networks and accounts that don't support EIP-1559 + * These estimates include low, medium and high all as strings representing gwei in + * decimal format. + * + * @property high - gasPrice, in decimal gwei string format, suggested for fast inclusion + * @property medium - gasPrice, in decimal gwei string format, suggested for avg inclusion + * @property low - gasPrice, in decimal gwei string format, suggested for slow inclusion + */ +export interface LegacyGasPriceEstimate { + high: string; + medium: string; + low: string; +} + /** * @type Eip1559GasFee * @@ -48,8 +83,8 @@ export interface LegacyGasPriceEstimate { export interface Eip1559GasFee { minWaitTimeEstimate: number; // a time duration in milliseconds maxWaitTimeEstimate: number; // a time duration in milliseconds - suggestedMaxPriorityFeePerGas: string; // a GWEI hex number - suggestedMaxFeePerGas: string; // a GWEI hex number + suggestedMaxPriorityFeePerGas: string; // a GWEI decimal number + suggestedMaxFeePerGas: string; // a GWEI decimal number } function isEIP1559GasFee(object: any): object is Eip1559GasFee { @@ -70,7 +105,7 @@ function isEIP1559GasFee(object: any): object is Eip1559GasFee { * @property low - A GasFee for a minimum necessary combination of tip and maxFee * @property medium - A GasFee for a recommended combination of tip and maxFee * @property high - A GasFee for a high combination of tip and maxFee - * @property estimatedNextBlockBaseFee - An estimate of what the base fee will be for the pending/next block. A GWEI hex number + * @property estimatedBaseFee - An estimate of what the base fee will be for the pending/next block. A GWEI dec number */ export interface GasFeeEstimates { @@ -95,6 +130,7 @@ function isEIP1559Estimate(object: any): object is GasFeeEstimates { const metadata = { gasFeeEstimates: { persist: true, anonymous: false }, estimatedGasFeeTimeBounds: { persist: true, anonymous: false }, + gasEstimateType: { persist: true, anonymous: false }, }; /** @@ -108,9 +144,11 @@ const metadata = { export type GasFeeState = { gasFeeEstimates: | GasFeeEstimates + | EthGasPriceEstimate | LegacyGasPriceEstimate | Record; estimatedGasFeeTimeBounds: EstimatedGasFeeTimeBounds | Record; + gasEstimateType: GasEstimateType; }; const name = 'GasFeeController'; @@ -128,6 +166,7 @@ export type GetGasFeeState = { const defaultState = { gasFeeEstimates: {}, estimatedGasFeeTimeBounds: {}, + gasEstimateType: GAS_ESTIMATE_TYPES.NONE, }; /** @@ -142,12 +181,16 @@ export class GasFeeController extends BaseController { private fetchGasEstimates; - private fetchLegacyGasPriceEstimate; + private fetchEthGasPriceEstimate; + + private fetchLegacyGasPriceEstimates; private getCurrentNetworkEIP1559Compatibility; private getCurrentAccountEIP1559Compatibility; + private getIsMainnet; + private ethQuery: any; /** @@ -159,9 +202,11 @@ export class GasFeeController extends BaseController { messenger, state, fetchGasEstimates = defaultFetchGasEstimates, - fetchLegacyGasPriceEstimate = defaultFetchLegacyGasPriceEstimate, + fetchEthGasPriceEstimate = defaultFetchEthGasPriceEstimate, + fetchLegacyGasPriceEstimates = defaultFetchLegacyGasPriceEstimates, getCurrentNetworkEIP1559Compatibility, getCurrentAccountEIP1559Compatibility, + getIsMainnet, getProvider, onNetworkStateChange, }: { @@ -175,9 +220,11 @@ export class GasFeeController extends BaseController { >; state?: Partial; fetchGasEstimates?: typeof defaultFetchGasEstimates; - fetchLegacyGasPriceEstimate?: typeof defaultFetchLegacyGasPriceEstimate; + fetchEthGasPriceEstimate?: typeof defaultFetchEthGasPriceEstimate; + fetchLegacyGasPriceEstimates?: typeof defaultFetchLegacyGasPriceEstimates; getCurrentNetworkEIP1559Compatibility: () => Promise; getCurrentAccountEIP1559Compatibility?: () => boolean; + getIsMainnet: () => boolean; getProvider: () => NetworkController['provider']; onNetworkStateChange: (listener: (state: NetworkState) => void) => void; }) { @@ -189,10 +236,12 @@ export class GasFeeController extends BaseController { }); this.intervalDelay = interval; this.fetchGasEstimates = fetchGasEstimates; - this.fetchLegacyGasPriceEstimate = fetchLegacyGasPriceEstimate; + this.fetchEthGasPriceEstimate = fetchEthGasPriceEstimate; + this.fetchLegacyGasPriceEstimates = fetchLegacyGasPriceEstimates; this.pollTokens = new Set(); this.getCurrentNetworkEIP1559Compatibility = getCurrentNetworkEIP1559Compatibility; this.getCurrentAccountEIP1559Compatibility = getCurrentAccountEIP1559Compatibility; + this.getIsMainnet = getIsMainnet; const provider = getProvider(); this.ethQuery = new EthQuery(provider); @@ -226,9 +275,11 @@ export class GasFeeController extends BaseController { * @returns GasFeeEstimates */ async _fetchGasFeeEstimateData(): Promise { - let estimates; + let estimates: GasFeeState['gasFeeEstimates']; let estimatedGasFeeTimeBounds = {}; let isEIP1559Compatible; + let gasEstimateType: GasEstimateType = GAS_ESTIMATE_TYPES.NONE; + const isMainnet = this.getIsMainnet(); try { isEIP1559Compatible = await this.getEIP1559Compatibility(); } catch (e) { @@ -236,6 +287,19 @@ export class GasFeeController extends BaseController { isEIP1559Compatible = false; } + const tryEthGasPrice = async () => { + try { + return { + estimates: await this.fetchEthGasPriceEstimate(this.ethQuery), + gasEstimateType: GAS_ESTIMATE_TYPES.ETH_GASPRICE, + }; + } catch (error) { + throw new Error( + `Gas fee/price estimation failed. Message: ${error.message}`, + ); + } + }; + if (isEIP1559Compatible) { try { estimates = await this.fetchGasEstimates(); @@ -247,28 +311,32 @@ export class GasFeeController extends BaseController { suggestedMaxPriorityFeePerGas, suggestedMaxFeePerGas, ); + gasEstimateType = GAS_ESTIMATE_TYPES.FEE_MARKET; } catch (error) { - try { - estimates = await this.fetchLegacyGasPriceEstimate(this.ethQuery); - } catch (error2) { - throw new Error( - `Gas fee/price estimation failed. Message: ${error2.message}`, - ); - } + const result = await tryEthGasPrice(); + estimates = result.estimates; + gasEstimateType = result.gasEstimateType; } - } else { + } else if (isMainnet) { try { - estimates = await this.fetchLegacyGasPriceEstimate(this.ethQuery); - } catch (error2) { - throw new Error( - `Gas fee/price estimation failed. Message: ${error2.message}`, - ); + estimates = await this.fetchLegacyGasPriceEstimates(); + gasEstimateType = GAS_ESTIMATE_TYPES.LEGACY; + } catch (error) { + console.log(error); + const result = await tryEthGasPrice(); + estimates = result.estimates; + gasEstimateType = result.gasEstimateType; } + } else { + const result = await tryEthGasPrice(); + estimates = result.estimates; + gasEstimateType = result.gasEstimateType; } const newState: GasFeeState = { gasFeeEstimates: estimates, estimatedGasFeeTimeBounds, + gasEstimateType, }; this.update(() => { diff --git a/src/gas/gas-util.test.ts b/src/gas/gas-util.test.ts new file mode 100644 index 00000000000..15323f1f20d --- /dev/null +++ b/src/gas/gas-util.test.ts @@ -0,0 +1,28 @@ +import nock from 'nock'; +import { + EXTERNAL_GAS_PRICES_API_URL, + fetchLegacyGasPriceEstimates, +} from './gas-util'; + +describe('gas utils', () => { + describe('fetchLegacyGasPriceEstimates', () => { + it('should fetch external gasPrices and return high/medium/low', async () => { + const scope = nock(EXTERNAL_GAS_PRICES_API_URL) + .get(/.+/u) + .reply(200, { + SafeGasPrice: '22', + ProposeGasPrice: '25', + FastGasPrice: '30', + }) + .persist(); + const result = await fetchLegacyGasPriceEstimates(); + expect(result).toMatchObject({ + high: '30', + medium: '25', + low: '22', + }); + scope.done(); + nock.cleanAll(); + }); + }); +}); diff --git a/src/gas/gas-util.ts b/src/gas/gas-util.ts index 0f98eeed08e..0b55adbacbb 100644 --- a/src/gas/gas-util.ts +++ b/src/gas/gas-util.ts @@ -1,24 +1,50 @@ import { BN } from 'ethereumjs-util'; -import { query, handleFetch, gweiDecToWEIBN } from '../util'; +import { query, handleFetch, gweiDecToWEIBN, weiHexToGweiDec } from '../util'; import { GasFeeEstimates, - LegacyGasPriceEstimate, + EthGasPriceEstimate, EstimatedGasFeeTimeBounds, unknownString, } from './GasFeeController'; const GAS_FEE_API = 'https://mock-gas-server.herokuapp.com/'; +export const EXTERNAL_GAS_PRICES_API_URL = `https://api.metaswap.codefi.network/gasPrices`; export async function fetchGasEstimates(): Promise { return await handleFetch(GAS_FEE_API); } -export async function fetchLegacyGasPriceEstimate( +/** + * Hit the legacy MetaSwaps gasPrices estimate api and return the low, medium + * high values from that API. + */ +export async function fetchLegacyGasPriceEstimates(): Promise<{ + low: string; + medium: string; + high: string; +}> { + const result = await handleFetch(EXTERNAL_GAS_PRICES_API_URL, { + referrer: EXTERNAL_GAS_PRICES_API_URL, + referrerPolicy: 'no-referrer-when-downgrade', + method: 'GET', + mode: 'cors', + headers: { + 'Content-Type': 'application/json', + }, + }); + return { + low: result.SafeGasPrice, + medium: result.ProposeGasPrice, + high: result.FastGasPrice, + }; +} + +export async function fetchEthGasPriceEstimate( ethQuery: any, -): Promise { +): Promise { const gasPrice = await query(ethQuery, 'gasPrice'); return { - gasPrice, + gasPrice: weiHexToGweiDec(gasPrice).toString(), }; } From 5bfcd2f51004b6f4c866161294d3cc2fda549867 Mon Sep 17 00:00:00 2001 From: Brad Decker Date: Thu, 1 Jul 2021 14:29:59 -0500 Subject: [PATCH 2/4] make try/catch logic cleaner --- src/gas/GasFeeController.ts | 45 ++++++++++++------------------------- 1 file changed, 14 insertions(+), 31 deletions(-) diff --git a/src/gas/GasFeeController.ts b/src/gas/GasFeeController.ts index 309dfa08db0..4e513accaf4 100644 --- a/src/gas/GasFeeController.ts +++ b/src/gas/GasFeeController.ts @@ -287,21 +287,8 @@ export class GasFeeController extends BaseController { isEIP1559Compatible = false; } - const tryEthGasPrice = async () => { - try { - return { - estimates: await this.fetchEthGasPriceEstimate(this.ethQuery), - gasEstimateType: GAS_ESTIMATE_TYPES.ETH_GASPRICE, - }; - } catch (error) { - throw new Error( - `Gas fee/price estimation failed. Message: ${error.message}`, - ); - } - }; - - if (isEIP1559Compatible) { - try { + try { + if (isEIP1559Compatible) { estimates = await this.fetchGasEstimates(); const { suggestedMaxPriorityFeePerGas, @@ -312,25 +299,21 @@ export class GasFeeController extends BaseController { suggestedMaxFeePerGas, ); gasEstimateType = GAS_ESTIMATE_TYPES.FEE_MARKET; - } catch (error) { - const result = await tryEthGasPrice(); - estimates = result.estimates; - gasEstimateType = result.gasEstimateType; - } - } else if (isMainnet) { - try { + } else if (isMainnet) { estimates = await this.fetchLegacyGasPriceEstimates(); gasEstimateType = GAS_ESTIMATE_TYPES.LEGACY; - } catch (error) { - console.log(error); - const result = await tryEthGasPrice(); - estimates = result.estimates; - gasEstimateType = result.gasEstimateType; + } else { + throw new Error('Main gas fee/price estimation failed. Use fallback'); + } + } catch (error) { + try { + estimates = await this.fetchEthGasPriceEstimate(this.ethQuery); + gasEstimateType = GAS_ESTIMATE_TYPES.ETH_GASPRICE; + } catch (error2) { + throw new Error( + `Gas fee/price estimation failed. Message: ${error2.message}`, + ); } - } else { - const result = await tryEthGasPrice(); - estimates = result.estimates; - gasEstimateType = result.gasEstimateType; } const newState: GasFeeState = { From 6ec51c3b7b20f06452c42fd8d100020796101be9 Mon Sep 17 00:00:00 2001 From: Brad Decker Date: Thu, 1 Jul 2021 14:34:08 -0500 Subject: [PATCH 3/4] remove unnecessary error param --- src/gas/GasFeeController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gas/GasFeeController.ts b/src/gas/GasFeeController.ts index 4e513accaf4..82c2b93af90 100644 --- a/src/gas/GasFeeController.ts +++ b/src/gas/GasFeeController.ts @@ -305,7 +305,7 @@ export class GasFeeController extends BaseController { } else { throw new Error('Main gas fee/price estimation failed. Use fallback'); } - } catch (error) { + } catch { try { estimates = await this.fetchEthGasPriceEstimate(this.ethQuery); gasEstimateType = GAS_ESTIMATE_TYPES.ETH_GASPRICE; From f39df362c0766591d8c1295813ebe54e1e48e4d9 Mon Sep 17 00:00:00 2001 From: Brad Decker Date: Thu, 1 Jul 2021 14:34:39 -0500 Subject: [PATCH 4/4] rename error2 to error --- src/gas/GasFeeController.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/gas/GasFeeController.ts b/src/gas/GasFeeController.ts index 82c2b93af90..47384441273 100644 --- a/src/gas/GasFeeController.ts +++ b/src/gas/GasFeeController.ts @@ -309,9 +309,9 @@ export class GasFeeController extends BaseController { try { estimates = await this.fetchEthGasPriceEstimate(this.ethQuery); gasEstimateType = GAS_ESTIMATE_TYPES.ETH_GASPRICE; - } catch (error2) { + } catch (error) { throw new Error( - `Gas fee/price estimation failed. Message: ${error2.message}`, + `Gas fee/price estimation failed. Message: ${error.message}`, ); } }