Conversation
|
|
||
| // Combine sortBy and pickBy to transform our state object into an array of | ||
| // matching transactions that are sorted by time. | ||
| const filteredTransactions = sortBy( |
There was a problem hiding this comment.
The previous iteration of this logic resulted in the potential for the entire transaction list to be iterated through N + 1 times where N was the number of search terms. The + 1 is because the entire list was iterated through once to get only the current network transactions.
This code should both be more performant and still return transactions in the order in which they were expected prior to my changes.
| }), | ||
| 'time', | ||
| ); | ||
| if (limit !== undefined) { |
There was a problem hiding this comment.
In the previous version of this code, the limit was computed before filtering (except current network filter). I don't think this will result in a functional change due to the fact that we aren't using filtering logic to get a subset of transactions in the UI.
a24b030 to
fe5944e
Compare
b5a7f27 to
9e5c019
Compare
Builds ready [a440ce3]
Page Load Metrics (638 ± 14 ms)
|
fff90f3 to
c4d2bde
Compare
Builds ready [c4d2bde]
Page Load Metrics (638 ± 27 ms)
|
2fdaed1 to
ae3a5a2
Compare
Builds ready [ae3a5a2]
Page Load Metrics (625 ± 32 ms)
|
Builds ready [feece70]
Page Load Metrics (690 ± 25 ms)
|
Gudahtt
left a comment
There was a problem hiding this comment.
Looks good so far! I only got partway through - I'll try to review the rest today though.
| break; | ||
| default: | ||
| if (typeof value !== 'string') { | ||
| throw ethErrors.rpc.invalidParams( |
There was a problem hiding this comment.
I see that additional validation was added throughout this function, but it looks to be identical to the validation already happening in tx-state-manager. Seems reasonable 👍
| * use throughout the extension. The argument here will override everything | ||
| * in the resulting transaction meta. | ||
| * | ||
| * TODO: Don't overwrite everything? |
| } | ||
|
|
||
| /** | ||
| * @returns {Array} the tx list with unapproved status |
There was a problem hiding this comment.
Wow, what a blatant lie. Good riddance!
Gudahtt
left a comment
There was a problem hiding this comment.
LGTM, great work!
Storing transactions by id makes much more sense. I like how you've consolidated all of those transaction list functions as well.
I've left lots of smaller suggestions, but nothing major stood out. The migration looks good too - it seems pretty robust.
| * @emits 'updateBadge' - When the number of transactions changes in state, | ||
| * the badge in the browser extension bar should be updated to reflect the | ||
| * number of pending transactions. This particular emit doesn't appear to | ||
| * bubble up anywhere that is actually used. TransactionController emits |
| status: TRANSACTION_STATUSES.SUBMITTED, | ||
| }; | ||
| const submittedTx0Dupe = { | ||
| id: 1, |
There was a problem hiding this comment.
Hmm. I see that this id has changed, so this is no longer an exact duplicate transaction. But I can't think of any benefit to us testing two transactions with the same ID - lots of things would already not be working correctly if someone ended up in that state.
| * @property {number} submittedTime - The time the transaction was submitted to | ||
| * @property {number} [submittedTime] - The time the transaction was submitted to | ||
| * the network, in Unix epoch time (ms). | ||
| * @property {TxError} [err] - The error encountered during the transaction |
There was a problem hiding this comment.
Awww 😞 Well, today I learned that this is a thing that we do. We should be preserving the Error object, not deconstructing it into this "TxError" object. Oh well. A task for a future PR.
Builds ready [69914db]
Page Load Metrics (624 ± 16 ms)
|
|
Changes look good to me! Ready to re-approve when the migration number has been updated to resolve that conflict. |
69914db to
40f5cec
Compare
Builds ready [40f5cec]
Page Load Metrics (630 ± 51 ms)
|
|
One more rebase needed, to get past the CI failure |
40f5cec to
4e48c28
Compare
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Builds ready [17b510d]
Page Load Metrics (563 ± 53 ms)
|
No description provided.