Disabling network and account changes after the send flow is initiated #18086

NiranjanaBinoy · 2023-03-09T19:47:01Z

Explanation

Fixes #18045
The initial issue was that if the user changes the network while performing a send transaction using ens, the ens gets resolved to the new network's wrong address.

The solution is to disable the ability to change the network and account once the user start a sends transactions.
#18045 (comment)

Screenshots/Screencaps

Before

After

Manual Testing Steps

Start a send transaction.
Click on the network display bar to change the network in the add recipient page; this should be disabled.
Click on the network display bar to change the network in the draft send page, this should also be disabled.

Pre-merge author checklist

I've clearly explained:
- What problem this PR is solving
- How this problem was solved
- How reviewers can test my changes
Sufficient automated test coverage has been added

Pre-merge reviewer checklist

Manual testing (e.g. pull and build branch, run in browser, test code being changed)
PR is linked to the appropriate GitHub issue
IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

github-actions · 2023-03-09T19:47:16Z

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 · 2023-03-09T19:59:35Z

Builds ready [4f9c043]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1808 ± 85 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	105	217	136	27	13
		domContentLoaded	1558	2219	1759	154	74
		load	1558	2370	1808	176	85
		domInteractive	1558	2219	1759	154	74

Bundle size diffs

background: 0 bytes
ui: 96 bytes
common: 0 bytes

codecov · 2023-03-09T19:59:42Z

Codecov Report

Merging #18086 (327597b) into develop (f92e463) will increase coverage by 0.38%.
The diff coverage is 100.00%.

❗ Current head 327597b differs from pull request most recent head 0ec236f. Consider uploading reports for the commit 0ec236f to get more accurate results

@@             Coverage Diff             @@
##           develop   #18086      +/-   ##
===========================================
+ Coverage    65.15%   65.53%   +0.38%     
===========================================
  Files          936      914      -22     
  Lines        35965    35288     -677     
  Branches      9231     8984     -247     
===========================================
- Hits         23432    23124     -308     
+ Misses       12533    12164     -369

Impacted Files	Coverage Δ
.../components/app/app-header/app-header.component.js	`97.44% <ø> (+2.56%)`	⬆️
ui/pages/routes/routes.component.js	`56.39% <100.00%> (+56.39%)`	⬆️

... and 288 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

jpuri · 2023-03-09T23:03:43Z

Plz check if it possible to add test coverage, it will be good and codecov will also be green 😄

metamaskbot · 2023-03-10T16:34:35Z

Builds ready [2a27b83]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1577 ± 43 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	94	218	122	26	13
		domContentLoaded	1444	1702	1546	84	40
		load	1444	1748	1577	89	43
		domInteractive	1443	1702	1546	84	40

Bundle size diffs

background: 0 bytes
ui: 96 bytes
common: 0 bytes

metamaskbot · 2023-03-13T15:03:28Z

Builds ready [0479804]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1608 ± 52 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	98	199	121	21	10
		domContentLoaded	1351	1760	1588	108	52
		load	1351	1760	1608	107	52
		domInteractive	1351	1760	1588	108	52

Bundle size diffs

background: 0 bytes
ui: 96 bytes
common: 0 bytes

jpuri · 2023-03-20T14:37:48Z

coverage-targets.js

+    lines: 66.58,
+    branches: 54.79,
+    statements: 65.74,
+    functions: 58.4,
  },
  transforms: {
    branches: 100,


jpuri · 2023-03-20T14:38:59Z

ui/pages/routes/routes.component.test.js

+      expect(getByTestId('account-menu-icon')).not.toBeDisabled();
+    });
+  });
+});


NIT: test case description can be made more intuitive.

jpuri

Just a small feedbacks, looks great otherwise 👍

metamaskbot · 2023-03-20T14:46:18Z

Builds ready [e275550]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1711 ± 56 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	109	339	143	50	24
		domContentLoaded	1526	1924	1698	107	51
		load	1543	1952	1711	116	56
		domInteractive	1526	1924	1698	107	51

Bundle size diffs

background: 0 bytes
ui: 116 bytes
common: 0 bytes

