Merge pull request #4350 from MetaMask/i3914-fix-newui-send-gas-estimation

NewUI gas estimation produces same results as old-ui (exception: contract addresses)

5 files changed, 228 insertions, 51 deletions

diff --git a/ui/app/components/send_/tests/send-component.test.js b/ui/app/components/send_/tests/send-component.test.js
index c82edd971..4e33d8f63 100644
--- a/ui/app/components/send_/tests/send-component.test.js
+++ b/ui/app/components/send_/tests/send-component.test.js
@@ -32,8 +32,8 @@ describe('Send Component', function () {
     wrapper = shallow(<SendTransactionScreen
       amount={'mockAmount'}
       amountConversionRate={'mockAmountConversionRate'}
+      blockGasLimit={'mockBlockGasLimit'}
       conversionRate={10}
-      data={'mockData'}
       editingTransactionId={'mockEditingTransactionId'}
       from={ { address: 'mockAddress', balance: 'mockBalance' } }
       gasLimit={'mockGasLimit'}
@@ -42,6 +42,7 @@ describe('Send Component', function () {
       history={{ mockProp: 'history-abc'}}
       network={'3'}
       primaryCurrency={'mockPrimaryCurrency'}
+      recentBlocks={['mockBlock']}
       selectedAddress={'mockSelectedAddress'}
       selectedToken={'mockSelectedToken'}
       tokenBalance={'mockTokenBalance'}
@@ -207,15 +208,24 @@ describe('Send Component', function () {
       assert.deepEqual(
         propsMethodSpies.updateAndSetGasTotal.getCall(0).args[0],
         {
-          data: 'mockData',
+          blockGasLimit: 'mockBlockGasLimit',
           editingTransactionId: 'mockEditingTransactionId',
           gasLimit: 'mockGasLimit',
           gasPrice: 'mockGasPrice',
+          recentBlocks: ['mockBlock'],
           selectedAddress: 'mockSelectedAddress',
           selectedToken: 'mockSelectedToken',
+          to: undefined,
+          value: 'mockAmount',
         }
       )
     })
+
+    it('should call updateAndSetGasTotal with to set to lowercase if passed', () => {
+      propsMethodSpies.updateAndSetGasTotal.resetHistory()
+      wrapper.instance().updateGas({ to: '0xABC' })
+      assert.equal(propsMethodSpies.updateAndSetGasTotal.getCall(0).args[0].to, '0xabc')
+    })
   })
 
   describe('render', () => {
diff --git a/ui/app/components/send_/tests/send-container.test.js b/ui/app/components/send_/tests/send-container.test.js
index 7b6ca1f7b..056aad148 100644
--- a/ui/app/components/send_/tests/send-container.test.js
+++ b/ui/app/components/send_/tests/send-container.test.js
@@ -7,7 +7,7 @@ let mapDispatchToProps
 
 const actionSpies = {
   updateSendTokenBalance: sinon.spy(),
-  updateGasTotal: sinon.spy(),
+  updateGasData: sinon.spy(),
   setGasTotal: sinon.spy(),
 }
 const duckActionSpies = {
@@ -26,12 +26,14 @@ proxyquire('../send.container.js', {
   'recompose': { compose: (arg1, arg2) => () => arg2() },
   './send.selectors': {
     getAmountConversionRate: (s) => `mockAmountConversionRate:${s}`,
+    getBlockGasLimit: (s) => `mockBlockGasLimit:${s}`,
     getConversionRate: (s) => `mockConversionRate:${s}`,
     getCurrentNetwork: (s) => `mockNetwork:${s}`,
     getGasLimit: (s) => `mockGasLimit:${s}`,
     getGasPrice: (s) => `mockGasPrice:${s}`,
     getGasTotal: (s) => `mockGasTotal:${s}`,
     getPrimaryCurrency: (s) => `mockPrimaryCurrency:${s}`,
+    getRecentBlocks: (s) => `mockRecentBlocks:${s}`,
     getSelectedAddress: (s) => `mockSelectedAddress:${s}`,
     getSelectedToken: (s) => `mockSelectedToken:${s}`,
     getSelectedTokenContract: (s) => `mockTokenContract:${s}`,
@@ -45,7 +47,6 @@ proxyquire('../send.container.js', {
   '../../ducks/send.duck': duckActionSpies,
   './send.utils.js': {
     calcGasTotal: (gasLimit, gasPrice) => gasLimit + gasPrice,
-    generateTokenTransferData: (a, b) => `mockData:${a + b}`,
   },
 })
 
@@ -57,8 +58,8 @@ describe('send container', () => {
       assert.deepEqual(mapStateToProps('mockState'), {
         amount: 'mockAmount:mockState',
         amountConversionRate: 'mockAmountConversionRate:mockState',
+        blockGasLimit: 'mockBlockGasLimit:mockState',
         conversionRate: 'mockConversionRate:mockState',
-        data: 'mockData:mockSelectedAddress:mockStatemockSelectedToken:mockState',
         editingTransactionId: 'mockEditingTransactionId:mockState',
         from: 'mockFrom:mockState',
         gasLimit: 'mockGasLimit:mockState',
@@ -66,6 +67,7 @@ describe('send container', () => {
         gasTotal: 'mockGasTotal:mockState',
         network: 'mockNetwork:mockState',
         primaryCurrency: 'mockPrimaryCurrency:mockState',
+        recentBlocks: 'mockRecentBlocks:mockState',
         selectedAddress: 'mockSelectedAddress:mockState',
         selectedToken: 'mockSelectedToken:mockState',
         tokenBalance: 'mockTokenBalance:mockState',
@@ -87,12 +89,15 @@ describe('send container', () => {
 
     describe('updateAndSetGasTotal()', () => {
       const mockProps = {
-        data: '0x1',
+        blockGasLimit: 'mockBlockGasLimit',
         editingTransactionId: '0x2',
         gasLimit: '0x3',
         gasPrice: '0x4',
+        recentBlocks: ['mockBlock'],
         selectedAddress: '0x4',
         selectedToken: { address: '0x1' },
+        to: 'mockTo',
+        value: 'mockValue',
       }
 
       it('should dispatch a setGasTotal action when editingTransactionId is truthy', () => {
@@ -104,15 +109,15 @@ describe('send container', () => {
         )
       })
 
-      it('should dispatch an updateGasTotal action when editingTransactionId is falsy', () => {
-        const { selectedAddress, selectedToken, data } = mockProps
+      it('should dispatch an updateGasData action when editingTransactionId is falsy', () => {
+        const { selectedAddress, selectedToken, recentBlocks, blockGasLimit, to, value } = mockProps
         mapDispatchToPropsObject.updateAndSetGasTotal(
-          Object.assign(mockProps, {editingTransactionId: false})
+          Object.assign({}, mockProps, {editingTransactionId: false})
         )
         assert(dispatchSpy.calledOnce)
         assert.deepEqual(
-          actionSpies.updateGasTotal.getCall(0).args[0],
-          { selectedAddress, selectedToken, data }
+          actionSpies.updateGasData.getCall(0).args[0],
+          { selectedAddress, selectedToken, recentBlocks, blockGasLimit, to, value }
         )
       })
     })
diff --git a/ui/app/components/send_/tests/send-selectors-test-data.js b/ui/app/components/send_/tests/send-selectors-test-data.js
index ecfe9022f..8f9c19314 100644
--- a/ui/app/components/send_/tests/send-selectors-test-data.js
+++ b/ui/app/components/send_/tests/send-selectors-test-data.js
@@ -22,6 +22,7 @@ module.exports = {
         'name': 'Send Account 4',
       },
     },
+    'currentBlockGasLimit': '0x4c1878',
     'currentCurrency': 'USD',
     'conversionRate': 1200.88200327,
     'conversionDate': 1489013762,
@@ -198,6 +199,7 @@ module.exports = {
       },
     },
     'currentLocale': 'en',
+    recentBlocks: ['mockBlock1', 'mockBlock2', 'mockBlock3'],
   },
   'appState': {
     'menuOpen': false,
diff --git a/ui/app/components/send_/tests/send-selectors.test.js b/ui/app/components/send_/tests/send-selectors.test.js
index 977fe2a47..152af8059 100644
--- a/ui/app/components/send_/tests/send-selectors.test.js
+++ b/ui/app/components/send_/tests/send-selectors.test.js
@@ -5,6 +5,7 @@ const {
   accountsWithSendEtherInfoSelector,
   // autoAddToBetaUI,
   getAddressBook,
+  getBlockGasLimit,
   getAmountConversionRate,
   getConversionRate,
   getConvertedCurrency,
@@ -17,6 +18,7 @@ const {
   getGasPrice,
   getGasTotal,
   getPrimaryCurrency,
+  getRecentBlocks,
   getSelectedAccount,
   getSelectedAddress,
   getSelectedIdentity,
@@ -134,6 +136,15 @@ describe('send selectors', () => {
     })
   })
 
+  describe('getBlockGasLimit', () => {
+    it('should return the current block gas limit', () => {
+      assert.deepEqual(
+        getBlockGasLimit(mockState),
+        '0x4c1878'
+      )
+    })
+  })
+
   describe('getConversionRate()', () => {
     it('should return the eth conversion rate', () => {
       assert.deepEqual(
@@ -239,6 +250,15 @@ describe('send selectors', () => {
     })
   })
 
+  describe('getRecentBlocks()', () => {
+    it('should return the recent blocks', () => {
+      assert.deepEqual(
+        getRecentBlocks(mockState),
+        ['mockBlock1', 'mockBlock2', 'mockBlock3']
+      )
+    })
+  })
+
   describe('getSelectedAccount()', () => {
     it('should return the currently selected account', () => {
       assert.deepEqual(
diff --git a/ui/app/components/send_/tests/send-utils.test.js b/ui/app/components/send_/tests/send-utils.test.js
index 4d471bcc1..b3f6372ef 100644
--- a/ui/app/components/send_/tests/send-utils.test.js
+++ b/ui/app/components/send_/tests/send-utils.test.js
@@ -1,6 +1,14 @@
 import assert from 'assert'
 import sinon from 'sinon'
 import proxyquire from 'proxyquire'
+import {
+  ONE_GWEI_IN_WEI_HEX,
+  SIMPLE_GAS_COST,
+} from '../send.constants'
+const {
+  addCurrencies,
+  subtractCurrencies,
+} = require('../../../conversion-util')
 
 const {
   INSUFFICIENT_FUNDS_ERROR,
@@ -11,7 +19,7 @@ const stubs = {
   addCurrencies: sinon.stub().callsFake((a, b, obj) => a + b),
   conversionUtil: sinon.stub().callsFake((val, obj) => parseInt(val, 16)),
   conversionGTE: sinon.stub().callsFake((obj1, obj2) => obj1.value > obj2.value),
-  multiplyCurrencies: sinon.stub().callsFake((a, b) => a * b),
+  multiplyCurrencies: sinon.stub().callsFake((a, b) => `${a}x${b}`),
   calcTokenAmount: sinon.stub().callsFake((a, d) => 'calc:' + a + d),
   rawEncode: sinon.stub().returns([16, 1100]),
 }
@@ -31,10 +39,11 @@ const sendUtils = proxyquire('../send.utils.js', {
 
 const {
   calcGasTotal,
+  estimateGas,
   doesAmountErrorRequireUpdate,
+  estimateGasPriceFromRecentBlocks,
   generateTokenTransferData,
   getAmountErrorObject,
-  getParamsForGasEstimate,
   calcTokenBalance,
   isBalanceSufficient,
   isTokenBalanceSufficient,
@@ -45,7 +54,7 @@ describe('send utils', () => {
   describe('calcGasTotal()', () => {
     it('should call multiplyCurrencies with the correct params and return the multiplyCurrencies return', () => {
       const result = calcGasTotal(12, 15)
-      assert.equal(result, 180)
+      assert.equal(result, '12x15')
       const call_ = stubs.multiplyCurrencies.getCall(0).args
       assert.deepEqual(
         call_,
@@ -97,11 +106,23 @@ describe('send utils', () => {
 
   describe('generateTokenTransferData()', () => {
     it('should return undefined if not passed a selected token', () => {
-      assert.equal(generateTokenTransferData('mockAddress', false), undefined)
+      assert.equal(generateTokenTransferData({ toAddress: 'mockAddress', amount: '0xa', selectedToken: false}), undefined)
+    })
+
+    it('should call abi.rawEncode with the correct params', () => {
+      stubs.rawEncode.resetHistory()
+      generateTokenTransferData({ toAddress: 'mockAddress', amount: 'ab', selectedToken: true})
+      assert.deepEqual(
+        stubs.rawEncode.getCall(0).args,
+        [['address', 'uint256'], ['mockAddress', '0xab']]
+      )
     })
 
     it('should return encoded token transfer data', () => {
-      assert.equal(generateTokenTransferData('mockAddress', true), '104c')
+      assert.equal(
+        generateTokenTransferData({ toAddress: 'mockAddress', amount: '0xa', selectedToken: true}),
+        '0xa9059cbb104c'
+      )
     })
   })
 
@@ -136,41 +157,6 @@ describe('send utils', () => {
     })
   })
 
-  describe('getParamsForGasEstimate()', () => {
-    it('should return from and gas properties if no symbol or data', () => {
-      assert.deepEqual(
-        getParamsForGasEstimate('mockAddress'),
-        {
-          from: 'mockAddress',
-          gas: '746a528800',
-        }
-      )
-    })
-
-    it('should return value property if symbol provided', () => {
-      assert.deepEqual(
-        getParamsForGasEstimate('mockAddress', 'ABC'),
-        {
-          from: 'mockAddress',
-          gas: '746a528800',
-          value: '0x0',
-        }
-      )
-    })
-
-    it('should return data property if data provided', () => {
-      assert.deepEqual(
-        getParamsForGasEstimate('mockAddress', 'ABC', 'somedata'),
-        {
-          from: 'mockAddress',
-          gas: '746a528800',
-          value: '0x0',
-          data: 'somedata',
-        }
-      )
-    })
-  })
-
   describe('calcTokenBalance()', () => {
     it('should return the calculated token blance', () => {
       assert.equal(calcTokenBalance({
@@ -261,4 +247,158 @@ describe('send utils', () => {
     })
   })
 
+  describe('estimateGas', () => {
+    const baseMockParams = {
+      blockGasLimit: '0x64',
+      selectedAddress: 'mockAddress',
+      to: '0xisContract',
+      estimateGasMethod: sinon.stub().callsFake(
+        (data, cb) => cb(
+          data.to.match(/willFailBecauseOf:/) ? { message: data.to.match(/:(.+)$/)[1] } : null,
+          { toString: (n) => `mockToString:${n}` }
+        )
+      ),
+    }
+    const baseExpectedCall = {
+      from: 'mockAddress',
+      gas: '0x64x0.95',
+      to: '0xisContract',
+    }
+
+    beforeEach(() => {
+      global.eth = {
+        getCode: sinon.stub().callsFake(
+          (address) => Promise.resolve(address.match(/isContract/) ? 'not-0x' : '0x')
+        ),
+      }
+    })
+
+    afterEach(() => {
+      baseMockParams.estimateGasMethod.resetHistory()
+      global.eth.getCode.resetHistory()
+    })
+
+    it('should call ethQuery.estimateGas with the expected params', async () => {
+      const result = await estimateGas(baseMockParams)
+      assert.equal(baseMockParams.estimateGasMethod.callCount, 1)
+      assert.deepEqual(
+        baseMockParams.estimateGasMethod.getCall(0).args[0],
+        Object.assign({ gasPrice: undefined, value: undefined }, baseExpectedCall)
+      )
+      assert.equal(result, 'mockToString:16')
+    })
+
+    it('should call ethQuery.estimateGas with a value of 0x0 and the expected data and to if passed a selectedToken', async () => {
+      const result = await estimateGas(Object.assign({ data: 'mockData', selectedToken: { address: 'mockAddress' } }, baseMockParams))
+      assert.equal(baseMockParams.estimateGasMethod.callCount, 1)
+      assert.deepEqual(
+        baseMockParams.estimateGasMethod.getCall(0).args[0],
+        Object.assign({}, baseExpectedCall, {
+          gasPrice: undefined,
+          value: '0x0',
+          data: '0xa9059cbb104c',
+          to: 'mockAddress',
+        })
+      )
+      assert.equal(result, 'mockToString:16')
+    })
+
+    it(`should return ${SIMPLE_GAS_COST} if ethQuery.getCode does not return '0x'`, async () => {
+      assert.equal(baseMockParams.estimateGasMethod.callCount, 0)
+      const result = await estimateGas(Object.assign({}, baseMockParams, { to: '0x123' }))
+      assert.equal(result, SIMPLE_GAS_COST)
+    })
+
+    it(`should not return ${SIMPLE_GAS_COST} if passed a selectedToken`, async () => {
+      assert.equal(baseMockParams.estimateGasMethod.callCount, 0)
+      const result = await estimateGas(Object.assign({}, baseMockParams, { to: '0x123', selectedToken: { address: '' } }))
+      assert.notEqual(result, SIMPLE_GAS_COST)
+    })
+
+    it(`should return the adjusted blockGasLimit if it fails with a 'Transaction execution error.'`, async () => {
+      const result = await estimateGas(Object.assign({}, baseMockParams, {
+        to: 'isContract willFailBecauseOf:Transaction execution error.',
+      }))
+      assert.equal(result, '0x64x0.95')
+    })
+
+    it(`should return the adjusted blockGasLimit if it fails with a 'gas required exceeds allowance or always failing transaction.'`, async () => {
+      const result = await estimateGas(Object.assign({}, baseMockParams, {
+        to: 'isContract willFailBecauseOf:gas required exceeds allowance or always failing transaction.',
+      }))
+      assert.equal(result, '0x64x0.95')
+    })
+
+    it(`should reject other errors`, async () => {
+      try {
+        await estimateGas(Object.assign({}, baseMockParams, {
+          to: 'isContract willFailBecauseOf:some other error',
+        }))
+      } catch (err) {
+        assert.deepEqual(err, { message: 'some other error' })
+      }
+    })
+  })
+
+  describe('estimateGasPriceFromRecentBlocks', () => {
+    const ONE_GWEI_IN_WEI_HEX_PLUS_ONE = addCurrencies(ONE_GWEI_IN_WEI_HEX, '0x1', {
+      aBase: 16,
+      bBase: 16,
+      toNumericBase: 'hex',
+    })
+    const ONE_GWEI_IN_WEI_HEX_PLUS_TWO = addCurrencies(ONE_GWEI_IN_WEI_HEX, '0x2', {
+      aBase: 16,
+      bBase: 16,
+      toNumericBase: 'hex',
+    })
+    const ONE_GWEI_IN_WEI_HEX_MINUS_ONE = subtractCurrencies(ONE_GWEI_IN_WEI_HEX, '0x1', {
+      aBase: 16,
+      bBase: 16,
+      toNumericBase: 'hex',
+    })
+
+    it(`should return ${ONE_GWEI_IN_WEI_HEX} if recentBlocks is falsy`, () => {
+      assert.equal(estimateGasPriceFromRecentBlocks(), ONE_GWEI_IN_WEI_HEX)
+    })
+
+    it(`should return ${ONE_GWEI_IN_WEI_HEX} if recentBlocks is empty`, () => {
+      assert.equal(estimateGasPriceFromRecentBlocks([]), ONE_GWEI_IN_WEI_HEX)
+    })
+
+    it(`should estimate a block's gasPrice as ${ONE_GWEI_IN_WEI_HEX} if it has no gas prices`, () => {
+      const mockRecentBlocks = [
+        { gasPrices: null },
+        { gasPrices: [ ONE_GWEI_IN_WEI_HEX_PLUS_ONE ] },
+        { gasPrices: [ ONE_GWEI_IN_WEI_HEX_MINUS_ONE ] },
+      ]
+      assert.equal(estimateGasPriceFromRecentBlocks(mockRecentBlocks), ONE_GWEI_IN_WEI_HEX)
+    })
+
+    it(`should estimate a block's gasPrice as ${ONE_GWEI_IN_WEI_HEX} if it has empty gas prices`, () => {
+      const mockRecentBlocks = [
+        { gasPrices: [] },
+        { gasPrices: [ ONE_GWEI_IN_WEI_HEX_PLUS_ONE ] },
+        { gasPrices: [ ONE_GWEI_IN_WEI_HEX_MINUS_ONE ] },
+      ]
+      assert.equal(estimateGasPriceFromRecentBlocks(mockRecentBlocks), ONE_GWEI_IN_WEI_HEX)
+    })
+
+    it(`should return the middle value of all blocks lowest prices`, () => {
+      const mockRecentBlocks = [
+        { gasPrices: [ ONE_GWEI_IN_WEI_HEX_PLUS_TWO ] },
+        { gasPrices: [ ONE_GWEI_IN_WEI_HEX_MINUS_ONE ] },
+        { gasPrices: [ ONE_GWEI_IN_WEI_HEX_PLUS_ONE ] },
+      ]
+      assert.equal(estimateGasPriceFromRecentBlocks(mockRecentBlocks), ONE_GWEI_IN_WEI_HEX_PLUS_ONE)
+    })
+
+    it(`should work if a block has multiple gas prices`, () => {
+      const mockRecentBlocks = [
+        { gasPrices: [ '0x1', '0x2', '0x3', '0x4', '0x5' ] },
+        { gasPrices: [ '0x101', '0x100', '0x103', '0x104', '0x102' ] },
+        { gasPrices: [ '0x150', '0x50', '0x100', '0x200', '0x5' ] },
+      ]
+      assert.equal(estimateGasPriceFromRecentBlocks(mockRecentBlocks), '0x5')
+    })
+  })
 })