Use chainId values for the address book (#9565)

* Delete CachedBalancesController.cachedBalances

* Migrate provider to Rinkeby instead of deleting it

* Convert hex transaction metamaskNetworkId values to decimal

* Don't migrate provider state in e2e tests

* Don't kick custom RPC users to Rinkeby unnecessarily

* Use provider.chainId for address book chainId values

* Add address book migration

* Fix failing unit test

* fixup! Merge branch 'develop' into address-book-use-chainId

* Select address book entries for display by chainId

* Merge all address book entry keys

* fixup! Merge all address book entry keys

app/scripts/migrations/048.js

@@ -10,6 +10,7 @@ const version = 48
  * 3.  Add localhost network to frequentRpcListDetail.
  * 4.  Delete CachedBalancesController.cachedBalances
  * 5.  Convert transactions metamaskNetworkId to decimal if they are hex
+ * 6.  Convert address book keys from decimal to hex
  */
 export default {
   version,
@@ -87,5 +88,78 @@ function transformState (state = {}) {
     })
   }
 
+  // 6.  Convert address book keys from decimal to hex
+  const addressBook = state.AddressBookController?.addressBook || {}
+  Object.keys(addressBook).forEach((networkKey) => {
+    if ((/^\d+$/ui).test(networkKey)) {
+      const chainId = `0x${networkKey.toString(16)}`
+      updateChainIds(addressBook[networkKey], chainId)
+
+      if (addressBook[chainId]) {
+        mergeAddressBookKeys(addressBook, networkKey, chainId)
+      } else {
+        addressBook[chainId] = addressBook[networkKey]
+      }
+      delete addressBook[networkKey]
+    }
+  })
+
   return state
 }
+
+/**
+ * Merges the two given keys for the given address book in place.
+ *
+ * @returns {void}
+ */
+function mergeAddressBookKeys (addressBook, networkKey, chainIdKey) {
+  const networkKeyEntries = addressBook[networkKey] || {}
+  // For the new entries, start by copying the existing entries for the chainId
+  const newEntries = { ...addressBook[chainIdKey] }
+
+  // For each address of the old/networkId key entries
+  Object.keys(networkKeyEntries).forEach((address) => {
+    if (newEntries[address] && typeof newEntries[address] === 'object') {
+      const mergedEntry = {}
+
+      // Collect all keys from both entries and merge the corresponding chainId
+      // entry with the networkId entry
+      new Set([
+        ...Object.keys(newEntries[address]),
+        ...Object.keys(networkKeyEntries[address] || {}),
+      ]).forEach((key) => {
+        // Use non-empty value for the current key, if any
+        mergedEntry[key] = (
+          newEntries[address][key] ||
+          networkKeyEntries[address]?.[key] ||
+          ''
+        )
+      })
+
+      newEntries[address] = mergedEntry
+    } else if (
+      networkKeyEntries[address] &&
+      typeof networkKeyEntries[address] === 'object'
+    ) {
+      // If there is no corresponding chainId entry, just use the networkId entry
+      // directly
+      newEntries[address] = networkKeyEntries[address]
+    }
+  })
+
+  addressBook[chainIdKey] = newEntries
+}
+
+/**
+ * Updates the chainId key values to the given chainId in place for all values
+ * of the given networkEntries object.
+ *
+ * @returns {void}
+ */
+function updateChainIds (networkEntries, chainId) {
+  Object.values(networkEntries).forEach((entry) => {
+    if (entry && typeof entry === 'object') {
+      entry.chainId = chainId
+    }
+  })
+}

test/data/mock-state.json

@@ -12,6 +12,9 @@
       "showIncomingTransactions": true
     },
     "network": "4",
