Skip to content
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.

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

[Sentry] TypeError: Cannot read properties of undefined (reading 'id') #26320

Closed
sentry-io bot opened this issue Aug 2, 2024 · 2 comments · Fixed by #26327
Closed

[Sentry] TypeError: Cannot read properties of undefined (reading 'id') #26320

sentry-io bot opened this issue Aug 2, 2024 · 2 comments · Fixed by #26327
Labels
area-Sentry error reporting to sentry regression-prod-12.0.0 Regression bug that was found in production in release 12.0.0 release-12.3.0 Issue or pull request that will be included in release 12.3.0 Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. type-bug

Comments

@sentry-io
Copy link

sentry-io bot commented Aug 2, 2024

Sentry Issue: METAMASK-XB8P

TypeError: Cannot read properties of undefined (reading 'id')
  at r.getNetworkToAutomaticallySwitchTo (ui/selectors/selectors.js:1861:22)
  at <anonymous> (ui/index.js:185:31)
@Gudahtt Gudahtt added regression-prod-12.0.0 Regression bug that was found in production in release 12.0.0 type-bug labels Aug 2, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Aug 2, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Aug 2, 2024
@Gudahtt Gudahtt added Sev2-normal Normal severity; minor loss of service or inconvenience. Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. and removed Sev2-normal Normal severity; minor loss of service or inconvenience. labels Aug 2, 2024
@Gudahtt
Copy link
Member

Gudahtt commented Aug 2, 2024

The stack trace seems to suggest that this is a UI crash, so labelling as sev1 for now, but I haven't confirmed that impact yet.

Gudahtt added a commit that referenced this issue Aug 2, 2024
…sing

The `providerConfig` state is not guaranteed to match a built-in or custom
network. It should in the vast majority of cases, but there are some edge
cases where that would not hold true. For example:
* If the wallet crashed partway through removing the current network
* If the user has had the same network selected since before we
introduced the `networkConfigurations` state

The `currentNetwork` selector has been updated to handle this case by
returning a configuration object constructed from the `providerConfig`
state directly.

Fixes #26320
Gudahtt added a commit that referenced this issue Aug 2, 2024
…sing

The `providerConfig` state is not guaranteed to match a built-in or custom
network. It should in the vast majority of cases, but there are some edge
cases where that would not hold true. For example:
* If the wallet crashed partway through removing the current network
* If the user has had the same network selected since before we
introduced the `networkConfigurations` state

The `currentNetwork` selector has been updated to handle this case by
returning a configuration object constructed from the `providerConfig`
state directly.

Fixes #26320
Gudahtt added a commit that referenced this issue Aug 2, 2024
…sing

The `providerConfig` state is not guaranteed to match a built-in or custom
network. It should in the vast majority of cases, but there are some edge
cases where that would not hold true. For example:
* If the wallet crashed partway through removing the current network
* If the user has had the same network selected since before we
introduced the `networkConfigurations` state

The `currentNetwork` selector has been updated to handle this case by
returning a configuration object constructed from the `providerConfig`
state directly.

Fixes #26320
Gudahtt added a commit that referenced this issue Aug 2, 2024
## **Description**

Remove invalid `providerConfig.id` state. This was one possible
explanation for some Sentry errors that we have been encountering in
production. The diagnostic state attached to the error was not
sufficient to confirm that this was the cause.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26310?quickstart=1)

## **Related issues**

Relates to #26309 and #26320

## **Manual testing steps**

N/A

## **Screenshots/Recordings**

N/A

## **Pre-merge author checklist**

- [x] 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).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.
Gudahtt added a commit that referenced this issue Aug 2, 2024
## **Description**

Remove invalid `providerConfig.id` state. This was one possible
explanation for some Sentry errors that we have been encountering in
production. The diagnostic state attached to the error was not
sufficient to confirm that this was the cause.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26310?quickstart=1)

## **Related issues**

Relates to #26309 and #26320

## **Manual testing steps**

N/A

## **Screenshots/Recordings**

N/A

## **Pre-merge author checklist**

