Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ describe('AssetsContractController with NetworkClientId', () => {
'invalidNetworkClientId',
),
).rejects.toThrow(
`No custom network client was found with the ID "invalidNetworkClientId".`,
`No network client found with ID "invalidNetworkClientId"`,
);
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});
Expand All @@ -45,7 +45,7 @@ describe('AssetsContractController with NetworkClientId', () => {
'invalidNetworkClientId',
),
).rejects.toThrow(
`No custom network client was found with the ID "invalidNetworkClientId".`,
`No network client found with ID "invalidNetworkClientId"`,
);
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});
Expand Down Expand Up @@ -153,7 +153,7 @@ describe('AssetsContractController with NetworkClientId', () => {
undefined,
'invalidNetworkClientId',
),
).rejects.toThrow('No custom network client was found');
).rejects.toThrow('No network client found');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

Expand Down Expand Up @@ -573,7 +573,7 @@ describe('AssetsContractController with NetworkClientId', () => {
ERC721_GODS_ADDRESS,
'invalidNetworkClientId',
),
).rejects.toThrow('No custom network client was found');
).rejects.toThrow('No network client found');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

Expand Down Expand Up @@ -689,7 +689,7 @@ describe('AssetsContractController with NetworkClientId', () => {
'148332',
'invalidNetworkClientId',
),
).rejects.toThrow('No custom network client was found');
).rejects.toThrow('No network client found');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

Expand Down Expand Up @@ -813,7 +813,7 @@ describe('AssetsContractController with NetworkClientId', () => {
'1',
'invalidNetworkClientId',
),
).rejects.toThrow('No custom network client was found');
).rejects.toThrow('No network client found');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

Expand Down Expand Up @@ -863,7 +863,7 @@ describe('AssetsContractController with NetworkClientId', () => {
ERC1155_ID,
'invalidNetworkClientId',
),
).rejects.toThrow('No custom network client was found');
).rejects.toThrow('No network client found');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

Expand Down
4 changes: 4 additions & 0 deletions packages/gas-fee-controller/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ 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 `fetchGasFeeEstimates` so that it no longer throws an error if given a reference to a network that does not exist ([#5084](https://github.com/MetaMask/core/pull/5084))

## [22.0.2]

### Changed
Expand Down
30 changes: 30 additions & 0 deletions packages/gas-fee-controller/src/GasFeeController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -999,6 +999,7 @@ describe('GasFeeController', () => {
);
});
});