+    "provider": {
+      "chainId": "0x4"
+    },
     "identities": {
       "0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc": {
         "address": "0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc",
@@ -111,10 +114,10 @@
     "nativeCurrency": "ETH",
     "conversionRate": 556.12,
     "addressBook": {
-      "4": {
+      "0x4": {
         "0xc42edfcc21ed14dda456aa0756c153f7985d8813": {
           "address": "0xc42edfcc21ed14dda456aa0756c153f7985d8813",
-          "chainId": "4",
+          "chainId": "0x4",
           "isEns": false,
           "memo": "",
           "name": "Address Book Account 1"

test/unit/migrations/048-test.js

@@ -348,4 +348,162 @@ describe('migration #48', function () {
       foo: 'bar',
     })
   })
+
+  it('should migrate the address book', async function () {
+    const oldStorage = {
+      meta: {},
+      data: {
+        AddressBookController: {
+          addressBook: {
+            '1': {
+              'address1': {
+                chainId: '1',
+                foo: 'bar',
+              },
+            },
+            '0x2': {
+              'address2': {
+                chainId: '0x2',
+                foo: 'bar',
+              },
+            },
+          },
+          bar: {
+            baz: 'buzz',
+          },
+        },
+        foo: 'bar',
+      },
+    }
+
+    const newStorage = await migration48.migrate(oldStorage)
+    assert.deepEqual(newStorage.data, {
+      ...expectedPreferencesState,
+      AddressBookController: {
+        addressBook: {
+          '0x1': {
+            'address1': {
+              chainId: '0x1',
+              foo: 'bar',
+            },
+          },
+          '0x2': {
+            'address2': {
+              chainId: '0x2',
+              foo: 'bar',
+            },
+          },
+        },
+        bar: {
+          baz: 'buzz',
+        },
+      },
+      foo: 'bar',
+    })
+  })
+
+  it('should migrate the address book and merge entries', async function () {
+    const oldStorage = {
+      meta: {},
+      data: {
+        AddressBookController: {
+          addressBook: {
+            '2': {
+              'address1': {
+                chainId: '2',
+                key2: 'kaplar',
+                key3: 'value3',
+                key4: null,
+                foo: 'bar',
+              },
+              'address2': {
+                chainId: '2',
+                foo: 'bar',
+              },
+            },
+            '0x2': {
+              'address1': {
+                chainId: '0x2',
+                key1: 'value1',
+                key2: 'value2',
+                foo: 'bar',
+              },
+              'address3': {
+                chainId: '0x2',
+                foo: 'bar',
+              },
+            },
+          },
+          bar: {
+            baz: 'buzz',
+          },
+        },
+        foo: 'bar',
+      },
+    }
+
+    const newStorage = await migration48.migrate(oldStorage)
+    assert.deepEqual(newStorage.data, {
+      ...expectedPreferencesState,
+      AddressBookController: {
+        addressBook: {
+          '0x2': {
+            'address1': {
+              chainId: '0x2',
+              key1: 'value1',
+              key2: 'value2',
+              key3: 'value3',
+              key4: '',
+              foo: 'bar',
+            },
+            'address2': {
+              chainId: '0x2',
+              foo: 'bar',
+            },
+            'address3': {
+              chainId: '0x2',
+              foo: 'bar',
+            },
+          },
+        },
+        bar: {
+          baz: 'buzz',
+        },
+      },
+      foo: 'bar',
+    })
+  })
+
+  it('should not modify address book if all entries are valid or un-parseable', async function () {
+    const oldStorage = {
+      meta: {},
+      data: {
+        AddressBookController: {
+          addressBook: {
+            '0x1': { foo: { bar: 'baz' } },
+            'kaplar': { foo: { bar: 'baz' } },
+          },
+          bar: {
+            baz: 'buzz',
+          },
+        },
+        foo: 'bar',
+      },
+    }
+
+    const newStorage = await migration48.migrate(oldStorage)
+    assert.deepEqual(newStorage.data, {
+      ...expectedPreferencesState,
+      AddressBookController: {
+        addressBook: {
+          '0x1': { foo: { bar: 'baz' } },
+          'kaplar': { foo: { bar: 'baz' } },
+        },
+        bar: {
+          baz: 'buzz',
+        },
+      },
+      foo: 'bar',
+    })
+  })
 })

test/unit/ui/app/actions.spec.js

@@ -14,7 +14,7 @@ import firstTimeState from '../../localhostState'
 
 const { provider } = createTestProviderTools({ scaffold: {} })
 const middleware = [thunk]
-const defaultState = { metamask: {} }
+const defaultState = { metamask: { provider: { chainId: '0x1' } } }
 const mockStore = (state = defaultState) => configureStore(middleware)(state)
 const extensionMock = {
   runtime: {

ui/app/pages/settings/contact-list-tab/edit-contact/edit-contact.container.js

@@ -22,7 +22,7 @@ const mapStateToProps = (state, ownProps) => {
   const contact = getAddressBookEntry(state, address) || state.metamask.identities[address]
   const { memo, name } = contact || {}
 
-  const chainId = state.metamask.network
+  const { chainId } = state.metamask.provider
 
   const showingMyAccounts = Boolean(pathname.match(CONTACT_MY_ACCOUNTS_EDIT_ROUTE))
 

ui/app/selectors/selectors.js

@@ -156,11 +156,11 @@ export function getAssetImages (state) {
 }
 
 export function getAddressBook (state) {
-  const { network } = state.metamask
-  if (!state.metamask.addressBook[network]) {
+  const { chainId } = state.metamask.provider
+  if (!state.metamask.addressBook[chainId]) {
     return []
   }
-  return Object.values(state.metamask.addressBook[network])
+  return Object.values(state.metamask.addressBook[chainId])
 }
 
 export function getAddressBookEntry (state, address) {

ui/app/selectors/tests/selectors.test.js

@@ -49,7 +49,7 @@ describe('Selectors', function () {
         [
           {
             'address': '0xc42edfcc21ed14dda456aa0756c153f7985d8813',
-            'chainId': '4',
+            'chainId': '0x4',
             'isEns': false,
             'memo': '',
             'name': 'Address Book Account 1',

ui/app/selectors/tests/send-selectors-test-data.js

@@ -30,6 +30,10 @@ const state = {
     'nativeCurrency': 'ETH',
     'frequentRpcList': [],
     'network': '3',
+    'provider': {
+      'type': 'testnet',
+      'chainId': '0x3',
+    },
     'accounts': {
       '0xfdea65c8e26263f6d9a1b5de9555d2931a33b825': {
         'code': '0x',
@@ -57,11 +61,11 @@ const state = {
       },
     },
     'addressBook': {
-      '3': {
+      '0x3': {
         '0x06195827297c7a80a443b6894d3bdb8824b43896': {
           'address': '0x06195827297c7a80a443b6894d3bdb8824b43896',
           'name': 'Address Book Account 1',
-          'chainId': '3',
+          'chainId': '0x3',
         },
       },
     },
@@ -150,9 +154,6 @@ const state = {
       },
     ],
     'selectedAddress': '0xd85a4b6a394794842887b8284293d69163007bbb',
-    'provider': {
-      'type': 'testnet',
-    },
     'send': {
       'gasLimit': '0xFFFF',
       'gasPrice': '0xaa',

ui/app/selectors/tests/send.test.js

@@ -382,7 +382,7 @@ describe('send selectors', function () {
           {
             address: '0x06195827297c7a80a443b6894d3bdb8824b43896',
             name: 'Address Book Account 1',
-            chainId: '3',
+            chainId: '0x3',
           },
         ],
       )

ui/app/store/actions.js

@@ -1605,7 +1605,7 @@ export function addToAddressBook (recipient, nickname = '', memo = '') {
   log.debug(`background.addToAddressBook`)
 
   return async (dispatch, getState) => {
-    const chainId = getState().metamask.network
+    const { chainId } = getState().metamask.provider
 
     let set
     try {