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

Error: No metadata found for 'networkStatus' #26297

Closed
sentry-io bot opened this issue Aug 1, 2024 · 5 comments · Fixed by #26302
Closed

Error: No metadata found for 'networkStatus' #26297

sentry-io bot opened this issue Aug 1, 2024 · 5 comments · Fixed by #26302
Labels
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. team-extension-platform type-bug

Comments

@sentry-io
Copy link

sentry-io bot commented Aug 1, 2024

Sentry Issue: METAMASK-X636

Error: No metadata found for 'networkStatus'
  at Array.reduce (<anonymous>)
  at handler (app/scripts/lib/ComposableObservableStore.js:70:30)
  at <object>.setProviderType (app/scripts/metamask-controller.js:4848:41)
  at switchEthereumChain.implementation (app/scripts/lib/rpc-method-middleware/handlers/switch-ethereum-chain.js:148:15)
  at <anonymous> (app/scripts/lib/rpc-method-middleware/createMethodMiddleware.js:55:9)
...
(8 additional frame(s) were not displayed)
Gudahtt added a commit that referenced this issue Aug 1, 2024
Migration 120.2 has been refactored to prepare for the addition of
further obsolete state cleanup steps. A few minor functional changes
and test changes have been made as well.

The changes are:
* The migration has been refactored to group together changes for each
controller in separate functions.
* When the `SnapController` state is invalid, we now capture a Sentry
exception
  * A test has been added for this case as well
* Tests have been grouped together by controller
* Tests have been added to ensure corrupted controller state does not
impact the obsolete state removal steps of other controllers
  * Effectively each obsolete state removal function is acting as an
independent migration here
* The tests have been updated to deep clone the input to guard against
the migration failing to do so

Relates to #26297
Gudahtt added a commit that referenced this issue Aug 1, 2024
Migration 120.2 has been refactored to prepare for the addition of
further obsolete state cleanup steps. A few minor functional changes
and test changes have been made as well.

The changes are:
* The migration has been refactored to group together changes for each
controller in separate functions.
* When the `SnapController` state is invalid, we now capture a Sentry
exception
  * A test has been added for this case as well
* Tests have been grouped together by controller
* Tests have been added to ensure corrupted controller state does not
impact the obsolete state removal steps of other controllers
  * Effectively each obsolete state removal function is acting as an
independent migration here
* The tests have been updated to deep clone the input to guard against
the migration failing to do so
* The `transformState` function was given an explicit return type, and
the unused return value was removed.

Relates to #26297
Gudahtt added a commit that referenced this issue Aug 1, 2024
Migration 120.2 has been refactored to prepare for the addition of
further obsolete state cleanup steps. A few minor functional changes
and test changes have been made as well.

The changes are:
* The migration has been refactored to group together changes for each
controller in separate functions.
* When the `SnapController` state is invalid, we now capture a Sentry
exception
  * A test has been added for this case as well
* Tests have been grouped together by controller
* Tests have been added to ensure corrupted controller state does not
impact the obsolete state removal steps of other controllers
  * Effectively each obsolete state removal function is acting as an
independent migration here
* A test was added for the case where `SnapController` state is not set
* The tests have been updated to deep clone the input to guard against
the migration failing to do so
* The `transformState` function was given an explicit return type, and
the unused return value was removed.

Relates to #26297
Copy link
Author

sentry-io bot commented Aug 1, 2024

Sentry Issue: METAMASK-X7XG

Copy link
Author

sentry-io bot commented Aug 1, 2024

Sentry Issue: METAMASK-XA2H

Copy link
Author

sentry-io bot commented Aug 1, 2024

Sentry Issue: METAMASK-XAK2

Gudahtt added a commit that referenced this issue Aug 1, 2024
Migration 120.2 has been updated to remove obsolete NetworkController
state as well.

Fixes #26297
@Gudahtt Gudahtt added type-bug Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. team-extension-platform labels Aug 1, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Aug 1, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Aug 1, 2024
Gudahtt added a commit that referenced this issue Aug 2, 2024
## **Description**

Migration 120.2 has been refactored to prepare for the addition of
further obsolete state cleanup steps. A few minor functional changes and
test changes have been made as well.

The changes are:
* The migration has been refactored to group together changes for each
controller in separate functions.
* The `SelectedNetworkController` migration has been updated to delete
state rather than set it to default. This is functionally equivalent
(for this controller) and it simplified the migration and tests a bit,
avoiding the need to verify that the default state was correct.
* When the `SnapController` state is invalid, we now capture a Sentry
exception
  * A test has been added for this case as well