describe('when passed a networkClientId in options object', () => {
const getDefaultOptions = () => ({
getIsEIP1559Compatible: jest.fn().mockResolvedValue(true),
Expand Down Expand Up @@ -1033,6 +1034,35 @@ describe('GasFeeController', () => {
);
});

it("should not update state if the network client doesn't exist", async () => {
await setupGasFeeController({
...getDefaultOptions(),
});
const initialState = gasFeeController.state;

await gasFeeController.fetchGasFeeEstimates({
networkClientId: 'nonexistent',
});

expect(gasFeeController.state).toBe(initialState);
});

it("should return an empty set of estimates if the network client doesn't exist", async () => {
await setupGasFeeController({
...getDefaultOptions(),
});

const estimates = await gasFeeController.fetchGasFeeEstimates({
networkClientId: 'nonexistent',
});

expect(estimates).toStrictEqual({
gasFeeEstimates: {},
estimatedGasFeeTimeBounds: {},
gasEstimateType: 'none',
});
});

it('should call determineGasFeeCalculations correctly', async () => {
await setupGasFeeController({
...getDefaultOptions(),
Expand Down
41 changes: 29 additions & 12 deletions packages/gas-fee-controller/src/GasFeeController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,15 @@ import {
toHex,
} from '@metamask/controller-utils';
import EthQuery from '@metamask/eth-query';
import type {
NetworkClientId,
NetworkControllerGetEIP1559CompatibilityAction,
NetworkControllerGetNetworkClientByIdAction,
NetworkControllerGetStateAction,
NetworkControllerNetworkDidChangeEvent,
NetworkState,
ProviderProxy,
import {
NoNetworkClientFoundError,
type NetworkClientId,
type NetworkControllerGetEIP1559CompatibilityAction,
type NetworkControllerGetNetworkClientByIdAction,
type NetworkControllerGetStateAction,
type NetworkControllerNetworkDidChangeEvent,
type NetworkState,
type ProviderProxy,
} from '@metamask/network-controller';
import { StaticIntervalPollingController } from '@metamask/polling-controller';
import type { Hex } from '@metamask/utils';
Expand All @@ -29,6 +30,9 @@ import {
fetchEthGasPriceEstimate,
calculateTimeEstimate,
} from './gas-util';
import { createModuleLogger, projectLogger } from './logger';

const log = createModuleLogger(projectLogger, 'GasFeeController');

export const LEGACY_GAS_PRICES_API_URL = `https://api.metaswap.codefi.network/gasPrices`;

Expand Down Expand Up @@ -443,10 +447,23 @@ export class GasFeeController extends StaticIntervalPollingController<GasFeePoll
decimalChainId: number;

if (networkClientId !== undefined) {
const networkClient = this.messagingSystem.call(
'NetworkController:getNetworkClientById',
networkClientId,
);
let networkClient;
try {
networkClient = this.messagingSystem.call(
'NetworkController:getNetworkClientById',
networkClientId,
);
} catch (error) {
if (error instanceof NoNetworkClientFoundError) {
log(error.message);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured that we ought to be aware of the problem on some level, I am just unsure what the best place is.

@Gudahtt What would be your recommendation here? I seem to recall a comment you made in some PR a while back about switching away from using console.error / console.warn. There's also this PR where we introduced a separate production-level logger, but it seems like this log message would continue to surface in Sentry which we might not want.

Copy link
Member

@Gudahtt Gudahtt Dec 20, 2024

Choose a reason for hiding this comment

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

The pattern in the linked PR seems ideal to me for non-debug logging because the client can choose whether it's enabled or not. That was generally the goal: let the client control logging, don't make it mandatory.

Logs can definitely be useful as Sentry breadcrumbs.

Copy link
Member

Choose a reason for hiding this comment

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

About this specific scenario, can we make this situation impossible? I don't quite follow how it could happen.

Is there an active poll on a deleted network that we haven't shut down, or is something else trying to get estimates from a non-existent network? I'd think the root problem lies elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I guess you're right. Maybe hiding the error isn't such a good thing in this case.

return {
gasFeeEstimates: {},
estimatedGasFeeTimeBounds: {},
gasEstimateType: GAS_ESTIMATE_TYPES.NONE,
};
}
throw error;
}
isLegacyGasAPICompatible = networkClient.configuration.chainId === '0x38';

decimalChainId = convertHexToDecimal(networkClient.configuration.chainId);
Expand Down
7 changes: 7 additions & 0 deletions packages/gas-fee-controller/src/logger.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/* istanbul ignore file */

import { createProjectLogger, createModuleLogger } from '@metamask/utils';

export const projectLogger = createProjectLogger('gas-fee-controller');

export { createModuleLogger };
46 changes: 9 additions & 37 deletions packages/network-controller/src/NetworkController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import type {
ProxyWithAccessibleTarget,
} from './create-auto-managed-network-client';
import { createAutoManagedNetworkClient } from './create-auto-managed-network-client';
import { NoNetworkClientFoundError } from './errors';
import { projectLogger, createModuleLogger } from './logger';
import { NetworkClientType } from './types';
import type {
Expand All @@ -41,6 +42,9 @@ import type {
CustomNetworkClientConfiguration,
InfuraNetworkClientConfiguration,
NetworkClientConfiguration,
NetworkClientId,
BuiltInNetworkClientId,
CustomNetworkClientId,
} from './types';

const debugLog = createModuleLogger(projectLogger, 'NetworkController');
Expand Down Expand Up @@ -294,21 +298,6 @@ function isErrorWithCode(error: unknown): error is { code: string | number } {
return typeof error === 'object' && error !== null && 'code' in error;
}

/**
* The string that uniquely identifies an Infura network client.
*/
export type BuiltInNetworkClientId = InfuraNetworkType;

/**
* The string that uniquely identifies a custom network client.
*/
export type CustomNetworkClientId = string;

/**
* The string that uniquely identifies a network client.
*/
export type NetworkClientId = BuiltInNetworkClientId | CustomNetworkClientId;

/**
* Extra information about each network, such as whether it is accessible or
* blocked and whether it supports EIP-1559, keyed by network client ID.
Expand Down Expand Up @@ -1107,11 +1096,7 @@ export class NetworkController extends BaseController<
// This is impossible to reach
/* istanbul ignore if */
if (!infuraNetworkClient) {
throw new Error(
// TODO: Either fix this lint violation or explain why it's necessary to ignore.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`No Infura network client was found with the ID "${networkClientId}".`,
);
throw NoNetworkClientFoundError.create(networkClientId);
}
return infuraNetworkClient;
}
Expand All @@ -1121,11 +1106,7 @@ export class NetworkController extends BaseController<
networkClientId
];
if (!customNetworkClient) {
throw new Error(
// TODO: Either fix this lint violation or explain why it's necessary to ignore.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`No custom network client was found with the ID "${networkClientId}".`,
);
throw NoNetworkClientFoundError.create(networkClientId);
}
return customNetworkClient;
}
Expand Down Expand Up @@ -1233,14 +1214,7 @@ export class NetworkController extends BaseController<
error,
);
}
} else if (
typeof Error !== 'undefined' &&
hasProperty(error as unknown as Error, 'message') &&
typeof (error as unknown as Error).message === 'string' &&
(error as unknown as Error).message.includes(
'No custom network client was found with the ID',
)
) {
} else if (error instanceof NoNetworkClientFoundError) {
throw error;
} else {
debugLog(
Expand Down Expand Up @@ -2619,9 +2593,7 @@ export class NetworkController extends BaseController<
// This is impossible to reach
/* istanbul ignore if */
if (!possibleAutoManagedNetworkClient) {
throw new Error(
`No Infura network client found with ID '${networkClientId}'`,
);
throw NoNetworkClientFoundError.create(networkClientId);
}

autoManagedNetworkClient = possibleAutoManagedNetworkClient;
Expand All @@ -2632,7 +2604,7 @@ export class NetworkController extends BaseController<
];

if (!possibleAutoManagedNetworkClient) {
throw new Error(`No network client found with ID '${networkClientId}'`);
throw NoNetworkClientFoundError.create(networkClientId);
}

autoManagedNetworkClient = possibleAutoManagedNetworkClient;
Expand Down
9 changes: 9 additions & 0 deletions packages/network-controller/src/errors.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import type { NetworkClientId } from './types';

export class NoNetworkClientFoundError extends Error {
static create(networkClientId: NetworkClientId) {
// ESLint is confused here; this is guaranteed to be a string.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
return new this(`No network client found with ID "${networkClientId}"`);
}
}
10 changes: 6 additions & 4 deletions packages/network-controller/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,6 @@ export type {
Block,
NetworkMetadata,
NetworkConfiguration,
BuiltInNetworkClientId,
CustomNetworkClientId,
NetworkClientId,
NetworksMetadata,
NetworkState,
BlockTrackerProxy,
Expand Down Expand Up @@ -44,11 +41,16 @@ export {
RpcEndpointType,
} from './NetworkController';
export * from './constants';
export type { BlockTracker, Provider } from './types';
export type {
BlockTracker,
Provider,
NetworkClientConfiguration,
InfuraNetworkClientConfiguration,
CustomNetworkClientConfiguration,
BuiltInNetworkClientId,
CustomNetworkClientId,
NetworkClientId,
} from './types';
export { NetworkClientType } from './types';
export type { NetworkClient } from './create-network-client';
export { NoNetworkClientFoundError } from './errors';
15 changes: 15 additions & 0 deletions packages/network-controller/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,18 @@ export type InfuraNetworkClientConfiguration = {
export type NetworkClientConfiguration =
| CustomNetworkClientConfiguration
| InfuraNetworkClientConfiguration;

/**
* The string that uniquely identifies an Infura network client.
*/
export type BuiltInNetworkClientId = InfuraNetworkType;

/**
* The string that uniquely identifies a custom network client.
*/
export type CustomNetworkClientId = string;

/**
* The string that uniquely identifies a network client.
*/
export type NetworkClientId = BuiltInNetworkClientId | CustomNetworkClientId;
Loading
Loading