Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NetworkController to handle clearing of messenger #5116

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix NetworkController to handle clearing of messenger #5116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/network-controller/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- Bump `@metamask/base-controller` from `^7.0.0` to `^7.1.0` ([#5079](https://github.com/MetaMask/core/pull/5079))

### Fixed

- Fix `lookupNetwork` so that it will no longer throw an error if `networkDidChange` subscriptions have been removed before it returns ([#5116](https://github.com/MetaMask/core/pull/5116))
- This error could occur if the NetworkController's messenger is cleared of subscriptions, as in a "destroy" step.

## [22.1.1]

### Changed
Expand Down
47 changes: 39 additions & 8 deletions packages/network-controller/src/NetworkController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1300,10 +1300,30 @@ export class NetworkController extends BaseController<
let networkChanged = false;
const listener = () => {
networkChanged = true;
this.messagingSystem.unsubscribe(
'NetworkController:networkDidChange',
listener,
);
try {
this.messagingSystem.unsubscribe(
'NetworkController:networkDidChange',
listener,
);
} catch (error) {
// In theory, this `catch` should not be necessary given that this error
// would occur "inside" of the call to `#determineEIP1559Compatibility`
// below and so it should be caught by the `try`/`catch` below (it is
// impossible to reproduce in tests for that reason). However, somehow
// it occurs within Mobile and so we have to add our own `try`/`catch`
// here.
/* istanbul ignore next */
if (
!(error instanceof Error) ||
error.message !==
'Subscription not found for event: NetworkController:networkDidChange'
) {
// Again, this error should not happen and is impossible to reproduce
// in tests.
/* istanbul ignore next */
throw error;
}
}
};
this.messagingSystem.subscribe(
'NetworkController:networkDidChange',
Expand Down Expand Up @@ -1370,10 +1390,21 @@ export class NetworkController extends BaseController<
// in the process of being called, so we don't need to go further.
return;
}
this.messagingSystem.unsubscribe(
'NetworkController:networkDidChange',
listener,
);

try {
this.messagingSystem.unsubscribe(
'NetworkController:networkDidChange',
listener,
);
} catch (error) {
if (
!(error instanceof Error) ||
error.message !==
'Subscription not found for event: NetworkController:networkDidChange'
) {
throw error;
}
}

this.update((state) => {
const meta = state.networksMetadata[state.selectedNetworkClientId];
Expand Down
228 changes: 228 additions & 0 deletions packages/network-controller/tests/NetworkController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1579,6 +1579,109 @@
});
});

describe('if all subscriptions are removed from the messenger before the call to lookupNetwork completes', () => {
it('does not throw an error', async () => {
const infuraProjectId = 'some-infura-project-id';

await withController(
{
state: {
selectedNetworkClientId: infuraNetworkType,
},
infuraProjectId,
},
async ({ controller, messenger }) => {
const fakeProvider = buildFakeProvider([
// Called during provider initialization
{
request: {
method: 'eth_getBlockByNumber',
},
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
},
// Called via `lookupNetwork` directly
{
request: {
method: 'eth_getBlockByNumber',
},
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
},
]);
const fakeNetworkClient = buildFakeClient(fakeProvider);
mockCreateNetworkClient()
.calledWith({
chainId: ChainId[infuraNetworkType],
infuraProjectId,
network: infuraNetworkType,
ticker: NetworksTicker[infuraNetworkType],
type: NetworkClientType.Infura,
})
.mockReturnValue(fakeNetworkClient);
await controller.initializeProvider();

const lookupNetworkPromise = controller.lookupNetwork();
messenger.clearSubscriptions();
expect(await lookupNetworkPromise).toBeUndefined();
},
);
});
});

