drop transactions who's nonce is lower then the known network nonce but were not included in a block #6388
Conversation
|
the do not merge label will get removed by EOD |
|
would like qa for this |
| 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) |
There was a problem hiding this comment.
emitting "dropped" here and _checkIftxWasDropped,
suggest to move all of that logic into one function or break the nonceIsTaken into a different status?
| @@ -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) { | |||
There was a problem hiding this comment.
where does this get set?
There was a problem hiding this comment.
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?
| }) | ||
|
|
||
| pendingTxTracker._checkPendingTx(txMeta).then(() => { | ||
| pendingTxTracker._checkPendingTx(txMeta).catch(done) |
There was a problem hiding this comment.
Why are we ignoring the returned promise? Should we be returning it to the chain?
There was a problem hiding this comment.
test: should emit tx:dropped with the txMetas id only after the second call
so we are testing the emit's logic here
@tmashuang do you know of a way to easily induce dropped txs? |
|
@tmashuang any ideas on how we can qa this? |
|
Wouldn't reproduction just be sending two transactions with the same nonce? Probably sending a transaction on two computers with the same account? Currently, we do the nonce check on the Confirm Transaction Screen, and shows an error if there is. Now, we are able to send the transaction at the same time to race. Is that the goal? |
Co-Authored-By: frankiebee <[email protected]>
|
@whymarrh is the changes sufficient enough, is this good to go? |
| }, | ||
| rawTx: '0xf86c808504a817c800827b0d940c62bb85faa3311a998d3aba8098c1235c564966880de0b6b3a7640000802aa08ff665feb887a25d4099e40e11f0fef93ee9608f404bd3f853dd9e84ed3317a6a02ec9d3d1d6e176d4d2593dd760e74ccac753e6a0ea0d00cc9789d0d7ff1f471d', | ||
| } | ||
| it('should return false', (done) => { |
There was a problem hiding this comment.
We should update this test title to include the condition when it returns false, e.g.,
should return false when ...
| }).catch(done) | ||
| }) | ||
|
|
||
| it('should return true', function (done) { |
There was a problem hiding this comment.
We should update this test title to include the condition when it returns true, e.g.,
should return true when ...
fixes #5031