feat: added test network as selected network if globally selected for connection Request #27980

NidhiKJha · 2024-10-21T09:39:53Z

This PR is to select a test network in the default selected networks list if its the globally selected network at the time of connection request.

Related issues

Fixes: #27891

Manual testing steps

Run extension with yarn start
Switch to Sepolia
Go to test-dapp, click on connect.
In the connections page, check Sepolia is the selected along with non test networks
Click confirm, dapp should be connected to MM and user should be on Sepolia network in MM.

Screenshots/Recordings

Before

Screen.Recording.2024-10-21.at.3.09.20.PM.mov

After

Screen.Recording.2024-10-21.at.3.07.23.PM.mov

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

NidhiKJha · 2024-10-21T14:58:29Z

test/e2e/tests/request-queuing/dapp1-switch-dapp2-send.spec.js

@@ -240,37 +222,19 @@ describe('Request Queuing Dapp 1, Switch Tx -> Dapp 2 Send Tx', function () {
          `window.ethereum.request(${switchEthereumChainRequest})`,
        );

-        await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog);


Since Dapp is already connected to Test Network, we don't need to show review permission screem

the goal of the switch call is to trigger an approval to permit a chain that is not already permitted. ~~This check still needs to occur. Instead the switch call above to should target 0x53a~~ It may be easier to unselect 0x539 (Localhost 8545) when Dapp One is connecting

test/e2e/tests/request-queuing/switchChain-watchAsset.spec.js

metamaskbot · 2024-10-23T13:42:53Z

Builds ready [0355cf1]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-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, 5, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2039 ± 204 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	358	3604	1936	558	268
		domContentLoaded	1575	3516	1991	414	199
		load	1587	3591	2039	425	204
		domInteractive	21	98	51	23	11
		backgroundConnect	9	90	46	28	13
		firstReactRender	46	200	97	41	20
		getState	5	68	22	22	10
		initialActions	0	1	0	0	0
		loadScripts	1113	2425	1456	293	141
		setupStore	11	95	32	24	11
		uiStartup	1802	3804	2258	430	206

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 242 Bytes (0.00%)
common: 0 Bytes (0.00%)

metamaskbot · 2024-10-23T15:15:13Z

Builds ready [aec3385]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-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, 5, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2213 ± 106 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1917	2668	2217	215	103
		domContentLoaded	1863	2654	2173	224	108
		load	1879	2668	2213	220	106
		domInteractive	30	259	69	50	24
		backgroundConnect	9	77	35	24	12
		firstReactRender	54	214	121	38	18
		getState	5	154	29	35	17
		initialActions	0	1	0	0	0
		loadScripts	1356	2142	1622	185	89
		setupStore	11	89	36	28	13
		uiStartup	2141	3114	2497	259	124

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 242 Bytes (0.00%)
common: 0 Bytes (0.00%)

NidhiKJha · 2024-10-23T18:46:50Z

test/e2e/api-specs/ConfirmationRejectionRule.ts

-              const editButtons = await this.driver.findElements(
-                '[data-testid="edit"]',
-              );
-              await editButtons[1].click();
-
-              await this.driver.clickElement({
-                text: 'Localhost 8545',
-                tag: 'p',
-              });
-
-              await this.driver.clickElement(
-                '[data-testid="connect-more-chains-button"]',
-              );
-


Not needed anymore since testnet is now included in the connections request

metamaskbot · 2024-10-23T19:04:28Z

Builds ready [a18a31d]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-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, 5, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1737 ± 127 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	300	1862	1550	405	194
		domContentLoaded	1522	2827	1718	265	127
		load	1531	2845	1737	265	127
		domInteractive	23	73	44	18	8
		backgroundConnect	9	63	20	14	7
		firstReactRender	43	196	96	40	19
		getState	4	59	15	17	8
		initialActions	0	1	0	0	0
		loadScripts	1111	2130	1289	207	100
		setupStore	11	68	24	21	10
		uiStartup	1714	3105	1952	287	138

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 242 Bytes (0.00%)
common: 0 Bytes (0.00%)

FrederikBolding

General logic looks good to me. Would appreciate if someone else reviews the E2E changes in detail

jiexi · 2024-10-24T15:25:30Z

ui/pages/permissions-connect/connect-page/connect-page.tsx

+  const selectedTestNetwork = testNetworks.filter(
+    (network: { chainId: string }) =>
+      network.chainId === currentlySelectedNetworkChainId,
+  );


there should only be one network that matches the chainId, does that sound right to you? We may want to use .find() here to make that clearer if so

jiexi · 2024-10-24T15:26:31Z

ui/pages/permissions-connect/connect-page/connect-page.tsx

+  );
+
+  const selectedNetworksList =
+    selectedTestNetwork === undefined


it's not clear to me how selectedTestNetwork would ever be undefined if it's the result of testNetworks.filter() which should always return an array