- [x] 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).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.
@Gudahtt Gudahtt closed this as completed in e5651cf Aug 2, 2024
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by team Aug 2, 2024
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by severity Aug 2, 2024
@metamaskbot metamaskbot added the release-12.4.0 Issue or pull request that will be included in release 12.4.0 label Aug 2, 2024
Gudahtt added a commit that referenced this issue Aug 2, 2024
…sing (#26327)

The `providerConfig` state is not guaranteed to match a built-in or
custom network. It should in the vast majority of cases, but there are
some edge cases where that would not hold true. For example:
* If the wallet crashed partway through removing the current network
* If the user has had the same network selected since before we
introduced the `networkConfigurations` state

The `currentNetwork` selector has been updated to handle this case by
returning a configuration object constructed from the `providerConfig`
state directly.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26327?quickstart=1)

Fixes #26320

* Create a dev build and proceed through onboarding
* Run this command in the background console to set `providerConfig` to
a custom network that isn't present in the user's tracked network
configurations:
  ```
  chrome.storage.local.get(
    null,
    (state) => {
      state.data.NetworkController.providerConfig = {
        rpcUrl: 'https://mainnet.optimism.io',
        chainId: '0xa',
        ticker: 'ETH',
        type: 'rpc',
      };
      chrome.storage.local.set(state, () => chrome.runtime.reload());
    }
  );
  ```
* See that the UI can be opened and does not crash.

N/A

- [x] 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).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.

- [ ] 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.
Gudahtt added a commit that referenced this issue Aug 2, 2024
…sing (#26327)

The `providerConfig` state is not guaranteed to match a built-in or
custom network. It should in the vast majority of cases, but there are
some edge cases where that would not hold true. For example:
* If the wallet crashed partway through removing the current network
* If the user has had the same network selected since before we
introduced the `networkConfigurations` state

The `currentNetwork` selector has been updated to handle this case by
returning a configuration object constructed from the `providerConfig`
state directly.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26327?quickstart=1)

Fixes #26320

* Create a dev build and proceed through onboarding
* Run this command in the background console to set `providerConfig` to
a custom network that isn't present in the user's tracked network
configurations:
  ```
  chrome.storage.local.get(
    null,
    (state) => {
      state.data.NetworkController.providerConfig = {
        rpcUrl: 'https://mainnet.optimism.io',
        chainId: '0xa',
        ticker: 'ETH',
        type: 'rpc',
      };
      chrome.storage.local.set(state, () => chrome.runtime.reload());
    }
  );
  ```
* See that the UI can be opened and does not crash.

N/A

- [x] 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).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.

- [ ] 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.
@metamaskbot metamaskbot changed the title TypeError: Cannot read properties of undefined (reading 'id') [Sentry] TypeError: Cannot read properties of undefined (reading 'id') Aug 5, 2024
@metamaskbot metamaskbot added the area-Sentry error reporting to sentry label Aug 5, 2024
Gudahtt added a commit that referenced this issue Aug 5, 2024
…sing (#26327)

The `providerConfig` state is not guaranteed to match a built-in or
custom network. It should in the vast majority of cases, but there are
some edge cases where that would not hold true. For example:
* If the wallet crashed partway through removing the current network
* If the user has had the same network selected since before we
introduced the `networkConfigurations` state

The `currentNetwork` selector has been updated to handle this case by
returning a configuration object constructed from the `providerConfig`
state directly.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26327?quickstart=1)

Fixes #26320

* Create a dev build and proceed through onboarding
* Run this command in the background console to set `providerConfig` to
a custom network that isn't present in the user's tracked network
configurations:
  ```
  chrome.storage.local.get(
    null,
    (state) => {
      state.data.NetworkController.providerConfig = {
        rpcUrl: 'https://mainnet.optimism.io',
        chainId: '0xa',
        ticker: 'ETH',
        type: 'rpc',
      };
      chrome.storage.local.set(state, () => chrome.runtime.reload());
    }
  );
  ```
* See that the UI can be opened and does not crash.

N/A

- [x] 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).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.

