add shared transaction constants by brad-decker · Pull Request #9459 · MetaMask/metamask-extension

brad-decker · 2020-09-23T19:52:38Z

Description

Unifies and codifies that disparate constants used in various places through the app. We had some consts exported from the background that we used in the UI and some in the UI imported by the background.

this adds a 'shared' folder at the root that can hold within it any utilities, constants, etc that we want to share between the two code bases.

app/scripts/controllers/transactions/tx-state-manager.js

app/scripts/controllers/transactions/index.js

Gudahtt · 2020-09-23T20:36:31Z

shared/constants/transaction.js

In an ideal world, these would not be included here. I don't think we should call anything a "transaction" that is not a transaction, in the code or the UI. It's unfortunate that this is how the signature requests were integrated. This is responsible for a lot of existing complicated UI logic.

It would seem more appropriate to have a higher-level abstraction (e.g. "activity"), something that could be either a transaction or a signature request/signing, or something else. The set of things we want to show in the activity tab.

Agreed. Although at this point in the code the things that get returned from nonceSortedTransactionSelector are called TransactionGroup, I intend on changing that in a future PR and updating these to reflect that. Also ideally I'll at some point move this logic into a background controller, enabling things like real pagination of activity, and that would make these variables -- whatever they end up being called -- make sense in a shared constant file.

Gudahtt · 2020-09-23T20:37:28Z

shared/constants/transaction.js

It might be worth clarifying that these are just the interactions that we haven't special-cased. approve is a contract interaction as well.

Was this comment addressed?

shared/constants/transaction.js

Gudahtt · 2020-09-23T20:38:54Z

shared/constants/transaction.js

Should "confirmed" and "failed" be represented here as well?

I could but these are the statuses that don't exist as transaction statuses. Honestly i could go either way with it. If we had separate consts for those it would enable us to change the string value for one without changing the other in the future. These two cases here represent areas where we override the status by inferring from the transaction data. For example, pending happens when the transaction is in one of the mentioned statuses, and cancelled happens when a cancel type transaction has been confirmed... the others are just inherited from the primary transaction.

shared/constants/transaction.js

metamaskbot · 2020-09-24T19:47:39Z

