transactions/pending - buffer 3 blocks before dropping a tx (#7483)

* transactions/pending - buffer 3 blocks before dropping a tx

* transactions/pending - only increment for dropped txs

app/scripts/controllers/transactions/index.js

@@ -502,7 +502,7 @@ class TransactionController extends EventEmitter {
    * @param {number} txId - The tx's ID
    * @returns {Promise<void>}
    */
-  async confirmTransaction (txId) {
+  async confirmTransaction (txId, txReceipt) {
     // get the txReceipt before marking the transaction confirmed
     // to ensure the receipt is gotten before the ui revives the tx
     const txMeta = this.txStateManager.getTx(txId)
@@ -512,7 +512,6 @@ class TransactionController extends EventEmitter {
     }
 
     try {
-      const txReceipt = await this.query.getTransactionReceipt(txMeta.hash)
 
       // It seems that sometimes the numerical values being returned from
       // this.query.getTransactionReceipt are BN instances and not strings.
@@ -626,7 +625,7 @@ class TransactionController extends EventEmitter {
       this.txStateManager.updateTx(txMeta, 'transactions/pending-tx-tracker#event: tx:warning')
     })
     this.pendingTxTracker.on('tx:failed', this.txStateManager.setTxStatusFailed.bind(this.txStateManager))
-    this.pendingTxTracker.on('tx:confirmed', (txId) => this.confirmTransaction(txId))
+    this.pendingTxTracker.on('tx:confirmed', (txId, transactionReceipt) => this.confirmTransaction(txId, transactionReceipt))
     this.pendingTxTracker.on('tx:dropped', this.txStateManager.setTxStatusDropped.bind(this.txStateManager))
     this.pendingTxTracker.on('tx:block-update', (txMeta, latestBlockNumber) => {
       if (!txMeta.firstRetryBlockNumber) {

app/scripts/controllers/transactions/pending-tx-tracker.js

@@ -132,6 +132,7 @@ class PendingTransactionTracker extends EventEmitter {
     Ask the network for the transaction to see if it has been include in a block
     @param txMeta {Object} - the txMeta object
     @emits tx:failed
+    @emits tx:dropped
     @emits tx:confirmed
     @emits tx:warning
   */
@@ -153,6 +154,9 @@ class PendingTransactionTracker extends EventEmitter {
 
       return
     }
+    // *note to self* hard failure point
+    const transactionReceipt = await this.query.getTransactionReceipt(txHash)
+
 
     // If another tx with the same nonce is mined, set as dropped.
     const taken = await this._checkIfNonceIsTaken(txMeta)
@@ -161,16 +165,23 @@ class PendingTransactionTracker extends EventEmitter {
       // 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)
+      dropped = await this._checkIftxWasDropped(txMeta, transactionReceipt)
       // 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
+      if (typeof this.droppedBuffer[txHash] !== 'number') {
+        this.droppedBuffer[txHash] = 0
+      }
+
+      // 3 block count buffer
+      if (dropped && this.droppedBuffer[txHash] < 3) {
         dropped = false
-      } else if (dropped && this.droppedBuffer[txHash]) {
+        ++this.droppedBuffer[txHash]
+      }
+
+      if (dropped && this.droppedBuffer[txHash] === 3) {
         // clean up
         delete this.droppedBuffer[txHash]
       }
@@ -184,9 +195,9 @@ class PendingTransactionTracker extends EventEmitter {
 
     // get latest transaction status
     try {
-      const { blockNumber } = await this.query.getTransactionReceipt(txHash) || {}
+      const { blockNumber } = transactionReceipt
       if (blockNumber) {
-        this.emit('tx:confirmed', txId)
+        this.emit('tx:confirmed', txId, transactionReceipt)
       }
     } catch (err) {
       txMeta.warning = {
@@ -199,14 +210,14 @@ class PendingTransactionTracker extends EventEmitter {
   /**
     checks to see if if the tx's nonce has been used by another transaction
     @param txMeta {Object} - txMeta object
+    @param transactionReceipt {Object} - transactionReceipt object
     @emits tx:dropped
     @returns {boolean}
   */
 
-  async _checkIftxWasDropped (txMeta) {
-    const { txParams: { nonce, from }, hash } = txMeta
+  async _checkIftxWasDropped (txMeta, { blockNumber }) {
+    const { txParams: { nonce, from } } = txMeta
     const nextNonce = await this.query.getTransactionCount(from)
-    const { blockNumber } = await this.query.getTransactionReceipt(hash) || {}
     if (!blockNumber && parseInt(nextNonce) > parseInt(nonce)) {
       return true
     }
@@ -214,7 +225,7 @@ class PendingTransactionTracker extends EventEmitter {
   }
 
   /**
-    checks to see if a confirmed txMeta has the same nonce
+    checks local txs to see if a confirmed txMeta has the same nonce
     @param txMeta {Object} - txMeta object
     @returns {boolean}
   */
@@ -224,6 +235,9 @@ class PendingTransactionTracker extends EventEmitter {
     const address = txMeta.txParams.from
     const completed = this.getCompletedTransactions(address)
     const sameNonce = completed.filter((otherMeta) => {
+      if (otherMeta.id === txMeta.id) {
+        return false
+      }
       return otherMeta.txParams.nonce === txMeta.txParams.nonce
     })
     return sameNonce.length > 0

test/unit/app/controllers/transactions/pending-tx-test.js

@@ -74,16 +74,14 @@ describe('PendingTransactionTracker', function () {
         value: '0x01',
         hash: '0xbad',
         status: 'confirmed',
-        nonce: '0x01',
-      }, { count: 1 })
+      }, { count: 1, fromNonce: '0x01' })
 
       const pending = txGen.generate({
         id: '123',
         value: '0x02',
-        hash: '0xfad',
+        hash: '0x2a919d2512ec963f524bfd9730fb66b6d5a2e399d1dd957abb5e2b544a12644b',
         status: 'submitted',
-        nonce: '0x01',
-      }, { count: 1 })[0]
+      }, { count: 1, fromNonce: '0x01' })[0]
 
       stub = sinon.stub(pendingTxTracker, 'getCompletedTransactions')
         .returns(txGen.txs)
@@ -112,7 +110,7 @@ describe('PendingTransactionTracker', function () {
       pendingTxTracker._checkPendingTx(txMetaNoHash)
     })
 
-    it('should emit tx:dropped with the txMetas id only after the second call', function (done) {
+    it('should emit tx:dropped with the txMetas id only after the fourth call', function (done) {
       txMeta = {
         id: 1,
         hash: '0x0593ee121b92e10d63150ad08b4b8f9c7857d1bd160195ee648fb9a0f8d00eeb',
@@ -126,20 +124,32 @@ describe('PendingTransactionTracker', function () {
         rawTx: '0xf86c808504a817c800827b0d940c62bb85faa3311a998d3aba8098c1235c564966880de0b6b3a7640000802aa08ff665feb887a25d4099e40e11f0fef93ee9608f404bd3f853dd9e84ed3317a6a02ec9d3d1d6e176d4d2593dd760e74ccac753e6a0ea0d00cc9789d0d7ff1f471d',
       }
 
+      let counter = 0
       providerResultStub['eth_getTransactionCount'] = '0x02'
-      providerResultStub['eth_getTransactionByHash'] = {}
+      providerResultStub['eth_getTransactionReceipt'] = {}
       pendingTxTracker.once('tx:dropped', (id) => {
         if (id === txMeta.id) {
           delete providerResultStub['eth_getTransactionCount']
-          delete providerResultStub['eth_getTransactionByHash']
-          return done()
+          delete providerResultStub['eth_getTransactionReceipt']
+          if (counter === 3) {
+            return done()
+          } else {
+            return done(new Error(`Counter does not equal 3 got ${counter} instead`))
+          }
         } else {
           done(new Error('wrong tx Id'))
         }
       })
 
       pendingTxTracker._checkPendingTx(txMeta).then(() => {
-        pendingTxTracker._checkPendingTx(txMeta).catch(done)
+        ++counter
+        pendingTxTracker._checkPendingTx(txMeta).then(() => {
+          ++counter
+          pendingTxTracker._checkPendingTx(txMeta).then(() => {
+            ++counter
+            pendingTxTracker._checkPendingTx(txMeta)
+          })
+        })
       }).catch(done)
     })
 
@@ -344,8 +354,8 @@ describe('PendingTransactionTracker', function () {
     }
     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) => {
+      providerResultStub['eth_getTransactionReceipt'] = {}
+      pendingTxTracker._checkIftxWasDropped(txMeta, {}).then((dropped) => {
         assert(!dropped, 'should be false')
         done()
       }).catch(done)
@@ -353,8 +363,8 @@ describe('PendingTransactionTracker', function () {
 
     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) => {
+      providerResultStub['eth_getTransactionReceipt'] = {}
+      pendingTxTracker._checkIftxWasDropped(txMeta, {}).then((dropped) => {
         assert(dropped, 'should be true')
         done()
       }).catch(done)