jiexi · 2024-10-24T15:49:00Z

test/e2e/tests/request-queuing/dapp1-switch-dapp2-send.spec.js

-        await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog);
-        await driver.findElement({
-          text: 'Use your enabled networks',
-          tag: 'p',
-        });
-


this check should still be valid. The globally selected network is 0x53a when Dapp Two connects, meaning that 0x539 should not be connected for Dapp Two and a Chain Permission approval should appear when trying to switch to it

jiexi · 2024-10-24T15:52:12Z

test/e2e/tests/request-queuing/dapp1-switch-dapp2-send.spec.js

-        await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog);
-
-        // Check correct network on the send confirmation.
-        await driver.findElement({
-          css: '[data-testid="network-display"]',
-          text: 'Localhost 8546',
-        });
-
-        await driver.clickElementAndWaitForWindowToClose({
-          text: 'Confirm',
-          tag: 'button',
-        });
-


this should also still be valid, but I would assume the confirmation is on Localhost 8545

jiexi · 2024-10-24T15:56:11Z

test/e2e/tests/request-queuing/dapp1-switch-dapp2-send.spec.js

        await driver.switchToWindowWithUrl(DAPP_ONE_URL);

        await driver.clickElement('#sendButton');

        await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog);

-        await driver.clickElement({ text: 'Cancel', tag: 'button' });


this needs to stay the same as well since we are testing the scenario when there is an approval for permitting a chain

jiexi · 2024-10-24T15:59:40Z

test/e2e/tests/request-queuing/dapp1-switch-dapp2-send.spec.js

-        await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog);
-
-        // Check correct network on the send confirmation.
-        await driver.findElement({
-          css: '[data-testid="network-display"]',
-          text: 'Localhost 8546',
-        });
-
-        await driver.clickElementAndWaitForWindowToClose({
-          text: 'Confirm',
-          tag: 'button',
-        });


this should stay as well. The goal here is to confirm that the network switch on Dapp One did not affect Dapp Two

jiexi · 2024-10-24T16:09:49Z

test/e2e/json-rpc/switchEthereumChain.spec.js

+        // Switch to Dapp One and connect it
+        await driver.switchToWindowWithUrl(DAPP_URL);
+        await driver.clickElement({ text: 'Connect', tag: 'button' });
+
+        await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog);
+
+        await driver.clickElementAndWaitForWindowToClose({
+          text: 'Connect',
+          tag: 'button',
+        });
+
+        // Switch to MM and disconnect localhost 8545 for Dapp One

+        await driver.switchToWindowWithTitle(
+          WINDOW_TITLES.ExtensionInFullScreenView,
+        );
+        await driver.clickElement(
+          '[data-testid ="account-options-menu-button"]',
+        );
+        await driver.clickElement({
+          text: 'All Permissions',
+          tag: 'div',
+        });
+        await driver.clickElementAndWaitToDisappear({
+          text: 'Got it',
+          tag: 'button',
+        });
+        await driver.clickElement({
+          text: '127.0.0.1:8080',
+          tag: 'p',
+        });
+        const editButtons = await driver.findElements('[data-testid="edit"]');
+
+        assert.ok(editButtons.length > 0, 'Edit buttons are available');
+
+        await editButtons[1].click();
+
+        // Disconnect Mainnet
+        await driver.clickElement({
+          text: 'Localhost 8546',
+          tag: 'p',
+        });
+
+        await driver.clickElement('[data-testid="connect-more-chains-button"]');
+        // Switch to Dapp Two
+        await driver.switchToWindowWithUrl(DAPP_ONE_URL);


thoughts on the connection happening right after line 147? You also had a nice pattern in the other test of deselecting a network during the connection flow which would be great to use here instead of opening the wallet and disconnecting the chain after connecting it

jiexi · 2024-10-24T16:11:41Z

test/e2e/json-rpc/switchEthereumChain.spec.js

+        await driver.switchToWindowWithTitle(
+          WINDOW_TITLES.ExtensionInFullScreenView,
+        );
+        await driver.clickElement(
+          '[data-testid ="account-options-menu-button"]',
+        );
+        await driver.clickElement({ text: 'All Permissions', tag: 'div' });
+        await driver.clickElementAndWaitToDisappear({
+          text: 'Got it',
+          tag: 'button',
+        });
+        await driver.clickElement({
+          text: '127.0.0.1:8081',
+          tag: 'p',
+        });
+        const editButtons = await driver.findElements('[data-testid="edit"]');

+        // Ensure there are edit buttons
+        assert.ok(editButtons.length > 0, 'Edit buttons are available');
+
+        // Click the first (0th) edit button
+        await editButtons[1].click();
+
+        // Disconnect Mainnet
+        await driver.clickElement({
+          text: 'Localhost 8545',
+          tag: 'p',
+        });
+
+        await driver.clickElement('[data-testid="connect-more-chains-button"]');


