Skip to content

Commit

Permalink
fix: Ensure that network client ID is updated before use (#4801)
Browse files Browse the repository at this point in the history
## 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
  • Loading branch information
FrederikBolding authored Oct 16, 2024
1 parent 08b9c52 commit 16d37a7
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -283,13 +283,16 @@ export class SelectedNetworkController extends BaseController<
'NetworkController:getNetworkClientById',
networkClientId,
);
const networkProxy = this.getProviderAndBlockTracker(domain);
networkProxy.provider.setTarget(networkClient.provider);
networkProxy.blockTracker.setTarget(networkClient.blockTracker);

// This needs to happen before getProviderAndBlockTracker,
// otherwise we may be referencing a network client ID that no longer exists.
this.update((state) => {
state.domains[domain] = networkClientId;
});

const networkProxy = this.getProviderAndBlockTracker(domain);
networkProxy.provider.setTarget(networkClient.provider);
networkProxy.blockTracker.setTarget(networkClient.blockTracker);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,52 @@ describe('SelectedNetworkController', () => {
'deleted-network.com': networkControllerState.selectedNetworkClientId,
});
});

it('redirects domains to the globally selected network when useRequestQueuePreference is true and handles garbage collected proxies', () => {
const domainProxyMap = new Map();
const {
controller,
messenger,
mockNetworkControllerGetState,
mockGetNetworkClientById,
} = setup({
state: { domains: initialDomains },
useRequestQueuePreference: true,
domainProxyMap,
});

// Simulate proxies being garbage collected
domainProxyMap.clear();

const networkControllerState = {
...getDefaultNetworkControllerState(),
selectedNetworkClientId: 'mainnet',
};

mockGetNetworkClientById.mockImplementation((id) => {
// Simulate the previous domain being deleted in NetworkController
if (id !== 'mainnet') {
throw new Error('Network client does not exist');
}

return {
provider: { request: jest.fn() },
blockTracker: { getLatestBlock: jest.fn() },
};
});

deleteNetwork(
'0x5',
networkControllerState,
messenger,
mockNetworkControllerGetState,
);

expect(controller.state.domains).toStrictEqual({
...initialDomains,
'deleted-network.com': networkControllerState.selectedNetworkClientId,
});
});
});

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

0 comments on commit 16d37a7

Please sign in to comment.