* JSDoc comments have been added to the migration
* Tests have been grouped together by controller
* Tests have been added to ensure corrupted controller state does not
impact the obsolete state removal steps of other controllers
* Effectively each obsolete state removal function is acting as an
independent migration here
* A test was added for the case where `SnapController` state is not set
* The tests have been updated to deep clone the input to guard against
the migration failing to do so
* The `transformState` function was given an explicit return type, and
the unused return value was removed.

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

## **Related issues**

Relates to #26297

## **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
Migration 120.2 has been updated to remove obsolete NetworkController
state as well.

Fixes #26297
Gudahtt added a commit that referenced this issue Aug 2, 2024
Migration 120.2 has been updated to remove obsolete NetworkController
state as well.

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

Migration 120.2 has been updated to remove obsolete NetworkController
state as well.

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

## **Related issues**

Fixes #26297

## **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.
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by severity 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
@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
## **Description**

Migration 120.2 has been refactored to prepare for the addition of
further obsolete state cleanup steps. A few minor functional changes and
test changes have been made as well.

The changes are:
* The migration has been refactored to group together changes for each
controller in separate functions.
* The `SelectedNetworkController` migration has been updated to delete
state rather than set it to default. This is functionally equivalent
(for this controller) and it simplified the migration and tests a bit,
avoiding the need to verify that the default state was correct.
* When the `SnapController` state is invalid, we now capture a Sentry
exception
  * A test has been added for this case as well
* JSDoc comments have been added to the migration
* Tests have been grouped together by controller
* Tests have been added to ensure corrupted controller state does not
impact the obsolete state removal steps of other controllers
* Effectively each obsolete state removal function is acting as an
independent migration here
* A test was added for the case where `SnapController` state is not set
* The tests have been updated to deep clone the input to guard against
the migration failing to do so
* The `transformState` function was given an explicit return type, and
the unused return value was removed.

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

## **Related issues**

Relates to #26297

## **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**

Migration 120.2 has been updated to remove obsolete NetworkController
state as well.

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

## **Related issues**

Fixes #26297

## **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
## **Description**

Migration 120.2 has been refactored to prepare for the addition of
further obsolete state cleanup steps. A few minor functional changes and
test changes have been made as well.

The changes are:
* The migration has been refactored to group together changes for each
controller in separate functions.
* The `SelectedNetworkController` migration has been updated to delete
state rather than set it to default. This is functionally equivalent
(for this controller) and it simplified the migration and tests a bit,
avoiding the need to verify that the default state was correct.
* When the `SnapController` state is invalid, we now capture a Sentry
exception
  * A test has been added for this case as well
* JSDoc comments have been added to the migration
* Tests have been grouped together by controller
* Tests have been added to ensure corrupted controller state does not
impact the obsolete state removal steps of other controllers
* Effectively each obsolete state removal function is acting as an
independent migration here
* A test was added for the case where `SnapController` state is not set
* The tests have been updated to deep clone the input to guard against
the migration failing to do so
* The `transformState` function was given an explicit return type, and
the unused return value was removed.

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

## **Related issues**

Relates to #26297

## **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
## **Description**

Migration 120.2 has been updated to remove obsolete NetworkController
state as well.

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

## **Related issues**

Fixes #26297

## **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.
@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
@Gudahtt
Copy link
Member

Gudahtt commented Oct 7, 2024

@benjisclowder

  1. What PR fixed the issue?

#26302

  1. Can you pinpoint the commit from which the issue originated?

It's not clear. This is one of many recent incidents where some obsolete state was found in production, despite there being no evidence of a migration failure.

We had some theories about how this may have happened, e.g. perhaps the browser or filesystem encountered an error partway through the operation of saving state after an update, leaving state partially outdated. But we don't have any evidence that would suggest a root cause here. The number of affected users appears small though.

  1. Write a short explanation of the technical cause of the bug

Unknown, see answer to 2

  1. 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?

None in particular, no

4.2. Were there any other elements lacking, such as typed code, comprehensive documentation, well-architected APIs, etc., that might have prevented this issue?

Our migration state validation is very much written by hand, and is verbose and error prone.

As far as we could tell, this was not a factor in this problem. But it's related at least, it would have simplified the investigation if we had better migration tooling that we could trust.

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?

  • Better Sentry monitoring+triage
  • Better visibility into Sentry errors (i.e. reduce Sentry volume until rate-limited errors are not a significant percentage)
  • Improved utilities for writing and testing migrations would have increased confidence that they worked correctly in the first place
  • Better Sentry diagnostic information, so we can more effectively rule out edge cases like development installations or forks (this has since been introduced)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. team-extension-platform type-bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants