Skip to content

Commit 409b54c

Browse files
mcmiremcmire
committed
Correct invalid initial selectedNetworkClientId
Currently, when NetworkController is instantiated with pre-existing state that contains an invalid `selectedNetworkClientId` — that is, no RPC endpoint exists which has the same network client ID — then it throws an error. This was intentionally done to bring attention to possible bugs in NetworkController, but this has the unfortunate side effect of bricking users' wallets. To fix this, we now correct an invalid `selectedNetworkClientId` to point to the default RPC endpoint of the first network sorted by chain ID (which in the vast majority of cases will be Mainnet). We still do want to know about this, though, so we log the error in Sentry.
1 parent 3fb3df4 commit 409b54c

File tree

8 files changed

+166
-73
lines changed

8 files changed

+166
-73
lines changed

‎packages/network-controller/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1515
- 429 responses now throw a "Rate Limiting" error
1616
- Other 4xx responses now throw a generic HTTP client error
1717
- Invalid JSON responses now throw a "Parse" error
18+
- Rather than throwing an error, NetworkController now corrects an invalid initial `selectedNetworkClientId` to point to the default RPC endpoint of the first network sorted by chain ID ([#5851](https://github.com/MetaMask/core/pull/5851))
1819

1920
## [23.5.0]
2021

‎packages/network-controller/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
"dependencies": {
5050
"@metamask/base-controller": "^8.0.1",
5151
"@metamask/controller-utils": "^11.9.0",
52+
"@metamask/error-reporting-service": "^0.0.0",
5253
"@metamask/eth-block-tracker": "^11.0.3",
5354
"@metamask/eth-json-rpc-infura": "^10.1.1",
5455
"@metamask/eth-json-rpc-middleware": "^16.0.1",

‎packages/network-controller/src/NetworkController.ts

Lines changed: 58 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
BUILT_IN_CUSTOM_NETWORKS_RPC,
1818
BUILT_IN_NETWORKS,
1919
} from '@metamask/controller-utils';
20+
import type { ErrorReportingServiceCaptureExceptionAction } from '@metamask/error-reporting-service';
2021
import type { PollingBlockTrackerOptions } from '@metamask/eth-block-tracker';
2122
import EthQuery from '@metamask/eth-query';
2223
import { errorCodes } from '@metamask/rpc-errors';
@@ -26,6 +27,7 @@ import type { Hex } from '@metamask/utils';
2627
import { hasProperty, isPlainObject, isStrictHexString } from '@metamask/utils';
2728
import deepEqual from 'fast-deep-equal';
2829
import type { Draft } from 'immer';
30+
import { produce } from 'immer';
2931
import { cloneDeep } from 'lodash';
3032
import type { Logger } from 'loglevel';
3133
import { createSelector } from 'reselect';
@@ -498,6 +500,11 @@ export type NetworkControllerEvents =
498500
| NetworkControllerRpcEndpointDegradedEvent
499501
| NetworkControllerRpcEndpointRequestRetriedEvent;
500502

503+
/**
504+
* All events that {@link NetworkController} calls internally.
505+
*/
506+
type AllowedEvents = never;
507+
501508
export type NetworkControllerGetStateAction = ControllerGetStateAction<
502509
typeof controllerName,
503510
NetworkState
@@ -590,12 +597,17 @@ export type NetworkControllerActions =
590597
| NetworkControllerRemoveNetworkAction
591598
| NetworkControllerUpdateNetworkAction;
592599

600+
/**
601+
* All actions that {@link NetworkController} calls internally.
602+
*/
603+
type AllowedActions = ErrorReportingServiceCaptureExceptionAction;
604+
593605
export type NetworkControllerMessenger = RestrictedMessenger<
594606
typeof controllerName,
595-
NetworkControllerActions,
596-
NetworkControllerEvents,
597-
never,
598-
never
607+
NetworkControllerActions | AllowedActions,
608+
NetworkControllerEvents | AllowedEvents,
609+
AllowedActions['type'],
610+
AllowedEvents['type']
599611
>;
600612

601613
/**
@@ -1003,7 +1015,7 @@ function deriveInfuraNetworkNameFromRpcEndpointUrl(
10031015
* @param state - The NetworkController state to verify.
10041016
* @throws if the state is invalid in some way.
10051017
*/
1006-
function validateNetworkControllerState(state: NetworkState) {
1018+
function validateInitialState(state: NetworkState) {
10071019
const networkConfigurationEntries = Object.entries(
10081020
state.networkConfigurationsByChainId,
10091021
);
@@ -1054,14 +1066,44 @@ function validateNetworkControllerState(state: NetworkState) {
10541066
'NetworkController state has invalid `networkConfigurationsByChainId`: Every RPC endpoint across all network configurations must have a unique `networkClientId`',
10551067
);
10561068
}
1069+
}
10571070

1058-
if (!networkClientIds.includes(state.selectedNetworkClientId)) {
1059-
throw new Error(
1060-
// This ESLint rule mistakenly produces an error.
1061-
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
1062-
`NetworkController state is invalid: \`selectedNetworkClientId\` '${state.selectedNetworkClientId}' does not refer to an RPC endpoint within a network configuration`,
1063-
);
1064-
}
1071+
/**
1072+
* Checks that the given initial NetworkController state is internally
1073+
* consistent similar to `validateInitialState`, but if an anomaly is detected,
1074+
* it does its best to correct the state and logs an error to Sentry.
1075+
*
1076+
* @param state - The NetworkController state to verify.
1077+
* @param messenger - The NetworkController messenger.
1078+
* @returns The corrected state.
1079+
*/
1080+
function correctInitialState(
1081+
state: NetworkState,
1082+
messenger: NetworkControllerMessenger,
1083+
): NetworkState {
1084+
const networkConfigurationsSortedByChainId = getNetworkConfigurations(
1085+
state,
1086+
).sort((a, b) => a.chainId.localeCompare(b.chainId));
1087+
const networkClientIds = getAvailableNetworkClientIds(
1088+
networkConfigurationsSortedByChainId,
1089+
);
1090+
1091+
return produce(state, (newState) => {
1092+
if (!networkClientIds.includes(state.selectedNetworkClientId)) {
1093+
const firstNetworkConfiguration = networkConfigurationsSortedByChainId[0];
1094+
const newSelectedNetworkClientId =
1095+
firstNetworkConfiguration.rpcEndpoints[
1096+
firstNetworkConfiguration.defaultRpcEndpointIndex
1097+
].networkClientId;
1098+
messenger.call(
1099+
'ErrorReportingService:captureException',
1100+
new Error(
1101+
`\`selectedNetworkClientId\` '${state.selectedNetworkClientId}' does not refer to an RPC endpoint within a network configuration; correcting to '${newSelectedNetworkClientId}'`,
1102+
),
1103+
);
1104+
newState.selectedNetworkClientId = newSelectedNetworkClientId;
1105+
}
1106+
});
10651107
}
10661108

10671109
/**
@@ -1146,7 +1188,9 @@ export class NetworkController extends BaseController<
11461188
...getDefaultNetworkControllerState(additionalDefaultNetworks),
11471189
...state,
11481190
};
1149-
validateNetworkControllerState(initialState);
1191+
validateInitialState(initialState);
1192+
const correctedInitialState = correctInitialState(initialState, messenger);
1193+
11501194
if (!infuraProjectId || typeof infuraProjectId !== 'string') {
11511195
throw new Error('Invalid Infura project ID');
11521196
}
@@ -1168,7 +1212,7 @@ export class NetworkController extends BaseController<
11681212
},
11691213
},
11701214
messenger,
1171-
state: initialState,
1215+
state: correctedInitialState,
11721216
});
11731217

11741218
this.#infuraProjectId = infuraProjectId;

‎packages/network-controller/tests/NetworkController.test.ts

Lines changed: 89 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
// A lot of the tests in this file have conditionals.
22
/* eslint-disable jest/no-conditional-in-test */
33

4-
import type { Messenger } from '@metamask/base-controller';
54
import {
65
BuiltInNetworkName,
76
ChainId,
@@ -37,6 +36,7 @@ import {
3736
INFURA_NETWORKS,
3837
TESTNET,
3938
} from './helpers';
39+
import type { RootMessenger } from './helpers';
4040
import { FakeBlockTracker } from '../../../tests/fake-block-tracker';
4141
import type { FakeProviderStub } from '../../../tests/fake-provider';
4242
import { FakeProvider } from '../../../tests/fake-provider';
@@ -51,7 +51,6 @@ import type {
5151
InfuraRpcEndpoint,
5252
NetworkClientId,
5353
NetworkConfiguration,
54-
NetworkControllerActions,
5554
NetworkControllerEvents,
5655
NetworkControllerMessenger,
5756
NetworkControllerOptions,
@@ -350,30 +349,90 @@ describe('NetworkController', () => {
350349
);
351350
});
352351

353-
it('throws if selectedNetworkClientId does not match the networkClientId of an RPC endpoint in networkConfigurationsByChainId', () => {
354-
const messenger = buildRootMessenger();
355-
const restrictedMessenger = buildNetworkControllerMessenger(messenger);
356-
expect(
357-
() =>
358-
new NetworkController({
359-
messenger: restrictedMessenger,
360-
state: {
361-
selectedNetworkClientId: 'nonexistent',
362-
networkConfigurationsByChainId: {
363-
'0x1337': buildCustomNetworkConfiguration({
364-
chainId: '0x1337',
365-
}),
366-
},
352+
describe('if selectedNetworkClientId does not match the networkClientId of an RPC endpoint in networkConfigurationsByChainId', () => {
353+
it('corrects selectedNetworkClientId to the default RPC endpoint of the first chain', () => {
354+
const messenger = buildRootMessenger();
355+
messenger.registerActionHandler(
356+
'ErrorReportingService:captureException',
357+
jest.fn(),
358+
);
359+
const restrictedMessenger = buildNetworkControllerMessenger(messenger);
360+
const controller = new NetworkController({
361+
messenger: restrictedMessenger,
362+
state: {
363+
selectedNetworkClientId: 'nonexistent',
364+
networkConfigurationsByChainId: {
365+
'0x1': buildCustomNetworkConfiguration({
366+
chainId: '0x1',
367+
defaultRpcEndpointIndex: 1,
368+
rpcEndpoints: [
369+
buildCustomRpcEndpoint({
370+
networkClientId: 'AAAA-AAAA-AAAA-AAAA',
371+
}),
372+
buildCustomRpcEndpoint({
373+
networkClientId: 'BBBB-BBBB-BBBB-BBBB',
374+
}),
375+
],
376+
}),
377+
'0x2': buildCustomNetworkConfiguration({ chainId: '0x2' }),
378+
'0x3': buildCustomNetworkConfiguration({ chainId: '0x3' }),
367379
},
368-
infuraProjectId: 'infura-project-id',
369-
getRpcServiceOptions: () => ({
370-
fetch,
371-
btoa,
372-
}),
380+
},
381+
infuraProjectId: 'infura-project-id',
382+
getRpcServiceOptions: () => ({
383+
fetch,
384+
btoa,
373385
}),
374-
).toThrow(
375-
"NetworkController state is invalid: `selectedNetworkClientId` 'nonexistent' does not refer to an RPC endpoint within a network configuration",
376-
);
386+
});
387+
388+
expect(controller.state.selectedNetworkClientId).toBe(
389+
'BBBB-BBBB-BBBB-BBBB',
390+
);
391+
});
392+
393+
it('logs a Sentry error', () => {
394+
const messenger = buildRootMessenger();
395+
const captureExceptionMock = jest.fn();
396+
messenger.registerActionHandler(
397+
'ErrorReportingService:captureException',
398+
captureExceptionMock,
399+
);
400+
const restrictedMessenger = buildNetworkControllerMessenger(messenger);
401+
402+
new NetworkController({
403+
messenger: restrictedMessenger,
404+
state: {
405+
selectedNetworkClientId: 'nonexistent',
406+
networkConfigurationsByChainId: {
407+
'0x1': buildCustomNetworkConfiguration({
408+
chainId: '0x1',
409+
defaultRpcEndpointIndex: 1,
410+
rpcEndpoints: [
411+
buildCustomRpcEndpoint({
412+
networkClientId: 'AAAA-AAAA-AAAA-AAAA',
413+
}),
414+
buildCustomRpcEndpoint({
415+
networkClientId: 'BBBB-BBBB-BBBB-BBBB',
416+
}),
417+
],
418+
}),
419+
'0x2': buildCustomNetworkConfiguration({ chainId: '0x2' }),
420+
'0x3': buildCustomNetworkConfiguration({ chainId: '0x3' }),
421+
},
422+
},
423+
infuraProjectId: 'infura-project-id',
424+
getRpcServiceOptions: () => ({
425+
fetch,
426+
btoa,
427+
}),
428+
});
429+
430+
expect(captureExceptionMock).toHaveBeenCalledWith(
431+
new Error(
432+
"`selectedNetworkClientId` 'nonexistent' does not refer to an RPC endpoint within a network configuration; correcting to 'BBBB-BBBB-BBBB-BBBB'",
433+
),
434+
);
435+
});
377436
});
378437

379438
const invalidInfuraProjectIds = [undefined, null, {}, 1];
@@ -1326,10 +1385,7 @@ describe('NetworkController', () => {
13261385
{
13271386
messenger,
13281387
}: {
1329-
messenger: Messenger<
1330-
NetworkControllerActions,
1331-
NetworkControllerEvents
1332-
>;
1388+
messenger: RootMessenger;
13331389
},
13341390
args: Parameters<NetworkController['findNetworkClientIdByChainId']>,
13351391
): ReturnType<NetworkController['findNetworkClientIdByChainId']> =>
@@ -3278,13 +3334,7 @@ describe('NetworkController', () => {
32783334
],
32793335
[
32803336
'NetworkController:getNetworkConfigurationByChainId',
3281-
({
3282-
messenger,
3283-
chainId,
3284-
}: {
3285-
messenger: Messenger<NetworkControllerActions, NetworkControllerEvents>;
3286-
chainId: Hex;
3287-
}) =>
3337+
({ messenger, chainId }: { messenger: RootMessenger; chainId: Hex }) =>
32883338
messenger.call(
32893339
'NetworkController:getNetworkConfigurationByChainId',
32903340
chainId,
@@ -3397,7 +3447,7 @@ describe('NetworkController', () => {
33973447
messenger,
33983448
networkClientId,
33993449
}: {
3400-
messenger: Messenger<NetworkControllerActions, NetworkControllerEvents>;
3450+
messenger: RootMessenger;
34013451
networkClientId: NetworkClientId;
34023452
}) =>
34033453
messenger.call(
@@ -15171,7 +15221,7 @@ type WithControllerCallback<ReturnValue> = ({
1517115221
controller,
1517215222
}: {
1517315223
controller: NetworkController;
15174-
messenger: Messenger<NetworkControllerActions, NetworkControllerEvents>;
15224+
messenger: RootMessenger;
1517515225
networkControllerMessenger: NetworkControllerMessenger;
1517615226
}) => Promise<ReturnValue> | ReturnValue;
1517715227

@@ -15350,7 +15400,7 @@ async function waitForPublishedEvents<E extends NetworkControllerEvents>({
1535015400
// do nothing
1535115401
},
1535215402
}: {
15353-
messenger: Messenger<NetworkControllerActions, NetworkControllerEvents>;
15403+
messenger: RootMessenger;
1535415404
eventType: E['type'];
1535515405
count?: number;
1535615406
filter?: (payload: E['payload']) => boolean;
@@ -15481,7 +15531,7 @@ async function waitForStateChanges({
1548115531
operation,
1548215532
beforeResolving,
1548315533
}: {
15484-
messenger: Messenger<NetworkControllerActions, NetworkControllerEvents>;
15534+
messenger: RootMessenger;
1548515535
propertyPath?: string[];
1548615536
count?: number;
1548715537
wait?: number;

‎packages/network-controller/tests/helpers.ts

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,10 @@ import { FakeBlockTracker } from '../../../tests/fake-block-tracker';
1212
import { FakeProvider } from '../../../tests/fake-provider';
1313
import type { FakeProviderStub } from '../../../tests/fake-provider';
1414
import { buildTestObject } from '../../../tests/helpers';
15+
import type {
16+
ExtractAvailableAction,
17+
ExtractAvailableEvent,
18+
} from '../../base-controller/tests/helpers';
1519
import {
1620
type BuiltInNetworkClientId,
1721
type CustomNetworkClientId,
@@ -27,8 +31,6 @@ import type {
2731
AddNetworkFields,
2832
CustomRpcEndpoint,
2933
InfuraRpcEndpoint,
30-
NetworkControllerActions,
31-
NetworkControllerEvents,
3234
NetworkControllerMessenger,
3335
UpdateNetworkCustomRpcEndpointFields,
3436
} from '../src/NetworkController';
@@ -40,8 +42,8 @@ import type {
4042
import { NetworkClientType } from '../src/types';
4143

4244
export type RootMessenger = Messenger<
43-
NetworkControllerActions,
44-
NetworkControllerEvents
45+
ExtractAvailableAction<NetworkControllerMessenger>,
46+
ExtractAvailableEvent<NetworkControllerMessenger>
4547
>;
4648

4749
/**
@@ -73,7 +75,7 @@ export const TESTNET = {
7375
* @returns The messenger.
7476
*/
7577
export function buildRootMessenger(): RootMessenger {
76-
return new Messenger<NetworkControllerActions, NetworkControllerEvents>();
78+
return new Messenger();
7779
}
7880

7981
/**
@@ -87,7 +89,7 @@ export function buildNetworkControllerMessenger(
8789
): NetworkControllerMessenger {
8890
return messenger.getRestricted({
8991
name: 'NetworkController',
90-
allowedActions: [],
92+
allowedActions: ['ErrorReportingService:captureException'],
9193
allowedEvents: [],
9294
});
9395
}

0 commit comments

Comments
 (0)