Skip to content

Correct invalid initial selectedNetworkClientId #5851

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

Merged
merged 2 commits into from
May 28, 2025
+166 −73
Merged

Correct invalid initial selectedNetworkClientId #5851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
409b54c
Correct invalid initial selectedNetworkClientId
mcmire May 21, 2025
f4c20a5
Merge branch 'main' into handle-invalid-selected-network-client-id
mcmire May 28, 2025
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
1 change: 1 addition & 0 deletions packages/network-controller/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- 429 responses now throw a "Rate Limiting" error
- Other 4xx responses now throw a generic HTTP client error
- Invalid JSON responses now throw a "Parse" error
- 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))

## [23.5.0]

Expand Down
1 change: 1 addition & 0 deletions packages/network-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
"dependencies": {
"@metamask/base-controller": "^8.0.1",
"@metamask/controller-utils": "^11.9.0",
"@metamask/error-reporting-service": "^0.0.0",
"@metamask/eth-block-tracker": "^11.0.3",
"@metamask/eth-json-rpc-infura": "^10.2.0",
"@metamask/eth-json-rpc-middleware": "^16.0.1",
Expand Down
72 changes: 58 additions & 14 deletions packages/network-controller/src/NetworkController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
BUILT_IN_CUSTOM_NETWORKS_RPC,
BUILT_IN_NETWORKS,
} from '@metamask/controller-utils';
import type { ErrorReportingServiceCaptureExceptionAction } from '@metamask/error-reporting-service';
import type { PollingBlockTrackerOptions } from '@metamask/eth-block-tracker';
import EthQuery from '@metamask/eth-query';
import { errorCodes } from '@metamask/rpc-errors';
Expand All @@ -26,6 +27,7 @@ import type { Hex } from '@metamask/utils';
import { hasProperty, isPlainObject, isStrictHexString } from '@metamask/utils';
import deepEqual from 'fast-deep-equal';
import type { Draft } from 'immer';
import { produce } from 'immer';
import { cloneDeep } from 'lodash';
import type { Logger } from 'loglevel';
import { createSelector } from 'reselect';
Expand Down Expand Up @@ -498,6 +500,11 @@ export type NetworkControllerEvents =
| NetworkControllerRpcEndpointDegradedEvent
| NetworkControllerRpcEndpointRequestRetriedEvent;

/**
* All events that {@link NetworkController} calls internally.
*/
type AllowedEvents = never;

export type NetworkControllerGetStateAction = ControllerGetStateAction<
typeof controllerName,
NetworkState
Expand Down Expand Up @@ -590,12 +597,17 @@ export type NetworkControllerActions =
| NetworkControllerRemoveNetworkAction
| NetworkControllerUpdateNetworkAction;

/**
* All actions that {@link NetworkController} calls internally.
*/
type AllowedActions = ErrorReportingServiceCaptureExceptionAction;

export type NetworkControllerMessenger = RestrictedMessenger<
typeof controllerName,
NetworkControllerActions,
NetworkControllerEvents,
never,
never
NetworkControllerActions | AllowedActions,
NetworkControllerEvents | AllowedEvents,
AllowedActions['type'],
AllowedEvents['type']
>;

/**
Expand Down Expand Up @@ -1003,7 +1015,7 @@ function deriveInfuraNetworkNameFromRpcEndpointUrl(
* @param state - The NetworkController state to verify.
* @throws if the state is invalid in some way.
*/
function validateNetworkControllerState(state: NetworkState) {
function validateInitialState(state: NetworkState) {
const networkConfigurationEntries = Object.entries(
state.networkConfigurationsByChainId,
);
Expand Down Expand Up @@ -1054,14 +1066,44 @@ function validateNetworkControllerState(state: NetworkState) {
'NetworkController state has invalid `networkConfigurationsByChainId`: Every RPC endpoint across all network configurations must have a unique `networkClientId`',
);
}
}

if (!networkClientIds.includes(state.selectedNetworkClientId)) {
throw new Error(
// This ESLint rule mistakenly produces an error.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`NetworkController state is invalid: \`selectedNetworkClientId\` '${state.selectedNetworkClientId}' does not refer to an RPC endpoint within a network configuration`,
);
}
/**
* Checks that the given initial NetworkController state is internally
* consistent similar to `validateInitialState`, but if an anomaly is detected,
* it does its best to correct the state and logs an error to Sentry.
*
* @param state - The NetworkController state to verify.
* @param messenger - The NetworkController messenger.
* @returns The corrected state.
*/
function correctInitialState(
state: NetworkState,
messenger: NetworkControllerMessenger,
): NetworkState {
const networkConfigurationsSortedByChainId = getNetworkConfigurations(
state,
).sort((a, b) => a.chainId.localeCompare(b.chainId));
const networkClientIds = getAvailableNetworkClientIds(
networkConfigurationsSortedByChainId,
);

return produce(state, (newState) => {
if (!networkClientIds.includes(state.selectedNetworkClientId)) {
const firstNetworkConfiguration = networkConfigurationsSortedByChainId[0];
const newSelectedNetworkClientId =
firstNetworkConfiguration.rpcEndpoints[
firstNetworkConfiguration.defaultRpcEndpointIndex
].networkClientId;
messenger.call(
'ErrorReportingService:captureException',
new Error(
`\`selectedNetworkClientId\` '${state.selectedNetworkClientId}' does not refer to an RPC endpoint within a network configuration; correcting to '${newSelectedNetworkClientId}'`,
),
);
newState.selectedNetworkClientId = newSelectedNetworkClientId;
}
});
}

