Refactor token send/method confirmation flow (trimmed down) #13788

adonesky1 · 2022-02-28T18:54:34Z

Fixes: #11611 (and generally improves the UX of NFT confirmation screens)

Overview

Creates a sub-component ConfirmTokenTransactionSwitch through which token/NF confirmation screens for the methods approve, transfer, and transferFrom are routed.
ConfirmTokenTransactionSwitch instantiates a newly created hook useAssetDetails which passes the pending transaction to the new method getTokenStandardAndDetails in the AssetsContractController and retrieves all the data ( assetName, userBalance, tokenSymbol, decimals, image etc.) we need for the three token specific method pages we currently support (approve, transfer, and transferFrom). This method call also returns the assetStandard to enable improved branching logic to provide suitable language depending on the token contract standard (ERC20, ERC721 or ERC1155).
- Importantly this eliminates the unwanted behavior of adding the token to the TokensController state whenever a user interacts with a confirmation transaction of a token not already in TokensController state

Additionally (though less importantly)

Refactors confirm-token-transaction-base and confirm-send-token from class based to functional components for improved ease of use.

When reviewing PR in action:

The behavior/UX should be unaltered for `ERC20` method calls listed above but be more accurate and clear for the same method calls for `ERC721` and `ERC1155` contracts.

For Example
ERC721 approve call before:

the tokenId is incorrectly being used as token amount ☝️ 👇

after:
the option to change the amount should not be present for ERC721 tokens 👇

ERC721 transferFrom call before:

after:

github-actions · 2022-02-28T18:54:49Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

adonesky1 · 2022-02-28T19:56:05Z

app/scripts/metamask-controller.js

decimals and balance are not converted from BNs to strings automagically in Firefox (as they are in Chrome) so we need to do this for now.

adonesky1 · 2022-02-28T19:59:12Z

ui/pages/confirm-approve/confirm-approve.js

the identicon we show on the approve screen should be for the address we are granting approval not the token for which we are granting approval:

adonesky1 · 2022-02-28T20:01:40Z

ui/pages/confirm-transaction/confirm-transaction.component.js

these are the routes that have been moved in to the sub routing component <ConfirmTokenTransactionSwitch>, added above on line 159.

adonesky1 · 2022-02-28T20:03:32Z

ui/store/actions.js

This method has been superseded by the more flexible/token standard agnostic method getTokenStandardAndDetails added on metamask-controller.js

adonesky1 · 2022-02-28T20:04:56Z

ui/pages/confirm-transaction/confirm-token-transaction-switch.js

This is a pretty good place to start reviewing this PR. This hook in the context of this token method confirmation routing component is the core of the changes.

adonesky1 · 2022-02-28T20:29:46Z

ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js

              className="confirm-approve-content__address-identicon"
              diameter={20}
              address={toAddress}
-              image={tokenImage}


we shouldn't use the tokenImage for the identicon of the address being granted approval.

metamaskbot · 2022-02-28T21:06:30Z

Builds ready [0f8b920]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1236 ± 51 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	70	1391	619	515	247
		domContentLoaded	1062	1420	1233	107	52
		load	1062	1420	1236	107	51
		domInteractive	1062	1420	1233	107	52

highlights:

storybook

metamaskbot · 2022-03-02T15:40:48Z

Builds ready [a23960b]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1106 ± 24 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	68	1106	257	359	172
		domContentLoaded	1032	1223	1103	49	24
		load	1033	1223	1106	50	24
		domInteractive	1032	1223	1103	49	24

highlights:

storybook

metamaskbot · 2022-03-07T16:28:23Z

Builds ready [86826e7]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1078 ± 22 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	65	1137	504	474	228
		domContentLoaded	1003	1205	1076	46	22
		load	1010	1212	1078	46	22
		domInteractive	1003	1205	1076	46	22

highlights:

storybook

adonesky1 · 2022-03-07T21:37:29Z

Just rebased to resolve conflicts. cc @jpuri @segun @mcmire

metamaskbot · 2022-03-08T00:58:03Z

Builds ready [1a76353]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1297 ± 40 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	73	122	85	14	7
		domContentLoaded	1202	1551	1294	82	39
		load	1202	1557	1297	84	40
		domInteractive	1201	1551	1294	82	39

highlights:

storybook

segun

LGTM

mcmire

Finally got some time to look at this. Everything looks good!

…ctController to determine how to represent the contract being interacted with in token contract method calls

adonesky1 · 2022-03-08T22:45:02Z

@segun and @mcmire I just rebased to resolve a conflict on the typography component. No other changes were required. Please re-approve when you have a chance?

metamaskbot · 2022-03-09T01:14:18Z

Builds ready [2274bc1]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1273 ± 27 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	70	137	92	19	9
		domContentLoaded	1192	1439	1271	57	27
		load	1192	1439	1273	57	27
		domInteractive	1192	1439	1271	57	27

highlights:

storybook

adonesky1 force-pushed the refactor-the-refactor branch from f5c5a4a to 23926a0 Compare February 28, 2022 18:55

