feat: allow `networkVersion` to be set to `null`. fire connection events based on new `isConnected` property value #404

jiexi · 2025-01-27T19:34:30Z

BREAKING: Interprets the provider's networkVersion value of loading to be null and for the networkChanged event to emit a null value
BREAKING: Removes disconnect event/behavior when receiving the loading value for networkVersion and replaces it with a check against isConnected that is now expected in the metamask_getProviderState result and metamask_chainChanged event

Extension: MetaMask/metamask-extension#29936
See: https://github.com/orgs/MetaMask/projects/146/views/1?filterQuery=label%3A%22team-wallet-api-platform%22+-status%3ABacklog&pane=issue&itemId=95374156&issue=MetaMask%7CMetaMask-planning%7C4039

github-advanced-security · 2025-01-27T19:37:28Z

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

jiexi · 2025-01-27T22:57:25Z

jest.config.js

try to bump these

src/utils.ts

src/StreamProvider.test.ts

adonesky1

Just a few non-blocking questions about tests, otherwise LGTM!

danjm · 2025-02-03T16:01:02Z

src/MetaMaskInpageProvider.ts

+    chainId?: string;
+    networkVersion?: string;
+    isConnected?: boolean;
+  } = {}) {


Can there ever be a difference between the isConnected property passed in here and this._state.isConnected that was used here previously?

yes, it is possible, but this method no longer relies on the this._state.isConnected value and will always fire if the networkVersion changes (which is what we want here)

danjm

This looks good to me. I believe it solves the problem as describe and I can't think of edge cases that it might unintentionally break

Gudahtt · 2025-02-03T16:45:13Z

src/StreamProvider.ts

  }: {
    chainId?: string | undefined;
    networkVersion?: string | undefined;
+    isConnected?: boolean | undefined;


When would this be undefined?

it shouldn't be, but there is some type gynamistics going on between StreamProvider and BaseProvider on this method

Hmm. Well, not a blocker, but this does seem sub-optimal from an API design standpoint. If this is omitted, then we'll wrongly assume that the wallet is disconnected. A user of this API might mistakenly think this was intended. And it would be tricky to discover that's what happened if we did see that happen (particularly if it was limited to a specific dapp/scenario/etc.).

I was thinking that maybe this "provider state" object is meant to be baked-in someplace down the hierarchy, i.e. not assumed as part of BaseProvider. But no, it is used throughout. So it may as well be accurate to how we use it.

Opting for this to be done separately later given this pattern has existed prior to this PR then

…provider (#30111)  ## **Description** Previously the inpage provider would withhold events for chainChanged events (and property value updates, i.e. window.ethereum.chainId and .networkVersion) when the dapp's network was changed to an rpc endpoint that was unresponsive or did not support net_version. The dapp would instead receive a disconnect event. Now the inpage provider always emits chainChanged and networkChanged events (and exposes the correct values on window.ethereum.chainId and .networkVersion) when the selected network for the dapp has changed regardless of if the network being changed to is responsive or if it supports net_version requests. It does this by having the wallet send a loading for networkVersion when it cannot be resolved (same behavior as before) AND a new isConnected property in the metamask_getProviderState request and metamask_chainChanged events (these are different from the events emittted by window.ethereum). isConnected is derived from whether the NetworkController.state.networkMetadata[].status value is the Available constant. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30109?quickstart=1) ## **Related issues** See: #29936 Providers patch from commit (d919ab6b): MetaMask/providers#404 ## **Manual testing steps** ``` window.ethereum.on('accountsChanged', (data) => console.log('accountsChanged', data)) window.ethereum.on('chainChanged', (data) => console.log('chainChanged', data)) window.ethereum.on('networkChanged', (data) => console.log('networkChanged', data)) window.ethereum.on('connect', (data) => console.log('connect', data)) window.ethereum.on('disconnect', (data) => console.log('disconnect', data)) ``` 1. Go to a webpage. Enter the following in console 2. Change to Linea, see that the correct values are emitted for the `chainChanged` and `networkChanged` events 3. Change to Sepolia, see that the correct values are emitted for the `chainChanged` and `networkChanged` events 4. Change to a network that is non-responsive, see that `chainChanged` emits with the correct chainId, but `networkChanged` emits with a null value, AND there is a `disconnect` event emitted 5. Change back to a working network, see that the correct values are emitted for the `chainChanged` and `networkChanged` events, AND there is a `connect` event emitted with the new chainId 6. Do the same above with a responsive network that does not have `net_version` implemented. Se that the correct values are emitted for the `chainChanged`, that `networkChanged` emits null, and that there is no `disconnect` event emitted ## **Screenshots/Recordings**  ### **Before**  ### **After** ![Screenshot 2025-02-04 at 8 34 13 AM](https://github.com/user-attachments/assets/f6558363-f848-445a-811e-d1a052a4e6c0) ![Screenshot 2025-02-04 at 8 39 33 AM](https://github.com/user-attachments/assets/12bff524-bb3c-4bf8-b4d2-c6a01ce63b33) ## **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/main/.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/main/.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.

…provider (#30111)  Previously the inpage provider would withhold events for chainChanged events (and property value updates, i.e. window.ethereum.chainId and .networkVersion) when the dapp's network was changed to an rpc endpoint that was unresponsive or did not support net_version. The dapp would instead receive a disconnect event. Now the inpage provider always emits chainChanged and networkChanged events (and exposes the correct values on window.ethereum.chainId and .networkVersion) when the selected network for the dapp has changed regardless of if the network being changed to is responsive or if it supports net_version requests. It does this by having the wallet send a loading for networkVersion when it cannot be resolved (same behavior as before) AND a new isConnected property in the metamask_getProviderState request and metamask_chainChanged events (these are different from the events emittted by window.ethereum). isConnected is derived from whether the NetworkController.state.networkMetadata[].status value is the Available constant. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30109?quickstart=1) See: #29936 Providers patch from commit (d919ab6b): MetaMask/providers#404 ``` window.ethereum.on('accountsChanged', (data) => console.log('accountsChanged', data)) window.ethereum.on('chainChanged', (data) => console.log('chainChanged', data)) window.ethereum.on('networkChanged', (data) => console.log('networkChanged', data)) window.ethereum.on('connect', (data) => console.log('connect', data)) window.ethereum.on('disconnect', (data) => console.log('disconnect', data)) ``` 1. Go to a webpage. Enter the following in console 2. Change to Linea, see that the correct values are emitted for the `chainChanged` and `networkChanged` events 3. Change to Sepolia, see that the correct values are emitted for the `chainChanged` and `networkChanged` events 4. Change to a network that is non-responsive, see that `chainChanged` emits with the correct chainId, but `networkChanged` emits with a null value, AND there is a `disconnect` event emitted 5. Change back to a working network, see that the correct values are emitted for the `chainChanged` and `networkChanged` events, AND there is a `connect` event emitted with the new chainId 6. Do the same above with a responsive network that does not have `net_version` implemented. Se that the correct values are emitted for the `chainChanged`, that `networkChanged` emits null, and that there is no `disconnect` event emitted   ![Screenshot 2025-02-04 at 8 34 13 AM](https://github.com/user-attachments/assets/f6558363-f848-445a-811e-d1a052a4e6c0) ![Screenshot 2025-02-04 at 8 39 33 AM](https://github.com/user-attachments/assets/12bff524-bb3c-4bf8-b4d2-c6a01ce63b33) - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.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/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [ ] 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.

…provider (#30111)  Previously the inpage provider would withhold events for chainChanged events (and property value updates, i.e. window.ethereum.chainId and .networkVersion) when the dapp's network was changed to an rpc endpoint that was unresponsive or did not support net_version. The dapp would instead receive a disconnect event. Now the inpage provider always emits chainChanged and networkChanged events (and exposes the correct values on window.ethereum.chainId and .networkVersion) when the selected network for the dapp has changed regardless of if the network being changed to is responsive or if it supports net_version requests. It does this by having the wallet send a loading for networkVersion when it cannot be resolved (same behavior as before) AND a new isConnected property in the metamask_getProviderState request and metamask_chainChanged events (these are different from the events emittted by window.ethereum). isConnected is derived from whether the NetworkController.state.networkMetadata[].status value is the Available constant. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30109?quickstart=1) See: #29936 Providers patch from commit (d919ab6b): MetaMask/providers#404 ``` window.ethereum.on('accountsChanged', (data) => console.log('accountsChanged', data)) window.ethereum.on('chainChanged', (data) => console.log('chainChanged', data)) window.ethereum.on('networkChanged', (data) => console.log('networkChanged', data)) window.ethereum.on('connect', (data) => console.log('connect', data)) window.ethereum.on('disconnect', (data) => console.log('disconnect', data)) ``` 1. Go to a webpage. Enter the following in console 2. Change to Linea, see that the correct values are emitted for the `chainChanged` and `networkChanged` events 3. Change to Sepolia, see that the correct values are emitted for the `chainChanged` and `networkChanged` events 4. Change to a network that is non-responsive, see that `chainChanged` emits with the correct chainId, but `networkChanged` emits with a null value, AND there is a `disconnect` event emitted 5. Change back to a working network, see that the correct values are emitted for the `chainChanged` and `networkChanged` events, AND there is a `connect` event emitted with the new chainId 6. Do the same above with a responsive network that does not have `net_version` implemented. Se that the correct values are emitted for the `chainChanged`, that `networkChanged` emits null, and that there is no `disconnect` event emitted   ![Screenshot 2025-02-04 at 8 34 13 AM](https://github.com/user-attachments/assets/f6558363-f848-445a-811e-d1a052a4e6c0) ![Screenshot 2025-02-04 at 8 39 33 AM](https://github.com/user-attachments/assets/12bff524-bb3c-4bf8-b4d2-c6a01ce63b33) - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.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/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [ ] 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.  ## **Description**  [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30114?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **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/main/.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/main/.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: jiexi <[email protected]> Co-authored-by: MetaMask Bot <[email protected]>

jiexi added 2 commits January 27, 2025 11:17

Allow null networkVersion value

9ea43ee

update specs

2ab37dd

jiexi requested a review from a team as a code owner January 27, 2025 19:34

jiexi added 2 commits January 27, 2025 14:56

add isConnected

bb5168a

jest thresholds

722f5a6

jiexi commented Jan 27, 2025

View reviewed changes

jest.config.js Outdated

Copy link

Contributor Author

jiexi Jan 27, 2025

Choose a reason for hiding this comment

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

try to bump these

jiexi added 2 commits January 27, 2025 15:11

Fix isValidNetworkVersion check

980a980

Fix test mock data

7060911

jiexi changed the title ~~feat: allow networkVersion to be set to null~~ feat: allow networkVersion to be set to null. fire connection events based on new isConnected property value Jan 27, 2025

jiexi added 2 commits January 27, 2025 15:24

bump jest

aea8f8c

bump jest?..

c0af3b9

jiexi mentioned this pull request Jan 28, 2025

feat: Bump @metamask/providers to ^20.0.0 MetaMask/metamask-extension#29936

Open

6 tasks

Gudahtt reviewed Jan 28, 2025

View reviewed changes

src/utils.ts Show resolved Hide resolved

jiexi added 4 commits January 28, 2025 12:41

interpret 'loading' as null

3f24e8b

update utils comment

d9ed19d

update comments

6abb5a6

jest thresholds

44abb41

adonesky1 reviewed Jan 28, 2025

View reviewed changes

src/StreamProvider.test.ts Show resolved Hide resolved

adonesky1 reviewed Jan 28, 2025

View reviewed changes

src/StreamProvider.test.ts Show resolved Hide resolved

adonesky1 approved these changes Jan 28, 2025

View reviewed changes

danjm reviewed Feb 3, 2025

View reviewed changes

adonesky1 requested a review from Gudahtt February 3, 2025 16:08

danjm approved these changes Feb 3, 2025

View reviewed changes

Gudahtt reviewed Feb 3, 2025

View reviewed changes

jiexi merged commit d919ab6 into main Feb 3, 2025
19 checks passed

jiexi deleted the jl/allow-null-networkVersion branch February 3, 2025 18:23

This was referenced Feb 4, 2025

Jl/version v12 10.4/hotfix network version inpage provider MetaMask/metamask-extension#30109

Closed

fix: cp-12.10.4 hotfix network version / unresponsive network inpage provider MetaMask/metamask-extension#30111

Merged

adonesky1 mentioned this pull request Feb 4, 2025

fix: hotfix network version / unresponsive network inpage … MetaMask/metamask-extension#30114

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow `networkVersion` to be set to `null`. fire connection events based on new `isConnected` property value #404

feat: allow `networkVersion` to be set to `null`. fire connection events based on new `isConnected` property value #404

jiexi commented Jan 27, 2025 •

edited

Loading

github-advanced-security bot commented Jan 27, 2025

jiexi Jan 27, 2025

adonesky1 left a comment

danjm Feb 3, 2025

jiexi Feb 3, 2025

danjm left a comment

Gudahtt Feb 3, 2025

jiexi Feb 3, 2025

Gudahtt Feb 3, 2025

jiexi Feb 3, 2025

feat: allow networkVersion to be set to null. fire connection events based on new isConnected property value #404

feat: allow networkVersion to be set to null. fire connection events based on new isConnected property value #404

Conversation

jiexi commented Jan 27, 2025 • edited Loading

github-advanced-security bot commented Jan 27, 2025

jiexi Jan 27, 2025

Choose a reason for hiding this comment

adonesky1 left a comment

Choose a reason for hiding this comment

danjm Feb 3, 2025

Choose a reason for hiding this comment

jiexi Feb 3, 2025

Choose a reason for hiding this comment

danjm left a comment

Choose a reason for hiding this comment

Gudahtt Feb 3, 2025

Choose a reason for hiding this comment

jiexi Feb 3, 2025

Choose a reason for hiding this comment

Gudahtt Feb 3, 2025

Choose a reason for hiding this comment

jiexi Feb 3, 2025

Choose a reason for hiding this comment

feat: allow `networkVersion` to be set to `null`. fire connection events based on new `isConnected` property value #404

feat: allow `networkVersion` to be set to `null`. fire connection events based on new `isConnected` property value #404

jiexi commented Jan 27, 2025 •

edited

Loading