Update address book state upon custom RPC chainId edit (#9493)

When the `chainId` for a custom RPC endpoint is edited, we now migrate
the corresponding address book entries to ensure they are not orphaned.

The address book entries are grouped by the `metamask.network` state,
which unfortunately was sometimes the `chainId`, and sometimes the
`networkId`. It was always the `networkId` for built-in Infura
networks, but for custom RPC endpoints it would be set to the user-set
`chainId` field, with a fallback to the `networkId` of the network.

A recent change will force users to enter valid `chainId`s on all
custom networks, which will be normalized to be hex-prefixed. As a
result, address book contacts will now be keyed by a different string.
The contact entries are now migrated when this edit takes place.

There are some edge cases where two separate entries share the same set
of contacts. For example, if two entries have the same `chainId`, or if
they had the same `networkId` and had no `chainId` set. When the
`chainId` is edited in such cases, the contacts are duplicated on both
networks. This is the best we can do, as we don't have any way to know
which network the contacts _should_ be on.

The `typed-message-manager` unit tests have also been updated as part
of this commit because the addition of `sinon.restore()` to the
preferences controller tests ended up clearing a test object in-between
individual tests in that file. The test object is now re-constructed
before each individual test.

app/scripts/controllers/preferences.js

@@ -1,8 +1,12 @@
+import { strict as assert } from 'assert'
 import ObservableStore from 'obs-store'
 import { normalize as normalizeAddress } from 'eth-sig-util'
 import { isValidAddress, sha3, bufferToHex } from 'ethereumjs-util'
+import ethers from 'ethers'
+import log from 'loglevel'
 import { isPrefixedFormattedHexString } from '../lib/util'
 import { addInternalMethodPrefix } from './permissions'
+import { NETWORK_TYPE_TO_ID_MAP } from './network/enums'
 
 export default class PreferencesController {
 
@@ -69,6 +73,7 @@ export default class PreferencesController {
     this.store = new ObservableStore(initState)
     this.store.setMaxListeners(12)
     this.openPopup = opts.openPopup
+    this.migrateAddressBookState = opts.migrateAddressBookState
     this._subscribeProviderType()
 
     global.setPreference = (key, value) => {
@@ -475,13 +480,15 @@ export default class PreferencesController {
   /**
    * updates custom RPC details
    *
-   * @param {string} url - The RPC url to add to frequentRpcList.
-   * @param {string} chainId - Optional chainId of the selected network.
-   * @param {string} ticker   - Optional ticker symbol of the selected network.
-   * @param {string} nickname - Optional nickname of the selected network.
+   * @param {Object} newRpcDetails - Options bag.
+   * @param {string} newRpcDetails.rpcUrl - The RPC url to add to frequentRpcList.
+   * @param {string} newRpcDetails.chainId - The chainId of the selected network.
+   * @param {string} [newRpcDetails.ticker] - Optional ticker symbol of the selected network.
+   * @param {string} [newRpcDetails.nickname] - Optional nickname of the selected network.
+   * @param {Object} [newRpcDetails.rpcPrefs] - Optional RPC preferences, such as the block explorer URL
    *
    */
-  updateRpc (newRpcDetails) {
+  async updateRpc (newRpcDetails) {
     const rpcList = this.getFrequentRpcListDetail()
     const index = rpcList.findIndex((element) => {
       return element.rpcUrl === newRpcDetails.rpcUrl
@@ -489,6 +496,44 @@ export default class PreferencesController {
     if (index > -1) {
       const rpcDetail = rpcList[index]
       const updatedRpc = { ...rpcDetail, ...newRpcDetails }
+      if (rpcDetail.chainId !== updatedRpc.chainId) {
+
+        // When the chainId is changed, associated address book entries should
+        // also be migrated. The address book entries are keyed by the `network` state,
+        // which for custom networks is the chainId with a fallback to the networkId
+        // if the chainId is not set.
+
+        let addressBookKey = rpcDetail.chainId
+        if (!addressBookKey) {
+          // We need to find the networkId to determine what these addresses were keyed by
+          const provider = new ethers.providers.JsonRpcProvider(rpcDetail.rpcUrl)
+          try {
+            addressBookKey = await provider.send('net_version')
+            assert(typeof addressBookKey === 'string')
+          } catch (error) {
+            log.debug(error)
+            log.warn(`Failed to get networkId from ${rpcDetail.rpcUrl}; skipping address book migration`)
+          }
+        }
+
+        // There is an edge case where two separate RPC endpoints are keyed by the same
+        // value. In this case, the contact book entries are duplicated so that they remain
+        // on both networks, since we don't know which network each contact is intended for.
+
+        let duplicate = false
+        const builtInProviderNetworkIds = Object.values(NETWORK_TYPE_TO_ID_MAP)
+          .map((ids) => ids.networkId)
+        const otherRpcEntries = rpcList
+          .filter((entry) => entry.rpcUrl !== newRpcDetails.rpcUrl)
+        if (
+          builtInProviderNetworkIds.includes(addressBookKey) ||
+          otherRpcEntries.some((entry) => entry.chainId === addressBookKey)
+        ) {
+          duplicate = true
+        }
+
+        this.migrateAddressBookState(addressBookKey, updatedRpc.chainId, duplicate)
+      }
       rpcList[index] = updatedRpc
       this.store.updateState({ frequentRpcListDetail: rpcList })
     } else {
@@ -504,6 +549,7 @@ export default class PreferencesController {
    * @param {string} chainId - The chainId of the selected network.
    * @param {string} [ticker] - Ticker symbol of the selected network.
    * @param {string} [nickname] - Nickname of the selected network.
+   * @param {Object} [rpcPrefs] - Optional RPC preferences, such as the block explorer URL
    *
    */
   addToFrequentRpcList (rpcUrl, chainId, ticker = 'ETH', nickname = '', rpcPrefs = {}) {

app/scripts/metamask-controller.js

@@ -111,6 +111,7 @@ export default class MetamaskController extends EventEmitter {
       initLangCode: opts.initLangCode,
       openPopup: opts.openPopup,
       network: this.networkController,
+      migrateAddressBookState: this.migrateAddressBookState.bind(this),
     })
 
     this.appStateController = new AppStateController({
@@ -1880,6 +1881,38 @@ export default class MetamaskController extends EventEmitter {
     )
   }
 
+  /**
+   * Migrate address book state from old to new chainId.
+   *
+   * Address book state is keyed by the `networkStore` state from the network controller. This value is set to the
+   * `networkId` for our built-in Infura networks, but it's set to the `chainId` for custom networks.
+   * When this `chainId` value is changed for custom RPC endpoints, we need to migrate any contacts stored under the
+   * old key to the new key.
+   *
+   * The `duplicate` parameter is used to specify that the contacts under the old key should not be removed. This is
+   * useful in the case where two RPC endpoints shared the same set of contacts, and we're not sure which one each
+   * contact belongs under. Duplicating the contacts under both keys is the only way to ensure they are not lost.
+   *
+   * @param {string} oldChainId - The old chainId
+   * @param {string} newChainId - The new chainId
+   * @param {boolean} [duplicate] - Whether to duplicate the addresses on both chainIds (default: false)
+   */
+  async migrateAddressBookState (oldChainId, newChainId, duplicate = false) {
+    const { addressBook } = this.addressBookController.state
+
+    if (!addressBook[oldChainId]) {
+      return
+    }
+
+    for (const address of Object.keys(addressBook[oldChainId])) {
+      const entry = addressBook[oldChainId][address]
+      this.addressBookController.set(address, entry.name, newChainId, entry.memo)
+      if (!duplicate) {
+        this.addressBookController.delete(oldChainId, address)
+      }
+    }
+  }
+
   //=============================================================================
   // CONFIG
   //=============================================================================

test/unit/app/controllers/preferences-controller-test.js

@@ -7,10 +7,15 @@ import { addInternalMethodPrefix } from '../../../../app/scripts/controllers/per
 describe('preferences controller', function () {
   let preferencesController
   let network
+  const migrateAddressBookState = sinon.stub()
 
   beforeEach(function () {
     network = { providerStore: new ObservableStore({ type: 'mainnet' }) }
-    preferencesController = new PreferencesController({ network })
+    preferencesController = new PreferencesController({ migrateAddressBookState, network })
+  })
+
+  afterEach(function () {
+    sinon.restore()
   })
 
   describe('setAddresses', function () {
@@ -497,15 +502,21 @@ describe('preferences controller', function () {
   })
 
   describe('#updateRpc', function () {
-    it('should update the rpcDetails properly', function () {
+    it('should update the rpcDetails properly', async function () {
       preferencesController.store.updateState({ frequentRpcListDetail: [{}, { rpcUrl: 'test', chainId: '0x1' }, {}] })
-      preferencesController.updateRpc({ rpcUrl: 'test', chainId: '0x1' })
-      preferencesController.updateRpc({ rpcUrl: 'test/1', chainId: '0x1' })
-      preferencesController.updateRpc({ rpcUrl: 'test/2', chainId: '0x1' })
-      preferencesController.updateRpc({ rpcUrl: 'test/3', chainId: '0x1' })
+      await preferencesController.updateRpc({ rpcUrl: 'test', chainId: '0x1' })
+      await preferencesController.updateRpc({ rpcUrl: 'test/1', chainId: '0x1' })
+      await preferencesController.updateRpc({ rpcUrl: 'test/2', chainId: '0x1' })
+      await preferencesController.updateRpc({ rpcUrl: 'test/3', chainId: '0x1' })
       const list = preferencesController.getFrequentRpcListDetail()
       assert.deepEqual(list[1], { rpcUrl: 'test', chainId: '0x1' })
     })
+
+    it('should migrate address book entries if chainId changes', async function () {
+      preferencesController.store.updateState({ frequentRpcListDetail: [{}, { rpcUrl: 'test', chainId: '1' }, {}] })
+      await preferencesController.updateRpc({ rpcUrl: 'test', chainId: '0x1' })
+      assert(migrateAddressBookState.calledWith('1', '0x1'))
+    })
   })
 
   describe('adding and removing from frequentRpcListDetail', function () {

test/unit/app/typed-message-manager.spec.js

@@ -7,10 +7,11 @@ describe('Typed Message Manager', function () {
   let typedMessageManager, msgParamsV1, msgParamsV3, typedMsgs, messages, msgId, numberMsgId
 
   const address = '0xc42edfcc21ed14dda456aa0756c153f7985d8813'
-  const networkController = new NetworkController()
-  sinon.stub(networkController, 'getNetworkState').returns('1')
 
-  beforeEach(function () {
+  beforeEach(async function () {
+    const networkController = new NetworkController()
+    sinon.stub(networkController, 'getNetworkState').returns('0x1')
+
     typedMessageManager = new TypedMessageManager({
       networkController,
     })
@@ -64,7 +65,7 @@ describe('Typed Message Manager', function () {
       }),
     }
 
-    typedMessageManager.addUnapprovedMessage(msgParamsV3, null, 'V3')
+    await typedMessageManager.addUnapprovedMessage(msgParamsV3, null, 'V3')
     typedMsgs = typedMessageManager.getUnapprovedMsgs()
     messages = typedMessageManager.messages
     msgId = Object.keys(typedMsgs)[0]