Skip to content

Remove transactionCategory in favor of expanding type to cover it's need#9545

Closed
brad-decker wants to merge 4 commits intodevelopfrom
remove-category
Closed

Remove transactionCategory in favor of expanding type to cover it's need#9545
brad-decker wants to merge 4 commits intodevelopfrom
remove-category

Conversation

@brad-decker
Copy link
Contributor

@brad-decker brad-decker commented Oct 9, 2020

Rationale

There were previously only three "types" of transaction:

  1. standard
  2. cancel
  3. retry

A fourth unintended case for type was undefined which was the case for incoming transactions

standard essentially means "not cancel or retry" and in any case where we look at standard what we actually care about is transactionCategory.

This pull request removes transactionCategory in favor of adding all of the 'categories' as additional types.

This should significantly cut down on some confusing thought cycles while having no impact on the extension. I might be missing some cases, though. Let me know what you think!

Details of Implementation

  1. _determineTransactionCategory -> _determineTransactionType
  2. transactionCategory: 'incoming' -> type: 'incoming'
  3. transactionCategory -> type for all code touching the category
  4. added typedefs for type and status
  5. move TransactionMeta typedef to the transactions constant and use new typedefs in it
  6. Migration for old state.

Prior work

this depends on #9459 and is based on it.

@brad-decker brad-decker changed the title Remove category Remove transactionCategory in favor of expanding type to cover it's need Oct 9, 2020
@brad-decker brad-decker changed the base branch from develop to unify-transaction-constants October 9, 2020 20:36
Comment on lines 714 to 715
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this can be removed/simplified now that I have the typedefs.

@metamaskbot
Copy link
Collaborator

Builds ready [ede2cee]
Page Load Metrics (455 ± 58 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29403542
domContentLoaded29560045412058
load29760245512058
domInteractive29560045412058

@brad-decker brad-decker force-pushed the unify-transaction-constants branch from 69787a0 to a7dcf7f Compare October 12, 2020 18:44
@brad-decker brad-decker force-pushed the unify-transaction-constants branch from a7dcf7f to 1931c06 Compare October 12, 2020 18:45
@metamaskbot
Copy link
Collaborator

Builds ready [c690f8d]
Page Load Metrics (415 ± 52 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3211440178
domContentLoaded31978641410852
load32078741510852
domInteractive31878541310852

@brad-decker brad-decker force-pushed the unify-transaction-constants branch 4 times, most recently from 67d5208 to 24a5ee3 Compare October 29, 2020 15:17
@danjm danjm added this to the v8.1.next? milestone Nov 2, 2020
@brad-decker brad-decker force-pushed the unify-transaction-constants branch from 1e7048b to 018b35b Compare November 3, 2020 22:33
Base automatically changed from unify-transaction-constants to develop November 3, 2020 22:57
@Gudahtt Gudahtt assigned brad-decker and unassigned Gudahtt and danjm Jan 26, 2021
@brad-decker
Copy link
Contributor Author

superseded by #10615

@brad-decker brad-decker closed this Mar 9, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Mar 9, 2021
@HowardBraham HowardBraham deleted the remove-category branch January 19, 2026 18:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants