Ensures the tx controller + state-manager orders transactions as received #7484
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…he order they are received
Merged
Builds ready [36ade8b]
|
Gudahtt
reviewed
Nov 21, 2019
Gudahtt
reviewed
Nov 21, 2019
| @@ -193,12 +193,12 @@ class TransactionController extends EventEmitter { | |||
| } | |||
| txUtils.validateTxParams(normalizedTxParams) | |||
| // construct txMeta | |||
There was a problem hiding this comment.
Nit: maybe a comment here would be useful, to explain that this step must happen synchronously, before anything else, because it assigns the tx id? It's really not obvious what's going on here at first glance.
This is a follow up from this discussion: #7473 (comment)
…n 1 in tx-state-manager
…TransactionCategory after generateTxMeta
Builds ready [47bd579]
|
Gudahtt
previously approved these changes
Nov 26, 2019
|
does this mean the history is reversed in the ui? |
frankiebee
reviewed
Nov 26, 2019
|
|
||
| newTxIndex === -1 | ||
| ? transactions.push(txMeta) | ||
| : transactions.splice(newTxIndex, 0, txMeta) |
There was a problem hiding this comment.
if this is a new tx why the splice?
There was a problem hiding this comment.
this inserts the transaction in the list according to its id, before the first transaction with a greater id
frankiebee
reviewed
Nov 26, 2019
| @@ -167,7 +167,12 @@ class TransactionStateManager extends EventEmitter { | |||
| transactions.splice(index, 1) | |||
| } | |||
| } | |||
| transactions.push(txMeta) | |||
| const newTxIndex = transactions | |||
| .findIndex((currentTxMeta) => currentTxMeta.id > txMeta.id) | |||
There was a problem hiding this comment.
so im not sure this works
There was a problem hiding this comment.
we could use timestamp instead
frankiebee
suggested changes
Nov 26, 2019
Gudahtt
approved these changes
Nov 26, 2019
Builds ready [81f0afe]
|
Gudahtt
pushed a commit
that referenced
this pull request
Dec 5, 2019
…ived (#7484) * Ensures the tx controller + tx-state-manager orders transactions in the order they are received * Handle transaction ordering in cases where tx ids are off by more than 1 in tx-state-manager * Add comment to addUnapprovedTransaction explaining calling _determineTransactionCategory after generateTxMeta * Sort txes by timestamp of creation instead of id
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These changes are needed to ensure consistency of transaction order. This was an issue for batch transactions and the same change is included in #7473 It was also needed to get my local e2e tests passing.
The strategy here is to ensure that the tx-state-manager orders transactions by id, and to ensure that ids are applied in the order they are received by the transaction controller (and not after an indeterminate async operation).
Although this commit is currently also part of #7473 and #7368, I believe it should be made directly to develop and then those can be rebased accordingly.