Merge pull request #1276 from MetaMask/ImproveGasEstimates

Improve UI gas calculation logic

8 files changed, 86 insertions, 173 deletions

diff --git a/app/scripts/lib/hex-to-bn.js b/app/scripts/lib/hex-to-bn.js
new file mode 100644
index 000000000..184217279
--- /dev/null
+++ b/app/scripts/lib/hex-to-bn.js
@@ -0,0 +1,7 @@
+const ethUtil = require('ethereumjs-util')
+const BN = ethUtil.BN
+
+module.exports = function hexToBn (hex) {
+  return new BN(ethUtil.stripHexPrefix(hex), 16)
+}
+
diff --git a/app/scripts/lib/tx-utils.js b/app/scripts/lib/tx-utils.js
index 7988f83e9..72df53631 100644
--- a/app/scripts/lib/tx-utils.js
+++ b/app/scripts/lib/tx-utils.js
@@ -12,48 +12,49 @@ and used to do things like calculate gas of a tx.
 */
 
 module.exports = class txProviderUtils {
+
   constructor (provider) {
     this.provider = provider
     this.query = new EthQuery(provider)
   }
 
-  analyzeGasUsage (txData, cb) {
+  analyzeGasUsage (txMeta, cb) {
     var self = this
     this.query.getBlockByNumber('latest', true, (err, block) => {
       if (err) return cb(err)
       async.waterfall([
-        self.estimateTxGas.bind(self, txData, block.gasLimit),
-        self.setTxGas.bind(self, txData, block.gasLimit),
+        self.estimateTxGas.bind(self, txMeta, block.gasLimit),
+        self.setTxGas.bind(self, txMeta, block.gasLimit),
       ], cb)
     })
   }
 
-  estimateTxGas (txData, blockGasLimitHex, cb) {
-    const txParams = txData.txParams
+  estimateTxGas (txMeta, blockGasLimitHex, cb) {
+    const txParams = txMeta.txParams
     // check if gasLimit is already specified
-    txData.gasLimitSpecified = Boolean(txParams.gas)
+    txMeta.gasLimitSpecified = Boolean(txParams.gas)
     // if not, fallback to block gasLimit
-    if (!txData.gasLimitSpecified) {
+    if (!txMeta.gasLimitSpecified) {
       txParams.gas = blockGasLimitHex
     }
     // run tx, see if it will OOG
     this.query.estimateGas(txParams, cb)
   }
 
-  setTxGas (txData, blockGasLimitHex, estimatedGasHex, cb) {
-    txData.estimatedGas = estimatedGasHex
-    const txParams = txData.txParams
+  setTxGas (txMeta, blockGasLimitHex, estimatedGasHex, cb) {
+    txMeta.estimatedGas = estimatedGasHex
+    const txParams = txMeta.txParams
 
     // if gasLimit was specified and doesnt OOG,
     // use original specified amount
-    if (txData.gasLimitSpecified) {
-      txData.estimatedGas = txParams.gas
+    if (txMeta.gasLimitSpecified) {
+      txMeta.estimatedGas = txParams.gas
       cb()
       return
     }
     // if gasLimit not originally specified,
     // try adding an additional gas buffer to our estimation for safety
-    const recommendedGasHex = this.addGasBuffer(txData.estimatedGas, blockGasLimitHex)
+    const recommendedGasHex = this.addGasBuffer(txMeta.estimatedGas, blockGasLimitHex)
     txParams.gas = recommendedGasHex
     cb()
     return
@@ -90,16 +91,13 @@ module.exports = class txProviderUtils {
 
   // builds ethTx from txParams object
   buildEthTxFromParams (txParams) {
-    // apply gas multiplyer
-    let gasPrice = hexToBn(txParams.gasPrice)
-    // multiply and divide by 100 so as to add percision to integer mul
-    txParams.gasPrice = ethUtil.intToHex(gasPrice.toNumber())
     // normalize values
     txParams.to = normalize(txParams.to)
     txParams.from = normalize(txParams.from)
     txParams.value = normalize(txParams.value)
     txParams.data = normalize(txParams.data)
     txParams.gas = normalize(txParams.gas || txParams.gasLimit)
+    txParams.gasPrice = normalize(txParams.gasPrice)
     txParams.nonce = normalize(txParams.nonce)
     // build ethTx
     log.info(`Prepared tx for signing: ${JSON.stringify(txParams)}`)
diff --git a/app/scripts/transaction-manager.js b/app/scripts/transaction-manager.js
index 7227bdc87..a70159680 100644
--- a/app/scripts/transaction-manager.js
+++ b/app/scripts/transaction-manager.js
@@ -4,7 +4,7 @@ const extend = require('xtend')
 const Semaphore = require('semaphore')
 const ObservableStore = require('obs-store')
 const ethUtil = require('ethereumjs-util')
-const BN = require('ethereumjs-util').BN
+const EthQuery = require('eth-query')
 const TxProviderUtil = require('./lib/tx-utils')
 const createId = require('./lib/random-id')
 
@@ -20,6 +20,7 @@ module.exports = class TransactionManager extends EventEmitter {
     this.txHistoryLimit = opts.txHistoryLimit
     this.provider = opts.provider
     this.blockTracker = opts.blockTracker
+    this.query = new EthQuery(this.provider)
     this.txProviderUtils = new TxProviderUtil(this.provider)
     this.blockTracker.on('block', this.checkForTxInBlock.bind(this))
     this.signEthTx = opts.signTransaction
@@ -78,8 +79,9 @@ module.exports = class TransactionManager extends EventEmitter {
       fullTxList.splice(index, 1)
     }
     fullTxList.push(txMeta)
-
     this._saveTxList(fullTxList)
+    this.emit('update')
+
     this.once(`${txMeta.id}:signed`, function (txId) {
       this.removeAllListeners(`${txMeta.id}:rejected`)
     })
@@ -121,44 +123,38 @@ module.exports = class TransactionManager extends EventEmitter {
     async.waterfall([
       // validate
       (cb) => this.txProviderUtils.validateTxParams(txParams, cb),
-      // prepare txMeta
+      // construct txMeta
       (cb) => {
-        // create txMeta obj with parameters and meta data
-        let time = (new Date()).getTime()
-        let txId = createId()
-        txParams.metamaskId = txId
-        txParams.metamaskNetworkId = this.getNetwork()
         txMeta = {
-          id: txId,
-          time: time,
+          id: createId(),
+          time: (new Date()).getTime(),
           status: 'unapproved',
           metamaskNetworkId: this.getNetwork(),
           txParams: txParams,
         }
-        // calculate metadata for tx
-        this.txProviderUtils.analyzeGasUsage(txMeta, cb)
+        cb()
       },
+      // add default tx params
+      (cb) => this.addTxDefaults(txMeta, cb),
       // save txMeta
       (cb) => {
         this.addTx(txMeta)
-        this.setMaxTxCostAndFee(txMeta)
         cb(null, txMeta)
       },
     ], done)
   }
 
-  setMaxTxCostAndFee (txMeta) {
-    var txParams = txMeta.txParams
-    var gasCost = new BN(ethUtil.stripHexPrefix(txParams.gas || txMeta.estimatedGas), 16)
-    var gasPrice = new BN(ethUtil.stripHexPrefix(txParams.gasPrice || '0x4a817c800'), 16)
-    var txFee = gasCost.mul(gasPrice)
-    var txValue = new BN(ethUtil.stripHexPrefix(txParams.value || '0x0'), 16)
-    var maxCost = txValue.add(txFee)
-    txMeta.txFee = txFee
-    txMeta.txValue = txValue
-    txMeta.maxCost = maxCost
-    txMeta.gasPrice = gasPrice
-    this.updateTx(txMeta)
+  addTxDefaults (txMeta, cb) {
+    const txParams = txMeta.txParams
+    // ensure value
+    txParams.value = txParams.value || '0x0'
+    this.query.gasPrice((err, gasPrice) => {
+      if (err) return cb(err)
+      // set gasPrice
+      txParams.gasPrice = gasPrice
+      // set gasLimit
+      this.txProviderUtils.analyzeGasUsage(txMeta, cb)
+    })
   }
 
   getUnapprovedTxList () {
@@ -336,7 +332,7 @@ module.exports = class TransactionManager extends EventEmitter {
         }
         return this.setTxStatusFailed(txId, errReason)
       }
-      this.txProviderUtils.query.getTransactionByHash(txHash, (err, txParams) => {
+      this.query.getTransactionByHash(txHash, (err, txParams) => {
         if (err || !txParams) {
           if (!txParams) return
           txMeta.err = {
diff --git a/ui/app/accounts/index.js b/ui/app/accounts/index.js
index e236a4e85..ae69d9297 100644
--- a/ui/app/accounts/index.js
+++ b/ui/app/accounts/index.js
@@ -11,7 +11,7 @@ module.exports = connect(mapStateToProps)(AccountsScreen)
 
 function mapStateToProps (state) {
   const pendingTxs = valuesFor(state.metamask.unapprovedTxs)
-  .filter(tx => tx.txParams.metamaskNetworkId === state.metamask.network)
+  .filter(txMeta => txMeta.metamaskNetworkId === state.metamask.network)
   const pendingMsgs = valuesFor(state.metamask.unapprovedMsgs)
   const pending = pendingTxs.concat(pendingMsgs)
 
diff --git a/ui/app/actions.js b/ui/app/actions.js
index 7288db256..19c0e66ad 100644
--- a/ui/app/actions.js
+++ b/ui/app/actions.js
@@ -421,6 +421,7 @@ function updateAndApproveTx (txData) {
   return (dispatch) => {
     log.debug(`actions calling background.updateAndApproveTx`)
     background.updateAndApproveTransaction(txData, (err) => {
+      dispatch(actions.hideLoadingIndication())
       if (err) {
         dispatch(actions.txError(err))
         return console.error(err.message)
diff --git a/ui/app/components/pending-tx.js b/ui/app/components/pending-tx.js
index 5bb088af9..1b83f5043 100644
--- a/ui/app/components/pending-tx.js
+++ b/ui/app/components/pending-tx.js
@@ -2,11 +2,11 @@ const Component = require('react').Component
 const connect = require('react-redux').connect
 const h = require('react-hyperscript')
 const inherits = require('util').inherits
-const extend = require('xtend')
 const actions = require('../actions')
 
 const ethUtil = require('ethereumjs-util')
 const BN = ethUtil.BN
+const hexToBn = require('../../../app/scripts/lib/hex-to-bn')
 
 const MiniAccountPanel = require('./mini-account-panel')
 const EthBalance = require('./eth-balance')
@@ -29,42 +29,43 @@ function PendingTx () {
   Component.call(this)
   this.state = {
     valid: true,
-    gas: null,
-    gasPrice: null,
     txData: null,
   }
 }
 
 PendingTx.prototype.render = function () {
   const props = this.props
-  const state = this.state
 
-  const txData = state.txData || props.txData
-  const txParams = txData.txParams || {}
+  const txMeta = this.gatherTxMeta()
+  const txParams = txMeta.txParams || {}
 
   const address = txParams.from || props.selectedAddress
   const identity = props.identities[address] || { address: address }
   const account = props.accounts[address]
   const balance = account ? account.balance : '0x0'
 
-  const gas = state.gas || txParams.gas
-  const gasPrice = state.gasPrice || txData.gasPrice
-  const gasBn = new BN(gas, 16)
-  const gasPriceBn = new BN(gasPrice, 16)
+  const gas = txParams.gas
+  const gasPrice = txParams.gasPrice
+
+  const gasBn = hexToBn(gas)
+  const gasPriceBn = hexToBn(gasPrice)
 
   const txFeeBn = gasBn.mul(gasPriceBn)
-  const valueBn = new BN(ethUtil.stripHexPrefix(txParams.value), 16)
+  const valueBn = hexToBn(txParams.value)
   const maxCost = txFeeBn.add(valueBn)
 
   const dataLength = txParams.data ? (txParams.data.length - 2) / 2 : 0
   const imageify = props.imageifyIdenticons === undefined ? true : props.imageifyIdenticons
 
+  const balanceBn = hexToBn(balance)
+  const insufficientBalance = balanceBn.lt(maxCost)
+
   this.inputs = []
 
   return (
 
     h('div', {
-      key: txData.id,
+      key: txMeta.id,
     }, [
 
       h('form#pending-tx-form', {
@@ -73,9 +74,8 @@ PendingTx.prototype.render = function () {
           const form = document.querySelector('form#pending-tx-form')
           const valid = form.checkValidity()
           this.setState({ valid })
-
           if (valid && this.verifyGasParams()) {
-            props.sendTransaction(txData, event)
+            props.sendTransaction(txMeta, event)
           } else {
             this.props.dispatch(actions.displayWarning('Invalid Gas Parameters'))
           }
@@ -162,7 +162,8 @@ PendingTx.prototype.render = function () {
                 h(HexInput, {
                   name: 'Gas Limit',
                   value: gas,
-                  min: MIN_GAS_LIMIT_BN.toString(10), // The hard lower limit for gas.
+                  // The hard lower limit for gas.
+                  min: MIN_GAS_LIMIT_BN.toString(10),
                   suffix: 'UNITS',
                   style: {
                     position: 'relative',
@@ -170,7 +171,9 @@ PendingTx.prototype.render = function () {
                   },
                   onChange: (newHex) => {
                     log.info(`Gas limit changed to ${newHex}`)
-                    this.setState({ gas: newHex })
+                    const txMeta = this.gatherTxMeta()
+                    txMeta.txParams.gas = newHex
+                    this.setState({ txData: txMeta })
                   },
                   ref: (hexInput) => { this.inputs.push(hexInput) },
                 }),
@@ -193,7 +196,9 @@ PendingTx.prototype.render = function () {
                   },
                   onChange: (newHex) => {
                     log.info(`Gas price changed to: ${newHex}`)
-                    this.setState({ gasPrice: newHex })
+                    const txMeta = this.gatherTxMeta()
+                    txMeta.txParams.gasPrice = newHex
+                    this.setState({ txData: txMeta })
                   },
                   ref: (hexInput) => { this.inputs.push(hexInput) },
                 }),
@@ -255,7 +260,7 @@ PendingTx.prototype.render = function () {
           }
         `),
 
-        txData.simulationFails ?
+        txMeta.simulationFails ?
           h('.error', {
             style: {
               marginLeft: 50,
@@ -264,7 +269,7 @@ PendingTx.prototype.render = function () {
           }, 'Transaction Error. Exception thrown in contract code.')
         : null,
 
-        props.insufficientBalance ?
+        insufficientBalance ?
           h('span.error', {
             style: {
               marginLeft: 50,
@@ -283,7 +288,7 @@ PendingTx.prototype.render = function () {
         }, [
 
 
-          props.insufficientBalance ?
+          insufficientBalance ?
             h('button', {
               onClick: props.buyEth,
             }, 'Buy Ether')
@@ -301,7 +306,7 @@ PendingTx.prototype.render = function () {
             type: 'submit',
             value: 'ACCEPT',
             style: { marginLeft: '10px' },
-            disabled: props.insufficientBalance || !this.state.valid,
+            disabled: insufficientBalance || !this.state.valid,
           }),
 
           h('button.cancel.btn-red', {
@@ -313,10 +318,6 @@ PendingTx.prototype.render = function () {
   )
 }
 
-PendingTx.prototype.validChanged = function (newValid) {
-  this.setState({ valid: newValid })
-}
-
 PendingTx.prototype.miniAccountPanelForRecipient = function () {
   const props = this.props
   const txData = props.txData
@@ -358,63 +359,8 @@ PendingTx.prototype.miniAccountPanelForRecipient = function () {
   }
 }
 
-PendingTx.prototype.componentDidUpdate = function (prevProps, previousState) {
-  log.debug(`pending-tx componentDidUpdate`)
-  const state = this.state || {}
-  const prevState = previousState || {}
-  const { gas, gasPrice } = state
-
-  // Only if gas or gasPrice changed:
-  if (!prevState ||
-      (gas !== prevState.gas ||
-      gasPrice !== prevState.gasPrice)) {
-    log.debug(`recalculating gas since prev state change: ${JSON.stringify({ prevState, state })}`)
-    this.calculateGas()
-  }
-}
-
-PendingTx.prototype.calculateGas = function () {
-  const state = this.state
-  const props = this.props
-  const txData = props.txData
-
-  const txMeta = this.gatherParams()
-  log.debug(`pending-tx calculating gas for ${JSON.stringify(txMeta)}`)
-
-  const txParams = txMeta.txParams
-  const gasLimit = new BN(ethUtil.stripHexPrefix(txParams.gas || txMeta.estimatedGas), 16)
-  const gasPriceHex = state.gasPrice || txData.gasPrice
-  const gasPrice = new BN(ethUtil.stripHexPrefix(gasPriceHex), 16)
-
-  const valid = !gasPrice.lt(MIN_GAS_PRICE_BN) && !gasLimit.lt(MIN_GAS_LIMIT_BN)
-  this.validChanged(valid)
-
-  const txFee = gasLimit.mul(gasPrice)
-  const txValue = new BN(ethUtil.stripHexPrefix(txParams.value || '0x0'), 16)
-  const maxCost = txValue.add(txFee)
-
-  const txFeeHex = '0x' + txFee.toString('hex')
-  const maxCostHex = '0x' + maxCost.toString('hex')
-
-  txMeta.txFee = txFeeHex
-  txMeta.maxCost = maxCostHex
-  txMeta.txParams.gasPrice = gasPriceHex
-
-  const newState = {
-    txFee: '0x' + txFee.toString('hex'),
-    maxCost: '0x' + maxCost.toString('hex'),
-  }
-  log.info(`tx form updating local state with ${JSON.stringify(newState)}`)
-  this.setState(newState)
-
-  if (this.props.onTxChange) {
-    this.props.onTxChange(txMeta)
-  }
-}
-
 PendingTx.prototype.resetGasFields = function () {
   log.debug(`pending-tx resetGasFields`)
-  const txData = this.props.txData
 
   this.inputs.forEach((hexInput) => {
     if (hexInput) {
@@ -423,36 +369,29 @@ PendingTx.prototype.resetGasFields = function () {
   })
 
   this.setState({
-    gas: txData.txParams.gas,
-    gasPrice: txData.gasPrice,
+    txData: null,
     valid: true,
   })
 }
 
 // After a customizable state value has been updated,
-PendingTx.prototype.gatherParams = function () {
-  log.debug(`pending-tx gatherParams`)
+PendingTx.prototype.gatherTxMeta = function () {
+  log.debug(`pending-tx gatherTxMeta`)
   const props = this.props
-  const state = this.state || {}
+  const state = this.state
   const txData = state.txData || props.txData
-  const txParams = txData.txParams
-  const gas = state.gas || txParams.gas
-  const gasPrice = state.gasPrice || txParams.gasPrice
-  const resultTx = extend(txParams, {
-    gas,
-    gasPrice,
-  })
-  const resultTxMeta = extend(txData, {
-    txParams: resultTx,
-  })
-  log.debug(`UI has computed tx params ${JSON.stringify(resultTx)}`)
-  return resultTxMeta
+
+  log.debug(`UI has defaulted to tx meta ${JSON.stringify(txData)}`)
+  return txData
 }
 
 PendingTx.prototype.verifyGasParams = function () {
   // We call this in case the gas has not been modified at all
   if (!this.state) { return true }
-  return this._notZeroOrEmptyString(this.state.gas) && this._notZeroOrEmptyString(this.state.gasPrice)
+  return (
+    this._notZeroOrEmptyString(this.state.gas) &&
+    this._notZeroOrEmptyString(this.state.gasPrice)
+  )
 }
 
 PendingTx.prototype._notZeroOrEmptyString = function (obj) {
diff --git a/ui/app/conf-tx.js b/ui/app/conf-tx.js
index 54c171a8a..3b8618992 100644
--- a/ui/app/conf-tx.js
+++ b/ui/app/conf-tx.js
@@ -7,8 +7,6 @@ const actions = require('./actions')
 const NetworkIndicator = require('./components/network')
 const txHelper = require('../lib/tx-helper')
 const isPopupOrNotification = require('../../app/scripts/lib/is-popup-or-notification')
-const ethUtil = require('ethereumjs-util')
-const BN = ethUtil.BN
 
 const PendingTx = require('./components/pending-tx')
 const PendingMsg = require('./components/pending-msg')
@@ -104,9 +102,6 @@ ConfirmTxScreen.prototype.render = function () {
           selectedAddress: props.selectedAddress,
           accounts: props.accounts,
           identities: props.identities,
-          insufficientBalance: this.checkBalanceAgainstTx(txData),
-          // State actions
-          onTxChange: this.onTxChange.bind(this),
           // Actions
           buyEth: this.buyEth.bind(this, txParams.from || props.selectedAddress),
           sendTransaction: this.sendTransaction.bind(this, txData),
@@ -145,37 +140,14 @@ function currentTxView (opts) {
   }
 }
 
-ConfirmTxScreen.prototype.checkBalanceAgainstTx = function (txData) {
-  if (!txData.txParams) return false
-  var props = this.props
-  var address = txData.txParams.from || props.selectedAddress
-  var account = props.accounts[address]
-  var balance = account ? account.balance : '0x0'
-  var maxCost = new BN(txData.maxCost, 16)
-
-  var balanceBn = new BN(ethUtil.stripHexPrefix(balance), 16)
-  return maxCost.gt(balanceBn)
-}
-
 ConfirmTxScreen.prototype.buyEth = function (address, event) {
   this.stopPropagation(event)
   this.props.dispatch(actions.buyEthView(address))
 }
 
-// Allows the detail view to update the gas calculations,
-// for manual gas controls.
-ConfirmTxScreen.prototype.onTxChange = function (txData) {
-  log.debug(`conf-tx onTxChange triggered with ${JSON.stringify(txData)}`)
-  this.setState({ txData })
-}
-
-// Must default to any local state txData,
-// to allow manual override of gas calculations.
 ConfirmTxScreen.prototype.sendTransaction = function (txData, event) {
   this.stopPropagation(event)
-  const state = this.state || {}
-  const txMeta = state.txData
-  this.props.dispatch(actions.updateAndApproveTx(txMeta || txData))
+  this.props.dispatch(actions.updateAndApproveTx(txData))
 }
 
 ConfirmTxScreen.prototype.cancelTransaction = function (txData, event) {
diff --git a/ui/lib/tx-helper.js b/ui/lib/tx-helper.js
index 2eefdff68..ec19daf64 100644
--- a/ui/lib/tx-helper.js
+++ b/ui/lib/tx-helper.js
@@ -4,7 +4,7 @@ module.exports = function (unapprovedTxs, unapprovedMsgs, personalMsgs, network)
   log.debug('tx-helper called with params:')
   log.debug({ unapprovedTxs, unapprovedMsgs, personalMsgs, network })
 
-  const txValues = network ? valuesFor(unapprovedTxs).filter(tx => tx.txParams.metamaskNetworkId === network) : valuesFor(unapprovedTxs)
+  const txValues = network ? valuesFor(unapprovedTxs).filter(txMeta => txMeta.metamaskNetworkId === network) : valuesFor(unapprovedTxs)
   log.debug(`tx helper found ${txValues.length} unapproved txs`)
   const msgValues = valuesFor(unapprovedMsgs)
   log.debug(`tx helper found ${msgValues.length} unsigned messages`)
@@ -13,5 +13,5 @@ module.exports = function (unapprovedTxs, unapprovedMsgs, personalMsgs, network)
   log.debug(`tx helper found ${personalValues.length} unsigned personal messages`)
   allValues = allValues.concat(personalValues)
 
-  return allValues.sort(tx => tx.time)
+  return allValues.sort(txMeta => txMeta.time)
 }