Skip to content

Commit

Permalink
fix: UX: Multichain: Fix dead network problem when switching tabs (#2…
Browse files Browse the repository at this point in the history 
…5425)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

This PR ensures that a connection to a given network does not block the
UI when switching to a dapp tab that remembers that down network, thus
preventing a non-respondent network from not displaying the MetaMask UI

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25425?quickstart=1)

## **Related issues**

Fixes: #25588

## **Manual testing steps**

### **Scenario 1: Kill network, manually click MetaMask extension icon**

1. Start local ganache
2. Add the local network to your network listing (Settings -> Networks
-> Add custom network)
3. In tab 1, connect to dapp 1 on Localhost
4. In tab 2, connect to dapp 2 on Ethereum Mainnet
5. Kill local ganache
6. Click tab 1
7. Click the MetaMask icon in the extension bar
8. See UI pop up as expected

### **Scenario 2:  Kill network, trigger transaction from dapp**

1. Start local ganache
2. Add the local network to your network listing (Settings -> Networks
-> Add custom network)
3. In tab 1, connect to dapp 1 on Localhost
4. In tab 2, connect to dapp 2 on Ethereum Mainnet
5. Kill local ganache
6. Click tab 1
7. Trigger a transaction from the dapp
8. See UI pop up quickly, with confirmation, as expected

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
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.

---------

Co-authored-by: Mark Stacey <markjstacey@gmail.com>
  • Loading branch information
@darkwing @Gudahtt
darkwing and Gudahtt authored Jul 3, 2024
1 parent bfaf535 commit 363a8f4
Show file tree
Hide file tree
Showing 3 changed files with 233 additions and 4 deletions.
5 changes: 5 additions & 0 deletions app/scripts/metamask-controller.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3109,6 +3109,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,
Expand Down
219 changes: 216 additions & 3 deletions test/e2e/tests/request-queuing/ui.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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();

Expand Down Expand Up @@ -165,7 +172,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(
Expand Down Expand Up @@ -355,4 +362,210 @@ 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', 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(
{
dapp: true,
fixtures: new FixtureBuilder()
.withNetworkControllerDoubleGanache()
.withPreferencesControllerUseRequestQueueEnabled()
.withSelectedNetworkControllerPerDomain()
.build(),
ganacheOptions: {
...defaultGanacheOptions,
concurrent: [
{
port,
chainId,
ganacheOptions2: defaultGanacheOptions,
},
],
},
// 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, 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', 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',
});

// 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
await selectDappClickPersonalSign(driver, DAPP_URL);
await driver.delay(veryLargeDelayMs);
await switchToNotificationPopoverValidateDetails(driver, {
chainId: '0x539',
networkText: 'Localhost 8545',
originText: DAPP_URL,
});
},
);
});

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()
.withSelectedNetworkControllerPerDomain()
.build(),
ganacheOptions: {
...defaultGanacheOptions,
concurrent: [
{
port,
chainId,
ganacheOptions2: defaultGanacheOptions,
},
],
},
// 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, 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', 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',
});

// 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
await selectDappClickSend(driver, DAPP_URL);
await switchToNotificationPopoverValidateDetails(driver, {
chainId: '0x539',
networkText: 'Localhost 8545',
originText: DAPP_URL,
});
},
);
});
});
13 changes: 12 additions & 1 deletion ui/store/actions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2159,7 +2159,7 @@ export function automaticallySwitchNetwork(
selectedTabOrigin: string,
): ThunkAction<void, MetaMaskReduxState, unknown, AnyAction> {
return async (dispatch: MetaMaskReduxDispatch) => {
await dispatch(setActiveNetwork(networkClientIdForThisDomain));
await setActiveNetworkConfigurationId(networkClientIdForThisDomain);
await dispatch(
setSwitchedNetworkDetails({
networkClientId: networkClientIdForThisDomain,
Expand Down Expand Up @@ -2495,6 +2495,17 @@ export function setActiveNetwork(
};
}

export async function setActiveNetworkConfigurationId(
networkConfigurationId: string,
): Promise<undefined> {
log.debug(
`background.setActiveNetworkConfigurationId: ${networkConfigurationId}`,
);
await submitRequestToBackground('setActiveNetworkConfigurationId', [
networkConfigurationId,
]);
}

export function rollbackToPreviousProvider(): ThunkAction<
void,
MetaMaskReduxState,
Expand Down

0 comments on commit 363a8f4

Please sign in to comment.