controllers - transactions - merge @frankiebee's work with mine

7 files changed, 78 insertions, 153 deletions

diff --git a/.gitignore b/.gitignore
index 0e91a7d04..21a13c904 100644
--- a/.gitignore
+++ b/.gitignore
@@ -9,6 +9,7 @@ package
 # IDEs
 .idea
 .vscode
+.sublime-project
 
 temp
 .tmp
@@ -37,4 +38,4 @@ ui/app/css/output/
 notes.txt
 
 .coveralls.yml
-.nyc_output
+.nyc_output
\ No newline at end of file
diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js
index 7cb8af3a8..f84fd95ff 100644
--- a/app/scripts/controllers/transactions/index.js
+++ b/app/scripts/controllers/transactions/index.js
@@ -78,7 +78,7 @@ class TransactionController extends EventEmitter {
     })
 
     this.txStateManager.store.subscribe(() => this.emit('update:badge'))
-    this._setupListners()
+    this._setupListeners()
     // memstore is computed from a few different stores
     this._updateMemstore()
     this.txStateManager.store.subscribe(() => this._updateMemstore())
@@ -382,8 +382,9 @@ class TransactionController extends EventEmitter {
     is called in constructor applies the listeners for pendingTxTracker txStateManager
     and blockTracker
   */
-  _setupListners () {
+  _setupListeners () {
     this.txStateManager.on('tx:status-update', this.emit.bind(this, 'tx:status-update'))
+    this._setupBlockTrackerListener()
     this.pendingTxTracker.on('tx:warning', (txMeta) => {
       this.txStateManager.updateTx(txMeta, 'transactions/pending-tx-tracker#event: tx:warning')
     })
@@ -399,13 +400,6 @@ class TransactionController extends EventEmitter {
       txMeta.retryCount++
       this.txStateManager.updateTx(txMeta, 'transactions/pending-tx-tracker#event: tx:retry')
     })
-
-    this.blockTracker.on('block', this.pendingTxTracker.checkForTxInBlock.bind(this.pendingTxTracker))
-    // this is a little messy but until ethstore has been either
-    // removed or redone this is to guard against the race condition
-    this.blockTracker.on('latest', this.pendingTxTracker.resubmitPendingTxs.bind(this.pendingTxTracker))
-    this.blockTracker.on('sync', this.pendingTxTracker.queryPendingTxs.bind(this.pendingTxTracker))
-
   }
 
   /**
@@ -429,6 +423,40 @@ class TransactionController extends EventEmitter {
     })
   }
 
+  _setupBlockTrackerListener () {
+    let listenersAreActive = false
+    const latestBlockHandler = this._onLatestBlock.bind(this)
+    const blockTracker = this.blockTracker
+    const txStateManager = this.txStateManager
+
+    txStateManager.on('tx:status-update', updateSubscription)
+    updateSubscription()
+
+    function updateSubscription() {
+      const pendingTxs = txStateManager.getPendingTransactions()
+      if (!listenersAreActive && pendingTxs.length > 0) {
+        blockTracker.on('latest', latestBlockHandler)
+        listenersAreActive = true
+      } else if (listenersAreActive && !pendingTxs.length) {
+        blockTracker.removeListener('latest', latestBlockHandler)
+        listenersAreActive = false
+      }
+    }
+  }
+
+  async _onLatestBlock (blockNumber) {
+    try {
+      await this.pendingTxTracker.updatePendingTxs()
+    } catch (err) {
+      log.error(err)
+    }
+    try {
+      await this.pendingTxTracker.resubmitPendingTxs(blockNumber)
+    } catch (err) {
+      log.error(err)
+    }
+  }
+
   /**
     Updates the memStore in transaction controller
   */
diff --git a/app/scripts/controllers/transactions/nonce-tracker.js b/app/scripts/controllers/transactions/nonce-tracker.js
index 490118c89..fe2d25fca 100644
--- a/app/scripts/controllers/transactions/nonce-tracker.js
+++ b/app/scripts/controllers/transactions/nonce-tracker.js
@@ -35,7 +35,7 @@ class NonceTracker {
    * @typedef NonceDetails
    * @property {number} highestLocallyConfirmed - A hex string of the highest nonce on a confirmed transaction.
    * @property {number} nextNetworkNonce - The next nonce suggested by the eth_getTransactionCount method.
-   * @property {number} highetSuggested - The maximum between the other two, the number returned.
+   * @property {number} highestSuggested - The maximum between the other two, the number returned.
    */
 
   /**
@@ -75,14 +75,6 @@ class NonceTracker {
     return { nextNonce, nonceDetails, releaseLock }
   }
 
-  async _getCurrentBlock () {
-    const currentBlock = this.blockTracker.getCurrentBlock()
-    if (currentBlock) return currentBlock
-    return await new Promise((reject, resolve) => {
-      this.blockTracker.once('latest', resolve)
-    })
-  }
-
   async _globalMutexFree () {
     const globalMutex = this._lookupMutex('global')
     const release = await globalMutex.acquire()
@@ -108,9 +100,8 @@ class NonceTracker {
     // calculate next nonce
     // we need to make sure our base count
     // and pending count are from the same block
-    const currentBlock = await this._getCurrentBlock()
-    const blockNumber = currentBlock.blockNumber
-    const baseCountBN = await this.ethQuery.getTransactionCount(address, blockNumber || 'latest')
+    const blockNumber = await this.blockTracker.getLatestBlock()
+    const baseCountBN = await this.ethQuery.getTransactionCount(address, blockNumber)
     const baseCount = baseCountBN.toNumber()
     assert(Number.isInteger(baseCount), `nonce-tracker - baseCount is not an integer - got: (${typeof baseCount}) "${baseCount}"`)
     const nonceDetails = { blockNumber, baseCount }
@@ -171,6 +162,7 @@ class NonceTracker {
 
     return { name: 'local', nonce: highest, details: { startPoint, highest } }
   }
+
 }
 
 module.exports = NonceTracker
diff --git a/app/scripts/controllers/transactions/pending-tx-tracker.js b/app/scripts/controllers/transactions/pending-tx-tracker.js
index e1bb67c90..e981e2991 100644
--- a/app/scripts/controllers/transactions/pending-tx-tracker.js
+++ b/app/scripts/controllers/transactions/pending-tx-tracker.js
@@ -24,60 +24,27 @@ class PendingTransactionTracker extends EventEmitter {
     super()
     this.query = new EthQuery(config.provider)
     this.nonceTracker = config.nonceTracker
-    // default is one day
     this.getPendingTransactions = config.getPendingTransactions
     this.getCompletedTransactions = config.getCompletedTransactions
     this.publishTransaction = config.publishTransaction
     this.confirmTransaction = config.confirmTransaction
-    this._checkPendingTxs()
+    this.updatePendingTxs()
   }
 
   /**
-    checks if a signed tx is in a block and
-    if it is included emits tx status as 'confirmed'
-    @param block {object}, a full block
-    @emits tx:confirmed
-    @emits tx:failed
-  */
-  async checkForTxInBlock (blockNumber) {
-    const block = await this._getBlock(blockNumber)
-    const signedTxList = this.getPendingTransactions()
-    if (!signedTxList.length) return
-    signedTxList.forEach((txMeta) => {
-      const txHash = txMeta.hash
-      const txId = txMeta.id
-
-      if (!txHash) {
-        const noTxHashErr = new Error('We had an error while submitting this transaction, please try again.')
-        noTxHashErr.name = 'NoTxHashError'
-        this.emit('tx:failed', txId, noTxHashErr)
-        return
-      }
-
-      if (!block.transactions.length) return
-
-      block.transactions.forEach((hash) => {
-        if (hash === txHash) {
-          this.confirmTransaction(txId)
-        }
-      })
-    })
-  }
-
-  /**
-    asks the network for the transaction to see if a block number is included on it
-    if we have skipped/missed blocks
-    @param object - oldBlock newBlock
+    checks the network for signed txs and releases the nonce global lock if it is
   */
-  queryPendingTxs ({ oldBlock, newBlock }) {
-    // check pending transactions on start
-    if (!oldBlock) {
-      this._checkPendingTxs()
-      return
+  async updatePendingTxs () {
+    const pendingTxs = this.getPendingTransactions()
+    // in order to keep the nonceTracker accurate we block it while updating pending transactions
+    const nonceGlobalLock = await this.nonceTracker.getGlobalLock()
+    try {
+      await Promise.all(pendingTxs.map((txMeta) => this._checkPendingTx(txMeta)))
+    } catch (err) {
+      log.error('PendingTransactionTracker - Error updating pending transactions')
+      log.error(err)
     }
-    // if we synced by more than one block, check for missed pending transactions
-    const diff = Number.parseInt(newBlock, 16) - Number.parseInt(oldBlock, 16)
-    if (diff > 1) this._checkPendingTxs()
+    nonceGlobalLock.releaseLock()
   }
 
   /**
@@ -151,6 +118,7 @@ class PendingTransactionTracker extends EventEmitter {
     this.emit('tx:retry', txMeta)
     return txHash
   }
+
   /**
     Ask the network for the transaction to see if it has been include in a block
     @param txMeta {Object} - the txMeta object
@@ -180,9 +148,8 @@ class PendingTransactionTracker extends EventEmitter {
     }
 
     // get latest transaction status
-    let txParams
     try {
-      txParams = await this.query.getTransactionByHash(txHash)
+      const txParams = await this.query.getTransactionByHash(txHash)
       if (!txParams) return
       if (txParams.blockNumber) {
         this.confirmTransaction(txId)
@@ -197,34 +164,6 @@ class PendingTransactionTracker extends EventEmitter {
   }
 
   /**
-    checks the network for signed txs and releases the nonce global lock if it is
-  */
-  async _checkPendingTxs () {
-    const signedTxList = this.getPendingTransactions()
-    // in order to keep the nonceTracker accurate we block it while updating pending transactions
-    const nonceGlobalLock = await this.nonceTracker.getGlobalLock()
-    try {
-      await Promise.all(signedTxList.map((txMeta) => this._checkPendingTx(txMeta)))
-    } catch (err) {
-      log.error('PendingTransactionWatcher - Error updating pending transactions')
-      log.error(err)
-    }
-    nonceGlobalLock.releaseLock()
-  }
-
-  async _getBlock (blockNumber) {
-    let block
-    while (!block) {
-      // block requests will sometimes return null due do the infura api
-      // being backed by multiple out-of-sync clients
-      block = await this.query.getBlockByNumber(blockNumber, false)
-      // if block is null, wait 1 sec then try again
-      if (!block) await timeout(1000)
-    }
-    return block
-  }
-
-  /**
     checks to see if a confirmed txMeta has the same nonce
     @param txMeta {Object} - txMeta object
     @returns {boolean}
diff --git a/app/scripts/lib/util.js b/app/scripts/lib/util.js
index 431d1e59c..7ceb9da3c 100644
--- a/app/scripts/lib/util.js
+++ b/app/scripts/lib/util.js
@@ -99,7 +99,21 @@ function BnMultiplyByFraction (targetBN, numerator, denominator) {
   return targetBN.mul(numBN).div(denomBN)
 }
 
+function applyListeners (listeners, emitter) {
+  Object.keys(listeners).forEach((key) => {
+    emitter.on(key, listeners[key])
+  })
+}
+
+function removeListeners (listeners, emitter) {
+  Object.keys(listeners).forEach((key) => {
+    emitter.removeListener(key, listeners[key])
+  })
+}
+
 module.exports = {
+  removeListeners,
+  applyListeners,
   getStack,
   getEnvironmentType,
   sufficientBalance,
diff --git a/test/unit/app/controllers/transactions/pending-tx-test.js b/test/unit/app/controllers/transactions/pending-tx-test.js
index c0d033007..f06f1c0dd 100644
--- a/test/unit/app/controllers/transactions/pending-tx-test.js
+++ b/test/unit/app/controllers/transactions/pending-tx-test.js
@@ -7,7 +7,7 @@ const { createTestProviderTools } = require('../../../../stub/provider')
 const PendingTransactionTracker = require('../../../../../app/scripts/controllers/transactions/pending-tx-tracker')
 const MockTxGen = require('../../../../lib/mock-tx-gen')
 const sinon = require('sinon')
-const noop = () => true
+const noop =()=>true
 const currentNetworkId = 42
 const otherNetworkId = 36
 const privKey = new Buffer('8718b9618a37d1fc78c436511fc6df3c8258d3250635bba617f33003270ec03e', 'hex')
@@ -108,56 +108,6 @@ describe('PendingTransactionTracker', function () {
     })
   })
 
-  describe('#checkForTxInBlock', function () {
-    it('should return if no pending transactions', function () {
-      // throw a type error if it trys to do anything on the block
-      // thus failing the test
-      const block = Proxy.revocable({}, {}).revoke()
-      pendingTxTracker.checkForTxInBlock(block)
-    })
-    it('should emit \'tx:failed\' if the txMeta does not have a hash', function (done) {
-      const block = Proxy.revocable({}, {}).revoke()
-      pendingTxTracker.getPendingTransactions = () => [txMetaNoHash]
-      pendingTxTracker.once('tx:failed', (txId, err) => {
-        assert(txId, txMetaNoHash.id, 'should pass txId')
-        done()
-      })
-      pendingTxTracker.checkForTxInBlock(block)
-    })
-  })
-  describe('#queryPendingTxs', function () {
-    it('should call #_checkPendingTxs if their is no oldBlock', function (done) {
-      let newBlock, oldBlock
-      newBlock = '0x01'
-      const originalFunction = pendingTxTracker._checkPendingTxs
-      pendingTxTracker._checkPendingTxs = () => { done() }
-      pendingTxTracker.queryPendingTxs({ oldBlock, newBlock })
-      pendingTxTracker._checkPendingTxs = originalFunction
-    })
-    it('should call #_checkPendingTxs if oldBlock and the newBlock have a diff of greater then 1', function (done) {
-      let newBlock, oldBlock
-      oldBlock = '0x01'
-      newBlock = '0x03'
-      const originalFunction = pendingTxTracker._checkPendingTxs
-      pendingTxTracker._checkPendingTxs = () => { done() }
-      pendingTxTracker.queryPendingTxs({ oldBlock, newBlock })
-      pendingTxTracker._checkPendingTxs = originalFunction
-    })
-    it('should not call #_checkPendingTxs if oldBlock and the newBlock have a diff of 1 or less', function (done) {
-      let newBlock, oldBlock
-      oldBlock = '0x1'
-      newBlock = '0x2'
-      const originalFunction = pendingTxTracker._checkPendingTxs
-      pendingTxTracker._checkPendingTxs = () => {
-        const err = new Error('should not call #_checkPendingTxs if oldBlock and the newBlock have a diff of 1 or less')
-        done(err)
-      }
-      pendingTxTracker.queryPendingTxs({ oldBlock, newBlock })
-      pendingTxTracker._checkPendingTxs = originalFunction
-      done()
-    })
-  })
-
   describe('#_checkPendingTx', function () {
     it('should emit \'tx:failed\' if the txMeta does not have a hash', function (done) {
       pendingTxTracker.once('tx:failed', (txId, err) => {
@@ -187,7 +137,6 @@ describe('PendingTransactionTracker', function () {
     it('should warp all txMeta\'s in #_checkPendingTx', function (done) {
       pendingTxTracker.getPendingTransactions = () => txList
       pendingTxTracker._checkPendingTx = (tx) => { tx.resolve(tx) }
-      const list = txList.map
       Promise.all(txList.map((tx) => tx.processed))
       .then((txCompletedList) => done())
       .catch(done)
@@ -201,7 +150,7 @@ describe('PendingTransactionTracker', function () {
     beforeEach(function () {
     const txMeta2 = txMeta3 = txMeta
     txList = [txMeta, txMeta2, txMeta3].map((tx) => {
-        tx.processed = new Promise ((resolve) => { tx.resolve = resolve })
+        tx.processed = new Promise((resolve) => { tx.resolve = resolve })
         return tx
       })
     })
@@ -218,7 +167,7 @@ describe('PendingTransactionTracker', function () {
       pendingTxTracker.resubmitPendingTxs(blockNuberStub)
     })
     it('should not emit \'tx:failed\' if the txMeta throws a known txError', function (done) {
-      knownErrors =[
+      knownErrors = [
         // geth
         '     Replacement transaction Underpriced            ',
         '       known transaction',
diff --git a/test/unit/app/controllers/transactions/tx-controller-test.js b/test/unit/app/controllers/transactions/tx-controller-test.js
index c450ed3ed..b0cc0acda 100644
--- a/test/unit/app/controllers/transactions/tx-controller-test.js
+++ b/test/unit/app/controllers/transactions/tx-controller-test.js
@@ -1,4 +1,5 @@
 const assert = require('assert')
+const EventEmitter = require('events')
 const ethUtil = require('ethereumjs-util')
 const EthTx = require('ethereumjs-tx')
 const EthjsQuery = require('ethjs-query')
@@ -26,12 +27,13 @@ describe('Transaction Controller', function () {
     provider = createTestProviderTools({ scaffold: providerResultStub }).provider
     query = new EthjsQuery(provider)
     fromAccount = getTestAccounts()[0]
-
+    const blockTrackerStub = new EventEmitter()
+    blockTrackerStub.getCurrentBlock = noop
     txController = new TransactionController({
       provider,
       networkStore: new ObservableStore(currentNetworkId),
       txHistoryLimit: 10,
-      blockTracker: { getCurrentBlock: noop, on: noop, once: noop },
+      blockTracker: blockTrackerStub,
       signTransaction: (ethTx) => new Promise((resolve) => {
         ethTx.sign(fromAccount.key)
         resolve()