From 17f7ca2cfe99aa7dee5ae7a1f4895b5dd524d033 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 7 Oct 2020 15:02:17 -0230 Subject: [PATCH] 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 | 56 +++++++++++++++++-- app/scripts/metamask-controller.js | 33 +++++++++++ .../preferences-controller-test.js | 23 ++++++-- test/unit/app/typed-message-manager.spec.js | 9 +-- 4 files changed, 106 insertions(+), 15 deletions(-) diff --git a/app/scripts/controllers/preferences.js b/app/scripts/controllers/preferences.js index 2d2cab5eb..3c6e8e629 100644 --- a/app/scripts/controllers/preferences.js +++ b/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 = {}) { diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 9ee785b9a..45bb20155 100644 --- a/app/scripts/metamask-controller.js +++ b/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 //============================================================================= diff --git a/test/unit/app/controllers/preferences-controller-test.js b/test/unit/app/controllers/preferences-controller-test.js index 7db592170..bcf2d5dd2 100644 --- a/test/unit/app/controllers/preferences-controller-test.js +++ b/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 () { diff --git a/test/unit/app/typed-message-manager.spec.js b/test/unit/app/typed-message-manager.spec.js index 5ab7258ad..559628dd4 100644 --- a/test/unit/app/typed-message-manager.spec.js +++ b/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]