Builds ready [4b84245]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (613 ± 73 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	30	111	46	20	10
		domContentLoaded	380	865	611	152	73
		load	381	867	613	152	73
		domInteractive	380	864	611	152	73

metamaskbot · 2020-10-07T17:45:40Z

Builds ready [530a83b]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (404 ± 51 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	91	41	17	8
		domContentLoaded	307	664	402	106	51
		load	309	666	404	106	51
		domInteractive	307	664	402	106	51

metamaskbot · 2020-10-08T20:34:34Z

Builds ready [9e90446]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (404 ± 59 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	30	83	41	13	6
		domContentLoaded	291	607	403	123	59
		load	293	608	404	123	59
		domInteractive	291	606	402	123	59

metamaskbot · 2020-10-09T17:06:14Z

Builds ready [69787a0]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (434 ± 57 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	53	35	5	2
		domContentLoaded	291	618	432	120	57
		load	293	619	434	119	57
		domInteractive	291	617	432	120	57

metamaskbot · 2020-10-12T19:10:28Z

Builds ready [1931c06]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (434 ± 57 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	105	55	23	11
		domContentLoaded	313	739	432	120	58
		load	314	740	434	120	57
		domInteractive	313	738	432	120	58

metamaskbot · 2020-10-23T18:17:13Z

Builds ready [bb89e98]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (462 ± 69 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	30	80	39	11	5
		domContentLoaded	300	667	461	144	69
		load	301	668	462	143	69
		domInteractive	299	667	460	144	69

brad-decker · 2020-10-23T18:24:26Z

@Gudahtt I think this is ready for another review

metamaskbot · 2020-10-28T21:15:13Z

Builds ready [67d5208]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (674 ± 94 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	33	112	63	24	12
		domContentLoaded	366	965	672	196	94
		load	368	970	674	197	94
		domInteractive	366	964	672	196	94

brad-decker · 2020-10-29T15:17:54Z

@darkwing would you mind reviewing this, specifically that transactions selector file that you recently modified. I only updated the name of constants but it'd be good to get a thumbs up on it.

metamaskbot · 2020-10-29T15:53:13Z

Builds ready [24a5ee3]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (436 ± 62 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	31	84	39	11	5
		domContentLoaded	284	637	435	128	62
		load	285	639	436	128	62
		domInteractive	283	637	434	128	62

rekmarks

I have some nits about documentation that I think are worthwhile, but otherwise I am very happy about this PR.

My one general piece of feedback is that I think it's more ergonomic to define objects of constants and then export that object, rather than adding multiple exports.

For example that'd allow us to do something like:

import { TRANSACTION_CATEGORIES } from '../shared/constants/transaction'
// ...
if (tx.category === TRANSACTION_CATEGORIES.SWAP) { /* ... */ }

Also, if you ever need a map of constant values, you already have it.

This is just a suggestion, and I won't block on it!

shared/constants/transaction.js

rekmarks · 2020-10-29T17:04:49Z

shared/constants/transaction.js

Was this comment addressed?

ui/app/hooks/useSwappedTokenValue.js

metamaskbot · 2020-10-29T21:41:05Z

Builds ready [1e7048b]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (526 ± 107 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	112	50	20	9
		domContentLoaded	285	968	524	222	107
		load	287	969	526	223	107
		domInteractive	285	968	524	222	107

rekmarks

Very nice!

@rekmarks

Thanks @rekmarks Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>

brad-decker · 2020-11-03T22:34:25Z

@rekmarks thanks for the suggestion to export objects with keys instead of individual variables. It worked out so well and now, by using @typedef for the object declaration, we have very nice developer tooling in vscode:

rekmarks

Great work with the typings. LGTM! 🙌

metamaskbot · 2020-11-03T22:56:31Z

Builds ready [018b35b]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (479 ± 70 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	32	113	53	22	11
		domContentLoaded	305	800	478	145	70
		load	307	801	479	145	70
		domInteractive	304	799	477	145	70

Gudahtt

Belated, but, LGTM!

I noticed a few things that can be addressed in a future PR (a typo, and a few places where these constants should be used instead of string literals)

Gudahtt · 2020-11-04T13:55:07Z

shared/constants/transaction.js

+ * @property {'rejected'} REJECTED - The user has rejected the transaction in the
+ *  MetaMask UI
+ * @property {'signed'} SIGNED - The transaction has been signed
+ * @property {'submitted'} SIGNED - The transaction has been submitted to network


Typo here (SIGNED should be SUBMITTED)

Gudahtt · 2020-11-04T14:11:05Z

ui/app/helpers/utils/transactions.util.js

-    status === TRANSACTION_STATUS_CONFIRMED &&
-    type === TRANSACTION_TYPE_CANCEL
+    status === TRANSACTION_STATUSES.CONFIRMED &&
+    type === TRANSACTION_TYPES.CANCEL


The hard-coded failed and cancelled values returned here should be using the constants as well

We should use these constants in tx-state-manager as well 🤔 That's where the status returned below is set.

brad-decker · 2020-11-04T15:14:14Z

@Gudahtt i'll fix these today! Thanks for pointing them out

brad-decker commented Sep 23, 2020

View reviewed changes

app/scripts/controllers/transactions/tx-state-manager.js Outdated Show resolved Hide resolved

brad-decker commented Sep 23, 2020

View reviewed changes

app/scripts/controllers/transactions/index.js Outdated Show resolved Hide resolved

brad-decker force-pushed the unify-transaction-constants branch from f4cd352 to cbca6d5 Compare September 23, 2020 20:22

Gudahtt reviewed Sep 23, 2020

View reviewed changes

brad-decker force-pushed the unify-transaction-constants branch 2 times, most recently from 3c0f25f to 4b84245 Compare September 24, 2020 19:29

brad-decker marked this pull request as ready for review September 24, 2020 22:22

brad-decker requested review from a team and kumavis as code owners September 24, 2020 22:22

brad-decker requested review from danfinlay and rekmarks September 24, 2020 22:23

brad-decker force-pushed the unify-transaction-constants branch from 4b84245 to d5e89a2 Compare October 7, 2020 15:49

brad-decker requested a review from danjm October 7, 2020 15:55

brad-decker force-pushed the unify-transaction-constants branch 2 times, most recently from 6222cd3 to 530a83b Compare October 7, 2020 17:25

brad-decker force-pushed the unify-transaction-constants branch from 9e90446 to 69787a0 Compare October 9, 2020 16:36

brad-decker mentioned this pull request Oct 9, 2020

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

Closed

brad-decker force-pushed the unify-transaction-constants branch 2 times, most recently from a7dcf7f to 1931c06 Compare October 12, 2020 18:45

brad-decker requested a review from Gudahtt October 14, 2020 17:00

brad-decker force-pushed the unify-transaction-constants branch 2 times, most recently from 4106748 to bb89e98 Compare October 23, 2020 17:53

brad-decker force-pushed the unify-transaction-constants branch from bb89e98 to 67d5208 Compare October 28, 2020 20:52

brad-decker force-pushed the unify-transaction-constants branch from 67d5208 to 24a5ee3 Compare October 29, 2020 15:17

brad-decker requested a review from darkwing October 29, 2020 15:18

brad-decker assigned darkwing, Gudahtt, rekmarks and danjm Oct 29, 2020

rekmarks requested changes Oct 29, 2020

View reviewed changes

rekmarks previously approved these changes Oct 30, 2020

View reviewed changes

danjm added this to the v8.1.next? milestone Nov 2, 2020

brad-decker and others added 5 commits November 3, 2020 14:41

unify transaction constants into shared folder

ec68874

scan shared folder for used translations

c8b294e

remove unused locale messages

860c9e4

Apply suggestions from code review

9bcd544

Thanks @rekmarks Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>

improve ergonomics of constants

018b35b

brad-decker dismissed rekmarks’s stale review via 018b35b November 3, 2020 22:33

brad-decker force-pushed the unify-transaction-constants branch from 1e7048b to 018b35b Compare November 3, 2020 22:33

rekmarks approved these changes Nov 3, 2020

View reviewed changes

brad-decker merged commit 026a06b into develop Nov 3, 2020

brad-decker deleted the unify-transaction-constants branch November 3, 2020 22:57

github-actions bot locked and limited conversation to collaborators Nov 3, 2020

Gudahtt reviewed Nov 4, 2020

View reviewed changes

Conversation

brad-decker commented Sep 23, 2020

Description

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

metamaskbot commented Sep 24, 2020

Uh oh!

metamaskbot commented Oct 7, 2020

Uh oh!

metamaskbot commented Oct 8, 2020

Uh oh!

metamaskbot commented Oct 9, 2020

Uh oh!

metamaskbot commented Oct 12, 2020

Uh oh!

metamaskbot commented Oct 23, 2020

Uh oh!

brad-decker commented Oct 23, 2020

Uh oh!

metamaskbot commented Oct 28, 2020

Uh oh!

brad-decker commented Oct 29, 2020

Uh oh!

metamaskbot commented Oct 29, 2020

Uh oh!

rekmarks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

metamaskbot commented Oct 29, 2020

Uh oh!

rekmarks left a comment

Choose a reason for hiding this comment

Uh oh!

brad-decker commented Nov 3, 2020

Uh oh!

rekmarks left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Nov 3, 2020

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brad-decker commented Nov 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

rekmarks left a comment •

edited

Loading