1
0
mirror of https://github.com/kremalicious/metamask-extension.git synced 2024-11-28 23:06:37 +01:00
metamask-extension/app/scripts/migrations/077-supplements/077-supplements.md
Dan J Miller 3b0d37c3bf
Fix migration 77 (#20276)
* Handle the case where tokensChainsCache data is undefined in migration 77

* Delete parts of state that should have been removed in migrations 82,84,86 and 88

* Create 077-supplements.md

* Update 077-supplements.md

* Update 077-supplements/*.js code comments

* Fix types and jsdoc

* Type/lint fix

* Cleanup

* Add 'should set data to an empty object if it is null' test case to 077.test.js

* Update app/scripts/migrations/077.test.js

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

* Modify deletion criteria so that all decimal chain id proprties are deleted in migration 88 supplement

* Readme.md

* Update app/scripts/migrations/077.test.js

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

* Update app/scripts/migrations/077.test.js

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

* Update app/scripts/migrations/077.test.js

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

* Lint fix

* Only delete decimal chain id keyed-entries in migration 88 supplement if there are hexadecimal keyed entries as well

* Remove redundant test

* Add 'does not delete' cases for nftcontroller related tests in 077.test.js

---------

Co-authored-by: Mark Stacey <markjstacey@gmail.com>
2023-07-31 19:43:51 -02:30

8.7 KiB

077 Supplements

As of the time this file was first PR'd, we had not yet had to do what was done in this PR, which is to fix an old migration and also supplement it with state transformations to handle problems that could be introduced by future migrations.

The document explains the need for these new state transformations and the rationale behind each. It also explains why other state transformations were not included.

Background

As of release 10.34.0, we started having a No metadata found for 'previousProviderStore' error thrown from the deriveStateFromMetadata function in BaseControllerV2.js. This was occuring when there was data on the NetworkController state for which the NetworkController + BaseController expect metadata, but no metadata exists. In particular, previousProviderStore was on the NetworkController state when it should not have been.

previousProviderStore should not have been on the NetworkController state because of migration 85, which explictly deletes it.

We discovered that for some users, that migration had failed to run because of an error in an earlier migration: TypeError#1: MetaMask Migration Error #77: Cannot convert undefined or null to object. This error was thrown from this line 8f18e04b97 (diff-9e76a7c60c1e37cd949f729222338b23ab743e44938ccf63a4a6dab7d84ed8bcR38)

So the data property of the objects within TokenListController.tokensChainsCache could be undefined, and migration 77 didn't handle that case. It could be undefined because of the way the assets controller code was as of the core controller libraries 14.0.2 release 19f7bf3b9f/src/assets/TokenListController.ts (L270) (the safelyExecute call there will return undefined if the network call failed)

For users who were in that situation, where a TokenListController.tokensChainsCache[chainId].data property was undefined, some significant problems would occur after updating to v10.24.0, which is the release where migration 77 was shipped. In particular, migration 77 would fail, and all subsequent migrations would not run. The most plain case of this would be a user who was on v10.23.0 with TokenListController.tokensChainsCache[chainId].data === undefined. Then suppose they didn't update until v10.34.0. None of migrations 77-89 would run. Leaving their state in a form that doesn't match with assumptions throughout our codebase. Various bugs could be caused, but the sentry error describe above is the worst case, where MetaMask simply could not be opened and users would hit the error screen.

To correct this situation we had to fix migration 77. Once we do that, all users who were in this situation (and then upgraded to the version which included the fixes for migration 77) would have all migrations from 77 upwards run for the first time. This could be problematic for users who had used the wallet on versions 10.24.0-10.34.0, where our controllers would be writing data to state under the assumption that the migrations had run.

As such, we had to also add code to migration 77 to avoid new errors being introduced by the migrations running on code that had been modified by controllers on versions 10.24.0 to 10.34.0

Introducing migration 77 supplements

To correct the aforementioned problems with the data, new state transformation functions were added to this directory, to be run in migration 77 after the pre-existing migration 77 code had run. Each file in this directory exports a state transformation function which is then imported in the migration 77 file and applied to state in sequence, after the state transformation function in 077.js itself has run and returns state. These have been split into their own files for each of use, and so that they could be grouped with this documentation.

The chosen supplements

We added supplements for migrations 82, 84, 86 and 88 for the following reasons and with the following effects ->

Migration 82

Attempts to convert frequentRpcListDetail - an array of objects with network related data - to a networkConfigurations object, with the objects that were in the array keyed by a unique id. If this migration had not run, then (prior to v10.34.0) a user would still have been able to use MetaMask, but the data they had in frequentRpcListDetail would now be ignored by application code, and subsequent network data would between written to and modified in state in the networkConfigurations object. If migration 82 was later run (after fixing migration 77), the old frequentRpcListDetail data could overwrite the existing networkConfigurations data, and the user could lose networkConfigurations data that had been written to their state since migration 82 had first failed to run.

To fix this, the migration 82 supplement deletes frequentRpcListDetail if the networkConfigurations object exists. Users in such a scenario will have network data in networkConfigurations that they have been using, while the frequentRpcListDetail data would not have been seen for some time. So the best thing to do for them is delete their old data and preserve the data they have most recently used.

Migration 84

Replaces the NetworkController.network property with a networkId and networkStatus property. If this migration had not run, the NetworkController would have a network property and networkId and networkStatus properties. If migration 84 later ran on this state, the old (and forgotten) network property could cause the networkId and networkStatus to be overwritten, affecting the state the user's experience was currently depending on.

The fix in the migration 84 supplement is to delete the NetworkController.network property if the NetworkId property already exists.

Migration 86

Renamed the NetworkController.provider property to providerConfig. If this migration had not run, the NetworkController would have a provider property and a providerConfig property. If migration 86 later ran on this state, the old (and forgotten) provider property could cause the providerConfig property to be overwritten, affecting the state the user's experience was currently depending on.

The fix in the migration 86 supplement is to delete the NetworkController.provider property if the providerConfig property already exists.

Migration 88

Attempted to change the keys of multiple parts of state related to tokens. In particular, NftController.allNftContracts, NftController.allNfts, TokenListController.tokensChainsCache, TokensController.allTokens, TokensController.allIgnoredTokens and TokensController.allDetectedTokens. All of these objects were keyed by chainId in decimal number form. The migration's purpose was to change those decimal chain ID keys to hexadecimal. If migration 77 failed, and then the user added or modified tokens, they could have duplicates within these parts of state: some with decimal keys and others with an equivalent hexadecimal key. If the data pointed to by those keys was modified at all, and the migration 88 was later run, the most recent data (under the hexadecimal key) could be overwritten by the old data under the decimal key.

The migration 88 supplement fixes this by deleting the properties with decimal keys if an equivalent hexadecimal key exists.

Migrations that were not supplemented

Migration 78 was not supplemented because it only deletes data; it does not overwrite data. It's failure to run will have left rogue data in state, but that will be removed when it is run after the migration 77 fix.

Migration 79 was not supplemented because it only deletes data; it does not overwrite data.

Migration 80 was not supplemented because it only deletes data; it does not overwrite data.

Migration 81 was not supplemented because it modifies data that could only be in state on a flask build. The bug that caused the undefined data in tokenlistcontroller state was present on v14.0.2 and v14.1.0 of the controllers, but fixed in v14.2.0 of the controllers. By the time flask was released to prod, controllers was at v25.0

Migration 83 just builds on migration 82. No additional fix is needed for 83 given that we have the 82 supplement.

Migration 85 was not supplemented because it only deletes data; it does not overwrite data.

Migration 87 was not supplemented because it only deletes data; it does not overwrite data.

Migration 89 just builds on migration 82 and 84. No additional fix is needed for 89 given that we have the 82 and 84 supplement.

Migration 90 was not supplemented because it only deletes data; it does not overwrite data.