describe('if removing the networkDidChange subscription fails for an unknown reason', () => {
it('re-throws the error', async () => {
const infuraProjectId = 'some-infura-project-id';

await withController(
{
state: {
selectedNetworkClientId: infuraNetworkType,
},
infuraProjectId,
},
async ({ controller, messenger }) => {
const fakeProvider = buildFakeProvider([
// Called during provider initialization
{
request: {
method: 'eth_getBlockByNumber',
},
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
},
// Called via `lookupNetwork` directly
{
request: {
method: 'eth_getBlockByNumber',
},
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
},
]);
const fakeNetworkClient = buildFakeClient(fakeProvider);
mockCreateNetworkClient()
.calledWith({
chainId: ChainId[infuraNetworkType],
infuraProjectId,
network: infuraNetworkType,
ticker: NetworksTicker[infuraNetworkType],
type: NetworkClientType.Infura,
})
.mockReturnValue(fakeNetworkClient);
await controller.initializeProvider();

const lookupNetworkPromise = controller.lookupNetwork();
const error = new Error('oops');
jest
.spyOn(messenger, 'unsubscribe')
.mockImplementation((eventType) => {
if (eventType === 'NetworkController:networkDidChange') {
throw error;
}
});
await expect(lookupNetworkPromise).rejects.toThrow(error);
},
);
});
});

lookupNetworkTests({
expectedNetworkClientType: NetworkClientType.Infura,
initialState: {
Expand Down Expand Up @@ -1889,6 +1992,131 @@
});
});

describe('if all subscriptions are removed from the messenger before the call to lookupNetwork completes', () => {
it('does not throw an error', async () => {

Check warning on line 1996 in packages/network-controller/tests/NetworkController.test.ts

View workflow job for this annotation

GitHub Actions / Lint, build, and test / Lint (20.x) 

Test has no assertions
const infuraProjectId = 'some-infura-project-id';

await withController(
{
state: {
selectedNetworkClientId: 'AAAA-AAAA-AAAA-AAAA',
networkConfigurationsByChainId: {
'0x1337': buildCustomNetworkConfiguration({
chainId: '0x1337',
nativeCurrency: 'TEST',
rpcEndpoints: [
buildCustomRpcEndpoint({
networkClientId: 'AAAA-AAAA-AAAA-AAAA',
url: 'https://test.network',
}),
],
}),
},
},
infuraProjectId,
},
async ({ controller, messenger }) => {
const fakeProvider = buildFakeProvider([
// Called during provider initialization
{
request: {
method: 'eth_getBlockByNumber',
},
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
},
// Called via `lookupNetwork` directly
{
request: {
method: 'eth_getBlockByNumber',
},
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
},
]);
const fakeNetworkClient = buildFakeClient(fakeProvider);
mockCreateNetworkClient()
.calledWith({
chainId: '0x1337',
rpcUrl: 'https://test.network',
ticker: 'TEST',
type: NetworkClientType.Custom,
})
.mockReturnValue(fakeNetworkClient);
await controller.initializeProvider();

const lookupNetworkPromise = controller.lookupNetwork();
messenger.clearSubscriptions();
await lookupNetworkPromise;
},
);
});
});

describe('if removing the networkDidChange subscription fails for an unknown reason', () => {
it('re-throws the error', async () => {
const infuraProjectId = 'some-infura-project-id';

await withController(
{
state: {
selectedNetworkClientId: 'AAAA-AAAA-AAAA-AAAA',
networkConfigurationsByChainId: {
'0x1337': buildCustomNetworkConfiguration({
chainId: '0x1337',
nativeCurrency: 'TEST',
rpcEndpoints: [
buildCustomRpcEndpoint({
networkClientId: 'AAAA-AAAA-AAAA-AAAA',
url: 'https://test.network',
}),
],
}),
},
},
infuraProjectId,
},
async ({ controller, messenger }) => {
const fakeProvider = buildFakeProvider([
// Called during provider initialization
{
request: {
method: 'eth_getBlockByNumber',
},
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
},
// Called via `lookupNetwork` directly
{
request: {
method: 'eth_getBlockByNumber',
},
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
},
]);
const fakeNetworkClient = buildFakeClient(fakeProvider);
mockCreateNetworkClient()
.calledWith({
chainId: '0x1337',
rpcUrl: 'https://test.network',
ticker: 'TEST',
type: NetworkClientType.Custom,
})
.mockReturnValue(fakeNetworkClient);
await controller.initializeProvider();

const lookupNetworkPromise = controller.lookupNetwork();
const error = new Error('oops');
jest
.spyOn(messenger, 'unsubscribe')
.mockImplementation((eventType) => {
if (eventType === 'NetworkController:networkDidChange') {
throw error;
}
});
await expect(lookupNetworkPromise).rejects.toThrow(error);
},
);
});
});

lookupNetworkTests({
expectedNetworkClientType: NetworkClientType.Custom,
initialState: {
Expand Down
Loading