Fix migration 88 to handle the case where chainId keys can be undefined (#20345)

* Fix migration 88 to handle the case where chainId keys can be undefined

* Add migration 91 to delete network configurations that have no chainId

* Lint fix

* Update migration number

* Update migration 91 description

* Update version numbers in 091.test.js

* Fix 088.test.ts typescript problem

* Fix 088.test.ts typescript problem

* Update app/scripts/migrations/091.ts

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

* Change app/scripts/migrations/091.test.js to typescript

* clone oldstorage for test result comparisons in 091.test.js

* Lint fix

* Add missing test case

---------

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

app/scripts/migrations/088.test.ts

@@ -156,6 +156,65 @@ describe('migration #88', () => {
     });
   });
 
+  it('deletes undefined-keyed properties from state of NftController.allNftContracts', async () => {
+    const oldStorage = {
+      meta: { version: 87 },
+      data: {
+        NftController: {
+          allNftContracts: {
+            '0x111': {
+              '16': [
+                {
+                  name: 'Contract 1',
+                  address: '0xaaa',
+                },
+              ],
+              undefined: [
+                {
+                  name: 'Contract 2',
+                  address: '0xbbb',
+                },
+              ],
+            },
+            '0x222': {
+              '64': [
+                {
+                  name: 'Contract 3',
+                  address: '0xccc',
+                },
+              ],
+            },
+          },
+        },
+      },
+    };
+
+    const newStorage = await migrate(oldStorage);
+
+    expect(newStorage.data).toStrictEqual({
+      NftController: {
+        allNftContracts: {
+          '0x111': {
+            '0x10': [
+              {
+                name: 'Contract 1',
+                address: '0xaaa',
+              },
+            ],
+          },
+          '0x222': {
+            '0x40': [
+              {
+                name: 'Contract 3',
+                address: '0xccc',
+              },
+            ],
+          },
+        },
+      },
+    });
+  });
+
   it('does not convert chain IDs in NftController.allNftContracts which are already hex strings', async () => {
     const oldStorage = {
       meta: { version: 87 },
@@ -395,6 +454,85 @@ describe('migration #88', () => {
     });
   });
 
+  it('deletes undefined-keyed properties from state of NftController.allNfts', async () => {
+    const oldStorage = {
+      meta: { version: 87 },
+      data: {
+        NftController: {
+          allNfts: {
+            '0x111': {
+              '16': [
+                {
+                  name: 'NFT 1',
+                  description: 'Description for NFT 1',
+                  image: 'nft1.jpg',
+                  standard: 'ERC721',
+                  tokenId: '1',
+                  address: '0xaaa',
+                },
+              ],
+              undefined: [
+                {
+                  name: 'NFT 2',
+                  description: 'Description for NFT 2',
+                  image: 'nft2.jpg',
+                  standard: 'ERC721',
+                  tokenId: '2',
+                  address: '0xbbb',
+                },
+              ],
+            },
+            '0x222': {
+              '64': [
+                {
+                  name: 'NFT 3',
+                  description: 'Description for NFT 3',
+                  image: 'nft3.jpg',
+                  standard: 'ERC721',
+                  tokenId: '3',
+                  address: '0xccc',
+                },
+              ],
+            },
+          },
+        },
+      },
+    };
+
+    const newStorage = await migrate(oldStorage);
+
+    expect(newStorage.data).toStrictEqual({
+      NftController: {
+        allNfts: {
+          '0x111': {
+            '0x10': [
+              {
+                name: 'NFT 1',
+                description: 'Description for NFT 1',
+                image: 'nft1.jpg',
+                standard: 'ERC721',
+                tokenId: '1',
+                address: '0xaaa',
+              },
+            ],
+          },
+          '0x222': {
+            '0x40': [
+              {
+                name: 'NFT 3',
+                description: 'Description for NFT 3',
+                image: 'nft3.jpg',
+                standard: 'ERC721',
+                tokenId: '3',
+                address: '0xccc',
+              },
+            ],
+          },
+        },
+      },
+    });
+  });
+
   it('does not convert chain IDs in NftController.allNfts which are already hex strings', async () => {
     const oldStorage = {
       meta: { version: 87 },
@@ -627,6 +765,69 @@ describe('migration #88', () => {
     });
   });
 
+  it('deletes undefined-keyed properties from state of TokenListController.tokensChainsCache', async () => {
+    const oldStorage = {
+      meta: { version: 87 },
+      data: {
+        TokenListController: {
+          tokensChainsCache: {
+            '16': {
+              timestamp: 111111,
+              data: {
+                '0x111': {
+                  address: '0x111',
+                  symbol: 'TEST1',
+                  decimals: 1,
+                  occurrences: 1,
+                  name: 'Token 1',
+                  iconUrl: 'https://url/to/token1.png',
+                  aggregators: [],
+                },
+              },
+            },
+            undefined: {
+              timestamp: 222222,
+              data: {
+                '0x222': {
+                  address: '0x222',
+                  symbol: 'TEST2',
+                  decimals: 1,
+                  occurrences: 1,
+                  name: 'Token 2',
+                  iconUrl: 'https://url/to/token2.png',
+                  aggregators: [],
+                },
+              },
+            },
+          },
+        },
+      },
+    };
+
+    const newStorage = await migrate(oldStorage);
+
+    expect(newStorage.data).toStrictEqual({
+      TokenListController: {
+        tokensChainsCache: {
+          '0x10': {
+            timestamp: 111111,
+            data: {
+              '0x111': {
+                address: '0x111',
+                symbol: 'TEST1',
+                decimals: 1,
+                occurrences: 1,
+                name: 'Token 1',
+                iconUrl: 'https://url/to/token1.png',
+                aggregators: [],
+              },
+            },
+          },
+        },
+      },
+    });
+  });
+
   it('does not convert chain IDs in TokenListController.tokensChainsCache which are already hex strings', async () => {
     const oldStorage = {
       meta: { version: 87 },
@@ -807,6 +1008,72 @@ describe('migration #88', () => {
     });
   });
 
+  it('deletes undefined keyed properties from TokensController.allTokens', async () => {
+    const oldStorage = {
+      meta: { version: 87 },
+      data: {
+        TokensController: {
+          allTokens: {
+            '16': {
+              '0x111': [
+                {
+                  address: '0xaaa',
+                  decimals: 1,
+                  symbol: 'TEST1',
+                },
+              ],
+            },
+            '32': {
+              '0x222': [
+                {
+                  address: '0xbbb',
+                  decimals: 1,
+                  symbol: 'TEST2',
+                },
+              ],
+            },
+            undefined: {
+              '0x333': [
+                {
+                  address: '0xbbb',
+                  decimals: 1,
+                  symbol: 'TEST2',
+                },
+              ],
+            },
+          },
+        },
+      },
+    };
+
+    const newStorage = await migrate(oldStorage);
+
+    expect(newStorage.data).toStrictEqual({
+      TokensController: {
+        allTokens: {
+          '0x10': {
+            '0x111': [
+              {
+                address: '0xaaa',
+                decimals: 1,
+                symbol: 'TEST1',
+              },
+            ],
+          },
+          '0x20': {
+            '0x222': [
+              {
+                address: '0xbbb',
+                decimals: 1,
+                symbol: 'TEST2',
+              },
+            ],
+          },
+        },
+      },
+    });
+  });
+
   it('does not convert chain IDs in TokensController.allTokens which are already hex strings', async () => {
     const oldStorage = {
       meta: { version: 87 },
@@ -937,6 +1204,52 @@ describe('migration #88', () => {
     });
   });
 
+  it('deletes undefined-keyed properties from TokensController.allIgnoredTokens', async () => {
+    const oldStorage = {
+      meta: { version: 87 },
+      data: {
+        TokensController: {
+          allIgnoredTokens: {
+            '16': {
+              '0x1': {
+                '0x111': ['0xaaa'],
+              },
+            },
+            '32': {
+              '0x2': {
+                '0x222': ['0xbbb'],
+              },
+            },
+            undefined: {
+              '0x2': {
+                '0x222': ['0xbbb'],
+              },
+            },
+          },
+        },
+      },
+    };
+
+    const newStorage = await migrate(oldStorage);
+
+    expect(newStorage.data).toStrictEqual({
+      TokensController: {
+        allIgnoredTokens: {
+          '0x10': {
+            '0x1': {
+              '0x111': ['0xaaa'],
+            },
+          },
+          '0x20': {
+            '0x2': {
+              '0x222': ['0xbbb'],
+            },
+          },
+        },
+      },
+    });
+  });
+
   it('does not convert chain IDs in TokensController.allIgnoredTokens which are already hex strings', async () => {
     const oldStorage = {
       meta: { version: 87 },
@@ -1051,6 +1364,42 @@ describe('migration #88', () => {
     });
   });
 
+  it('deletes undefined-keyed properties from  TokensController.allDetectedTokens', async () => {
+    const oldStorage = {
+      meta: { version: 87 },
+      data: {
+        TokensController: {
+          allDetectedTokens: {
+            '16': {
+              '0x1': {
+                '0x111': ['0xaaa'],
+              },
+            },
+            undefined: {
+              '0x2': {
+                '0x222': ['0xbbb'],
+              },
+            },
+          },
+        },
+      },
+    };
+
+    const newStorage = await migrate(oldStorage);
+
+    expect(newStorage.data).toStrictEqual({
+      TokensController: {
+        allDetectedTokens: {
+          '0x10': {
+            '0x1': {
+              '0x111': ['0xaaa'],
+            },
+          },
+        },
+      },
+    });
+  });
+
   it('does not convert chain IDs in TokensController.allDetectedTokens which are already hex strings', async () => {
     const oldStorage = {
       meta: { version: 87 },

app/scripts/migrations/088.ts

@@ -16,8 +16,11 @@ export const version = 88;
  * by a hex chain ID rather than a decimal chain ID.
  * - Rebuilds `tokensChainsCache` in TokenListController to be keyed by a hex
  * chain ID rather than a decimal chain ID.
- * - Rebuilds `allTokens` and `allIgnoredTokens` in TokensController to be keyed
+ * - Rebuilds `allTokens`, `allDetectedTokens`, and `allIgnoredTokens` in
- * by a hex chain ID rather than a decimal chain ID.
+ * TokensController to be keyed by a hex chain ID rather than a decimal chain ID.
+ * - removes any entries in `allNftContracts`, `allNfts`, `tokensChainsCache`,
+ * `allTokens`, `allIgnoredTokens` or `allDetectedTokens` that are keyed by the
+ * string 'undefined'
  *
  * @param originalVersionedData - Versioned MetaMask extension state, exactly what we persist to dist.
  * @param originalVersionedData.meta - State metadata.
@@ -54,6 +57,12 @@ function migrateData(state: Record<string, unknown>): void {
           const nftContractsByChainId = allNftContracts[address];
 
           if (isObject(nftContractsByChainId)) {
+            for (const chainId of Object.keys(nftContractsByChainId)) {
+              if (chainId === 'undefined' || chainId === undefined) {
+                delete nftContractsByChainId[chainId];
+              }
+            }
+
             allNftContracts[address] = mapKeys(
               nftContractsByChainId,
               (_, chainId: string) => toHex(chainId),
@@ -75,6 +84,12 @@ function migrateData(state: Record<string, unknown>): void {
           const nftsByChainId = allNfts[address];
 
           if (isObject(nftsByChainId)) {
+            for (const chainId of Object.keys(nftsByChainId)) {
+              if (chainId === 'undefined' || chainId === undefined) {
+                delete nftsByChainId[chainId];
+              }
+            }
+
             allNfts[address] = mapKeys(nftsByChainId, (_, chainId: string) =>
               toHex(chainId),
             );
@@ -97,6 +112,14 @@ function migrateData(state: Record<string, unknown>): void {
       hasProperty(tokenListControllerState, 'tokensChainsCache') &&
       isObject(tokenListControllerState.tokensChainsCache)
     ) {
+      for (const chainId of Object.keys(
+        tokenListControllerState.tokensChainsCache,
+      )) {
+        if (chainId === 'undefined' || chainId === undefined) {
+          delete tokenListControllerState.tokensChainsCache[chainId];
+        }
+      }
+
       tokenListControllerState.tokensChainsCache = mapKeys(
         tokenListControllerState.tokensChainsCache,
         (_, chainId: string) => toHex(chainId),
@@ -117,6 +140,12 @@ function migrateData(state: Record<string, unknown>): void {
     ) {
       const { allTokens } = tokensControllerState;
 
+      for (const chainId of Object.keys(allTokens)) {
+        if (chainId === 'undefined' || chainId === undefined) {
+          delete allTokens[chainId];
+        }
+      }
+
       tokensControllerState.allTokens = mapKeys(
         allTokens,
         (_, chainId: string) => toHex(chainId),
@@ -130,6 +159,12 @@ function migrateData(state: Record<string, unknown>): void {
     ) {
       const { allIgnoredTokens } = tokensControllerState;
 
+      for (const chainId of Object.keys(allIgnoredTokens)) {
+        if (chainId === 'undefined' || chainId === undefined) {
+          delete allIgnoredTokens[chainId];
+        }
+      }
+
       tokensControllerState.allIgnoredTokens = mapKeys(
         allIgnoredTokens,
         (_, chainId: string) => toHex(chainId),
@@ -143,6 +178,12 @@ function migrateData(state: Record<string, unknown>): void {
     ) {
       const { allDetectedTokens } = tokensControllerState;
 
+      for (const chainId of Object.keys(allDetectedTokens)) {
+        if (chainId === 'undefined' || chainId === undefined) {
+          delete allDetectedTokens[chainId];
+        }
+      }
+
       tokensControllerState.allDetectedTokens = mapKeys(
         allDetectedTokens,
         (_, chainId: string) => toHex(chainId),

app/scripts/migrations/091.test.ts

@@ -0,0 +1,150 @@
+import { cloneDeep } from 'lodash';
+import { migrate, version } from './091';
+
+jest.mock('uuid', () => {
+  const actual = jest.requireActual('uuid');
+
+  return {
+    ...actual,
+    v4: jest.fn(),
+  };
+});
+
+describe('migration #91', () => {
+  it('should update the version metadata', async () => {
+    const oldStorage = {
+      meta: {
+        version: 90,
+      },
+      data: {},
+    };
+
+    const newStorage = await migrate(oldStorage);
+    expect(newStorage.meta).toStrictEqual({
+      version,
+    });
+  });
+
+  it('should return state unaltered if there is no network controller state', async () => {
+    const oldData = {
+      other: 'data',
+    };
+    const oldStorage = {
+      meta: {
+        version: 90,
+      },
+      data: oldData,
+    };
+
+    const newStorage = await migrate(cloneDeep(oldStorage));
+    expect(newStorage.data).toStrictEqual(oldData);
+  });
+
+  it('should return state unaltered if there is no network controller networkConfigurations state', async () => {
+    const oldData = {
+      other: 'data',
+      NetworkController: {
+        providerConfig: {
+          foo: 'bar',
+        },
+      },
+    };
+    const oldStorage = {
+      meta: {
+        version: 90,
+      },
+      data: oldData,
+    };
+
+    const newStorage = await migrate(cloneDeep(oldStorage));
+    expect(newStorage.data).toStrictEqual(oldData);
+  });
+
+  it('should return state unaltered if the networkConfigurations all have a chainId', async () => {
+    const oldData = {
+      other: 'data',
+      NetworkController: {
+        networkConfigurations: {
+          id1: {
+            foo: 'bar',
+            chainId: '0x1',
+          },
+          id2: {
+            fizz: 'buzz',
+            chainId: '0x2',
+          },
+        },
+        providerConfig: {
+          id: 'test',
+        },
+      },
+    };
+    const oldStorage = {
+      meta: {
+        version: 90,
+      },
+      data: oldData,
+    };
+
+    const newStorage = await migrate(cloneDeep(oldStorage));
+    expect(newStorage.data).toStrictEqual(oldData);
+  });
+
+  it('should delete networks that have an undefined or null chainId', async () => {
+    const oldData = {
+      other: 'data',
+      NetworkController: {
+        networkConfigurations: {
+          id1: {
+            foo: 'bar',
+            chainId: '0x1',
+          },
+          id2: {
+            fizz: 'buzz',
+            chainId: '0x2',
+          },
+          id3: {
+            buzz: 'baz',
+            chainId: undefined,
+          },
+          id4: {
+            foo: 'bar',
+            chainId: null,
+          },
+          id5: {
+            fizz: 'buzz',
+          },
+        },
+        providerConfig: {
+          rpcUrl: 'http://foo.bar',
+        },
+      },
+    };
+    const oldStorage = {
+      meta: {
+        version: 90,
+      },
+      data: oldData,
+    };
+
+    const newStorage = await migrate(cloneDeep(oldStorage));
+    expect(newStorage.data).toStrictEqual({
+      other: 'data',
+      NetworkController: {
+        networkConfigurations: {
+          id1: {
+            foo: 'bar',
+            chainId: '0x1',
+          },
+          id2: {
+            fizz: 'buzz',
+            chainId: '0x2',
+          },
+        },
+        providerConfig: {
+          rpcUrl: 'http://foo.bar',
+        },
+      },
+    });
+  });
+});

app/scripts/migrations/091.ts

@@ -0,0 +1,55 @@
+import { hasProperty, isObject } from '@metamask/utils';
+import { cloneDeep } from 'lodash';
+
+export const version = 91;
+
+/**
+ * Delete network configurations if they do not have a chain id
+ *
+ * @param originalVersionedData - Versioned MetaMask extension state, exactly what we persist to dist.
+ * @param originalVersionedData.meta - State metadata.
+ * @param originalVersionedData.meta.version - The current state version.
+ * @param originalVersionedData.data - The persisted MetaMask state, keyed by controller.
+ * @returns Updated versioned MetaMask extension state.
+ */
+export async function migrate(originalVersionedData: {
+  meta: { version: number };
+  data: Record<string, unknown>;
+}) {
+  const versionedData = cloneDeep(originalVersionedData);
+  versionedData.meta.version = version;
+  versionedData.data = transformState(versionedData.data);
+  return versionedData;
+}
+
+function transformState(state: Record<string, unknown>) {
+  if (
+    hasProperty(state, 'NetworkController') &&
+    isObject(state.NetworkController) &&
+    hasProperty(state.NetworkController, 'networkConfigurations') &&
+    isObject(state.NetworkController.networkConfigurations)
+  ) {
+    const { networkConfigurations } = state.NetworkController;
+
+    for (const [networkConfigurationId, networkConfiguration] of Object.entries(
+      networkConfigurations,
+    )) {
+      if (isObject(networkConfiguration)) {
+        if (!networkConfiguration.chainId) {
+          delete networkConfigurations[networkConfigurationId];
+        }
+      }
+    }
+
+    state.NetworkController = {
+      ...state.NetworkController,
+      networkConfigurations,
+    };
+
+    return {
+      ...state,
+      NetworkController: state.NetworkController,
+    };
+  }
+  return state;
+}

app/scripts/migrations/index.js

@@ -94,6 +94,7 @@ import * as m087 from './087';
 import * as m088 from './088';
 import * as m089 from './089';
 import * as m090 from './090';
+import * as m091 from './091';
 
 const migrations = [
   m002,
@@ -185,6 +186,7 @@ const migrations = [
   m088,
   m089,
   m090,
+  m091,
 ];
 
 export default migrations;