same here, would be nice to just not connect Dapp Two to 0x539 during the connection flow itself rather than disconnecting it afterwards

jiexi · 2024-10-24T16:12:07Z

test/e2e/json-rpc/switchEthereumChain.spec.js

+        await driver.switchToWindowWithTitle(
+          WINDOW_TITLES.ExtensionInFullScreenView,
+        );
+        await driver.clickElement(
+          '[data-testid ="account-options-menu-button"]',
+        );
+        await driver.clickElement({ text: 'All Permissions', tag: 'div' });
+        await driver.clickElementAndWaitToDisappear({
+          text: 'Got it',
+          tag: 'button',
+        });
+        await driver.clickElement({
+          text: '127.0.0.1:8081',
+          tag: 'p',
+        });
+        const editButtons = await driver.findElements('[data-testid="edit"]');
+
+        // Ensure there are edit buttons
+        assert.ok(editButtons.length > 0, 'Edit buttons are available');
+
+        // Click the first (0th) edit button
+        await editButtons[1].click();
+
+        // Disconnect Mainnet
+        await driver.clickElement({
+          text: 'Localhost 8545',
+          tag: 'p',
+        });

+        await driver.clickElement('[data-testid="connect-more-chains-button"]');


added test network as selected network if globally selected

e9878b4

NidhiKJha added team-wallet-ux needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. labels Oct 21, 2024

NidhiKJha requested a review from a team as a code owner October 21, 2024 09:39

metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Oct 21, 2024

updated test

c4dac99

NidhiKJha requested a review from a team as a code owner October 21, 2024 09:51

NidhiKJha added 5 commits October 21, 2024 16:46

updated edit-network spec file

8ffa4d6

removed unnecessary function

c9686c8

updated test for multi-dap-sendTx spec file

5b6ca71

updated test network to be added in connections flow

019d03d

updated switch case test

9aa9634

NidhiKJha commented Oct 21, 2024

View reviewed changes

NidhiKJha and others added 2 commits October 21, 2024 23:58

updated switch chain spec file

1f27dc0

Merge branch 'develop' into fix-connection-page-test-net

49390c6

Gudahtt reviewed Oct 23, 2024

View reviewed changes

test/e2e/tests/request-queuing/switchChain-watchAsset.spec.js Outdated Show resolved Hide resolved

Gudahtt reviewed Oct 23, 2024

View reviewed changes

test/e2e/tests/request-queuing/switchChain-watchAsset.spec.js Outdated Show resolved Hide resolved

NidhiKJha and others added 4 commits October 23, 2024 16:56

updated test for switchEthereum spec

7b09dec

Merge branch 'develop' into fix-connection-page-test-net

b1c1d3c

updated close window function

b8b0724

updated test

0355cf1

NidhiKJha force-pushed the fix-connection-page-test-net branch from 006e40d to 0355cf1 Compare October 23, 2024 14:28

Merge branch 'develop' into fix-connection-page-test-net

aec3385

NidhiKJha and others added 3 commits October 23, 2024 22:42

Merge branch 'develop' into fix-connection-page-test-net

028c1dd

updated api spec

e9d817f

updated switchChain watch spec file

a18a31d

NidhiKJha requested a review from Gudahtt October 23, 2024 18:21

NidhiKJha enabled auto-merge October 23, 2024 18:23

NidhiKJha commented Oct 23, 2024

View reviewed changes

FrederikBolding approved these changes Oct 24, 2024

View reviewed changes

jiexi reviewed Oct 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added test network as selected network if globally selected for connection Request #27980

feat: added test network as selected network if globally selected for connection Request #27980

NidhiKJha commented Oct 21, 2024 •

edited

Loading

NidhiKJha Oct 21, 2024

jiexi Oct 24, 2024 •

edited

Loading

metamaskbot commented Oct 23, 2024

metamaskbot commented Oct 23, 2024

NidhiKJha Oct 23, 2024

metamaskbot commented Oct 23, 2024

FrederikBolding left a comment

jiexi Oct 24, 2024

jiexi Oct 24, 2024

jiexi Oct 24, 2024

jiexi Oct 24, 2024

jiexi Oct 24, 2024

jiexi Oct 24, 2024

jiexi Oct 24, 2024

jiexi Oct 24, 2024

jiexi Oct 24, 2024

feat: added test network as selected network if globally selected for connection Request #27980

Are you sure you want to change the base?

feat: added test network as selected network if globally selected for connection Request #27980

Conversation

NidhiKJha commented Oct 21, 2024 • edited Loading

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Choose a reason for hiding this comment

jiexi Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

metamaskbot commented Oct 23, 2024

metamaskbot commented Oct 23, 2024

Choose a reason for hiding this comment

metamaskbot commented Oct 23, 2024

FrederikBolding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NidhiKJha commented Oct 21, 2024 •

edited

Loading

jiexi Oct 24, 2024 •

edited

Loading