From a32d71e8ed4c91c8ad73f4a7afc52e506ccf5247 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Fri, 6 Oct 2017 12:29:27 -0700 Subject: [PATCH 1/3] Add failing test for issue #2294 --- test/unit/pending-tx-test.js | 53 +++++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/test/unit/pending-tx-test.js b/test/unit/pending-tx-test.js index 6b62bb5b1..554bd5591 100644 --- a/test/unit/pending-tx-test.js +++ b/test/unit/pending-tx-test.js @@ -5,6 +5,8 @@ const ObservableStore = require('obs-store') const clone = require('clone') const { createStubedProvider } = require('../stub/provider') const PendingTransactionTracker = require('../../app/scripts/lib/pending-tx-tracker') +const MockTxGen = require('../lib/mock-tx-gen') +const sinon = require('sinon') const noop = () => true const currentNetworkId = 42 const otherNetworkId = 36 @@ -50,6 +52,55 @@ describe('PendingTransactionTracker', function () { }) }) + describe('_checkPendingTx state management', function () { + let stub + + afterEach(function () { + if (stub) { + stub.restore() + } + }) + + it('should become failed if another tx with the same nonce succeeds', async function () { + + // SETUP + const txGen = new MockTxGen() + + txGen.generate({ + id: '456', + value: '0x01', + hash: '0xbad', + status: 'confirmed', + nonce: '0x01', + }, { count: 1 }) + + const pending = txGen.generate({ + id: '123', + value: '0x02', + hash: '0xfad', + status: 'submitted', + nonce: '0x01', + }, { count: 1 })[0] + + stub = sinon.stub(pendingTxTracker, 'getPendingTransactions') + .returns(txGen.txs) + + // THE EXPECTATION + const spy = sinon.spy() + pendingTxTracker.on('tx:failed', (txId, err) => { + assert.equal(txId, pending.id, 'should fail the pending tx') + assert.equal(err.name, 'NonceTakenErr', 'should emit a nonce taken error.') + spy(txId, err) + }) + + // THE METHOD + await pendingTxTracker._checkPendingTx(pending) + + // THE ASSERTION + return sinon.assert.calledWith(spy, pending.id, 'tx failed should be emitted') + }) + }) + describe('#checkForTxInBlock', function () { it('should return if no pending transactions', function () { // throw a type error if it trys to do anything on the block @@ -239,4 +290,4 @@ describe('PendingTransactionTracker', function () { }) }) }) -}) \ No newline at end of file +}) From 94513cae7bf3c8310ae6a248e12a9b7dd73e306f Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Fri, 6 Oct 2017 12:50:33 -0700 Subject: [PATCH 2/3] Provide method for tx tracker to refer to all txs --- app/scripts/controllers/transactions.js | 1 + app/scripts/lib/tx-state-manager.js | 8 +++++++- test/unit/pending-tx-test.js | 5 +++-- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index a0f983deb..ef659a300 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -62,6 +62,7 @@ module.exports = class TransactionController extends EventEmitter { retryTimePeriod: 86400000, // Retry 3500 blocks, or about 1 day. publishTransaction: (rawTx) => this.query.sendRawTransaction(rawTx), getPendingTransactions: this.txStateManager.getPendingTransactions.bind(this.txStateManager), + getCompletedTransactions: this.txStateManager.getConfirmedTransactions.bind(this.txStateManager), }) this.txStateManager.store.subscribe(() => this.emit('update:badge')) diff --git a/app/scripts/lib/tx-state-manager.js b/app/scripts/lib/tx-state-manager.js index cf8117864..2250403f6 100644 --- a/app/scripts/lib/tx-state-manager.js +++ b/app/scripts/lib/tx-state-manager.js @@ -46,6 +46,12 @@ module.exports = class TransactionStateManger extends EventEmitter { return this.getFilteredTxList(opts) } + getConfirmedTransactions (address) { + const opts = { status: 'confirmed' } + if (address) opts.from = address + return this.getFilteredTxList(opts) + } + addTx (txMeta) { this.once(`${txMeta.id}:signed`, function (txId) { this.removeAllListeners(`${txMeta.id}:rejected`) @@ -242,4 +248,4 @@ module.exports = class TransactionStateManger extends EventEmitter { _saveTxList (transactions) { this.store.updateState({ transactions }) } -} \ No newline at end of file +} diff --git a/test/unit/pending-tx-test.js b/test/unit/pending-tx-test.js index 554bd5591..32421a44f 100644 --- a/test/unit/pending-tx-test.js +++ b/test/unit/pending-tx-test.js @@ -48,6 +48,7 @@ describe('PendingTransactionTracker', function () { } }, getPendingTransactions: () => {return []}, + getCompletedTransactions: () => {return []}, publishTransaction: () => {}, }) }) @@ -82,7 +83,7 @@ describe('PendingTransactionTracker', function () { nonce: '0x01', }, { count: 1 })[0] - stub = sinon.stub(pendingTxTracker, 'getPendingTransactions') + stub = sinon.stub(pendingTxTracker, 'getCompletedTransactions') .returns(txGen.txs) // THE EXPECTATION @@ -97,7 +98,7 @@ describe('PendingTransactionTracker', function () { await pendingTxTracker._checkPendingTx(pending) // THE ASSERTION - return sinon.assert.calledWith(spy, pending.id, 'tx failed should be emitted') + assert.ok(spy.calledWith(pending.id), 'tx failed should be emitted') }) }) From a417fab0ebd71d22f51a8e30590c259b32164fd2 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Fri, 6 Oct 2017 12:51:13 -0700 Subject: [PATCH 3/3] When checking pending txs, check for successful txs with same nonce. If a successful tx with the same nonce exists, transition tx to the failed state. Fixes #2294 --- app/scripts/lib/pending-tx-tracker.js | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/app/scripts/lib/pending-tx-tracker.js b/app/scripts/lib/pending-tx-tracker.js index 8a626e222..2d8f22ae8 100644 --- a/app/scripts/lib/pending-tx-tracker.js +++ b/app/scripts/lib/pending-tx-tracker.js @@ -25,6 +25,7 @@ module.exports = class PendingTransactionTracker extends EventEmitter { // default is one day this.retryTimePeriod = config.retryTimePeriod || 86400000 this.getPendingTransactions = config.getPendingTransactions + this.getCompletedTransactions = config.getCompletedTransactions this.publishTransaction = config.publishTransaction } @@ -120,6 +121,7 @@ module.exports = class PendingTransactionTracker extends EventEmitter { async _checkPendingTx (txMeta) { const txHash = txMeta.hash const txId = txMeta.id + // extra check in case there was an uncaught error during the // signature and submission process if (!txHash) { @@ -128,6 +130,15 @@ module.exports = class PendingTransactionTracker extends EventEmitter { this.emit('tx:failed', txId, noTxHashErr) return } + + // If another tx with the same nonce is mined, set as failed. + 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) + } + // get latest transaction status let txParams try { @@ -159,4 +170,13 @@ module.exports = class PendingTransactionTracker extends EventEmitter { } nonceGlobalLock.releaseLock() } + + async _checkIfNonceIsTaken (txMeta) { + const completed = this.getCompletedTransactions() + const sameNonce = completed.filter((otherMeta) => { + return otherMeta.txParams.nonce === txMeta.txParams.nonce + }) + return sameNonce.length > 0 + } + }