seaona · 2023-03-21T14:14:55Z

Great job @NiranjanaBinoy I can see how network is disabled while performing a transaction.

I've noticed that the Account is also disabled. Is this the intended behaviour? cc @bschorchit

extension-change-account.mp4

NiranjanaBinoy · 2023-03-21T16:25:28Z

@seaona The account was disabled for the other scenarios where the network drop-down was disabled, so I replicated the same on this.
@bschorchit Should we stick to this behavior or keep the account active while the user is in the Add Reciepent and draft send pages?

bschorchit · 2023-03-21T17:30:20Z

I wonder how common is for users to change accounts during this flow. I'm inclined to say we keep the account changing blocked in those screens as well mainly because there's no info on the from account in these screens besides the icon on the top right for the user to review and confirm. And because changing account also creates some bugs (e.g. if you're sending an asset you own on one account but not in another and you change it to that second account). What do you think? Any strong reason to keep it enabled?

Minor detail: Is the difference on how the cursor looks like for hovering over the network dropdown and the account dropdown intentional?

seaona · 2023-03-21T17:59:41Z

Make sense to me! Thank you @NiranjanaBinoy @bschorchit !! Maybe we could update the PR description/title to reference to the Account Disabling too 👍

jpuri

🚀

segun · 2023-03-22T15:11:19Z

Can you please fix the lint failures, then I will approve

metamaskbot · 2023-03-22T20:42:12Z

Builds ready [327597b]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2008 ± 100 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	124	232	170	36	17
		domContentLoaded	1481	2299	2002	205	99
		load	1481	2300	2008	209	100
		domInteractive	1481	2299	2002	205	99

Bundle size diffs

background: 0 bytes
ui: 116 bytes
common: 0 bytes

metamaskbot · 2023-04-11T19:04:43Z

Builds ready [0ec236f]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1592 ± 34 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	95	299	119	42	20
		domContentLoaded	1442	1748	1578	78	38
		load	1442	1748	1592	71	34
		domInteractive	1442	1748	1578	78	38

Bundle size diffs

background: 0 bytes
ui: 116 bytes
common: 0 bytes

jpuri

LGTM

NiranjanaBinoy self-assigned this Mar 9, 2023

NiranjanaBinoy marked this pull request as ready for review March 9, 2023 19:47

NiranjanaBinoy requested a review from a team as a code owner March 9, 2023 19:47

NiranjanaBinoy requested a review from pedronfigueiredo March 9, 2023 19:47

NiranjanaBinoy added team-confirmations-secure-ux-PR PRs from the confirmations team type-bug Sev2-normal Normal severity; minor loss of service or inconvenience. area-transactions labels Mar 9, 2023

jpuri previously approved these changes Mar 9, 2023

View reviewed changes

NiranjanaBinoy dismissed jpuri’s stale review via 70c1c97 March 16, 2023 14:22

NiranjanaBinoy requested review from jpuri and segun March 18, 2023 01:23

NiranjanaBinoy force-pushed the network-change-ens-resolution branch from 69ce715 to f103c7b Compare March 18, 2023 01:40

NiranjanaBinoy removed the request for review from pedronfigueiredo March 18, 2023 01:42

NiranjanaBinoy force-pushed the network-change-ens-resolution branch from 91b9a85 to 13de2c6 Compare March 20, 2023 13:58

jpuri reviewed Mar 20, 2023

View reviewed changes

coverage-targets.js

lines: 66.58,

branches: 54.79,

statements: 65.74,

functions: 58.4,

},