adonesky1 changed the title ~~ake use of getTokenStandardAndDetails method exposed on assetsContractController to determine how to represent the contract being interacted with in token contract method calls~~ Refactor token send/method confirmation flow Feb 28, 2022

adonesky1 changed the title ~~Refactor token send/method confirmation flow~~ Refactor token send/method confirmation flow (trimmed down) Feb 28, 2022

adonesky1 force-pushed the refactor-the-refactor branch 5 times, most recently from 26ff8fc to e9f8461 Compare February 28, 2022 19:51

adonesky1 commented Feb 28, 2022

View reviewed changes

adonesky1 changed the title ~~Refactor token send/method confirmation flow (trimmed down)~~ Refactor token send/method confirmation flow (slighly trimmed down) Feb 28, 2022

adonesky1 force-pushed the refactor-the-refactor branch from e9f8461 to 0f8b920 Compare February 28, 2022 20:26

adonesky1 commented Feb 28, 2022

View reviewed changes

adonesky1 mentioned this pull request Feb 28, 2022

refactor confirm-transaction component from class to functional #13789

Closed

adonesky1 marked this pull request as ready for review February 28, 2022 20:57

adonesky1 requested a review from a team as a code owner February 28, 2022 20:57

adonesky1 requested review from PeterYinusa, brad-decker, danjm and darkwing February 28, 2022 20:57

adonesky1 force-pushed the refactor-the-refactor branch 2 times, most recently from 2da6987 to a23960b Compare March 2, 2022 14:57

adonesky1 mentioned this pull request Mar 2, 2022

Refactor token send/method confirmation flow #13131

Closed

adonesky1 changed the title ~~Refactor token send/method confirmation flow (slighly trimmed down)~~ Refactor token send/method confirmation flow (trimmed down) Mar 2, 2022

DELJUJAZI approved these changes Mar 7, 2022

View reviewed changes

DELJUJAZI previously approved these changes Mar 7, 2022

View reviewed changes

adonesky1 dismissed DELJUJAZI’s stale review via 86826e7 March 7, 2022 15:44

adonesky1 requested review from jpuri and segun March 7, 2022 16:36

adonesky1 force-pushed the refactor-the-refactor branch from 86826e7 to 1a76353 Compare March 7, 2022 21:36

segun previously approved these changes Mar 8, 2022

View reviewed changes

mcmire previously approved these changes Mar 8, 2022

View reviewed changes

adonesky1 added 6 commits March 8, 2022 16:41

make use of getTokenStandardAndDetails method exposed on assetsContra…

08b88fe

…ctController to determine how to represent the contract being interacted with in token contract method calls

addressing feedback

4c6812a

fix test

13c6b83

reduce title size on confirmation view for longer assets names

a2d975b

address feedback

4716090

adapt rebase

cad8876

adonesky1 dismissed stale reviews from mcmire and segun via cad8876 March 8, 2022 22:44

adonesky1 force-pushed the refactor-the-refactor branch from 1a76353 to cad8876 Compare March 8, 2022 22:44

adonesky1 requested review from mcmire and segun March 8, 2022 22:45

issue from rebase

2274bc1

mcmire approved these changes Mar 8, 2022

View reviewed changes

brad-decker approved these changes Mar 9, 2022

View reviewed changes

adonesky1 merged commit 3747ace into develop Mar 9, 2022

adonesky1 deleted the refactor-the-refactor branch March 9, 2022 14:38

github-actions bot locked and limited conversation to collaborators Mar 9, 2022

Refactor token send/method confirmation flow (trimmed down) #13788

Refactor token send/method confirmation flow (trimmed down) #13788

Uh oh!

Conversation

adonesky1 commented Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

When reviewing PR in action:

The behavior/UX should be unaltered for ERC20 method calls listed above but be more accurate and clear for the same method calls for ERC721 and ERC1155 contracts.

Uh oh!

github-actions bot commented Feb 28, 2022

Uh oh!

adonesky1 Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

adonesky1 Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

adonesky1 Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

adonesky1 Feb 28, 2022 • 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!

adonesky1 Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

adonesky1 Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Feb 28, 2022

highlights:

Uh oh!

metamaskbot commented Mar 2, 2022

highlights:

Uh oh!

metamaskbot commented Mar 7, 2022

highlights:

Uh oh!

adonesky1 commented Mar 7, 2022

Uh oh!

metamaskbot commented Mar 8, 2022

highlights:

Uh oh!

segun left a comment

Choose a reason for hiding this comment

Uh oh!

mcmire left a comment

Choose a reason for hiding this comment

Uh oh!

adonesky1 commented Mar 8, 2022

Uh oh!

metamaskbot commented Mar 9, 2022

highlights:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

adonesky1 commented Feb 28, 2022 •

edited

Loading

The behavior/UX should be unaltered for `ERC20` method calls listed above but be more accurate and clear for the same method calls for `ERC721` and `ERC1155` contracts.

adonesky1 Feb 28, 2022 •

edited

Loading