From 044faeb8cf4323d6a467d70f7ef3d135c389f4f9 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Wed, 19 Jun 2024 10:30:23 -0500 Subject: [PATCH 1/8] fix: UX: Multichain: Fix dead network problem when switching tabs --- app/scripts/metamask-controller.js | 5 ++++ ui/store/actions.ts | 42 ++++++++++++++++++++++++------ 2 files changed, 39 insertions(+), 8 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 360e30a89303..0be8e2459f3a 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -3187,6 +3187,11 @@ export default class MetamaskController extends EventEmitter { setActiveNetwork: (networkConfigurationId) => { return this.networkController.setActiveNetwork(networkConfigurationId); }, + // Avoids returning the promise so that initial call to switch network + // doesn't block on the network lookup step + setActiveNetworkConfigurationId: (networkConfigurationId) => { + this.networkController.setActiveNetwork(networkConfigurationId); + }, setNetworkClientIdForDomain: (origin, networkClientId) => { return this.selectedNetworkController.setNetworkClientIdForDomain( origin, diff --git a/ui/store/actions.ts b/ui/store/actions.ts index 186e59ccb8df..8b459a9699d5 100644 --- a/ui/store/actions.ts +++ b/ui/store/actions.ts @@ -2158,14 +2158,22 @@ export function automaticallySwitchNetwork( selectedTabOrigin: string, ): ThunkAction { return async (dispatch: MetaMaskReduxDispatch) => { - await dispatch(setActiveNetwork(networkClientIdForThisDomain)); - await dispatch( - setSwitchedNetworkDetails({ - networkClientId: networkClientIdForThisDomain, - origin: selectedTabOrigin, - }), - ); - await forceUpdateMetamaskState(dispatch); + try { + await dispatch( + setActiveNetworkConfigurationId(networkClientIdForThisDomain), + ); + await dispatch( + setSwitchedNetworkDetails({ + networkClientId: networkClientIdForThisDomain, + origin: selectedTabOrigin, + }), + ); + await forceUpdateMetamaskState(dispatch); + } catch (e) { + // The network did not load in time; let network try to load in background + // so that the normal UI switching can go through + console.log("Network wasn't available, try again but async"); + } }; } @@ -2494,6 +2502,24 @@ export function setActiveNetwork( }; } +export function setActiveNetworkConfigurationId( + networkConfigurationId: string, +): ThunkAction { + return async (dispatch) => { + log.debug( + `background.setActiveNetworkConfigurationId: ${networkConfigurationId}`, + ); + try { + await submitRequestToBackground('setActiveNetworkConfigurationId', [ + networkConfigurationId, + ]); + } catch (error) { + logErrorWithMessage(error); + dispatch(displayWarning('Had a problem changing networks!')); + } + }; +} + export function rollbackToPreviousProvider(): ThunkAction< void, MetaMaskReduxState, From 95988c64c83d2588d6201d07d908a1e9c3a04297 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Wed, 19 Jun 2024 11:46:45 -0500 Subject: [PATCH 2/8] Add representative test scenario --- test/e2e/tests/request-queuing/ui.spec.js | 61 ++++++++++++++++++++++- 1 file changed, 60 insertions(+), 1 deletion(-) diff --git a/test/e2e/tests/request-queuing/ui.spec.js b/test/e2e/tests/request-queuing/ui.spec.js index 45429ad32263..185367830f4b 100644 --- a/test/e2e/tests/request-queuing/ui.spec.js +++ b/test/e2e/tests/request-queuing/ui.spec.js @@ -165,7 +165,7 @@ async function validateBalanceAndActivity( } describe('Request-queue UI changes', function () { - it('UI should show network specific to domain @no-mmi', async function () { + it('should show network specific to domain @no-mmi', async function () { const port = 8546; const chainId = 1338; // 0x53a await withFixtures( @@ -355,4 +355,63 @@ describe('Request-queue UI changes', function () { }, ); }); + + it.only('should gracefully handle network connectivity failure @no-mmi', async function () { + const port = 8546; + const chainId = 1338; + await withFixtures( + { + dapp: true, + fixtures: new FixtureBuilder() + .withNetworkControllerDoubleGanache() + .withPreferencesControllerUseRequestQueueEnabled() + .withSelectedNetworkControllerPerDomain() + .build(), + ganacheOptions: { + ...defaultGanacheOptions, + concurrent: [ + { + port, + chainId, + ganacheOptions2: defaultGanacheOptions, + }, + ], + }, + dappOptions: { numberOfDapps: 2 }, + title: this.test.fullTitle(), + }, + async ({ driver, ganacheServer, secondaryGanacheServer }) => { + await unlockWallet(driver); + + // Navigate to extension home screen + await driver.navigate(PAGES.HOME); + + // Open the first dapp + await openDappAndSwitchChain(driver, DAPP_URL); + + // Open the second dapp and switch chains + await openDappAndSwitchChain(driver, DAPP_ONE_URL, '0x1'); + + // Go to wallet fullscreen, ensure that the global network changed to Ethereum Mainnet + await driver.switchToWindowWithTitle( + WINDOW_TITLES.ExtensionInFullScreenView, + ); + await driver.findElement({ + css: '[data-testid="network-display"]', + text: 'Ethereum Mainnet', + }); + + // Kill ganache servers + await ganacheServer.quit(); + await secondaryGanacheServer[0].quit(); + + // Go back to first dapp, try an action, ensure network connection failure doesn't block UI + const dappOneNetworkPillText = await selectDappClickSendGetNetwork( + driver, + DAPP_URL, + ); + assert.equal(dappOneNetworkPillText, 'Localhost 8545'); + }, + ); + }); }); From c15ce3c76f2b0b001660020824e527adae3b51b2 Mon Sep 17 00:00:00 2001 From: David Walsh Date: Wed, 19 Jun 2024 12:10:59 -0500 Subject: [PATCH 3/8] Add test for deleted network --- test/e2e/tests/request-queuing/ui.spec.js | 75 +++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/test/e2e/tests/request-queuing/ui.spec.js b/test/e2e/tests/request-queuing/ui.spec.js index 185367830f4b..5a571ca0de23 100644 --- a/test/e2e/tests/request-queuing/ui.spec.js +++ b/test/e2e/tests/request-queuing/ui.spec.js @@ -414,4 +414,79 @@ describe('Request-queue UI changes', function () { }, ); }); + + it('should gracefully handle deleted network @no-mmi', async function () { + const port = 8546; + const chainId = 1338; + await withFixtures( + { + dapp: true, + fixtures: new FixtureBuilder() + .withNetworkControllerDoubleGanache() + .withPreferencesControllerUseRequestQueueEnabled() + .withSelectedNetworkControllerPerDomain() + .build(), + ganacheOptions: { + ...defaultGanacheOptions, + concurrent: [ + { + port, + chainId, + ganacheOptions2: defaultGanacheOptions, + }, + ], + }, + dappOptions: { numberOfDapps: 2 }, + title: this.test.fullTitle(), + }, + async ({ driver }) => { + await unlockWallet(driver); + + // Navigate to extension home screen + await driver.navigate(PAGES.HOME); + + // Open the first dapp + await openDappAndSwitchChain(driver, DAPP_URL); + + // Open the second dapp and switch chains + await openDappAndSwitchChain(driver, DAPP_ONE_URL, '0x1'); + + // Go to wallet fullscreen, ensure that the global network changed to Ethereum Mainnet + await driver.switchToWindowWithTitle( + WINDOW_TITLES.ExtensionInFullScreenView, + ); + await driver.findElement({ + css: '[data-testid="network-display"]', + text: 'Ethereum Mainnet', + }); + + // Go to Settings, delete the first dapp's network + await driver.clickElement( + '[data-testid="account-options-menu-button"]', + ); + await driver.clickElement('[data-testid="global-menu-settings"]'); + await driver.clickElement({ + css: '.tab-bar__tab__content__title', + text: 'Networks', + }); + await driver.clickElement({ + css: '.networks-tab__networks-list-name', + text: 'Localhost 8545', + }); + await driver.clickElement({ css: '.btn-danger', text: 'Delete' }); + await driver.clickElement({ + css: '.modal-container__footer-button', + text: 'Delete', + }); + + // Go back to first dapp, try an action, ensure deleted network doesn't block UI + // The current globally selected network, Ethereum Mainnet, should be used + const dappOneNetworkPillText = await selectDappClickSendGetNetwork( + driver, + DAPP_URL, + ); + assert.equal(dappOneNetworkPillText, 'Ethereum Mainnet'); + }, + ); + }); }); From 951c19e69cf42ddf0e1b2b516abdcb1e86fd4c7b Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 19 Jun 2024 21:28:37 -0230 Subject: [PATCH 4/8] Ignore expected console errors from quitting ganache --- test/e2e/tests/request-queuing/ui.spec.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/e2e/tests/request-queuing/ui.spec.js b/test/e2e/tests/request-queuing/ui.spec.js index 5a571ca0de23..dc8105571173 100644 --- a/test/e2e/tests/request-queuing/ui.spec.js +++ b/test/e2e/tests/request-queuing/ui.spec.js @@ -377,6 +377,9 @@ describe('Request-queue UI changes', function () { }, ], }, + // This test intentionally quits Ganache while the extension is using it, causing + // PollingBlockTracker errors. These are expected. + ignoredConsoleErrors: ['PollingBlockTracker'], dappOptions: { numberOfDapps: 2 }, title: this.test.fullTitle(), }, From 33a9ebe44fadc6534d4b824e4d073256f963eddc Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 19 Jun 2024 21:29:18 -0230 Subject: [PATCH 5/8] Remove unnecessary try..catch block --- ui/store/actions.ts | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/ui/store/actions.ts b/ui/store/actions.ts index 8b459a9699d5..e677c45f9d33 100644 --- a/ui/store/actions.ts +++ b/ui/store/actions.ts @@ -2158,22 +2158,14 @@ export function automaticallySwitchNetwork( selectedTabOrigin: string, ): ThunkAction { return async (dispatch: MetaMaskReduxDispatch) => { - try { - await dispatch( - setActiveNetworkConfigurationId(networkClientIdForThisDomain), - ); - await dispatch( - setSwitchedNetworkDetails({ - networkClientId: networkClientIdForThisDomain, - origin: selectedTabOrigin, - }), - ); - await forceUpdateMetamaskState(dispatch); - } catch (e) { - // The network did not load in time; let network try to load in background - // so that the normal UI switching can go through - console.log("Network wasn't available, try again but async"); - } + await setActiveNetworkConfigurationId(networkClientIdForThisDomain); + await dispatch( + setSwitchedNetworkDetails({ + networkClientId: networkClientIdForThisDomain, + origin: selectedTabOrigin, + }), + ); + await forceUpdateMetamaskState(dispatch); }; } From 1cfb4abd870b5b740c94d686084fbed33626d4a6 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 19 Jun 2024 21:29:37 -0230 Subject: [PATCH 6/8] Convert action creator to simpler async function --- ui/store/actions.ts | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/ui/store/actions.ts b/ui/store/actions.ts index e677c45f9d33..95f7f976f67f 100644 --- a/ui/store/actions.ts +++ b/ui/store/actions.ts @@ -2494,22 +2494,15 @@ export function setActiveNetwork( }; } -export function setActiveNetworkConfigurationId( +export async function setActiveNetworkConfigurationId( networkConfigurationId: string, -): ThunkAction { - return async (dispatch) => { - log.debug( - `background.setActiveNetworkConfigurationId: ${networkConfigurationId}`, - ); - try { - await submitRequestToBackground('setActiveNetworkConfigurationId', [ - networkConfigurationId, - ]); - } catch (error) { - logErrorWithMessage(error); - dispatch(displayWarning('Had a problem changing networks!')); - } - }; +): Promise { + log.debug( + `background.setActiveNetworkConfigurationId: ${networkConfigurationId}`, + ); + await submitRequestToBackground('setActiveNetworkConfigurationId', [ + networkConfigurationId, + ]); } export function rollbackToPreviousProvider(): ThunkAction< From d0f727516421a27313799ff4fdef04a00c87af2e Mon Sep 17 00:00:00 2001 From: David Walsh Date: Fri, 21 Jun 2024 01:06:43 -0500 Subject: [PATCH 7/8] Remove test .only --- test/e2e/tests/request-queuing/ui.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/tests/request-queuing/ui.spec.js b/test/e2e/tests/request-queuing/ui.spec.js index dc8105571173..9720732d9b25 100644 --- a/test/e2e/tests/request-queuing/ui.spec.js +++ b/test/e2e/tests/request-queuing/ui.spec.js @@ -356,7 +356,7 @@ describe('Request-queue UI changes', function () { ); }); - it.only('should gracefully handle network connectivity failure @no-mmi', async function () { + it('should gracefully handle network connectivity failure @no-mmi', async function () { const port = 8546; const chainId = 1338; await withFixtures( From 84786ebdd732e67a8927b6bd7b14c3d5bc3c2bef Mon Sep 17 00:00:00 2001 From: David Walsh Date: Fri, 28 Jun 2024 13:54:09 -0500 Subject: [PATCH 8/8] Fix rebase, add test for personal sign --- test/e2e/tests/request-queuing/ui.spec.js | 150 ++++++++++++++++------ 1 file changed, 113 insertions(+), 37 deletions(-) diff --git a/test/e2e/tests/request-queuing/ui.spec.js b/test/e2e/tests/request-queuing/ui.spec.js index 9720732d9b25..d81252b0c4f5 100644 --- a/test/e2e/tests/request-queuing/ui.spec.js +++ b/test/e2e/tests/request-queuing/ui.spec.js @@ -80,6 +80,11 @@ async function selectDappClickSend(driver, dappUrl) { await driver.clickElement('#sendButton'); } +async function selectDappClickPersonalSign(driver, dappUrl) { + await driver.switchToWindowWithUrl(dappUrl); + await driver.clickElement('#personalSign'); +} + async function switchToNotificationPopoverValidateDetails( driver, expectedDetails, @@ -90,11 +95,13 @@ async function switchToNotificationPopoverValidateDetails( // Get UI details const networkPill = await driver.findElement( - '[data-testid="network-display"]', + // Differs between confirmation and signature + '[data-testid="network-display"], [data-testid="signature-request-network-display"]', ); const networkText = await networkPill.getText(); const originElement = await driver.findElement( - '.confirm-page-container-summary__origin bdi', + // Differs between confirmation and signature + '.confirm-page-container-summary__origin bdi, .request-signature__origin .chip__label', ); const originText = await originElement.getText(); @@ -356,7 +363,84 @@ describe('Request-queue UI changes', function () { ); }); - it('should gracefully handle network connectivity failure @no-mmi', async function () { + it('should gracefully handle deleted network @no-mmi', async function () { + const port = 8546; + const chainId = 1338; + await withFixtures( + { + dapp: true, + fixtures: new FixtureBuilder() + .withNetworkControllerDoubleGanache() + .withPreferencesControllerUseRequestQueueEnabled() + .withSelectedNetworkControllerPerDomain() + .build(), + ganacheOptions: { + ...defaultGanacheOptions, + concurrent: [ + { + port, + chainId, + ganacheOptions2: defaultGanacheOptions, + }, + ], + }, + dappOptions: { numberOfDapps: 2 }, + title: this.test.fullTitle(), + }, + async ({ driver }) => { + await unlockWallet(driver); + + // Navigate to extension home screen + await driver.navigate(PAGES.HOME); + + // Open the first dapp + await openDappAndSwitchChain(driver, DAPP_URL); + + // Open the second dapp and switch chains + await openDappAndSwitchChain(driver, DAPP_ONE_URL, '0x1', 4); + + // Go to wallet fullscreen, ensure that the global network changed to Ethereum Mainnet + await driver.switchToWindowWithTitle( + WINDOW_TITLES.ExtensionInFullScreenView, + ); + await driver.findElement({ + css: '[data-testid="network-display"]', + text: 'Ethereum Mainnet', + }); + + // Go to Settings, delete the first dapp's network + await driver.clickElement( + '[data-testid="account-options-menu-button"]', + ); + await driver.clickElement('[data-testid="global-menu-settings"]'); + await driver.clickElement({ + css: '.tab-bar__tab__content__title', + text: 'Networks', + }); + await driver.clickElement({ + css: '.networks-tab__networks-list-name', + text: 'Localhost 8545', + }); + await driver.clickElement({ css: '.btn-danger', text: 'Delete' }); + await driver.clickElement({ + css: '.modal-container__footer-button', + text: 'Delete', + }); + + // Go back to first dapp, try an action, ensure deleted network doesn't block UI + // The current globally selected network, Ethereum Mainnet, should be used + await selectDappClickSend(driver, DAPP_URL); + await driver.delay(veryLargeDelayMs); + await switchToNotificationPopoverValidateDetails(driver, { + chainId: '0x1', + networkText: 'Ethereum Mainnet', + originText: DAPP_URL, + }); + }, + ); + }); + + it('should gracefully handle network connectivity failure for signatures @no-mmi', async function () { const port = 8546; const chainId = 1338; await withFixtures( @@ -393,7 +477,7 @@ describe('Request-queue UI changes', function () { await openDappAndSwitchChain(driver, DAPP_URL); // Open the second dapp and switch chains - await openDappAndSwitchChain(driver, DAPP_ONE_URL, '0x1'); + await openDappAndSwitchChain(driver, DAPP_ONE_URL, '0x1', 4); // Go to wallet fullscreen, ensure that the global network changed to Ethereum Mainnet await driver.switchToWindowWithTitle( @@ -409,21 +493,25 @@ describe('Request-queue UI changes', function () { await secondaryGanacheServer[0].quit(); // Go back to first dapp, try an action, ensure network connection failure doesn't block UI - const dappOneNetworkPillText = await selectDappClickSendGetNetwork( - driver, - DAPP_URL, - ); - assert.equal(dappOneNetworkPillText, 'Localhost 8545'); + await selectDappClickPersonalSign(driver, DAPP_URL); + await driver.delay(veryLargeDelayMs); + await switchToNotificationPopoverValidateDetails(driver, { + chainId: '0x539', + networkText: 'Localhost 8545', + originText: DAPP_URL, + }); }, ); }); - it('should gracefully handle deleted network @no-mmi', async function () { + it('should gracefully handle network connectivity failure for confirmations @no-mmi', async function () { const port = 8546; const chainId = 1338; await withFixtures( { dapp: true, + // Presently confirmations take up to 10 seconds to display on a dead network + driverOptions: { timeOut: 30000 }, fixtures: new FixtureBuilder() .withNetworkControllerDoubleGanache() .withPreferencesControllerUseRequestQueueEnabled() @@ -439,10 +527,13 @@ describe('Request-queue UI changes', function () { }, ], }, + // This test intentionally quits Ganache while the extension is using it, causing + // PollingBlockTracker errors. These are expected. + ignoredConsoleErrors: ['PollingBlockTracker'], dappOptions: { numberOfDapps: 2 }, title: this.test.fullTitle(), }, - async ({ driver }) => { + async ({ driver, ganacheServer, secondaryGanacheServer }) => { await unlockWallet(driver); // Navigate to extension home screen @@ -452,7 +543,7 @@ describe('Request-queue UI changes', function () { await openDappAndSwitchChain(driver, DAPP_URL); // Open the second dapp and switch chains - await openDappAndSwitchChain(driver, DAPP_ONE_URL, '0x1'); + await openDappAndSwitchChain(driver, DAPP_ONE_URL, '0x1', 4); // Go to wallet fullscreen, ensure that the global network changed to Ethereum Mainnet await driver.switchToWindowWithTitle( @@ -463,32 +554,17 @@ describe('Request-queue UI changes', function () { text: 'Ethereum Mainnet', }); - // Go to Settings, delete the first dapp's network - await driver.clickElement( - '[data-testid="account-options-menu-button"]', - ); - await driver.clickElement('[data-testid="global-menu-settings"]'); - await driver.clickElement({ - css: '.tab-bar__tab__content__title', - text: 'Networks', - }); - await driver.clickElement({ - css: '.networks-tab__networks-list-name', - text: 'Localhost 8545', - }); - await driver.clickElement({ css: '.btn-danger', text: 'Delete' }); - await driver.clickElement({ - css: '.modal-container__footer-button', - text: 'Delete', - }); + // Kill ganache servers + await ganacheServer.quit(); + await secondaryGanacheServer[0].quit(); - // Go back to first dapp, try an action, ensure deleted network doesn't block UI - // The current globally selected network, Ethereum Mainnet, should be used - const dappOneNetworkPillText = await selectDappClickSendGetNetwork( - driver, - DAPP_URL, - ); - assert.equal(dappOneNetworkPillText, 'Ethereum Mainnet'); + // Go back to first dapp, try an action, ensure network connection failure doesn't block UI + await selectDappClickSend(driver, DAPP_URL); + await switchToNotificationPopoverValidateDetails(driver, { + chainId: '0x539', + networkText: 'Localhost 8545', + originText: DAPP_URL, + }); }, ); });