transforms: {

branches: 100,

Copy link

Contributor

jpuri Mar 20, 2023

Choose a reason for hiding this comment

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

🚀

jpuri reviewed Mar 20, 2023

View reviewed changes

jpuri previously approved these changes Mar 20, 2023

View reviewed changes

NiranjanaBinoy changed the title ~~Disabling network change after the send flow is initiated~~ Disabling network change and account change after the send flow is initiated Mar 21, 2023

NiranjanaBinoy changed the title ~~Disabling network change and account change after the send flow is initiated~~ Disabling network and account changes after the send flow is initiated Mar 21, 2023

NiranjanaBinoy dismissed jpuri’s stale review via 409f1a6 March 22, 2023 14:25

jpuri previously approved these changes Mar 22, 2023

View reviewed changes

NiranjanaBinoy dismissed jpuri’s stale review via 327597b March 22, 2023 20:17

NiranjanaBinoy requested a review from jpuri March 22, 2023 20:18

bschorchit mentioned this pull request Mar 23, 2023

Fix for changing to Goerli network causes insufficient error despite having enough funds #18018

Closed

jpuri previously approved these changes Mar 24, 2023

View reviewed changes

segun previously approved these changes Apr 11, 2023

View reviewed changes

NiranjanaBinoy and others added 8 commits April 11, 2023 13:25

diabling network change once the send flow is initiate

e31a1d6

adding test file

e7b9760

Fix test cases

fe17e4b

updating test files

631ce2f

bumping coverage target

ea44966

bumping coverage

cab3198

updating test case description

c4e1fd4

lint fix

741200c

NiranjanaBinoy dismissed stale reviews from segun and jpuri via 741200c April 11, 2023 17:27

NiranjanaBinoy force-pushed the network-change-ens-resolution branch from 327597b to 741200c Compare April 11, 2023 17:27

NiranjanaBinoy requested review from segun and jpuri April 11, 2023 17:31

updating coverage

0ec236f

jpuri approved these changes Apr 12, 2023

View reviewed changes

segun approved these changes Apr 13, 2023

View reviewed changes

NiranjanaBinoy merged commit 643a89f into develop Apr 13, 2023

NiranjanaBinoy deleted the network-change-ens-resolution branch April 13, 2023 16:51

github-actions bot locked and limited conversation to collaborators Apr 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disabling network and account changes after the send flow is initiated #18086

Disabling network and account changes after the send flow is initiated #18086

NiranjanaBinoy commented Mar 9, 2023 •

edited

Loading

github-actions bot commented Mar 9, 2023

metamaskbot commented Mar 9, 2023

codecov bot commented Mar 9, 2023 •

edited

Loading

jpuri commented Mar 9, 2023

metamaskbot commented Mar 10, 2023

metamaskbot commented Mar 13, 2023

jpuri Mar 20, 2023

jpuri Mar 20, 2023

jpuri left a comment

metamaskbot commented Mar 20, 2023

seaona commented Mar 21, 2023

NiranjanaBinoy commented Mar 21, 2023

bschorchit commented Mar 21, 2023

seaona commented Mar 21, 2023

jpuri left a comment

segun commented Mar 22, 2023

metamaskbot commented Mar 22, 2023

metamaskbot commented Apr 11, 2023

jpuri left a comment

Disabling network and account changes after the send flow is initiated #18086

Disabling network and account changes after the send flow is initiated #18086

Conversation

NiranjanaBinoy commented Mar 9, 2023 • edited Loading

Explanation

Screenshots/Screencaps

Before

After

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Mar 9, 2023

metamaskbot commented Mar 9, 2023

codecov bot commented Mar 9, 2023 • edited Loading

Codecov Report

jpuri commented Mar 9, 2023

metamaskbot commented Mar 10, 2023

metamaskbot commented Mar 13, 2023

jpuri Mar 20, 2023

Choose a reason for hiding this comment

jpuri Mar 20, 2023

Choose a reason for hiding this comment

jpuri left a comment

Choose a reason for hiding this comment

metamaskbot commented Mar 20, 2023

seaona commented Mar 21, 2023

NiranjanaBinoy commented Mar 21, 2023

bschorchit commented Mar 21, 2023

seaona commented Mar 21, 2023

jpuri left a comment

Choose a reason for hiding this comment

segun commented Mar 22, 2023

metamaskbot commented Mar 22, 2023

metamaskbot commented Apr 11, 2023

jpuri left a comment

Choose a reason for hiding this comment

NiranjanaBinoy commented Mar 9, 2023 •

edited

Loading

codecov bot commented Mar 9, 2023 •

edited

Loading