Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
May 16, 2019
9 changes: 9 additions & 0 deletions app/scripts/controllers/transactions/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,15 @@ 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', (txId) => {
const txMeta = this.txStateManager.getTx(txId)
if (!txMeta.dropBlockBuffer) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where does this get set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the line bellow

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i meant what is the case in which this code path is not always true?
i'm confused on the flow, it seems:
on tx:dropped, update the txMeta with a tx:drop-update
then on another dropped event for the same tx, we setTxStatusDropped?

are we expecting multiple dropped events for the same tx?

txMeta.dropBlockBuffer = 1
this.txStateManager.updateTx(txMeta, 'transactions/pending-tx-tracker#event: tx:drop-update')
return
}
this.setTxStatusDropped(txId)
jennypollack marked this conversation as resolved.
Show resolved Hide resolved
})
this.pendingTxTracker.on('tx:block-update', (txMeta, latestBlockNumber) => {
if (!txMeta.firstRetryBlockNumber) {
txMeta.firstRetryBlockNumber = latestBlockNumber
Expand Down
29 changes: 23 additions & 6 deletions app/scripts/controllers/transactions/pending-tx-tracker.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,16 +145,16 @@ class PendingTransactionTracker extends EventEmitter {
// If another tx with the same nonce is mined, set as failed.
jennypollack marked this conversation as resolved.
Show resolved Hide resolved
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)
return this.emit('tx:dropped', txId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emitting "dropped" here and _checkIftxWasDropped,
suggest to move all of that logic into one function or break the nonceIsTaken into a different status?

jennypollack marked this conversation as resolved.
Show resolved Hide resolved
}

const dropped = await this._checkIftxWasDropped(txMeta)
if (dropped) return

// 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) {
Expand All @@ -165,6 +165,23 @@ 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, id } = txMeta
const nextNonce = await this.query.getTransactionCount(from)
const { blockNumber } = await this.query.getTransactionByHash(hash) || {}
if (!blockNumber && parseInt(nextNonce) > nonce) {
this.emit('tx:dropped', id)
return true
}
return false
}

/**
checks to see if a confirmed txMeta has the same nonce
Expand Down
49 changes: 44 additions & 5 deletions test/unit/app/controllers/transactions/pending-tx-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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')
})
})

Expand Down Expand Up @@ -283,6 +282,46 @@ describe('PendingTransactionTracker', function () {
})
})

describe('#_checkIftxWasDropped', () => {
const txMeta = {
id: 1,
hash: '0x0593ee121b92e10d63150ad08b4b8f9c7857d1bd160195ee648fb9a0f8d00eeb',
status: 'confirmed',
txParams: {
from: '0x1678a085c290ebd122dc42cba69373b5953b831d',
nonce: '0x1',
value: '0xfffff',
},
rawTx: '0xf86c808504a817c800827b0d940c62bb85faa3311a998d3aba8098c1235c564966880de0b6b3a7640000802aa08ff665feb887a25d4099e40e11f0fef93ee9608f404bd3f853dd9e84ed3317a6a02ec9d3d1d6e176d4d2593dd760e74ccac753e6a0ea0d00cc9789d0d7ff1f471d',
}
it('should return false', (done) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update this test title to include the condition when it returns false, e.g.,

should return false when ...

providerResultStub['eth_getTransactionCount'] = '0x01'
providerResultStub['eth_getTransactionByHash'] = {}
pendingTxTracker._checkIftxWasDropped(txMeta).then((dropped) => {
assert(!dropped, 'should be false')
done()
}).catch(done)
})

it('should return true', function (done) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update this test title to include the condition when it returns true, e.g.,

should return true when ...

providerResultStub['eth_getTransactionCount'] = '0x02'
providerResultStub['eth_getTransactionByHash'] = {}
pendingTxTracker._checkIftxWasDropped(txMeta).then((dropped) => {
assert(dropped, 'should be true')
done()
}).catch(done)
})
it('should emit tx:dropped with th txMetas id', function (done) {
providerResultStub['eth_getTransactionCount'] = '0x02'
providerResultStub['eth_getTransactionByHash'] = {}
pendingTxTracker.once('tx:dropped', (id) => {
if (id === txMeta.id) return done()
else done(new Error('wrong tx Id'))
})
pendingTxTracker._checkIftxWasDropped(txMeta).catch(done)
})
})

describe('#_checkIfNonceIsTaken', function () {
beforeEach(function () {
const confirmedTxList = [{
Expand Down