From 80f538eeef8200221fb18773ff5b9c67533b0008 Mon Sep 17 00:00:00 2001 From: Alex Donesky Date: Fri, 2 Aug 2024 11:47:04 -0500 Subject: [PATCH] fix: issue where `setNetworkClientIdForDomain` was called without checking whether the origin was eligible for setting its own network (#26323) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR fixes a issue where `setNetworkClientIdForDomain` was frequently called with origins that had no account permissions yet (which is the thresshold we currently set for giving site's their own network) resulting in [a large number of silent errors in the background that are clogging up sentry](https://metamask.sentry.io/issues/5659582204/?environment=production&project=273505&qu%5B%E2%80%A6%5Derrer=issue-stream&sort=freq&statsPeriod=90d&stream_index=1). [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26323?quickstart=1) ## **Related issues** Fixes: https://metamask.sentry.io/issues/5659582204/?environment=production&project=273505&qu%5B%E2%80%A6%5Derrer=issue-stream&sort=freq&statsPeriod=90d&stream_index=1 ## **Manual testing steps** N/A ## **Screenshots/Recordings** ### **Before** N/A ### **After** N/A ## **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. --- .../network-list-menu/network-list-menu.js | 14 ++++-- .../network-list-menu.test.js | 45 +++++++++++++------ 2 files changed, 41 insertions(+), 18 deletions(-) diff --git a/ui/components/multichain/network-list-menu/network-list-menu.js b/ui/components/multichain/network-list-menu/network-list-menu.js index 8c44bf368132..6a1f54556b4c 100644 --- a/ui/components/multichain/network-list-menu/network-list-menu.js +++ b/ui/components/multichain/network-list-menu/network-list-menu.js @@ -34,6 +34,7 @@ import { getUseRequestQueue, getNetworkConfigurations, getEditedNetwork, + getAllDomains, } from '../../../selectors'; import ToggleButton from '../../ui/toggle-button'; import { @@ -100,6 +101,7 @@ export const NetworkListMenu = ({ onClose }) => { const selectedTabOrigin = useSelector(getOriginOfCurrentTab); const useRequestQueue = useSelector(getUseRequestQueue); const networkConfigurations = useSelector(getNetworkConfigurations); + const domains = useSelector(getAllDomains); const dispatch = useDispatch(); const history = useHistory(); @@ -307,10 +309,14 @@ export const NetworkListMenu = ({ onClose }) => { dispatch(setActiveNetwork(network.id)); } - // If presently on a dapp, communicate a change to - // the dapp via silent switchEthereumChain that the - // network has changed due to user action - if (useRequestQueue && selectedTabOrigin) { + // If presently on and connected to a dapp, communicate a change to + // the dapp via silent switchEthereumChain that the network has + // changed due to user action + if ( + useRequestQueue && + selectedTabOrigin && + domains[selectedTabOrigin] + ) { setNetworkClientIdForDomain(selectedTabOrigin, network.id); } diff --git a/ui/components/multichain/network-list-menu/network-list-menu.test.js b/ui/components/multichain/network-list-menu/network-list-menu.test.js index 544b0880048e..4a170fc139d7 100644 --- a/ui/components/multichain/network-list-menu/network-list-menu.test.js +++ b/ui/components/multichain/network-list-menu/network-list-menu.test.js @@ -34,6 +34,7 @@ const render = ({ providerConfigId = 'chain5', isUnlocked = true, origin = MOCK_ORIGIN, + selectedTabOriginInDomainsState = true, } = {}) => { const state = { metamask: { @@ -48,6 +49,11 @@ const render = ({ showTestNetworks, }, useRequestQueue: true, + domains: { + ...(selectedTabOriginInDomainsState + ? { [origin]: providerConfigId } + : {}), + }, }, activeTab: { origin, @@ -61,6 +67,7 @@ const render = ({ describe('NetworkListMenu', () => { beforeEach(() => { process.env.ENABLE_NETWORK_UI_REDESIGN = 'false'; + jest.clearAllMocks(); }); it('renders properly', () => { @@ -158,22 +165,32 @@ describe('NetworkListMenu', () => { ).toHaveLength(0); }); - it('fires setNetworkClientIdForDomain when network item is clicked', () => { - const { getByText } = render(); - fireEvent.click(getByText(MAINNET_DISPLAY_NAME)); - expect(mockSetNetworkClientIdForDomain).toHaveBeenCalledWith( - MOCK_ORIGIN, - NETWORK_TYPES.MAINNET, - ); + describe('selectedTabOrigin is connected to wallet', () => { + it('fires setNetworkClientIdForDomain when network item is clicked', () => { + const { getByText } = render(); + fireEvent.click(getByText(MAINNET_DISPLAY_NAME)); + expect(mockSetNetworkClientIdForDomain).toHaveBeenCalledWith( + MOCK_ORIGIN, + NETWORK_TYPES.MAINNET, + ); + }); + + it('fires setNetworkClientIdForDomain when test network item is clicked', () => { + const { getByText } = render({ showTestNetworks: true }); + fireEvent.click(getByText(SEPOLIA_DISPLAY_NAME)); + expect(mockSetNetworkClientIdForDomain).toHaveBeenCalledWith( + MOCK_ORIGIN, + NETWORK_TYPES.SEPOLIA, + ); + }); }); - it('fires setNetworkClientIdForDomain when test network item is clicked', () => { - const { getByText } = render({ showTestNetworks: true }); - fireEvent.click(getByText(SEPOLIA_DISPLAY_NAME)); - expect(mockSetNetworkClientIdForDomain).toHaveBeenCalledWith( - MOCK_ORIGIN, - NETWORK_TYPES.SEPOLIA, - ); + describe('selectedTabOrigin is not connected to wallet', () => { + it('does not fire setNetworkClientIdForDomain when network item is clicked', () => { + const { getByText } = render({ selectedTabOriginInDomainsState: false }); + fireEvent.click(getByText(MAINNET_DISPLAY_NAME)); + expect(mockSetNetworkClientIdForDomain).not.toHaveBeenCalled(); + }); }); describe('NetworkListMenu with ENABLE_NETWORK_UI_REDESIGN', () => {