From 5009ceae53c8f692ec60a046c133af0d06e9e235 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Thu, 20 May 2021 00:27:51 -0230 Subject: [PATCH] Migrate to new CurrencyRateController (#11005) The CurrencyRateController has been migrated to the BaseControllerV2 API, which includes various API changes. These changes include: * The constructor now expects to be passed a `RestrictedControllerMessenger`. * State changes are subscribed to via the `ControllerMessenger` now, rather than via a `subscribe` function. * The state and configration are passed in as one "options" object, rather than as two separate parameters * The polling needs to be started explicitly by calling `start`. It can be stopped and started on-demand now as well. * Changing the current currency or native currency will now throw an error if we fail to update the conversion rate. The `ComposableObservableStore` has been updated to accomodate these new types of controllers. The constructor has been updated to use an options bag pattern as well, to make the addition of the new required `controllerMessenger` parameter a bit less unweildly. --- app/scripts/lib/ComposableObservableStore.js | 52 +++++- .../lib/ComposableObservableStore.test.js | 174 +++++++++++++++++- app/scripts/metamask-controller.js | 128 ++++++------- app/scripts/metamask-controller.test.js | 38 +--- package.json | 2 +- ui/store/actions.test.js | 8 +- yarn.lock | 29 ++- 7 files changed, 303 insertions(+), 128 deletions(-) diff --git a/app/scripts/lib/ComposableObservableStore.js b/app/scripts/lib/ComposableObservableStore.js index 7e9892bea2b8..789a12d80f4d 100644 --- a/app/scripts/lib/ComposableObservableStore.js +++ b/app/scripts/lib/ComposableObservableStore.js @@ -1,18 +1,36 @@ import { ObservableStore } from '@metamask/obs-store'; +/** + * @typedef {import('@metamask/controllers').ControllerMessenger} ControllerMessenger + */ + /** * An ObservableStore that can composes a flat * structure of child stores based on configuration */ export default class ComposableObservableStore extends ObservableStore { + /** + * Describes which stores are being composed. The key is the name of the + * store, and the value is either an ObserableStore, or a controller that + * extends one of the two base controllers in the `@metamask/controllers` + * package. + * @type {Record} + */ + config = {}; + /** * Create a new store * - * @param {Object} [initState] - The initial store state - * @param {Object} [config] - Map of internal state keys to child stores + * @param {Object} options + * @param {Object} [options.config] - Map of internal state keys to child stores + * @param {ControllerMessenger} options.controllerMessenger - The controller + * messenger, used for subscribing to events from BaseControllerV2-based + * controllers. + * @param {Object} [options.state] - The initial store state */ - constructor(initState, config) { - super(initState); + constructor({ config, controllerMessenger, state }) { + super(state); + this.controllerMessenger = controllerMessenger; if (config) { this.updateStructure(config); } @@ -21,15 +39,31 @@ export default class ComposableObservableStore extends ObservableStore { /** * Composes a new internal store subscription structure * - * @param {Object} [config] - Map of internal state keys to child stores + * @param {Record} config - Describes which stores are being + * composed. The key is the name of the store, and the value is either an + * ObserableStore, or a controller that extends one of the two base + * controllers in the `@metamask/controllers` package. */ updateStructure(config) { this.config = config; this.removeAllListeners(); - for (const key of Object.keys(this.config)) { - config[key].subscribe((state) => { - this.updateState({ [key]: state }); - }); + for (const key of Object.keys(config)) { + if (!config[key]) { + throw new Error(`Undefined '${key}'`); + } + const store = config[key]; + if (store.subscribe) { + config[key].subscribe((state) => { + this.updateState({ [key]: state }); + }); + } else { + this.controllerMessenger.subscribe( + `${store.name}:stateChange`, + (state) => { + this.updateState({ [key]: state }); + }, + ); + } } } diff --git a/app/scripts/lib/ComposableObservableStore.test.js b/app/scripts/lib/ComposableObservableStore.test.js index 620b6df85a9a..063f97cbfac6 100644 --- a/app/scripts/lib/ComposableObservableStore.test.js +++ b/app/scripts/lib/ComposableObservableStore.test.js @@ -1,40 +1,194 @@ import { strict as assert } from 'assert'; import { ObservableStore } from '@metamask/obs-store'; +import { + BaseController, + BaseControllerV2, + ControllerMessenger, +} from '@metamask/controllers'; import ComposableObservableStore from './ComposableObservableStore'; +class OldExampleController extends BaseController { + name = 'OldExampleController'; + + defaultState = { + baz: 'baz', + }; + + constructor() { + super(); + this.initialize(); + } + + updateBaz(contents) { + this.update({ baz: contents }); + } +} +class ExampleController extends BaseControllerV2 { + static defaultState = { + bar: 'bar', + }; + + static metadata = { + bar: { persist: true, anonymous: true }, + }; + + constructor({ messenger }) { + super({ + messenger, + name: 'ExampleController', + metadata: ExampleController.metadata, + state: ExampleController.defaultState, + }); + } + + updateBar(contents) { + this.update(() => { + return { bar: contents }; + }); + } +} + describe('ComposableObservableStore', function () { it('should register initial state', function () { - const store = new ComposableObservableStore('state'); + const controllerMessenger = new ControllerMessenger(); + const store = new ComposableObservableStore({ + controllerMessenger, + state: 'state', + }); assert.strictEqual(store.getState(), 'state'); }); it('should register initial structure', function () { + const controllerMessenger = new ControllerMessenger(); const testStore = new ObservableStore(); - const store = new ComposableObservableStore(null, { TestStore: testStore }); + const store = new ComposableObservableStore({ + config: { TestStore: testStore }, + controllerMessenger, + }); testStore.putState('state'); assert.deepEqual(store.getState(), { TestStore: 'state' }); }); - it('should update structure', function () { + it('should update structure with observable store', function () { + const controllerMessenger = new ControllerMessenger(); const testStore = new ObservableStore(); - const store = new ComposableObservableStore(); + const store = new ComposableObservableStore({ controllerMessenger }); store.updateStructure({ TestStore: testStore }); testStore.putState('state'); assert.deepEqual(store.getState(), { TestStore: 'state' }); }); + it('should update structure with BaseController-based controller', function () { + const controllerMessenger = new ControllerMessenger(); + const oldExampleController = new OldExampleController(); + const store = new ComposableObservableStore({ controllerMessenger }); + store.updateStructure({ OldExample: oldExampleController }); + oldExampleController.updateBaz('state'); + assert.deepEqual(store.getState(), { OldExample: { baz: 'state' } }); + }); + + it('should update structure with BaseControllerV2-based controller', function () { + const controllerMessenger = new ControllerMessenger(); + const exampleController = new ExampleController({ + messenger: controllerMessenger, + }); + const store = new ComposableObservableStore({ controllerMessenger }); + store.updateStructure({ Example: exampleController }); + exampleController.updateBar('state'); + console.log(exampleController.state); + assert.deepEqual(store.getState(), { Example: { bar: 'state' } }); + }); + + it('should update structure with all three types of stores', function () { + const controllerMessenger = new ControllerMessenger(); + const exampleStore = new ObservableStore(); + const exampleController = new ExampleController({ + messenger: controllerMessenger, + }); + const oldExampleController = new OldExampleController(); + const store = new ComposableObservableStore({ controllerMessenger }); + store.updateStructure({ + Example: exampleController, + OldExample: oldExampleController, + Store: exampleStore, + }); + exampleStore.putState('state'); + exampleController.updateBar('state'); + oldExampleController.updateBaz('state'); + assert.deepEqual(store.getState(), { + Example: { bar: 'state' }, + OldExample: { baz: 'state' }, + Store: 'state', + }); + }); + it('should return flattened state', function () { + const controllerMessenger = new ControllerMessenger(); const fooStore = new ObservableStore({ foo: 'foo' }); - const barStore = new ObservableStore({ bar: 'bar' }); - const store = new ComposableObservableStore(null, { - FooStore: fooStore, - BarStore: barStore, + const barController = new ExampleController({ + messenger: controllerMessenger, + }); + const bazController = new OldExampleController(); + const store = new ComposableObservableStore({ + config: { + FooStore: fooStore, + BarStore: barController, + BazStore: bazController, + }, + controllerMessenger, + state: { + FooStore: fooStore.getState(), + BarStore: barController.state, + BazStore: bazController.state, + }, + }); + assert.deepEqual(store.getFlatState(), { + foo: 'foo', + bar: 'bar', + baz: 'baz', }); - assert.deepEqual(store.getFlatState(), { foo: 'foo', bar: 'bar' }); }); it('should return empty flattened state when not configured', function () { - const store = new ComposableObservableStore(); + const controllerMessenger = new ControllerMessenger(); + const store = new ComposableObservableStore({ controllerMessenger }); assert.deepEqual(store.getFlatState(), {}); }); + + it('should throw if the controller messenger is omitted and the config includes a BaseControllerV2 controller', function () { + const controllerMessenger = new ControllerMessenger(); + const exampleController = new ExampleController({ + messenger: controllerMessenger, + }); + assert.throws( + () => + new ComposableObservableStore({ + config: { + Example: exampleController, + }, + }), + ); + }); + + it('should throw if the controller messenger is omitted and updateStructure called with a BaseControllerV2 controller', function () { + const controllerMessenger = new ControllerMessenger(); + const exampleController = new ExampleController({ + messenger: controllerMessenger, + }); + const store = new ComposableObservableStore({}); + assert.throws(() => store.updateStructure({ Example: exampleController })); + }); + + it('should throw if initialized with undefined config entry', function () { + const controllerMessenger = new ControllerMessenger(); + assert.throws( + () => + new ComposableObservableStore({ + config: { + Example: undefined, + }, + controllerMessenger, + }), + ); + }); }); diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 970ef98046f1..16c055f00c8c 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -20,6 +20,7 @@ import contractMap from '@metamask/contract-metadata'; import { AddressBookController, ApprovalController, + ControllerMessenger, CurrencyRateController, PhishingController, NotificationController, @@ -96,8 +97,13 @@ export default class MetamaskController extends EventEmitter { this.getRequestAccountTabIds = opts.getRequestAccountTabIds; this.getOpenMetamaskTabsIds = opts.getOpenMetamaskTabsIds; + const controllerMessenger = new ControllerMessenger(); + // observable state store - this.store = new ComposableObservableStore(initState); + this.store = new ComposableObservableStore({ + state: initState, + controllerMessenger, + }); // external connections by origin // Do not modify directly. Use the associated methods. @@ -157,10 +163,14 @@ export default class MetamaskController extends EventEmitter { preferencesStore: this.preferencesController.store, }); - this.currencyRateController = new CurrencyRateController( - { includeUSDRate: true }, - initState.CurrencyController, - ); + const currencyRateMessenger = controllerMessenger.getRestricted({ + name: 'CurrencyRateController', + }); + this.currencyRateController = new CurrencyRateController({ + includeUSDRate: true, + messenger: currencyRateMessenger, + state: initState.CurrencyController, + }); this.phishingController = new PhishingController(); @@ -222,10 +232,12 @@ export default class MetamaskController extends EventEmitter { this.accountTracker.start(); this.incomingTransactionsController.start(); this.tokenRatesController.start(); + this.currencyRateController.start(); } else { this.accountTracker.stop(); this.incomingTransactionsController.stop(); this.tokenRatesController.stop(); + this.currencyRateController.stop(); } }); @@ -363,18 +375,15 @@ export default class MetamaskController extends EventEmitter { } }); - this.networkController.on(NETWORK_EVENTS.NETWORK_DID_CHANGE, () => { - this.setCurrentCurrency( - this.currencyRateController.state.currentCurrency, - (error) => { - if (error) { - throw error; - } - }, - ); + this.networkController.on(NETWORK_EVENTS.NETWORK_DID_CHANGE, async () => { + const { ticker } = this.networkController.getProviderConfig(); + try { + await this.currencyRateController.setNativeCurrency(ticker); + } catch (error) { + // TODO: Handle failure to get conversion rate more gracefully + console.error(error); + } }); - const { ticker } = this.networkController.getProviderConfig(); - this.currencyRateController.configure({ nativeCurrency: ticker ?? 'ETH' }); this.networkController.lookupNetwork(); this.messageManager = new MessageManager(); this.personalMessageManager = new PersonalMessageManager(); @@ -438,33 +447,37 @@ export default class MetamaskController extends EventEmitter { NotificationController: this.notificationController, }); - this.memStore = new ComposableObservableStore(null, { - AppStateController: this.appStateController.store, - NetworkController: this.networkController.store, - AccountTracker: this.accountTracker.store, - TxController: this.txController.memStore, - CachedBalancesController: this.cachedBalancesController.store, - TokenRatesController: this.tokenRatesController.store, - MessageManager: this.messageManager.memStore, - PersonalMessageManager: this.personalMessageManager.memStore, - DecryptMessageManager: this.decryptMessageManager.memStore, - EncryptionPublicKeyManager: this.encryptionPublicKeyManager.memStore, - TypesMessageManager: this.typedMessageManager.memStore, - KeyringController: this.keyringController.memStore, - PreferencesController: this.preferencesController.store, - MetaMetricsController: this.metaMetricsController.store, - AddressBookController: this.addressBookController, - CurrencyController: this.currencyRateController, - AlertController: this.alertController.store, - OnboardingController: this.onboardingController.store, - IncomingTransactionsController: this.incomingTransactionsController.store, - PermissionsController: this.permissionsController.permissions, - PermissionsMetadata: this.permissionsController.store, - ThreeBoxController: this.threeBoxController.store, - SwapsController: this.swapsController.store, - EnsController: this.ensController.store, - ApprovalController: this.approvalController, - NotificationController: this.notificationController, + this.memStore = new ComposableObservableStore({ + config: { + AppStateController: this.appStateController.store, + NetworkController: this.networkController.store, + AccountTracker: this.accountTracker.store, + TxController: this.txController.memStore, + CachedBalancesController: this.cachedBalancesController.store, + TokenRatesController: this.tokenRatesController.store, + MessageManager: this.messageManager.memStore, + PersonalMessageManager: this.personalMessageManager.memStore, + DecryptMessageManager: this.decryptMessageManager.memStore, + EncryptionPublicKeyManager: this.encryptionPublicKeyManager.memStore, + TypesMessageManager: this.typedMessageManager.memStore, + KeyringController: this.keyringController.memStore, + PreferencesController: this.preferencesController.store, + MetaMetricsController: this.metaMetricsController.store, + AddressBookController: this.addressBookController, + CurrencyController: this.currencyRateController, + AlertController: this.alertController.store, + OnboardingController: this.onboardingController.store, + IncomingTransactionsController: this.incomingTransactionsController + .store, + PermissionsController: this.permissionsController.permissions, + PermissionsMetadata: this.permissionsController.store, + ThreeBoxController: this.threeBoxController.store, + SwapsController: this.swapsController.store, + EnsController: this.ensController.store, + ApprovalController: this.approvalController, + NotificationController: this.notificationController, + }, + controllerMessenger, }); this.memStore.subscribe(this.sendUpdate.bind(this)); @@ -648,7 +661,11 @@ export default class MetamaskController extends EventEmitter { return { // etc getState: (cb) => cb(null, this.getState()), - setCurrentCurrency: this.setCurrentCurrency.bind(this), + setCurrentCurrency: nodeify( + this.currencyRateController.setCurrentCurrency.bind( + this.currencyRateController, + ), + ), setUseBlockie: this.setUseBlockie.bind(this), setUseNonceField: this.setUseNonceField.bind(this), setUsePhishDetect: this.setUsePhishDetect.bind(this), @@ -2507,29 +2524,6 @@ export default class MetamaskController extends EventEmitter { // Log blocks - /** - * A method for setting the user's preferred display currency. - * @param {string} currencyCode - The code of the preferred currency. - * @param {Function} cb - A callback function returning currency info. - */ - setCurrentCurrency(currencyCode, cb) { - const { ticker } = this.networkController.getProviderConfig(); - try { - const currencyState = { - nativeCurrency: ticker, - currentCurrency: currencyCode, - }; - this.currencyRateController.update(currencyState); - this.currencyRateController.configure(currencyState); - cb(null); - return; - } catch (err) { - cb(err); - // eslint-disable-next-line no-useless-return - return; - } - } - /** * A method for selecting a custom URL for an ethereum RPC provider and updating it * @param {string} rpcUrl - A URL for a valid Ethereum RPC API. diff --git a/app/scripts/metamask-controller.test.js b/app/scripts/metamask-controller.test.js index 3d8cb57cfc4d..87fed2aefbef 100644 --- a/app/scripts/metamask-controller.test.js +++ b/app/scripts/metamask-controller.test.js @@ -654,46 +654,24 @@ describe('MetaMaskController', function () { }); describe('#setCustomRpc', function () { - let rpcUrl; - - beforeEach(function () { - rpcUrl = metamaskController.setCustomRpc( + it('returns custom RPC that when called', async function () { + const rpcUrl = await metamaskController.setCustomRpc( CUSTOM_RPC_URL, CUSTOM_RPC_CHAIN_ID, ); + assert.equal(rpcUrl, CUSTOM_RPC_URL); }); - it('returns custom RPC that when called', async function () { - assert.equal(await rpcUrl, CUSTOM_RPC_URL); - }); - - it('changes the network controller rpc', function () { + it('changes the network controller rpc', async function () { + await metamaskController.setCustomRpc( + CUSTOM_RPC_URL, + CUSTOM_RPC_CHAIN_ID, + ); const networkControllerState = metamaskController.networkController.store.getState(); assert.equal(networkControllerState.provider.rpcUrl, CUSTOM_RPC_URL); }); }); - describe('#setCurrentCurrency', function () { - let defaultMetaMaskCurrency; - - beforeEach(function () { - defaultMetaMaskCurrency = - metamaskController.currencyRateController.state.currentCurrency; - }); - - it('defaults to usd', function () { - assert.equal(defaultMetaMaskCurrency, 'usd'); - }); - - it('sets currency to JPY', function () { - metamaskController.setCurrentCurrency('JPY', noop); - assert.equal( - metamaskController.currencyRateController.state.currentCurrency, - 'JPY', - ); - }); - }); - describe('#addNewAccount', function () { it('errors when an primary keyring is does not exist', async function () { const addNewAccount = metamaskController.addNewAccount(); diff --git a/package.json b/package.json index 7842865ad728..c46468f07f4d 100644 --- a/package.json +++ b/package.json @@ -95,7 +95,7 @@ "@lavamoat/preinstall-always-fail": "^1.0.0", "@material-ui/core": "^4.11.0", "@metamask/contract-metadata": "^1.22.0", - "@metamask/controllers": "^8.0.0", + "@metamask/controllers": "^9.0.0", "@metamask/eth-ledger-bridge-keyring": "^0.5.0", "@metamask/eth-token-tracker": "^3.0.1", "@metamask/etherscan-link": "^2.1.0", diff --git a/ui/store/actions.test.js b/ui/store/actions.test.js index 409f038538d2..73975843da20 100644 --- a/ui/store/actions.test.js +++ b/ui/store/actions.test.js @@ -618,7 +618,7 @@ describe('Actions', () => { it('calls setCurrentCurrency', async () => { const store = mockStore(); - background.setCurrentCurrency.callsFake((_, cb) => cb()); + background.setCurrentCurrency = sinon.stub().callsFake((_, cb) => cb()); actions._setBackgroundConnection(background); await store.dispatch(actions.setCurrentCurrency('jpy')); @@ -627,9 +627,9 @@ describe('Actions', () => { it('throws if setCurrentCurrency throws', async () => { const store = mockStore(); - background.setCurrentCurrency.callsFake((_, cb) => - cb(new Error('error')), - ); + background.setCurrentCurrency = sinon + .stub() + .callsFake((_, cb) => cb(new Error('error'))); actions._setBackgroundConnection(background); const expectedActions = [ diff --git a/yarn.lock b/yarn.lock index 5c006c735a74..f1cfb51845b3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2630,7 +2630,7 @@ semver "^7.3.5" yargs "^17.0.1" -"@metamask/contract-metadata@^1.19.0", "@metamask/contract-metadata@^1.22.0", "@metamask/contract-metadata@^1.24.0": +"@metamask/contract-metadata@^1.19.0", "@metamask/contract-metadata@^1.22.0", "@metamask/contract-metadata@^1.25.0": version "1.25.0" resolved "https://registry.yarnpkg.com/@metamask/contract-metadata/-/contract-metadata-1.25.0.tgz#442ace91fb40165310764b68d8096d0017bb0492" integrity sha512-yhmYB9CQPv0dckNcPoWDcgtrdUp0OgK0uvkRE5QIBv4b3qENI1/03BztvK2ijbTuMlORUpjPq7/1MQDUPoRPVw== @@ -2663,18 +2663,18 @@ web3 "^0.20.7" web3-provider-engine "^16.0.1" -"@metamask/controllers@^8.0.0": - version "8.0.0" - resolved "https://registry.yarnpkg.com/@metamask/controllers/-/controllers-8.0.0.tgz#42ac5aaef67a03d3fe599a67a36597e01902ca8d" - integrity sha512-TrteMifsCxV1g3WHcSD1X98fF4hKep3sXZNGfrvkPqa8mrF03hJke21WBSTRtvJ3vkNLRWgi+5I6lVXFTzbYuQ== +"@metamask/controllers@^9.0.0": + version "9.0.0" + resolved "https://registry.yarnpkg.com/@metamask/controllers/-/controllers-9.0.0.tgz#b8661f6fad246b20b92f684a63ab268b0e5248a7" + integrity sha512-qj6sKw5j7a542i+EnN/XJW2JsZZzYbj5j8NjYBiRE70kNsyTQ7iXaaEdyXO6re+OAI5M1ULuWgTULoFlWiuHcA== dependencies: - "@metamask/contract-metadata" "^1.24.0" + "@metamask/contract-metadata" "^1.25.0" "@types/uuid" "^8.3.0" async-mutex "^0.2.6" babel-runtime "^6.26.0" eth-ens-namehash "^2.0.8" eth-json-rpc-infura "^5.1.0" - eth-keyring-controller "^6.1.0" + eth-keyring-controller "^6.2.1" eth-method-registry "1.1.0" eth-phishing-detect "^1.1.13" eth-query "^2.1.2" @@ -10639,6 +10639,21 @@ eth-keyring-controller@^6.2.0: loglevel "^1.5.0" obs-store "^4.0.3" +eth-keyring-controller@^6.2.1: + version "6.2.1" + resolved "https://registry.yarnpkg.com/eth-keyring-controller/-/eth-keyring-controller-6.2.1.tgz#61901071fc74059ed37cb5ae93870fdcae6e3781" + integrity sha512-x2gTM1iHp2Kbvdtd9Eslysw0qzVZiqOzpVB3AU/ni2Xiit+rlcv2H80zYKjrEwlfWFDj4YILD3bOqlnEMmRJOA== + dependencies: + bip39 "^2.4.0" + bluebird "^3.5.0" + browser-passworder "^2.0.3" + eth-hd-keyring "^3.6.0" + eth-sig-util "^3.0.1" + eth-simple-keyring "^4.2.0" + ethereumjs-util "^7.0.9" + loglevel "^1.5.0" + obs-store "^4.0.3" + eth-lib@0.2.8: version "0.2.8" resolved "https://registry.yarnpkg.com/eth-lib/-/eth-lib-0.2.8.tgz#b194058bef4b220ad12ea497431d6cb6aa0623c8"