Skip to content

Commit 1975c85

Browse files
jiexilegobeat
andauthored
fix: Gracefully handle bad responses from net_version calls to RPC endpoint when getting Provider Network State (#27509)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** Currently some users are seeing `Error: Cannot parse as a valid network ID: 'undefined'` errors when connected to RPC endpoints that fail to provide a value from `net_version` calls that can be parsed into a decimal string. This PR makes our internally handling of this case more flexibly by using `null` as the network version value sent to the inpage provider when receiving an unexpected `net_version` call value. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27509?quickstart=1) ## **Related issues** Fixes: #27487 ## **Manual testing steps** 1. Open extension background 2. Intercept a request for net_version (you can use a proxy like Burpsuite) and make it return a bad value 3. There should be no error in extension background related to this 4. Open a dapp 5. Open the dapp developer console 6. Expect to see an error like `JsonRpcError: MetaMask: Disconnected from chain. Attempting to connect.` 7. Test that `window.ethereum.request` works as expected despite this ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: legobeat <[email protected]>
1 parent d440363 commit 1975c85

File tree

3 files changed

+55
-6
lines changed

3 files changed

+55
-6
lines changed

app/scripts/metamask-controller.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3209,7 +3209,7 @@ export default class MetamaskController extends EventEmitter {
32093209
const { completedOnboarding } = this.onboardingController.state;
32103210

32113211
let networkVersion = this.deprecatedNetworkVersions[networkClientId];
3212-
if (!networkVersion && completedOnboarding) {
3212+
if (networkVersion === undefined && completedOnboarding) {
32133213
const ethQuery = new EthQuery(networkClient.provider);
32143214
networkVersion = await new Promise((resolve) => {
32153215
ethQuery.sendAsync({ method: 'net_version' }, (error, result) => {

shared/modules/network.utils.test.ts

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import {
33
isSafeChainId,
44
isPrefixedFormattedHexString,
55
isTokenDetectionEnabledForNetwork,
6+
convertNetworkId,
67
} from './network.utils';
78

89
describe('network utils', () => {
@@ -83,4 +84,52 @@ describe('network utils', () => {
8384
expect(isTokenDetectionEnabledForNetwork(undefined)).toBe(false);
8485
});
8586
});
87+
88+
describe('convertNetworkId', () => {
89+
it('returns decimal strings for postive integer number values', () => {
90+
expect(convertNetworkId(0)).toStrictEqual('0');
91+
expect(convertNetworkId(123)).toStrictEqual('123');
92+
expect(convertNetworkId(1337)).toStrictEqual('1337');
93+
});
94+
95+
it('returns null for negative numbers', () => {
96+
expect(convertNetworkId(-1)).toStrictEqual(null);
97+
});
98+
99+
it('returns null for non integer numbers', () => {
100+
expect(convertNetworkId(0.1)).toStrictEqual(null);
101+
expect(convertNetworkId(1.1)).toStrictEqual(null);
102+
});
103+
104+
it('returns null for NaN', () => {
105+
expect(convertNetworkId(Number.NaN)).toStrictEqual(null);
106+
});
107+
108+
it('returns decimal strings for strict valid hex values', () => {
109+
expect(convertNetworkId('0x0')).toStrictEqual('0');
110+
expect(convertNetworkId('0x1')).toStrictEqual('1');
111+
expect(convertNetworkId('0x539')).toStrictEqual('1337');
112+
});
113+
114+
it('returns null for invalid hex values', () => {
115+
expect(convertNetworkId('0xG')).toStrictEqual(null);
116+
expect(convertNetworkId('0x@')).toStrictEqual(null);
117+
expect(convertNetworkId('0xx1')).toStrictEqual(null);
118+
});
119+
120+
it('returns the value as is if already a postive decimal string', () => {
121+
expect(convertNetworkId('0')).toStrictEqual('0');
122+
expect(convertNetworkId('1')).toStrictEqual('1');
123+
expect(convertNetworkId('1337')).toStrictEqual('1337');
124+
});
125+
126+
it('returns null for negative number strings', () => {
127+
expect(convertNetworkId('-1')).toStrictEqual(null);
128+
});
129+
130+
it('returns null for non integer number strings', () => {
131+
expect(convertNetworkId('0.1')).toStrictEqual(null);
132+
expect(convertNetworkId('1.1')).toStrictEqual(null);
133+
});
134+
});
86135
});

shared/modules/network.utils.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -78,16 +78,16 @@ function isSafeInteger(value: unknown): value is number {
7878
* as either a number, a decimal string, or a 0x-prefixed hex string.
7979
*
8080
* @param value - The network ID to convert, in an unknown format.
81-
* @returns A valid network ID (as a decimal string)
82-
* @throws If the given value cannot be safely parsed.
81+
* @returns A valid network ID (as a decimal string) or null if
82+
* the given value cannot be parsed.
8383
*/
84-
export function convertNetworkId(value: unknown): string {
85-
if (typeof value === 'number' && !Number.isNaN(value)) {
84+
export function convertNetworkId(value: unknown): string | null {
85+
if (typeof value === 'number' && Number.isInteger(value) && value >= 0) {
8686
return `${value}`;
8787
} else if (isStrictHexString(value)) {
8888
return `${convertHexToDecimal(value)}`;
8989
} else if (typeof value === 'string' && /^\d+$/u.test(value)) {
9090
return value;
9191
}
92-
throw new Error(`Cannot parse as a valid network ID: '${value}'`);
92+
return null;
9393
}

0 commit comments

Comments
 (0)