Check for invalid gas estimates from local storage (#6800)

* Remove unused state 'gas.basicPriceAndTimeEstimates' * Check for invalid estimates from local storage Gas estimates were being cached in local storage then later retrieved, but the retrieved values were not being checked. If the data failed to save, failed to load, or was cleared since being saved, it would result in the gas estimates being set to undefined. The estimates retrieved from local storage are now checked before they are used. If they are falsy, the estimates are retrieved from the network instead. This should fix this Sentry issue: [METAMASK-6W0T](https://sentry.io/share/issue/cfe470314a5741768b19050815322aa4/) A few additional changes were made to the gas-duck tests to accommodate the use of `sinon.restore`. `restore` is strongly recommended by the `sinon` team, as neglecting to use it can result in memory leaks. It has the additional benefit of ensuring you create fresh stubs/spies for each test, which means they no longer need to be reset between tests.
5 years ago · ec40b2a325
parent c7608e6fe9
commit ec40b2a325
8 changed files with 347 additions and 139 deletions
--- a/development/states/confirm-new-ui.json
+++ b/development/states/confirm-new-ui.json
@ -202,7 +202,6 @@
    },
    "basicEstimateIsLoading": false,
    "gasEstimatesLoading": false,
-    "basicPriceAndTimeEstimates": [],
    "priceAndTimeEstimates": [
      {
        "expectedTime": "1374.1168296452973076627",
--- a/development/states/confirm-sig-requests.json
+++ b/development/states/confirm-sig-requests.json
@ -228,7 +228,6 @@
    },
    "basicEstimateIsLoading": false,
    "gasEstimatesLoading": false,
-    "basicPriceAndTimeEstimates": [],
    "priceAndTimeEstimates": [
      {
        "expectedTime": "1374.1168296452973076627",
--- a/development/states/currency-localization.json
+++ b/development/states/currency-localization.json
@ -178,7 +178,6 @@
    },
    "basicEstimateIsLoading": false,
    "gasEstimatesLoading": false,
-    "basicPriceAndTimeEstimates": [],
    "priceAndTimeEstimates": [
      {
        "expectedTime": "1374.1168296452973076627",
--- a/development/states/send-edit.json
+++ b/development/states/send-edit.json
@ -206,7 +206,6 @@
    },
    "basicEstimateIsLoading": false,
    "gasEstimatesLoading": false,
-    "basicPriceAndTimeEstimates": [],
    "priceAndTimeEstimates": [
      {
        "expectedTime": "1374.1168296452973076627",
--- a/development/states/send-new-ui.json
+++ b/development/states/send-new-ui.json
@ -186,7 +186,6 @@
    },
    "basicEstimateIsLoading": false,
    "gasEstimatesLoading": false,
-    "basicPriceAndTimeEstimates": [],
    "priceAndTimeEstimates": [
      {
        "expectedTime": "1374.1168296452973076627",
--- a/development/states/tx-list-items.json
+++ b/development/states/tx-list-items.json
@ -1109,7 +1109,6 @@
    },
    "basicEstimateIsLoading": false,
    "gasEstimatesLoading": false,
-    "basicPriceAndTimeEstimates": [],
    "priceAndTimeEstimates": [
      {
        "expectedTime": "1374.1168296452973076627",
--- a/ui/app/ducks/gas/gas-duck.test.js
+++ b/ui/app/ducks/gas/gas-duck.test.js
@ -2,12 +2,10 @@ import assert from 'assert'
 import sinon from 'sinon'
 import proxyquire from 'proxyquire'

+const fakeLocalStorage = {}

 const GasDuck = proxyquire('./gas.duck.js', {
-  '../../../lib/local-storage-helpers': {
-    loadLocalStorageData: sinon.spy(),
-    saveLocalStorageData: sinon.spy(),
-  },
+  '../../../lib/local-storage-helpers': fakeLocalStorage,
 })

 const {
@ -68,24 +66,28 @@ describe('Gas Duck', () => {
    { expectedTime: 1.1, expectedWait: 0.6, gasprice: 19.9, somethingElse: 'foobar' },
    { expectedTime: 1, expectedWait: 0.5, gasprice: 20, somethingElse: 'foobar' },
  ]
-  const fetchStub = sinon.stub().callsFake((url) => new Promise(resolve => {
+  const fakeFetch = (url) => new Promise(resolve => {
    const dataToResolve = url.match(/ethgasAPI|gasexpress/)
      ? mockEthGasApiResponse
      : mockPredictTableResponse
    resolve({
      json: () => new Promise(resolve => resolve(dataToResolve)),
    })
-  }))
+  })

  beforeEach(() => {
    tempFetch = global.fetch
    tempDateNow = global.Date.now
-    global.fetch = fetchStub
+
+    fakeLocalStorage.loadLocalStorageData = sinon.stub()
+    fakeLocalStorage.saveLocalStorageData = sinon.spy()
+    global.fetch = sinon.stub().callsFake(fakeFetch)
    global.Date.now = () => 2000000
  })

  afterEach(() => {
-    fetchStub.resetHistory()
+    sinon.restore()
+
    global.fetch = tempFetch
    global.Date.now = tempDateNow
  })
@ -118,7 +120,6 @@ describe('Gas Duck', () => {
    gasEstimatesLoading: true,
    priceAndTimeEstimates: [],
    priceAndTimeEstimatesLastRetrieved: 0,
-    basicPriceAndTimeEstimates: [],
    basicPriceAndTimeEstimatesLastRetrieved: 0,
    basicPriceEstimatesLastRetrieved: 0,
  }
@ -305,8 +306,9 @@ describe('Gas Duck', () => {
  })

  describe('fetchBasicGasEstimates', () => {
-    const mockDistpatch = sinon.spy()
    it('should call fetch with the expected params', async () => {
+      const mockDistpatch = sinon.spy()
+
      await fetchBasicGasEstimates()(mockDistpatch, () => ({ gas: Object.assign(
        {},
        initState,
@ -330,12 +332,109 @@ describe('Gas Duck', () => {
          },
        ]
      )
-
      assert.deepEqual(
        mockDistpatch.getCall(1).args,
        [{ type: SET_BASIC_PRICE_ESTIMATES_LAST_RETRIEVED, value: 2000000 } ]
      )
+      assert.deepEqual(
+        mockDistpatch.getCall(2).args,
+        [{
+          type: SET_BASIC_GAS_ESTIMATE_DATA,
+          value: {
+            average: 20,
+            blockTime: 'mockBlock_time',
+            blockNum: 'mockBlockNum',
+            fast: 30,
+            fastest: 40,
+            safeLow: 10,
+          },
+        }]
+      )
+      assert.deepEqual(
+        mockDistpatch.getCall(3).args,
+        [{ type: BASIC_GAS_ESTIMATE_LOADING_FINISHED }]
+      )
+    })
+
+    it('should fetch recently retrieved estimates from local storage', async () => {
+      const mockDistpatch = sinon.spy()
+      fakeLocalStorage.loadLocalStorageData
+        .withArgs('BASIC_PRICE_ESTIMATES_LAST_RETRIEVED')
+        .returns(2000000 - 1) // one second ago from "now"
+      fakeLocalStorage.loadLocalStorageData
+        .withArgs('BASIC_PRICE_ESTIMATES')
+        .returns({
+          average: 25,
+          blockTime: 'mockBlock_time',
+          blockNum: 'mockBlockNum',
+          fast: 35,
+          fastest: 45,
+          safeLow: 15,
+        })

+      await fetchBasicGasEstimates()(mockDistpatch, () => ({ gas: Object.assign(
+        {},
+        initState,
+        {}
+      ) }))
+      assert.deepEqual(
+        mockDistpatch.getCall(0).args,
+        [{ type: BASIC_GAS_ESTIMATE_LOADING_STARTED} ]
+      )
+      assert.ok(global.fetch.notCalled)
+      assert.deepEqual(
+        mockDistpatch.getCall(1).args,
+        [{
+          type: SET_BASIC_GAS_ESTIMATE_DATA,
+          value: {
+            average: 25,
+            blockTime: 'mockBlock_time',
+            blockNum: 'mockBlockNum',
+            fast: 35,
+            fastest: 45,
+            safeLow: 15,
+          },
+        }]
+      )
+      assert.deepEqual(
+        mockDistpatch.getCall(2).args,
+        [{ type: BASIC_GAS_ESTIMATE_LOADING_FINISHED }]
+      )
+    })
+
+    it('should fallback to network if retrieving estimates from local storage fails', async () => {
+      const mockDistpatch = sinon.spy()
+      fakeLocalStorage.loadLocalStorageData
+        .withArgs('BASIC_PRICE_ESTIMATES_LAST_RETRIEVED')
+        .returns(2000000 - 1) // one second ago from "now"
+
+      await fetchBasicGasEstimates()(mockDistpatch, () => ({ gas: Object.assign(
+        {},
+        initState,
+        {}
+      ) }))
+      assert.deepEqual(
+        mockDistpatch.getCall(0).args,
+        [{ type: BASIC_GAS_ESTIMATE_LOADING_STARTED} ]
+      )
+      assert.deepEqual(
+        global.fetch.getCall(0).args,
+        [
+          'https://dev.blockscale.net/api/gasexpress.json',
+          {
+            'headers': {},
+            'referrer': 'https://dev.blockscale.net/api/',
+            'referrerPolicy': 'no-referrer-when-downgrade',
+            'body': null,
+            'method': 'GET',
+            'mode': 'cors',
+          },
+        ]
+      )
+      assert.deepEqual(
+        mockDistpatch.getCall(1).args,
+        [{ type: SET_BASIC_PRICE_ESTIMATES_LAST_RETRIEVED, value: 2000000 } ]
+      )
      assert.deepEqual(
        mockDistpatch.getCall(2).args,
        [{
@ -358,8 +457,9 @@ describe('Gas Duck', () => {
  })

  describe('fetchBasicGasAndTimeEstimates', () => {
-    const mockDistpatch = sinon.spy()
    it('should call fetch with the expected params', async () => {
+      const mockDistpatch = sinon.spy()
+
      await fetchBasicGasAndTimeEstimates()(mockDistpatch, () => ({ gas: Object.assign(
        {},
        initState,
@ -415,17 +515,133 @@ describe('Gas Duck', () => {
        [{ type: BASIC_GAS_ESTIMATE_LOADING_FINISHED }]
      )
    })
-  })

-  describe('fetchGasEstimates', () => {
-    const mockDistpatch = sinon.spy()
+    it('should fetch recently retrieved estimates from local storage', async () => {
+      const mockDistpatch = sinon.spy()
+      fakeLocalStorage.loadLocalStorageData
+        .withArgs('BASIC_GAS_AND_TIME_API_ESTIMATES_LAST_RETRIEVED')
+        .returns(2000000 - 1) // one second ago from "now"
+      fakeLocalStorage.loadLocalStorageData
+        .withArgs('BASIC_GAS_AND_TIME_API_ESTIMATES')
+        .returns({
+          average: 5,
+          avgWait: 'mockAvgWait',
+          blockTime: 'mockBlock_time',
+          blockNum: 'mockBlockNum',
+          fast: 6,
+          fastest: 7,
+          fastestWait: 'mockFastestWait',
+          fastWait: 'mockFastWait',
+          safeLow: 1,
+          safeLowWait: 'mockSafeLowWait',
+          speed: 'mockSpeed',
+        })
+
+      await fetchBasicGasAndTimeEstimates()(mockDistpatch, () => ({ gas: Object.assign(
+        {},
+        initState,
+        {}
+      ),
+        metamask: { provider: { type: 'ropsten' } },
+      }))
+      assert.deepEqual(
+        mockDistpatch.getCall(0).args,
+        [{ type: BASIC_GAS_ESTIMATE_LOADING_STARTED} ]
+      )
+      assert.ok(global.fetch.notCalled)

-    beforeEach(() => {
-      mockDistpatch.resetHistory()
+      assert.deepEqual(
+        mockDistpatch.getCall(1).args,
+        [{
+          type: SET_BASIC_GAS_ESTIMATE_DATA,
+          value: {
+            average: 5,
+            avgWait: 'mockAvgWait',
+            blockTime: 'mockBlock_time',
+            blockNum: 'mockBlockNum',
+            fast: 6,
+            fastest: 7,
+            fastestWait: 'mockFastestWait',
+            fastWait: 'mockFastWait',
+            safeLow: 1,
+            safeLowWait: 'mockSafeLowWait',
+            speed: 'mockSpeed',
+          },
+        }]
+      )
+      assert.deepEqual(
+        mockDistpatch.getCall(2).args,
+        [{ type: BASIC_GAS_ESTIMATE_LOADING_FINISHED }]
+      )
    })

+    it('should fallback to network if retrieving estimates from local storage fails', async () => {
+      const mockDistpatch = sinon.spy()
+      fakeLocalStorage.loadLocalStorageData
+        .withArgs('BASIC_GAS_AND_TIME_API_ESTIMATES_LAST_RETRIEVED')
+        .returns(2000000 - 1) // one second ago from "now"
+
+      await fetchBasicGasAndTimeEstimates()(mockDistpatch, () => ({ gas: Object.assign(
+        {},
+        initState,
+        {}
+      ),
+        metamask: { provider: { type: 'ropsten' } },
+      }))
+      assert.deepEqual(
+        mockDistpatch.getCall(0).args,
+        [{ type: BASIC_GAS_ESTIMATE_LOADING_STARTED} ]
+      )
+      assert.deepEqual(
+        global.fetch.getCall(0).args,
+        [
+          'https://ethgasstation.info/json/ethgasAPI.json',
+          {
+            'headers': {},
+            'referrer': 'http://ethgasstation.info/json/',
+            'referrerPolicy': 'no-referrer-when-downgrade',
+            'body': null,
+            'method': 'GET',
+            'mode': 'cors',
+          },
+        ]
+      )
+
+      assert.deepEqual(
+        mockDistpatch.getCall(1).args,
+        [{ type: SET_BASIC_API_ESTIMATES_LAST_RETRIEVED, value: 2000000 } ]
+      )
+
+      assert.deepEqual(
+        mockDistpatch.getCall(2).args,
+        [{
+          type: SET_BASIC_GAS_ESTIMATE_DATA,
+          value: {
+            average: 2,
+            avgWait: 'mockAvgWait',
+            blockTime: 'mockBlock_time',
+            blockNum: 'mockBlockNum',
+            fast: 3,
+            fastest: 4,
+            fastestWait: 'mockFastestWait',
+            fastWait: 'mockFastWait',
+            safeLow: 1,
+            safeLowWait: 'mockSafeLowWait',
+            speed: 'mockSpeed',
+          },
+        }]
+      )
+      assert.deepEqual(
+        mockDistpatch.getCall(3).args,
+        [{ type: BASIC_GAS_ESTIMATE_LOADING_FINISHED }]
+      )
+    })
+  })
+
+  describe('fetchGasEstimates', () => {
    it('should call fetch with the expected params', async () => {
-      global.fetch.resetHistory()
+      const mockDistpatch = sinon.spy()
+
      await fetchGasEstimates(5)(mockDistpatch, () => ({ gas: Object.assign(
        {},
        initState,
@ -471,7 +687,8 @@ describe('Gas Duck', () => {
    })

    it('should not call fetch if the estimates were retrieved < 75000 ms ago', async () => {
-      global.fetch.resetHistory()
+      const mockDistpatch = sinon.spy()
+
      await fetchGasEstimates(5)(mockDistpatch, () => ({ gas: Object.assign(
        {},
        initState,
--- a/ui/app/ducks/gas/gas.duck.js
+++ b/ui/app/ducks/gas/gas.duck.js
@ -50,7 +50,6 @@ const initState = {
  basicEstimateIsLoading: true,
  gasEstimatesLoading: true,
  priceAndTimeEstimates: [],
-  basicPriceAndTimeEstimates: [],
  priceAndTimeEstimatesLastRetrieved: 0,
  basicPriceAndTimeEstimatesLastRetrieved: 0,
  basicPriceEstimatesLastRetrieved: 0,
@ -177,134 +176,132 @@ export function gasEstimatesLoadingFinished () {
 }

 export function fetchBasicGasEstimates () {
-  return (dispatch, getState) => {
-    const {
-      basicPriceEstimatesLastRetrieved,
-      basicPriceAndTimeEstimates,
-    } = getState().gas
+  return async (dispatch, getState) => {
+    const { basicPriceEstimatesLastRetrieved } = getState().gas
    const timeLastRetrieved = basicPriceEstimatesLastRetrieved || loadLocalStorageData('BASIC_PRICE_ESTIMATES_LAST_RETRIEVED') || 0

    dispatch(basicGasEstimatesLoadingStarted())

-    const promiseToFetch = Date.now() - timeLastRetrieved > 75000
-    ? fetch('https://dev.blockscale.net/api/gasexpress.json', {
-      'headers': {},
-      'referrer': 'https://dev.blockscale.net/api/',
-      'referrerPolicy': 'no-referrer-when-downgrade',
-      'body': null,
-      'method': 'GET',
-      'mode': 'cors'}
-    )
-      .then(r => r.json())
-      .then(({
-        safeLow,
-        standard: average,
-        fast,
-        fastest,
-        block_time: blockTime,
-        blockNum,
-      }) => {
-        const basicEstimates = {
-          safeLow,
-          average,
-          fast,
-          fastest,
-          blockTime,
-          blockNum,
-        }
-
-        const timeRetrieved = Date.now()
-        dispatch(setBasicPriceEstimatesLastRetrieved(timeRetrieved))
-        saveLocalStorageData(timeRetrieved, 'BASIC_PRICE_ESTIMATES_LAST_RETRIEVED')
-        saveLocalStorageData(basicEstimates, 'BASIC_PRICE_ESTIMATES')
-
-        return basicEstimates
-      })
-    : Promise.resolve(basicPriceAndTimeEstimates.length
-        ? basicPriceAndTimeEstimates
-        : loadLocalStorageData('BASIC_PRICE_ESTIMATES')
-      )
-
-    return promiseToFetch.then(basicEstimates => {
-      dispatch(setBasicGasEstimateData(basicEstimates))
-      dispatch(basicGasEstimatesLoadingFinished())
-      return basicEstimates
-    })
+    let basicEstimates
+    if (Date.now() - timeLastRetrieved > 75000) {
+      basicEstimates = await fetchExternalBasicGasEstimates(dispatch)
+    } else {
+      const cachedBasicEstimates = loadLocalStorageData('BASIC_PRICE_ESTIMATES')
+      basicEstimates = cachedBasicEstimates || await fetchExternalBasicGasEstimates(dispatch)
+    }
+
+    dispatch(setBasicGasEstimateData(basicEstimates))
+    dispatch(basicGasEstimatesLoadingFinished())
+
+    return basicEstimates
  }
 }

+async function fetchExternalBasicGasEstimates (dispatch) {
+  const response = await fetch('https://dev.blockscale.net/api/gasexpress.json', {
+    'headers': {},
+    'referrer': 'https://dev.blockscale.net/api/',
+    'referrerPolicy': 'no-referrer-when-downgrade',
+    'body': null,
+    'method': 'GET',
+    'mode': 'cors'}
+  )
+  const {
+    safeLow,
+    standard: average,
+    fast,
+    fastest,
+    block_time: blockTime,
+    blockNum,
+  } = await response.json()
+
+  const basicEstimates = {
+    safeLow,
+    average,
+    fast,
+    fastest,
+    blockTime,
+    blockNum,
+  }
+
+  const timeRetrieved = Date.now()
+  saveLocalStorageData(basicEstimates, 'BASIC_PRICE_ESTIMATES')
+  saveLocalStorageData(timeRetrieved, 'BASIC_PRICE_ESTIMATES_LAST_RETRIEVED')
+  dispatch(setBasicPriceEstimatesLastRetrieved(timeRetrieved))
+
+  return basicEstimates
+}
+
 export function fetchBasicGasAndTimeEstimates () {
-  return (dispatch, getState) => {
-    const {
-      basicPriceAndTimeEstimatesLastRetrieved,
-      basicPriceAndTimeEstimates,
-    } = getState().gas
+  return async (dispatch, getState) => {
+    const { basicPriceAndTimeEstimatesLastRetrieved } = getState().gas
    const timeLastRetrieved = basicPriceAndTimeEstimatesLastRetrieved || loadLocalStorageData('BASIC_GAS_AND_TIME_API_ESTIMATES_LAST_RETRIEVED') || 0

    dispatch(basicGasEstimatesLoadingStarted())

-    const promiseToFetch = Date.now() - timeLastRetrieved > 75000
-      ? fetch('https://ethgasstation.info/json/ethgasAPI.json', {
-        'headers': {},
-        'referrer': 'http://ethgasstation.info/json/',
-        'referrerPolicy': 'no-referrer-when-downgrade',
-        'body': null,
-        'method': 'GET',
-        'mode': 'cors'}
-      )
-        .then(r => r.json())
-        .then(({
-          average: averageTimes10,
-          avgWait,
-          block_time: blockTime,
-          blockNum,
-          fast: fastTimes10,
-          fastest: fastestTimes10,
-          fastestWait,
-          fastWait,
-          safeLow: safeLowTimes10,
-          safeLowWait,
-          speed,
-        }) => {
-          const [average, fast, fastest, safeLow] = [
-            averageTimes10,
-            fastTimes10,
-            fastestTimes10,
-            safeLowTimes10,
-          ].map(price => (new BigNumber(price)).div(10).toNumber())
-
-          const basicEstimates = {
-            average,
-            avgWait,
-            blockTime,
-            blockNum,
-            fast,
-            fastest,
-            fastestWait,
-            fastWait,
-            safeLow,
-            safeLowWait,
-            speed,
-          }
-
-          const timeRetrieved = Date.now()
-          dispatch(setBasicApiEstimatesLastRetrieved(timeRetrieved))
-          saveLocalStorageData(timeRetrieved, 'BASIC_GAS_AND_TIME_API_ESTIMATES_LAST_RETRIEVED')
-          saveLocalStorageData(basicEstimates, 'BASIC_GAS_AND_TIME_API_ESTIMATES')
+    let basicEstimates
+    if (Date.now() - timeLastRetrieved > 75000) {
+      basicEstimates = await fetchExternalBasicGasAndTimeEstimates(dispatch)
+    } else {
+      const cachedBasicEstimates = loadLocalStorageData('BASIC_GAS_AND_TIME_API_ESTIMATES')
+      basicEstimates = cachedBasicEstimates || await fetchExternalBasicGasAndTimeEstimates(dispatch)
+    }

-          return basicEstimates
-        })
-      : Promise.resolve(basicPriceAndTimeEstimates.length
-          ? basicPriceAndTimeEstimates
-          : loadLocalStorageData('BASIC_GAS_AND_TIME_API_ESTIMATES')
-        )
+    dispatch(setBasicGasEstimateData(basicEstimates))
+    dispatch(basicGasEstimatesLoadingFinished())
+    return basicEstimates
+  }
+}

-      return promiseToFetch.then(basicEstimates => {
-        dispatch(setBasicGasEstimateData(basicEstimates))
-        dispatch(basicGasEstimatesLoadingFinished())
-        return basicEstimates
-      })
+async function fetchExternalBasicGasAndTimeEstimates (dispatch) {
+  const response = await fetch('https://ethgasstation.info/json/ethgasAPI.json', {
+    'headers': {},
+    'referrer': 'http://ethgasstation.info/json/',
+    'referrerPolicy': 'no-referrer-when-downgrade',
+    'body': null,
+    'method': 'GET',
+    'mode': 'cors'}
+  )
+  const {
+    average: averageTimes10,
+    avgWait,
+    block_time: blockTime,
+    blockNum,
+    fast: fastTimes10,
+    fastest: fastestTimes10,
+    fastestWait,
+    fastWait,
+    safeLow: safeLowTimes10,
+    safeLowWait,
+    speed,
+  } = await response.json()
+  const [average, fast, fastest, safeLow] = [
+    averageTimes10,
+    fastTimes10,
+    fastestTimes10,
+    safeLowTimes10,
+  ].map(price => (new BigNumber(price)).div(10).toNumber())
+
+  const basicEstimates = {
+    average,
+    avgWait,
+    blockTime,
+    blockNum,
+    fast,
+    fastest,
+    fastestWait,
+    fastWait,
+    safeLow,
+    safeLowWait,
+    speed,
  }
+
+  const timeRetrieved = Date.now()
+  saveLocalStorageData(basicEstimates, 'BASIC_GAS_AND_TIME_API_ESTIMATES')
+  saveLocalStorageData(timeRetrieved, 'BASIC_GAS_AND_TIME_API_ESTIMATES_LAST_RETRIEVED')
+  dispatch(setBasicApiEstimatesLastRetrieved(timeRetrieved))
+
+  return basicEstimates
 }

 function extrapolateY ({ higherY, lowerY, higherX, lowerX, xForExtrapolation }) {