Adding option to set Custom Nonce to Confirm Approve Page by NiranjanaBinoy · Pull Request #10595 · MetaMask/metamask-extension

NiranjanaBinoy · 2021-03-05T00:05:54Z

Explanation:

The option to set custom nonce was not available in Confirm Approve Page when the Customize transaction nonce is ON.

This was because <ConfirmApproveContent/> is passed as the contentComponent propertery of <ConfirmTransactionBase/> which prevents rendering of renderDetails() (method in Confirm-Transaction-Base.component.js), which contains the code for Custom Nonce settings.

The new changes includes a section to display the nextNonce number in the approve screen after Transaction Fee and before the collapsing section with Edit button. Upon clicking the edit a new modal will open enabling the user to update the nonce number.

Manual testing steps:

Set Customize transaction nonce to 'ON'
From the test-dapp connect to the wallet and Create Token
Once the Token is added, Click Approve token.
In the Approve Token window click the edit button the nonce section , a new modal will appear where the user can customize the nonce value.

Screen.Recording.2021-04-16.at.5.28.42.PM.mov

github-actions · 2021-03-05T00:06:09Z

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.

metamaskbot · 2021-03-05T02:58:33Z

Builds ready [d785737]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (680 ± 39 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	45	98	65	13	6
		domContentLoaded	520	860	679	81	39
		load	521	861	680	81	39
		domInteractive	519	859	678	81	39

darkwing · 2021-03-08T15:56:31Z

Wow @NiranjanaBinoy, this is awesome, super useful! Two quick comments:

I agree we may want to approach the design team; for example, we're showing two different types of errors here, which makes the page "bounce" a bit:

ErrorMadness.mp4

Is there a way we can always treat the nonce value as a number type?

NiranjanaBinoy · 2021-03-09T02:01:51Z

Thanks, @darkwing.

Regarding your questions:

As you have mentioned, we need input from the Design Team on how to manage two types of errors on the page, @rachelcope will be able to help us out 😊
Using type number for nonce might be possible (but have to dig deeper), as the nextNonce returned by nonce-tracker is of type number after typescript migration. But I am afraid that change might be a little too big for this PR, we might need to confirm that with the team.

NiranjanaBinoy · 2021-03-11T02:25:10Z

Design update from @rachelcope
https://www.figma.com/file/Emfl6CRPRtuQtzJEOJJ3hy/Approve-Screen?node-id=1417%3A1

metamaskbot · 2021-03-16T14:39:45Z

Builds ready [a11627d]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (608 ± 31 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	45	67	55	6	3
		domContentLoaded	388	772	607	65	31
		load	389	774	608	65	31
		domInteractive	388	772	607	65	31

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

ui/app/pages/confirm-approve/confirm-approve-content/index.scss

metamaskbot · 2021-03-23T19:31:12Z

Builds ready [60d47d4]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (609 ± 36 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	44	79	57	9	4
		domContentLoaded	371	788	608	75	36
		load	372	789	609	75	36
		domInteractive	371	788	607	75	36

metamaskbot · 2021-03-23T20:00:12Z

Builds ready [c7a7653]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (627 ± 28 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	48	77	59	8	4
		domContentLoaded	564	804	626	59	28
		load	565	805	627	59	28
		domInteractive	563	803	626	59	28

metamaskbot · 2021-03-23T21:15:34Z

Builds ready [efebbcc]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (583 ± 27 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	43	81	57	12	6
		domContentLoaded	361	646	581	55	27
		load	362	647	583	55	27
		domInteractive	360	646	581	55	27

metamaskbot · 2021-03-23T21:55:22Z

Builds ready [c1e6ae5]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (635 ± 27 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	44	95	61	14	7
		domContentLoaded	557	767	634	56	27
		load	559	768	635	55	27
		domInteractive	557	766	634	55	27

app/_locales/en/messages.json

brad-decker

Took my first pass on this, in addition, i'd say as a challenge to see how many of your custom classes and styles can be eliminated by making use of the Typography component as per one of my request comments.

ui/app/components/app/modals/customize-nonce/index.scss

ui/app/components/app/modals/customize-nonce/customize-nonce.component.js

ui/app/pages/confirm-approve/confirm-approve-content/index.scss

ui/app/components/app/modals/customize-nonce/customize-nonce.component.js

metamaskbot · 2021-03-26T13:48:57Z

Builds ready [5dd3463]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (654 ± 29 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	46	107	59	14	7
		domContentLoaded	546	807	653	60	29
		load	548	807	654	60	29
		domInteractive	546	806	653	60	29

metamaskbot · 2021-03-28T02:33:52Z

Builds ready [4ec500a]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (610 ± 26 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	45	86	58	10	5
		domContentLoaded	422	736	609	54	26
		load	423	737	610	54	26
		domInteractive	421	736	608	54	26

metamaskbot · 2021-03-29T15:06:47Z

Builds ready [db224e6]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (638 ± 18 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	44	71	58	8	4
		domContentLoaded	570	716	636	38	18
		load	575	717	638	38	18
		domInteractive	570	715	636	38	18

metamaskbot · 2021-03-29T16:27:48Z

Builds ready [c6527f0]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (664 ± 25 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	49	86	61	9	4
		domContentLoaded	594	797	663	52	25
		load	595	799	664	52	25
		domInteractive	594	797	663	52	25

metamaskbot · 2021-03-29T19:36:30Z

Builds ready [20da7f8]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (1032 ± 72 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	NaN	NaN	NaN	NaN	NaN
		domContentLoaded	673	1321	1028	150	72
		load	674	1322	1032	150	72
		domInteractive	673	1321	1028	150	72

app/_locales/en/messages.json

ui/app/components/app/modal/modal.component.js

ui/app/components/app/modals/customize-nonce/customize-nonce.component.js

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

ui/app/components/app/modals/customize-nonce/customize-nonce.component.js

metamaskbot · 2021-04-13T02:51:18Z

Builds ready [b68233d]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (612 ± 33 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	43	82	56	12	6
		domContentLoaded	441	777	610	70	33
		load	442	778	612	69	33
		domInteractive	441	777	610	70	33

brad-decker

Getting close :D. My goal with this review was to help reduce overhead of maintaining this feature in the future. This really focused on:

Eliminating as much custom css as possible to achieve the effect.
Looking at CSS patterns to make sure that known maintenance issues like using absolute positioning are not used.
Semantic HTML

Let me know if I can help in anyway!

ui/app/components/app/modals/customize-nonce/index.scss

ui/app/components/app/modals/customize-nonce/customize-nonce.component.js

ui/app/components/app/modals/customize-nonce/index.scss

ui/app/components/app/modals/customize-nonce/customize-nonce.component.js

ui/app/components/app/modals/customize-nonce/index.scss

ui/app/components/app/modals/customize-nonce/customize-nonce.component.js

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

ui/app/pages/confirm-approve/confirm-approve-content/index.scss

metamaskbot · 2021-04-16T20:42:03Z

Builds ready [6d5d9ee]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (578 ± 31 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	44	71	56	8	4
		domContentLoaded	367	649	575	63	30
		load	369	669	578	65	31
		domInteractive	367	648	575	63	30

…depreciating .container

metamaskbot · 2021-04-16T21:13:40Z

Builds ready [4734776]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (647 ± 39 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	46	84	61	11	5
		domContentLoaded	409	864	646	82	39
		load	410	865	647	82	39
		domInteractive	409	863	646	82	39

brad-decker

c'est magnifique!

NiranjanaBinoy requested a review from a team as a code owner March 5, 2021 00:05

NiranjanaBinoy self-assigned this Mar 5, 2021

NiranjanaBinoy requested a review from shanejonas March 5, 2021 00:05

NiranjanaBinoy force-pushed the approve-token-custom-nonce branch from 9cca8ef to d785737 Compare March 5, 2021 02:41

NiranjanaBinoy force-pushed the approve-token-custom-nonce branch from d785737 to a11627d Compare March 16, 2021 14:22

brad-decker suggested changes Mar 16, 2021

View reviewed changes

NiranjanaBinoy force-pushed the approve-token-custom-nonce branch from 60d47d4 to c7a7653 Compare March 23, 2021 19:43

Gudahtt reviewed Mar 24, 2021

View reviewed changes

app/_locales/en/messages.json Outdated Show resolved Hide resolved

brad-decker suggested changes Mar 25, 2021

View reviewed changes

brad-decker force-pushed the approve-token-custom-nonce branch from 2e4a5b7 to c6527f0 Compare March 29, 2021 16:10

brad-decker suggested changes Mar 29, 2021

View reviewed changes

app/_locales/en/messages.json Outdated Show resolved Hide resolved

ui/app/components/app/modal/modal.component.js Outdated Show resolved Hide resolved

ui/app/components/app/modals/customize-nonce/customize-nonce.component.js Outdated Show resolved Hide resolved

ryanml reviewed Mar 30, 2021

View reviewed changes

ui/app/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js Outdated Show resolved Hide resolved

ryanml reviewed Mar 30, 2021

View reviewed changes

ui/app/components/app/modals/customize-nonce/customize-nonce.component.js Outdated Show resolved Hide resolved

brad-decker suggested changes Apr 14, 2021

View reviewed changes

NiranjanaBinoy and others added 16 commits April 16, 2021 16:49

adding Custom Nonce to Approve Token Page

b56fe85

updating to new UI design

b360b3c

adjusting padding

ed18494

lint fix

9a2c39b

removing onkeypress

aecf9d6

updating message

aba0319

review changes

c073c0d

adding Button for reset

66c163f

suggested changes

33e1f14

minor styles update

2b0d457

review suggestions including refactoring to functional component and …

0db3eb0

…depreciating .container

improvments in styles based on review comments

7e09dd0

replacing the edit <div/> in renderApproveContentCard with <Button/>

bd0a6fa

replacing anchor tag with Button

26e8681

replacing anchor tag with Button and removing absolute positioning

6f45ad7

fixing e2e test failure'

4734776

NiranjanaBinoy force-pushed the approve-token-custom-nonce branch from 6d5d9ee to 4734776 Compare April 16, 2021 20:58

brad-decker approved these changes Apr 16, 2021

View reviewed changes

NiranjanaBinoy merged commit 18aa540 into develop Apr 16, 2021

NiranjanaBinoy deleted the approve-token-custom-nonce branch April 16, 2021 22:00

github-actions bot locked and limited conversation to collaborators Apr 16, 2021

Conversation

NiranjanaBinoy commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 5, 2021

Uh oh!

metamaskbot commented Mar 5, 2021

Uh oh!

darkwing commented Mar 8, 2021

Uh oh!

NiranjanaBinoy commented Mar 9, 2021

Uh oh!

NiranjanaBinoy commented Mar 11, 2021

Uh oh!

metamaskbot commented Mar 16, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metamaskbot commented Mar 23, 2021

Uh oh!

metamaskbot commented Mar 23, 2021

Uh oh!

metamaskbot commented Mar 23, 2021

Uh oh!

metamaskbot commented Mar 23, 2021

Uh oh!

Uh oh!

brad-decker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metamaskbot commented Mar 26, 2021

Uh oh!

metamaskbot commented Mar 28, 2021

Uh oh!

metamaskbot commented Mar 29, 2021

Uh oh!

metamaskbot commented Mar 29, 2021

Uh oh!

metamaskbot commented Mar 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metamaskbot commented Apr 13, 2021

Uh oh!

brad-decker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metamaskbot commented Apr 16, 2021

Uh oh!

metamaskbot commented Apr 16, 2021

Uh oh!

brad-decker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

NiranjanaBinoy commented Mar 5, 2021 •

edited

Loading