Skip to content

Commit 16d37a7

Browse files
fix: Ensure that network client ID is updated before use (#4801)
## Explanation This PR fixes a rare problem that would occur when deleting a network after a proxy in `SelectedNetworkController` has been garbage collected. When this happens, `getProviderAndBlockTracker` will fail to find the old network client ID and thus updating the network client ID for the domain will fail. This could be reproduced in an E2E test that would only fail on this branch: MetaMask/metamask-extension#27306 which enables `initialConnections` for a preinstalled Snap. `initialConnections` adds a permission for an origin to connect to a Snap which causes the `SelectedNetworkController` to register the domain. My theory is that since the user has not accessed the origin in question yet, the proxy may not be used and may be garbage collected. If a network is deleted following that, an error occurs. ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/selected-network-controller` - **Fixed**: Ensure that network client ID is updated before using it ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes
1 parent 08b9c52 commit 16d37a7

File tree

2 files changed

+52
-3
lines changed

2 files changed

+52
-3
lines changed

packages/selected-network-controller/src/SelectedNetworkController.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -283,13 +283,16 @@ export class SelectedNetworkController extends BaseController<
283283
'NetworkController:getNetworkClientById',
284284
networkClientId,
285285
);
286-
const networkProxy = this.getProviderAndBlockTracker(domain);
287-
networkProxy.provider.setTarget(networkClient.provider);
288-
networkProxy.blockTracker.setTarget(networkClient.blockTracker);
289286

287+
// This needs to happen before getProviderAndBlockTracker,
288+
// otherwise we may be referencing a network client ID that no longer exists.
290289
this.update((state) => {
291290
state.domains[domain] = networkClientId;
292291
});
292+
293+
const networkProxy = this.getProviderAndBlockTracker(domain);
294+
networkProxy.provider.setTarget(networkClient.provider);
295+
networkProxy.blockTracker.setTarget(networkClient.blockTracker);
293296
}
294297

295298
/**

packages/selected-network-controller/tests/SelectedNetworkController.test.ts

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,52 @@ describe('SelectedNetworkController', () => {
363363
'deleted-network.com': networkControllerState.selectedNetworkClientId,
364364
});
365365
});
366+
367+
it('redirects domains to the globally selected network when useRequestQueuePreference is true and handles garbage collected proxies', () => {
368+
const domainProxyMap = new Map();
369+
const {
370+
controller,
371+
messenger,
372+
mockNetworkControllerGetState,
373+
mockGetNetworkClientById,
374+
} = setup({
375+
state: { domains: initialDomains },
376+
useRequestQueuePreference: true,
377+
domainProxyMap,
378+
});
379+
380+
// Simulate proxies being garbage collected
381+
domainProxyMap.clear();
382+
383+
const networkControllerState = {
384+
...getDefaultNetworkControllerState(),
385+
selectedNetworkClientId: 'mainnet',
386+
};
387+
388+
mockGetNetworkClientById.mockImplementation((id) => {
389+
// Simulate the previous domain being deleted in NetworkController
390+
if (id !== 'mainnet') {
391+
throw new Error('Network client does not exist');
392+
}
393+
394+
return {
395+
provider: { request: jest.fn() },
396+
blockTracker: { getLatestBlock: jest.fn() },
397+
};
398+
});
399+
400+
deleteNetwork(
401+
'0x5',
402+
networkControllerState,
403+
messenger,
404+
mockNetworkControllerGetState,
405+
);
406+
407+
expect(controller.state.domains).toStrictEqual({
408+
...initialDomains,
409+
'deleted-network.com': networkControllerState.selectedNetworkClientId,
410+
});
411+
});
366412
});
367413

368414
describe('when a network is updated', () => {

0 commit comments

Comments
 (0)