remove transactionCategory in favor of more types#10615
Conversation
1c68fcf to
c07abb4
Compare
Builds ready [c07abb4]
Page Load Metrics (722 ± 22 ms)
|
6367332 to
e74cc64
Compare
Builds ready [e74cc64]
Page Load Metrics (624 ± 41 ms)
|
ryanml
left a comment
There was a problem hiding this comment.
looks like a well applied sweeping update of categories -> types ++
e74cc64 to
1ca9abe
Compare
|
@ryanml had to rebase, no changes were made :) |
Builds ready [1ca9abe]
Page Load Metrics (834 ± 52 ms)
|
|
Waiting for other reviewers to surface until tomorrow, at which point I'll remove the do not merge label and merge. |
Gudahtt
left a comment
There was a problem hiding this comment.
Looks great! I found one minor issue with a stubs file, but everything looks good aside from that.
I found a few remaining references to a "transaction category" but they were in reference to the transaction group category, which is still a thing that exists. We might want to consider changing the nomenclature there at some point to avoid this confusion, but that can happen at another time.
| contractDeployment, contractMethodCall | ||
| */ | ||
| async _determineTransactionCategory(txParams) { | ||
| * @typedef { 'transfer' | 'approve' | 'transferfrom' | 'contractInteraction'| 'sentEther' } InferrableTransactionTypes |
rekmarks
left a comment
There was a problem hiding this comment.
I didn't do a full review, so I won't throw in my full weight with an approval, but the changes make sense to me and the migration looks good 👍
@Gudahtt 100% agree. I almost did that here but changed my mind haha. I will eliminate this as part of my proposal for regrouping transactions |
|
I investigated the impact this might have on mobile, since we do sync some transactions. They use our transactions here: https://github.com/MetaMask/metamask-mobile/blob/develop/app/core/Engine.js#L394 It looks like they don't use the |
Builds ready [577f9df]
Page Load Metrics (607 ± 27 ms)
|
Rationale
Our Transaction object has keys for both
typeandtransactionCategorybut the following are true:types.standardtypewhich just meant to look at thetransactionCategoryThis pull request attempts to simplify the process of getting a view of what a transaction is by eliminating the
transactionCategoryfield in preference of thetypefield.There is only one additional case that needs to be covered in the future possibly: Signature requests, which currently have no category or type.
Supersedes #9545