Merge pull request #4584 from MetaMask/fix-send-token

Add hex-prefix to gas estimate result

11 files changed, 140 insertions, 19 deletions

diff --git a/ui/app/components/send/currency-display.js b/ui/app/components/send/currency-display.js
index 3bc9ad226..e410bc070 100644
--- a/ui/app/components/send/currency-display.js
+++ b/ui/app/components/send/currency-display.js
@@ -57,6 +57,7 @@ CurrencyDisplay.prototype.getValueToRender = function ({ selectedToken, conversi
   return selectedToken
     ? conversionUtil(ethUtil.addHexPrefix(value), {
       fromNumericBase: 'hex',
+      toNumericBase: 'dec',
       toCurrency: symbol,
       conversionRate: multiplier,
       invertConversionRate: true,
@@ -91,8 +92,12 @@ CurrencyDisplay.prototype.getConvertedValueToRender = function (nonFormattedValu
     : convertedValue
 }
 
+function removeLeadingZeroes (str) {
+  return str.replace(/^0*(?=\d)/, '')
+}
+
 CurrencyDisplay.prototype.handleChange = function (newVal) {
-  this.setState({ valueToRender: newVal })
+  this.setState({ valueToRender: removeLeadingZeroes(newVal) })
   this.props.onChange(this.getAmount(newVal))
 }
 
diff --git a/ui/app/components/send_/send-content/send-amount-row/send-amount-row.component.js b/ui/app/components/send_/send-content/send-amount-row/send-amount-row.component.js
index 8aefeed4a..8da36d3b7 100644
--- a/ui/app/components/send_/send-content/send-amount-row/send-amount-row.component.js
+++ b/ui/app/components/send_/send-content/send-amount-row/send-amount-row.component.js
@@ -23,6 +23,7 @@ export default class SendAmountRow extends Component {
     tokenBalance: PropTypes.string,
     updateSendAmount: PropTypes.func,
     updateSendAmountError: PropTypes.func,
+    updateGas: PropTypes.func,
   }
 
   validateAmount (amount) {
@@ -56,6 +57,14 @@ export default class SendAmountRow extends Component {
     updateSendAmount(amount)
   }
 
+  updateGas (amount) {
+    const { selectedToken, updateGas } = this.props
+
+    if (selectedToken) {
+      updateGas({ amount })
+    }
+  }
+
   render () {
     const {
       amount,
@@ -77,12 +86,15 @@ export default class SendAmountRow extends Component {
         <CurrencyDisplay
           conversionRate={amountConversionRate}
           convertedCurrency={convertedCurrency}
-          onBlur={newAmount => this.updateAmount(newAmount)}
+          onBlur={newAmount => {
+            this.updateGas(newAmount)
+            this.updateAmount(newAmount)
+          }}
           onChange={newAmount => this.validateAmount(newAmount)}
           inError={inError}
           primaryCurrency={primaryCurrency || 'ETH'}
           selectedToken={selectedToken}
-          value={amount || '0x0'}
+          value={amount}
         />
       </SendRowWrapper>
     )
diff --git a/ui/app/components/send_/send-content/send-amount-row/tests/send-amount-row-component.test.js b/ui/app/components/send_/send-content/send-amount-row/tests/send-amount-row-component.test.js
index 2205579ca..579e18585 100644
--- a/ui/app/components/send_/send-content/send-amount-row/tests/send-amount-row-component.test.js
+++ b/ui/app/components/send_/send-content/send-amount-row/tests/send-amount-row-component.test.js
@@ -12,10 +12,12 @@ const propsMethodSpies = {
   setMaxModeTo: sinon.spy(),
   updateSendAmount: sinon.spy(),
   updateSendAmountError: sinon.spy(),
+  updateGas: sinon.spy(),
 }
 
 sinon.spy(SendAmountRow.prototype, 'updateAmount')
 sinon.spy(SendAmountRow.prototype, 'validateAmount')
+sinon.spy(SendAmountRow.prototype, 'updateGas')
 
 describe('SendAmountRow Component', function () {
   let wrapper
@@ -36,6 +38,7 @@ describe('SendAmountRow Component', function () {
       tokenBalance={'mockTokenBalance'}
       updateSendAmount={propsMethodSpies.updateSendAmount}
       updateSendAmountError={propsMethodSpies.updateSendAmountError}
+      updateGas={propsMethodSpies.updateGas}
     />, { context: { t: str => str + '_t' } })
     instance = wrapper.instance()
   })
@@ -139,8 +142,14 @@ describe('SendAmountRow Component', function () {
       assert.equal(primaryCurrency, 'mockPrimaryCurrency')
       assert.deepEqual(selectedToken, { address: 'mockTokenAddress' })
       assert.equal(value, 'mockAmount')
+      assert.equal(SendAmountRow.prototype.updateGas.callCount, 0)
       assert.equal(SendAmountRow.prototype.updateAmount.callCount, 0)
       onBlur('mockNewAmount')
+      assert.equal(SendAmountRow.prototype.updateGas.callCount, 1)
+      assert.deepEqual(
+        SendAmountRow.prototype.updateGas.getCall(0).args,
+        ['mockNewAmount']
+      )
       assert.equal(SendAmountRow.prototype.updateAmount.callCount, 1)
       assert.deepEqual(
         SendAmountRow.prototype.updateAmount.getCall(0).args,
diff --git a/ui/app/components/send_/send-content/send-content.component.js b/ui/app/components/send_/send-content/send-content.component.js
index 3a14054eb..adc114c0e 100644
--- a/ui/app/components/send_/send-content/send-content.component.js
+++ b/ui/app/components/send_/send-content/send-content.component.js
@@ -18,7 +18,7 @@ export default class SendContent extends Component {
         <div className="send-v2__form">
           <SendFromRow />
           <SendToRow updateGas={(updateData) => this.props.updateGas(updateData)} />
-          <SendAmountRow />
+          <SendAmountRow updateGas={(updateData) => this.props.updateGas(updateData)} />
           <SendGasRow />
         </div>
       </PageContainerContent>
diff --git a/ui/app/components/send_/send.component.js b/ui/app/components/send_/send.component.js
index 516251e22..219b362f2 100644
--- a/ui/app/components/send_/send.component.js
+++ b/ui/app/components/send_/send.component.js
@@ -3,6 +3,7 @@ import PropTypes from 'prop-types'
 import PersistentForm from '../../../lib/persistent-form'
 import {
   getAmountErrorObject,
+  getToAddressForGasUpdate,
   doesAmountErrorRequireUpdate,
 } from './send.utils'
 
@@ -38,7 +39,7 @@ export default class SendTransactionScreen extends PersistentForm {
     updateSendTokenBalance: PropTypes.func,
   };
 
-  updateGas ({ to } = {}) {
+  updateGas ({ to: updatedToAddress, amount: value } = {}) {
     const {
       amount,
       blockGasLimit,
@@ -48,6 +49,7 @@ export default class SendTransactionScreen extends PersistentForm {
       recentBlocks,
       selectedAddress,
       selectedToken = {},
+      to: currentToAddress,
       updateAndSetGasTotal,
     } = this.props
 
@@ -59,8 +61,8 @@ export default class SendTransactionScreen extends PersistentForm {
       recentBlocks,
       selectedAddress,
       selectedToken,
-      to: to && to.toLowerCase(),
-      value: amount,
+      to: getToAddressForGasUpdate(updatedToAddress, currentToAddress),
+      value: value || amount,
     })
   }
 
diff --git a/ui/app/components/send_/send.container.js b/ui/app/components/send_/send.container.js
index 1fd96d61f..185653c5f 100644
--- a/ui/app/components/send_/send.container.js
+++ b/ui/app/components/send_/send.container.js
@@ -19,6 +19,7 @@ import {
   getSendAmount,
   getSendEditingTransactionId,
   getSendFromObject,
+  getSendTo,
   getTokenBalance,
 } from './send.selectors'
 import {
@@ -54,6 +55,7 @@ function mapStateToProps (state) {
     recentBlocks: getRecentBlocks(state),
     selectedAddress: getSelectedAddress(state),
     selectedToken: getSelectedToken(state),
+    to: getSendTo(state),
     tokenBalance: getTokenBalance(state),
     tokenContract: getSelectedTokenContract(state),
     tokenToFiatRate: getSelectedTokenToFiatRate(state),
diff --git a/ui/app/components/send_/send.utils.js b/ui/app/components/send_/send.utils.js
index 67699be77..dfd459731 100644
--- a/ui/app/components/send_/send.utils.js
+++ b/ui/app/components/send_/send.utils.js
@@ -4,6 +4,7 @@ const {
   conversionGTE,
   multiplyCurrencies,
   conversionGreaterThan,
+  conversionLessThan,
 } = require('../../conversion-util')
 const {
   calcTokenAmount,
@@ -20,6 +21,7 @@ const abi = require('ethereumjs-abi')
 const ethUtil = require('ethereumjs-util')
 
 module.exports = {
+  addGasBuffer,
   calcGasTotal,
   calcTokenBalance,
   doesAmountErrorRequireUpdate,
@@ -27,6 +29,7 @@ module.exports = {
   estimateGasPriceFromRecentBlocks,
   generateTokenTransferData,
   getAmountErrorObject,
+  getToAddressForGasUpdate,
   isBalanceSufficient,
   isTokenBalanceSufficient,
 }
@@ -175,9 +178,8 @@ async function estimateGas ({ selectedAddress, selectedToken, blockGasLimit, to,
   }
 
   // if recipient has no code, gas is 21k max:
-  const hasRecipient = Boolean(to)
-  if (hasRecipient && !selectedToken) {
-    const code = await global.eth.getCode(to)
+  if (!selectedToken) {
+    const code = Boolean(to) && await global.eth.getCode(to)
     if (!code || code === '0x') {
       return SIMPLE_GAS_COST
     }
@@ -201,16 +203,46 @@ async function estimateGas ({ selectedAddress, selectedToken, blockGasLimit, to,
           err.message.includes('gas required exceeds allowance or always failing transaction')
         )
         if (simulationFailed) {
-          return resolve(paramsForGasEstimate.gas)
+          const estimateWithBuffer = addGasBuffer(paramsForGasEstimate.gas, blockGasLimit, 1.5)
+          return resolve(ethUtil.addHexPrefix(estimateWithBuffer))
         } else {
           return reject(err)
         }
       }
-      return resolve(estimatedGas.toString(16))
+      const estimateWithBuffer = addGasBuffer(estimatedGas.toString(16), blockGasLimit, 1.5)
+      return resolve(ethUtil.addHexPrefix(estimateWithBuffer))
     })
   })
 }
 
+function addGasBuffer (initialGasLimitHex, blockGasLimitHex, bufferMultiplier = 1.5) {
+  const upperGasLimit = multiplyCurrencies(blockGasLimitHex, 0.9, {
+    toNumericBase: 'hex',
+    multiplicandBase: 16,
+    multiplierBase: 10,
+    numberOfDecimals: '0',
+  })
+  const bufferedGasLimit = multiplyCurrencies(initialGasLimitHex, bufferMultiplier, {
+    toNumericBase: 'hex',
+    multiplicandBase: 16,
+    multiplierBase: 10,
+    numberOfDecimals: '0',
+  })
+
+  // if initialGasLimit is above blockGasLimit, dont modify it
+  if (conversionGreaterThan(
+    { value: initialGasLimitHex, fromNumericBase: 'hex' },
+    { value: upperGasLimit, fromNumericBase: 'hex' },
+  )) return initialGasLimitHex
+  // if bufferedGasLimit is below blockGasLimit, use bufferedGasLimit
+  if (conversionLessThan(
+    { value: bufferedGasLimit, fromNumericBase: 'hex' },
+    { value: upperGasLimit, fromNumericBase: 'hex' },
+  )) return bufferedGasLimit
+  // otherwise use blockGasLimit
+  return upperGasLimit
+}
+
 function generateTokenTransferData ({ toAddress = '0x0', amount = '0x0', selectedToken }) {
   if (!selectedToken) return
   return TOKEN_TRANSFER_FUNCTION_SIGNATURE + Array.prototype.map.call(
@@ -237,3 +269,7 @@ function estimateGasPriceFromRecentBlocks (recentBlocks) {
 
   return lowestPrices[Math.floor(lowestPrices.length / 2)]
 }
+
+function getToAddressForGasUpdate (...addresses) {
+  return [...addresses, ''].find(str => str !== undefined && str !== null).toLowerCase()
+}
diff --git a/ui/app/components/send_/tests/send-component.test.js b/ui/app/components/send_/tests/send-component.test.js
index 4e33d8f63..4ba9b226d 100644
--- a/ui/app/components/send_/tests/send-component.test.js
+++ b/ui/app/components/send_/tests/send-component.test.js
@@ -201,7 +201,7 @@ describe('Send Component', function () {
   })
 
   describe('updateGas', () => {
-    it('should call updateAndSetGasTotal with the correct params', () => {
+    it('should call updateAndSetGasTotal with the correct params if no to prop is passed', () => {
       propsMethodSpies.updateAndSetGasTotal.resetHistory()
       wrapper.instance().updateGas()
       assert.equal(propsMethodSpies.updateAndSetGasTotal.callCount, 1)
@@ -215,12 +215,22 @@ describe('Send Component', function () {
           recentBlocks: ['mockBlock'],
           selectedAddress: 'mockSelectedAddress',
           selectedToken: 'mockSelectedToken',
-          to: undefined,
+          to: '',
           value: 'mockAmount',
         }
       )
     })
 
+    it('should call updateAndSetGasTotal with the correct params if a to prop is passed', () => {
+      propsMethodSpies.updateAndSetGasTotal.resetHistory()
+      wrapper.setProps({ to: 'someAddress' })
+      wrapper.instance().updateGas()
+      assert.equal(
+        propsMethodSpies.updateAndSetGasTotal.getCall(0).args[0].to,
+        'someaddress',
+      )
+    })
+
     it('should call updateAndSetGasTotal with to set to lowercase if passed', () => {
       propsMethodSpies.updateAndSetGasTotal.resetHistory()
       wrapper.instance().updateGas({ to: '0xABC' })
diff --git a/ui/app/components/send_/tests/send-container.test.js b/ui/app/components/send_/tests/send-container.test.js
index 056aad148..91484f4d8 100644
--- a/ui/app/components/send_/tests/send-container.test.js
+++ b/ui/app/components/send_/tests/send-container.test.js
@@ -39,6 +39,7 @@ proxyquire('../send.container.js', {
     getSelectedTokenContract: (s) => `mockTokenContract:${s}`,
     getSelectedTokenToFiatRate: (s) => `mockTokenToFiatRate:${s}`,
     getSendAmount: (s) => `mockAmount:${s}`,
+    getSendTo: (s) => `mockTo:${s}`,
     getSendEditingTransactionId: (s) => `mockEditingTransactionId:${s}`,
     getSendFromObject: (s) => `mockFrom:${s}`,
     getTokenBalance: (s) => `mockTokenBalance:${s}`,
@@ -70,6 +71,7 @@ describe('send container', () => {
         recentBlocks: 'mockRecentBlocks:mockState',
         selectedAddress: 'mockSelectedAddress:mockState',
         selectedToken: 'mockSelectedToken:mockState',
+        to: 'mockTo:mockState',
         tokenBalance: 'mockTokenBalance:mockState',
         tokenContract: 'mockTokenContract:mockState',
         tokenToFiatRate: 'mockTokenToFiatRate:mockState',
diff --git a/ui/app/components/send_/tests/send-utils.test.js b/ui/app/components/send_/tests/send-utils.test.js
index b3f6372ef..f3d5674b7 100644
--- a/ui/app/components/send_/tests/send-utils.test.js
+++ b/ui/app/components/send_/tests/send-utils.test.js
@@ -18,10 +18,12 @@ const {
 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),
+  conversionGTE: sinon.stub().callsFake((obj1, obj2) => obj1.value >= obj2.value),
   multiplyCurrencies: sinon.stub().callsFake((a, b) => `${a}x${b}`),
   calcTokenAmount: sinon.stub().callsFake((a, d) => 'calc:' + a + d),
   rawEncode: sinon.stub().returns([16, 1100]),
+  conversionGreaterThan: sinon.stub().callsFake((obj1, obj2) => obj1.value > obj2.value),
+  conversionLessThan: sinon.stub().callsFake((obj1, obj2) => obj1.value < obj2.value),
 }
 
 const sendUtils = proxyquire('../send.utils.js', {
@@ -30,6 +32,8 @@ const sendUtils = proxyquire('../send.utils.js', {
     conversionUtil: stubs.conversionUtil,
     conversionGTE: stubs.conversionGTE,
     multiplyCurrencies: stubs.multiplyCurrencies,
+    conversionGreaterThan: stubs.conversionGreaterThan,
+    conversionLessThan: stubs.conversionLessThan,
   },
   '../../token-util': { calcTokenAmount: stubs.calcTokenAmount },
   'ethereumjs-abi': {
@@ -44,6 +48,7 @@ const {
   estimateGasPriceFromRecentBlocks,
   generateTokenTransferData,
   getAmountErrorObject,
+  getToAddressForGasUpdate,
   calcTokenBalance,
   isBalanceSufficient,
   isTokenBalanceSufficient,
@@ -255,7 +260,7 @@ describe('send utils', () => {
       estimateGasMethod: sinon.stub().callsFake(
         (data, cb) => cb(
           data.to.match(/willFailBecauseOf:/) ? { message: data.to.match(/:(.+)$/)[1] } : null,
-          { toString: (n) => `mockToString:${n}` }
+          { toString: (n) => `0xabc${n}` }
         )
       ),
     }
@@ -279,13 +284,23 @@ describe('send utils', () => {
     })
 
     it('should call ethQuery.estimateGas with the expected params', async () => {
-      const result = await estimateGas(baseMockParams)
+      const result = await sendUtils.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')
+      assert.equal(result, '0xabc16')
+    })
+
+    it('should call ethQuery.estimateGas with the expected params when initialGasLimitHex is lower than the upperGasLimit', async () => {
+      const result = await estimateGas(Object.assign({}, baseMockParams, { blockGasLimit: '0xbcd' }))
+      assert.equal(baseMockParams.estimateGasMethod.callCount, 1)
+      assert.deepEqual(
+        baseMockParams.estimateGasMethod.getCall(0).args[0],
+        Object.assign({ gasPrice: undefined, value: undefined }, baseExpectedCall, { gas: '0xbcdx0.95' })
+      )
+      assert.equal(result, '0xabc16x1.5')
     })
 
     it('should call ethQuery.estimateGas with a value of 0x0 and the expected data and to if passed a selectedToken', async () => {
@@ -300,7 +315,7 @@ describe('send utils', () => {
           to: 'mockAddress',
         })
       )
-      assert.equal(result, 'mockToString:16')
+      assert.equal(result, '0xabc16')
     })
 
     it(`should return ${SIMPLE_GAS_COST} if ethQuery.getCode does not return '0x'`, async () => {
@@ -309,6 +324,12 @@ describe('send utils', () => {
       assert.equal(result, SIMPLE_GAS_COST)
     })
 
+    it(`should return ${SIMPLE_GAS_COST} if not passed a selectedToken or truthy to address`, async () => {
+      assert.equal(baseMockParams.estimateGasMethod.callCount, 0)
+      const result = await estimateGas(Object.assign({}, baseMockParams, { to: null }))
+      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: '' } }))
@@ -401,4 +422,15 @@ describe('send utils', () => {
       assert.equal(estimateGasPriceFromRecentBlocks(mockRecentBlocks), '0x5')
     })
   })
+
+  describe('getToAddressForGasUpdate()', () => {
+    it('should return empty string if all params are undefined or null', () => {
+      assert.equal(getToAddressForGasUpdate(undefined, null), '')
+    })
+
+    it('should return the first string that is not defined or null in lower case', () => {
+      assert.equal(getToAddressForGasUpdate('A', null), 'a')
+      assert.equal(getToAddressForGasUpdate(undefined, 'B'), 'b')
+    })
+  })
 })
diff --git a/ui/app/conversion-util.js b/ui/app/conversion-util.js
index 100402d95..337763067 100644
--- a/ui/app/conversion-util.js
+++ b/ui/app/conversion-util.js
@@ -190,6 +190,16 @@ const conversionGreaterThan = (
   return firstValue.gt(secondValue)
 }
 
+const conversionLessThan = (
+  { ...firstProps },
+  { ...secondProps },
+) => {
+  const firstValue = converter({ ...firstProps })
+  const secondValue = converter({ ...secondProps })
+
+  return firstValue.lt(secondValue)
+}
+
 const conversionMax = (
   { ...firstProps },
   { ...secondProps },
@@ -229,6 +239,7 @@ module.exports = {
   addCurrencies,
   multiplyCurrencies,
   conversionGreaterThan,
+  conversionLessThan,
   conversionGTE,
   conversionLTE,
   conversionMax,