/**
Expand Down Expand Up @@ -1146,7 +1188,9 @@ export class NetworkController extends BaseController<
...getDefaultNetworkControllerState(additionalDefaultNetworks),
...state,
};
validateNetworkControllerState(initialState);
validateInitialState(initialState);
const correctedInitialState = correctInitialState(initialState, messenger);

if (!infuraProjectId || typeof infuraProjectId !== 'string') {
throw new Error('Invalid Infura project ID');
}
Expand All @@ -1168,7 +1212,7 @@ export class NetworkController extends BaseController<
},
},
messenger,
state: initialState,
state: correctedInitialState,
});

this.#infuraProjectId = infuraProjectId;
Expand Down
128 changes: 89 additions & 39 deletions packages/network-controller/tests/NetworkController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// A lot of the tests in this file have conditionals.
/* eslint-disable jest/no-conditional-in-test */

import type { Messenger } from '@metamask/base-controller';
import {
BuiltInNetworkName,
ChainId,
Expand Down Expand Up @@ -37,6 +36,7 @@ import {
INFURA_NETWORKS,
TESTNET,
} from './helpers';
import type { RootMessenger } from './helpers';
import { FakeBlockTracker } from '../../../tests/fake-block-tracker';
import type { FakeProviderStub } from '../../../tests/fake-provider';
import { FakeProvider } from '../../../tests/fake-provider';
Expand All @@ -51,7 +51,6 @@ import type {
InfuraRpcEndpoint,
NetworkClientId,
NetworkConfiguration,
NetworkControllerActions,
NetworkControllerEvents,
NetworkControllerMessenger,
NetworkControllerOptions,
Expand Down Expand Up @@ -350,30 +349,90 @@ describe('NetworkController', () => {
);
});

it('throws if selectedNetworkClientId does not match the networkClientId of an RPC endpoint in networkConfigurationsByChainId', () => {
const messenger = buildRootMessenger();
const restrictedMessenger = buildNetworkControllerMessenger(messenger);
expect(
() =>
new NetworkController({
messenger: restrictedMessenger,
state: {
selectedNetworkClientId: 'nonexistent',
networkConfigurationsByChainId: {
'0x1337': buildCustomNetworkConfiguration({
chainId: '0x1337',
}),
},
describe('if selectedNetworkClientId does not match the networkClientId of an RPC endpoint in networkConfigurationsByChainId', () => {
it('corrects selectedNetworkClientId to the default RPC endpoint of the first chain', () => {
const messenger = buildRootMessenger();
messenger.registerActionHandler(
'ErrorReportingService:captureException',
jest.fn(),
);
const restrictedMessenger = buildNetworkControllerMessenger(messenger);
const controller = new NetworkController({
messenger: restrictedMessenger,
state: {
selectedNetworkClientId: 'nonexistent',
networkConfigurationsByChainId: {
'0x1': buildCustomNetworkConfiguration({
chainId: '0x1',
defaultRpcEndpointIndex: 1,
rpcEndpoints: [
buildCustomRpcEndpoint({
networkClientId: 'AAAA-AAAA-AAAA-AAAA',
}),
buildCustomRpcEndpoint({
networkClientId: 'BBBB-BBBB-BBBB-BBBB',
}),
],
}),
'0x2': buildCustomNetworkConfiguration({ chainId: '0x2' }),
'0x3': buildCustomNetworkConfiguration({ chainId: '0x3' }),
},
infuraProjectId: 'infura-project-id',
getRpcServiceOptions: () => ({
fetch,
btoa,
}),
},
infuraProjectId: 'infura-project-id',
getRpcServiceOptions: () => ({
fetch,
btoa,
}),
).toThrow(
"NetworkController state is invalid: `selectedNetworkClientId` 'nonexistent' does not refer to an RPC endpoint within a network configuration",
);
});

expect(controller.state.selectedNetworkClientId).toBe(
'BBBB-BBBB-BBBB-BBBB',
);
});

it('logs a Sentry error', () => {
const messenger = buildRootMessenger();
const captureExceptionMock = jest.fn();
messenger.registerActionHandler(
'ErrorReportingService:captureException',
captureExceptionMock,
);
const restrictedMessenger = buildNetworkControllerMessenger(messenger);

new NetworkController({
messenger: restrictedMessenger,
state: {
selectedNetworkClientId: 'nonexistent',
networkConfigurationsByChainId: {
'0x1': buildCustomNetworkConfiguration({
chainId: '0x1',
defaultRpcEndpointIndex: 1,
rpcEndpoints: [
buildCustomRpcEndpoint({
networkClientId: 'AAAA-AAAA-AAAA-AAAA',
}),
buildCustomRpcEndpoint({
networkClientId: 'BBBB-BBBB-BBBB-BBBB',
}),
],
}),
'0x2': buildCustomNetworkConfiguration({ chainId: '0x2' }),
'0x3': buildCustomNetworkConfiguration({ chainId: '0x3' }),
},
},
infuraProjectId: 'infura-project-id',
getRpcServiceOptions: () => ({
fetch,
btoa,
}),
});

expect(captureExceptionMock).toHaveBeenCalledWith(
new Error(
"`selectedNetworkClientId` 'nonexistent' does not refer to an RPC endpoint within a network configuration; correcting to 'BBBB-BBBB-BBBB-BBBB'",
),
);
});
});

const invalidInfuraProjectIds = [undefined, null, {}, 1];
Expand Down Expand Up @@ -1326,10 +1385,7 @@ describe('NetworkController', () => {
{
messenger,
}: {
messenger: Messenger<
NetworkControllerActions,
NetworkControllerEvents
>;
messenger: RootMessenger;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type of the messenger in these tests only took internal actions and events into consideration and not external actions, so I've had to correct all of them.

},
args: Parameters<NetworkController['findNetworkClientIdByChainId']>,
): ReturnType<NetworkController['findNetworkClientIdByChainId']> =>
Expand Down Expand Up @@ -3278,13 +3334,7 @@ describe('NetworkController', () => {
],
[
'NetworkController:getNetworkConfigurationByChainId',
({
messenger,
chainId,
}: {
messenger: Messenger<NetworkControllerActions, NetworkControllerEvents>;
chainId: Hex;
}) =>
({ messenger, chainId }: { messenger: RootMessenger; chainId: Hex }) =>
messenger.call(
'NetworkController:getNetworkConfigurationByChainId',
chainId,
Expand Down Expand Up @@ -3397,7 +3447,7 @@ describe('NetworkController', () => {
messenger,
networkClientId,
}: {
messenger: Messenger<NetworkControllerActions, NetworkControllerEvents>;
messenger: RootMessenger;
networkClientId: NetworkClientId;
}) =>
messenger.call(
Expand Down Expand Up @@ -15171,7 +15221,7 @@ type WithControllerCallback<ReturnValue> = ({
controller,
}: {
controller: NetworkController;
messenger: Messenger<NetworkControllerActions, NetworkControllerEvents>;
messenger: RootMessenger;
networkControllerMessenger: NetworkControllerMessenger;
}) => Promise<ReturnValue> | ReturnValue;

Expand Down Expand Up @@ -15350,7 +15400,7 @@ async function waitForPublishedEvents<E extends NetworkControllerEvents>({
// do nothing
},
}: {
messenger: Messenger<NetworkControllerActions, NetworkControllerEvents>;
messenger: RootMessenger;
eventType: E['type'];
count?: number;
filter?: (payload: E['payload']) => boolean;
Expand Down Expand Up @@ -15481,7 +15531,7 @@ async function waitForStateChanges({
operation,
beforeResolving,
}: {
messenger: Messenger<NetworkControllerActions, NetworkControllerEvents>;
messenger: RootMessenger;
propertyPath?: string[];
count?: number;
wait?: number;
Expand Down
14 changes: 8 additions & 6 deletions packages/network-controller/tests/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ import { FakeBlockTracker } from '../../../tests/fake-block-tracker';
import { FakeProvider } from '../../../tests/fake-provider';
import type { FakeProviderStub } from '../../../tests/fake-provider';
import { buildTestObject } from '../../../tests/helpers';
import type {
ExtractAvailableAction,
ExtractAvailableEvent,
} from '../../base-controller/tests/helpers';
import {
type BuiltInNetworkClientId,
type CustomNetworkClientId,
Expand All @@ -27,8 +31,6 @@ import type {
AddNetworkFields,
CustomRpcEndpoint,
InfuraRpcEndpoint,
NetworkControllerActions,
NetworkControllerEvents,
NetworkControllerMessenger,
UpdateNetworkCustomRpcEndpointFields,
} from '../src/NetworkController';
Expand All @@ -40,8 +42,8 @@ import type {
import { NetworkClientType } from '../src/types';

export type RootMessenger = Messenger<
NetworkControllerActions,
NetworkControllerEvents
ExtractAvailableAction<NetworkControllerMessenger>,
ExtractAvailableEvent<NetworkControllerMessenger>
>;

/**
Expand Down Expand Up @@ -73,7 +75,7 @@ export const TESTNET = {
* @returns The messenger.
*/
export function buildRootMessenger(): RootMessenger {
return new Messenger<NetworkControllerActions, NetworkControllerEvents>();
return new Messenger();
}

/**
Expand All @@ -87,7 +89,7 @@ export function buildNetworkControllerMessenger(
): NetworkControllerMessenger {
return messenger.getRestricted({
name: 'NetworkController',
allowedActions: [],
allowedActions: ['ErrorReportingService:captureException'],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we need to add ErrorReportingService:captureException to the list of external actions because we are now using it in the controller.

allowedEvents: [],
});
}
Expand Down
Loading
Loading