- [ ] 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.
danjm pushed a commit that referenced this issue Aug 5, 2024
…sing (#26327) (#26331)

This is a cherry-pick of #26327 for v12.0.1. Original description below:

## **Description**

The `providerConfig` state is not guaranteed to match a built-in or
custom network. It should in the vast majority of cases, but there are
some edge cases where that would not hold true. For example:
* If the wallet crashed partway through removing the current network
* If the user has had the same network selected since before we
introduced the `networkConfigurations` state

The `currentNetwork` selector has been updated to handle this case by
returning a configuration object constructed from the `providerConfig`
state directly.

[![Open in GitHub

Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26327?quickstart=1)

## **Related issues**

Fixes #26320

## **Manual testing steps**

* Create a dev build and proceed through onboarding
* Run this command in the background console to set `providerConfig` to
a custom network that isn't present in the user's tracked network
configurations:
  ```
  chrome.storage.local.get(
    null,
    (state) => {
      state.data.NetworkController.providerConfig = {
        rpcUrl: 'https://mainnet.optimism.io',
        chainId: '0xa',
        ticker: 'ETH',
        type: 'rpc',
      };
      chrome.storage.local.set(state, () => chrome.runtime.reload());
    }
  );
  ```
* See that the UI can be opened and does not crash.

## **Screenshots/Recordings**

N/A

## **Pre-merge author checklist**

- [x] 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).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.
dawnseeker8 pushed a commit that referenced this issue Aug 12, 2024
## **Description**

Remove invalid `providerConfig.id` state. This was one possible
explanation for some Sentry errors that we have been encountering in
production. The diagnostic state attached to the error was not
sufficient to confirm that this was the cause.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26310?quickstart=1)

## **Related issues**

Relates to #26309 and #26320

## **Manual testing steps**

N/A

## **Screenshots/Recordings**

N/A

## **Pre-merge author checklist**

- [x] 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).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.
dawnseeker8 pushed a commit that referenced this issue Aug 12, 2024
…sing (#26327)

## **Description**

The `providerConfig` state is not guaranteed to match a built-in or
custom network. It should in the vast majority of cases, but there are
some edge cases where that would not hold true. For example:
* If the wallet crashed partway through removing the current network
* If the user has had the same network selected since before we
introduced the `networkConfigurations` state

The `currentNetwork` selector has been updated to handle this case by
returning a configuration object constructed from the `providerConfig`
state directly.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26327?quickstart=1)

## **Related issues**

Fixes #26320

## **Manual testing steps**

* Create a dev build and proceed through onboarding
* Run this command in the background console to set `providerConfig` to
a custom network that isn't present in the user's tracked network
configurations:
  ```
  chrome.storage.local.get(
    null,
    (state) => {
      state.data.NetworkController.providerConfig = {
        rpcUrl: 'https://mainnet.optimism.io',
        chainId: '0xa',
        ticker: 'ETH',
        type: 'rpc',
      };
      chrome.storage.local.set(state, () => chrome.runtime.reload());
    }
  );
  ```
* See that the UI can be opened and does not crash.

## **Screenshots/Recordings**

N/A

## **Pre-merge author checklist**

- [x] 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).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.
@benjisclowder
Copy link
Contributor

Collaborative Effort Required for Root Cause Analysis (RCA) on Critical Issues

We are quickly approaching the end of the quarter and we encourage you once again to take some moments and perform RCA on this critical issue. You may do so by answering the questions below:

1. What PR fixed the issue?
2. Can you pinpoint the commit from which the issue originated?
3. Write a short explanation of the technical cause of the bug
4. How could we have avoided merging this bug? What would have had to be different about our code, tests or processes?

4.1. Were there any missing unit, e2e or manual tests that could have preempted this issue?
4.2. Were there any other elements lacking, such as typed code, comprehensive documentation, well-architected APIs, etc., that might have prevented this issue?
4.3. If your answer to a and b is no, then is there anything at all that you can think of that, if it had been different before this bug was introduced, would have prevented it from being merged?

Please provide your answers as a reply to this comment and tag me as well.

You can read more about the initiative here. Thank you!

Tagging eng. who added the fix: @Gudahtt

@gauthierpetetin gauthierpetetin added release-12.3.0 Issue or pull request that will be included in release 12.3.0 and removed release-12.4.0 Issue or pull request that will be included in release 12.4.0 labels Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Sentry error reporting to sentry regression-prod-12.0.0 Regression bug that was found in production in release 12.0.0 release-12.3.0 Issue or pull request that will be included in release 12.3.0 Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. type-bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants