Resubmit approved transactions on new block (#5752)

* Add beginning of test

* Resubmit approved transactions on new block

May fix #4343 and related issues, where an error could leave
transactions stranded in the approved state.

* Remove unused test

* Re-approve transactions when retrying approved

* Add retry approved test

* Include approved in pending tx count

* Fix getPendingTxs()

* Linted

* Only throw hash error in submitted state

* Only check submitted txs for block inclusion

* Fix test expectations

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Current Develop Branch
 
+- Resubmit approved transactions on new block, to fix bug where an error can stick transactions in this state.
+
 ## 5.0.2 Friday November 9 2018
 
 - Fixed bug that caused accounts to update slowly to sites. #5717

app/scripts/controllers/transactions/index.js

@@ -82,7 +82,12 @@ class TransactionController extends EventEmitter {
       provider: this.provider,
       nonceTracker: this.nonceTracker,
       publishTransaction: (rawTx) => this.query.sendRawTransaction(rawTx),
-      getPendingTransactions: this.txStateManager.getPendingTransactions.bind(this.txStateManager),
+      getPendingTransactions: () => {
+        const pending = this.txStateManager.getPendingTransactions()
+        const approved = this.txStateManager.getApprovedTransactions()
+        return [...pending, ...approved]
+      },
+      approveTransaction: this.approveTransaction.bind(this),
       getCompletedTransactions: this.txStateManager.getConfirmedTransactions.bind(this.txStateManager),
     })
 

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

@@ -27,6 +27,7 @@ class PendingTransactionTracker extends EventEmitter {
     this.getPendingTransactions = config.getPendingTransactions
     this.getCompletedTransactions = config.getCompletedTransactions
     this.publishTransaction = config.publishTransaction
+    this.approveTransaction = config.approveTransaction
     this.confirmTransaction = config.confirmTransaction
   }
 
@@ -108,7 +109,7 @@ class PendingTransactionTracker extends EventEmitter {
     if (txBlockDistance <= Math.pow(2, retryCount) - 1) return
 
     // Only auto-submit already-signed txs:
-    if (!('rawTx' in txMeta)) return
+    if (!('rawTx' in txMeta)) return this.approveTransaction(txMeta.id)
 
     const rawTx = txMeta.rawTx
     const txHash = await this.publishTransaction(rawTx)
@@ -129,6 +130,9 @@ class PendingTransactionTracker extends EventEmitter {
     const txHash = txMeta.hash
     const txId = txMeta.id
 
+    // Only check submitted txs
+    if (txMeta.status !== 'submitted') return
+
     // extra check in case there was an uncaught error during the
     // signature and submission process
     if (!txHash) {

app/scripts/controllers/transactions/tx-state-manager.js

@@ -81,6 +81,17 @@ class TransactionStateManager extends EventEmitter {
     }, {})
   }
 
+  /**
+    @param [address] {string} - hex prefixed address to sort the txMetas for [optional]
+    @returns {array} the tx list whos status is approved if no address is provide
+    returns all txMetas who's status is approved for the current network
+  */
+  getApprovedTransactions(address) {
+    const opts = { status: 'approved' }
+    if (address) opts.from = address
+    return this.getFilteredTxList(opts)
+  }
+
   /**
     @param [address] {string} - hex prefixed address to sort the txMetas for [optional]
     @returns {array} the tx list whos status is submitted if no address is provide

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

@@ -24,7 +24,7 @@ describe('PendingTransactionTracker', function () {
     }
     txMetaNoHash = {
       id: 2,
-      status: 'signed',
+      status: 'submitted',
       txParams: { from: '0x1678a085c290ebd122dc42cba69373b5953b831d'},
     }
 
@@ -212,6 +212,7 @@ describe('PendingTransactionTracker', function () {
       pendingTxTracker.publishTransaction = async (rawTx) => {
         assert.equal(rawTx, txMeta.rawTx, 'Should pass the rawTx')
       }
+      pendingTxTracker.approveTransaction = async () => {}
       sinon.spy(pendingTxTracker, 'publishTransaction')
 
       txMetaToTestExponentialBackoff = Object.assign({}, txMeta, {
@@ -266,6 +267,18 @@ describe('PendingTransactionTracker', function () {
 
       assert.equal(pendingTxTracker.publishTransaction.callCount, 1, 'Should call publish transaction')
     })
+
+    it('should call opts.approveTransaction with the id if the tx is not signed', async () => {
+      const stubTx = {
+        id: 40,
+      }
+      const approveMock = sinon.stub(pendingTxTracker, 'approveTransaction')
+
+      pendingTxTracker._resubmitTx(stubTx)
+
+      assert.ok(approveMock.called)
+      approveMock.restore()
+    })
   })
 
   describe('#_checkIfNonceIsTaken', function () {

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

@@ -313,6 +313,7 @@ describe('Transaction Controller', function () {
         assert.equal(params.gas, originalValue, 'gas unmodified')
         assert.equal(params.gasPrice, originalValue, 'gas price unmodified')
         assert.equal(result.hash, originalValue, `hash was set \n got: ${result.hash} \n expected: ${originalValue}`)
+        assert.equal(result.status, 'submitted', 'Should have reached the submitted status.')
         signStub.restore()
         pubStub.restore()
         done()
@@ -469,9 +470,11 @@ describe('Transaction Controller', function () {
         { id: 7, status: 'failed', metamaskNetworkId: currentNetworkId, txParams: {} },
       ])
     })
-    it('should show only submitted transactions as pending transasction', function () {
-      assert(txController.pendingTxTracker.getPendingTransactions().length, 1)
-      assert(txController.pendingTxTracker.getPendingTransactions()[0].status, 'submitted')
+    it('should show only submitted and approved transactions as pending transasction', function () {
+      assert(txController.pendingTxTracker.getPendingTransactions().length, 2)
+      const states = txController.pendingTxTracker.getPendingTransactions().map(tx => tx.status)
+      assert(states.includes('approved'), 'includes approved')
+      assert(states.includes('submitted'), 'includes submitted')
     })
   })
 })