diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js index d190b4e52..d5e8ce803 100644 --- a/app/scripts/controllers/transactions/index.js +++ b/app/scripts/controllers/transactions/index.js @@ -502,7 +502,7 @@ class TransactionController extends EventEmitter { * @param {number} txId - The tx's ID * @returns {Promise} */ - 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) { diff --git a/app/scripts/controllers/transactions/pending-tx-tracker.js b/app/scripts/controllers/transactions/pending-tx-tracker.js index 4e2db5ead..8a135c430 100644 --- a/app/scripts/controllers/transactions/pending-tx-tracker.js +++ b/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 diff --git a/test/unit/app/controllers/transactions/pending-tx-test.js b/test/unit/app/controllers/transactions/pending-tx-test.js index 59b80bda5..358182f42 100644 --- a/test/unit/app/controllers/transactions/pending-tx-test.js +++ b/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)