drop transactions who's nonce is lower then the known network nonce but were not included in a block (#6388)

* transactions/pending - check nonce against the network and mark as dropped if not included in a block

* transactions/pending - unifiy "dropped" txs

* transactions/pending - test - fix for new expected behavior

* fix comment

* transactions/pending - clean up dropped event

* fix spelling

Co-Authored-By: frankiebee <frankie.diamond@gmail.com>

* nit fix

* test/tx-pending - clarify test description

3 files changed, 113 insertions, 13 deletions

diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js
index 79dba7833..dd497a11e 100644
--- a/app/scripts/controllers/transactions/index.js
+++ b/app/scripts/controllers/transactions/index.js
@@ -555,6 +555,7 @@ class TransactionController extends EventEmitter {
     })
     this.pendingTxTracker.on('tx:failed', this.txStateManager.setTxStatusFailed.bind(this.txStateManager))
     this.pendingTxTracker.on('tx:confirmed', (txId) => this.confirmTransaction(txId))
+    this.pendingTxTracker.on('tx:dropped', this.txStateManager.setTxStatusDropped.bind(this.txStateManager))
     this.pendingTxTracker.on('tx:block-update', (txMeta, latestBlockNumber) => {
       if (!txMeta.firstRetryBlockNumber) {
         txMeta.firstRetryBlockNumber = latestBlockNumber
diff --git a/app/scripts/controllers/transactions/pending-tx-tracker.js b/app/scripts/controllers/transactions/pending-tx-tracker.js
index 4bf40b1db..bc11f6633 100644
--- a/app/scripts/controllers/transactions/pending-tx-tracker.js
+++ b/app/scripts/controllers/transactions/pending-tx-tracker.js
@@ -22,6 +22,7 @@ const EthQuery = require('ethjs-query')
 class PendingTransactionTracker extends EventEmitter {
   constructor (config) {
     super()
+    this.droppedBuffer = {}
     this.query = new EthQuery(config.provider)
     this.nonceTracker = config.nonceTracker
     this.getPendingTransactions = config.getPendingTransactions
@@ -139,22 +140,42 @@ class PendingTransactionTracker extends EventEmitter {
       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 another tx with the same nonce is mined, set as failed.
+    // If another tx with the same nonce is mined, set as dropped.
     const taken = await this._checkIfNonceIsTaken(txMeta)
-    if (taken) {
-      const nonceTakenErr = new Error('Another transaction with this nonce has been mined.')
-      nonceTakenErr.name = 'NonceTakenErr'
-      return this.emit('tx:failed', txId, nonceTakenErr)
+    let dropped
+    try {
+      // check the network if the nonce is ahead the tx
+      // and the tx has not been mined into a block
+
+      dropped = await this._checkIftxWasDropped(txMeta)
+      // the dropped buffer is in case we ask a node for the tx
+      // that is behind the node we asked for tx count
+      // IS A SECURITY FOR HITTING NODES IN INFURA THAT COULD GO OUT
+      // OF SYNC.
+      // on the next block event it will return fire as dropped
+      if (dropped && !this.droppedBuffer[txHash]) {
+        this.droppedBuffer[txHash] = true
+        dropped = false
+      } else if (dropped && this.droppedBuffer[txHash]) {
+        // clean up
+        delete this.droppedBuffer[txHash]
+      }
+
+    } catch (e) {
+      log.error(e)
+    }
+    if (taken || dropped) {
+      return this.emit('tx:dropped', txId)
     }
 
     // get latest transaction status
     try {
-      const txParams = await this.query.getTransactionByHash(txHash)
-      if (!txParams) return
-      if (txParams.blockNumber) {
+      const { blockNumber } = await this.query.getTransactionByHash(txHash) || {}
+      if (blockNumber) {
         this.emit('tx:confirmed', txId)
       }
     } catch (err) {
@@ -165,6 +186,22 @@ class PendingTransactionTracker extends EventEmitter {
       this.emit('tx:warning', txMeta, err)
     }
   }
+    /**
+    checks to see if if the tx's nonce has been used by another transaction
+    @param txMeta {Object} - txMeta object
+    @emits tx:dropped
+    @returns {boolean}
+  */
+
+  async _checkIftxWasDropped (txMeta) {
+    const { txParams: { nonce, from }, hash } = txMeta
+    const nextNonce = await this.query.getTransactionCount(from)
+    const { blockNumber } = await this.query.getTransactionByHash(hash) || {}
+    if (!blockNumber && parseInt(nextNonce) > parseInt(nonce)) {
+        return true
+    }
+    return false
+  }
 
   /**
     checks to see if a confirmed txMeta has the same nonce
diff --git a/test/unit/app/controllers/transactions/pending-tx-test.js b/test/unit/app/controllers/transactions/pending-tx-test.js
index 1c5f59f5a..b37ac2766 100644
--- a/test/unit/app/controllers/transactions/pending-tx-test.js
+++ b/test/unit/app/controllers/transactions/pending-tx-test.js
@@ -58,7 +58,7 @@ describe('PendingTransactionTracker', function () {
       }
     })
 
-    it('should become failed if another tx with the same nonce succeeds', async function () {
+    it('should emit dropped if another tx with the same nonce succeeds', async function () {
 
       // SETUP
       const txGen = new MockTxGen()
@@ -84,17 +84,16 @@ describe('PendingTransactionTracker', function () {
 
       // THE EXPECTATION
       const spy = sinon.spy()
-      pendingTxTracker.on('tx:failed', (txId, err) => {
+      pendingTxTracker.on('tx:dropped', (txId) => {
         assert.equal(txId, pending.id, 'should fail the pending tx')
-        assert.equal(err.name, 'NonceTakenErr', 'should emit a nonce taken error.')
-        spy(txId, err)
+        spy(txId)
       })
 
       // THE METHOD
       await pendingTxTracker._checkPendingTx(pending)
 
       // THE ASSERTION
-      assert.ok(spy.calledWith(pending.id), 'tx failed should be emitted')
+      assert.ok(spy.calledWith(pending.id), 'tx dropped should be emitted')
     })
   })
 
@@ -107,6 +106,38 @@ describe('PendingTransactionTracker', function () {
       pendingTxTracker._checkPendingTx(txMetaNoHash)
     })
 
+    it('should emit tx:dropped with the txMetas id only after the second call', function (done) {
+    txMeta = {
+      id: 1,
+      hash: '0x0593ee121b92e10d63150ad08b4b8f9c7857d1bd160195ee648fb9a0f8d00eeb',
+      status: 'submitted',
+      txParams: {
+        from: '0x1678a085c290ebd122dc42cba69373b5953b831d',
+        nonce: '0x1',
+        value: '0xfffff',
+      },
+      history: [{}],
+      rawTx: '0xf86c808504a817c800827b0d940c62bb85faa3311a998d3aba8098c1235c564966880de0b6b3a7640000802aa08ff665feb887a25d4099e40e11f0fef93ee9608f404bd3f853dd9e84ed3317a6a02ec9d3d1d6e176d4d2593dd760e74ccac753e6a0ea0d00cc9789d0d7ff1f471d',
+    }
+
+      providerResultStub['eth_getTransactionCount'] = '0x02'
+      providerResultStub['eth_getTransactionByHash'] = {}
+      pendingTxTracker.once('tx:dropped', (id) => {
+        if (id === txMeta.id) {
+          delete providerResultStub['eth_getTransactionCount']
+          delete providerResultStub['eth_getTransactionByHash']
+          return done()
+        } else {
+          done(new Error('wrong tx Id'))
+        }
+      })
+
+      pendingTxTracker._checkPendingTx(txMeta).then(() => {
+        pendingTxTracker._checkPendingTx(txMeta).catch(done)
+      }).catch(done)
+    })
+
+
     it('should should return if query does not return txParams', function () {
       providerResultStub.eth_getTransactionByHash = null
       pendingTxTracker._checkPendingTx(txMeta)
@@ -283,6 +314,37 @@ describe('PendingTransactionTracker', function () {
     })
   })
 
+  describe('#_checkIftxWasDropped', () => {
+    const txMeta = {
+      id: 1,
+      hash: '0x0593ee121b92e10d63150ad08b4b8f9c7857d1bd160195ee648fb9a0f8d00eeb',
+      status: 'submitted',
+      txParams: {
+        from: '0x1678a085c290ebd122dc42cba69373b5953b831d',
+        nonce: '0x1',
+        value: '0xfffff',
+      },
+      rawTx: '0xf86c808504a817c800827b0d940c62bb85faa3311a998d3aba8098c1235c564966880de0b6b3a7640000802aa08ff665feb887a25d4099e40e11f0fef93ee9608f404bd3f853dd9e84ed3317a6a02ec9d3d1d6e176d4d2593dd760e74ccac753e6a0ea0d00cc9789d0d7ff1f471d',
+    }
+    it('should return false when the nonce is the suggested network nonce', (done) => {
+      providerResultStub['eth_getTransactionCount'] = '0x01'
+      providerResultStub['eth_getTransactionByHash'] = {}
+      pendingTxTracker._checkIftxWasDropped(txMeta).then((dropped) => {
+        assert(!dropped, 'should be false')
+        done()
+      }).catch(done)
+    })
+
+    it('should return true when the network nonce is higher then the txMeta nonce', function (done) {
+      providerResultStub['eth_getTransactionCount'] = '0x02'
+      providerResultStub['eth_getTransactionByHash'] = {}
+      pendingTxTracker._checkIftxWasDropped(txMeta).then((dropped) => {
+        assert(dropped, 'should be true')
+        done()
+      }).catch(done)
+    })
+  })
+
   describe('#_checkIfNonceIsTaken', function () {
     beforeEach(function () {
       const confirmedTxList = [{