From 6d86c424867030785e4e7ef6e533ce3df9de6a3b Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 5 Sep 2023 11:16:21 +0200 Subject: [PATCH 1/3] refactor: remove keyringMemStore from DetectTokensController --- app/scripts/controllers/detect-tokens.js | 29 +++++-------------- app/scripts/controllers/detect-tokens.test.js | 22 ++++++-------- app/scripts/metamask-controller.js | 7 +++-- 3 files changed, 21 insertions(+), 37 deletions(-) diff --git a/app/scripts/controllers/detect-tokens.js b/app/scripts/controllers/detect-tokens.js index 46719aebcced..c1c7efea07e4 100644 --- a/app/scripts/controllers/detect-tokens.js +++ b/app/scripts/controllers/detect-tokens.js @@ -40,7 +40,6 @@ export default class DetectTokensController { interval = DEFAULT_INTERVAL, preferences, network, - keyringMemStore, tokenList, tokensController, assetsContractController = null, @@ -52,7 +51,6 @@ export default class DetectTokensController { this.preferences = preferences; this.interval = interval; this.network = network; - this.keyringMemStore = keyringMemStore; this.tokenList = tokenList; this.useTokenDetection = this.preferences?.store.getState().useTokenDetection; @@ -91,6 +89,13 @@ export default class DetectTokensController { this.restartTokenDetection({ chainId: this.chainId }); } }); + messenger.subscribe('KeyringController:unlock', () => { + this.isUnlocked = true; + this.restartTokenDetection(); + }); + messenger.subscribe('KeyringController:lock', () => { + this.isUnlocked = false; + }); } /** @@ -236,26 +241,6 @@ export default class DetectTokensController { }, interval); } - /** - * In setter when isUnlocked is updated to true, detectNewTokens and restart polling - * - * @type {object} - */ - set keyringMemStore(keyringMemStore) { - if (!keyringMemStore) { - return; - } - this._keyringMemStore = keyringMemStore; - this._keyringMemStore.subscribe(({ isUnlocked }) => { - if (this.isUnlocked !== isUnlocked) { - this.isUnlocked = isUnlocked; - if (isUnlocked) { - this.restartTokenDetection(); - } - } - }); - } - /** * @type {object} */ diff --git a/app/scripts/controllers/detect-tokens.test.js b/app/scripts/controllers/detect-tokens.test.js index 8d1c182c249e..25a959cf9102 100644 --- a/app/scripts/controllers/detect-tokens.test.js +++ b/app/scripts/controllers/detect-tokens.test.js @@ -1,7 +1,6 @@ import { strict as assert } from 'assert'; import sinon from 'sinon'; import nock from 'nock'; -import { ObservableStore } from '@metamask/obs-store'; import BigNumber from 'bignumber.js'; import { ControllerMessenger } from '@metamask/base-controller'; import { @@ -19,14 +18,17 @@ import PreferencesController from './preferences'; function buildMessenger() { return new ControllerMessenger().getRestricted({ name: 'DetectTokensController', - allowedEvents: ['NetworkController:stateChange'], + allowedEvents: [ + 'NetworkController:stateChange', + 'KeyringController:lock', + 'KeyringController:unlock', + ], }); } describe('DetectTokensController', function () { let sandbox, assetsContractController, - keyringMemStore, network, preferences, provider, @@ -198,7 +200,6 @@ describe('DetectTokensController', function () { .reply(200, { error: 'ChainId 3 is not supported' }) .persist(); - keyringMemStore = new ObservableStore({ isUnlocked: false }); const networkControllerMessenger = new ControllerMessenger(); network = new NetworkController({ messenger: networkControllerMessenger, @@ -275,7 +276,6 @@ describe('DetectTokensController', function () { messenger: buildMessenger(), preferences, network, - keyringMemStore, tokenList: tokenListController, tokensController, assetsContractController, @@ -312,7 +312,6 @@ describe('DetectTokensController', function () { messenger: buildMessenger(), preferences, network, - keyringMemStore, tokenList: tokenListController, tokensController, assetsContractController, @@ -336,7 +335,6 @@ describe('DetectTokensController', function () { messenger: buildMessenger(), preferences, network, - keyringMemStore, tokenList: tokenListController, tokensController, assetsContractController, @@ -388,7 +386,6 @@ describe('DetectTokensController', function () { messenger: buildMessenger(), preferences, network, - keyringMemStore, tokenList: tokenListController, tokensController, assetsContractController, @@ -447,7 +444,6 @@ describe('DetectTokensController', function () { messenger: buildMessenger(), preferences, network, - keyringMemStore, tokenList: tokenListController, tokensController, assetsContractController, @@ -463,11 +459,11 @@ describe('DetectTokensController', function () { it('should trigger detect new tokens when submit password', async function () { sandbox.useFakeTimers(); + const messenger = buildMessenger(); const controller = new DetectTokensController({ messenger: buildMessenger(), preferences, network, - keyringMemStore, tokenList: tokenListController, tokensController, assetsContractController, @@ -475,7 +471,9 @@ describe('DetectTokensController', function () { controller.isOpen = true; controller.selectedAddress = '0x0'; const stub = sandbox.stub(controller, 'detectNewTokens'); - await controller._keyringMemStore.updateState({ isUnlocked: true }); + + messenger.publish('KeyringController:unlock'); + sandbox.assert.called(stub); }); @@ -486,7 +484,6 @@ describe('DetectTokensController', function () { messenger: buildMessenger(), preferences, network, - keyringMemStore, tokenList: tokenListController, tokensController, assetsContractController, @@ -508,7 +505,6 @@ describe('DetectTokensController', function () { messenger: buildMessenger(), preferences, network, - keyringMemStore, tokensController, assetsContractController, }); diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index ee08fe92682b..fa8080932bae 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -1142,7 +1142,11 @@ export default class MetamaskController extends EventEmitter { const detectTokensControllerMessenger = this.controllerMessenger.getRestricted({ name: 'DetectTokensController', - allowedEvents: ['NetworkController:stateChange'], + allowedEvents: [ + 'NetworkController:stateChange', + 'KeyringController:lock', + 'KeyringController:unlock', + ], }); this.detectTokensController = new DetectTokensController({ messenger: detectTokensControllerMessenger, @@ -1150,7 +1154,6 @@ export default class MetamaskController extends EventEmitter { tokensController: this.tokensController, assetsContractController: this.assetsContractController, network: this.networkController, - keyringMemStore: this.keyringController.memStore, tokenList: this.tokenListController, trackMetaMetricsEvent: this.metaMetricsController.trackEvent.bind( this.metaMetricsController, From db3bd03b4a3b3c1843b1f9a9c5f07521032cd0c4 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 5 Sep 2023 14:47:40 +0200 Subject: [PATCH 2/3] fix: use getState to get initial lock state --- app/scripts/controllers/detect-tokens.js | 28 +++++++--- app/scripts/controllers/detect-tokens.test.js | 54 +++++++++++-------- app/scripts/metamask-controller.js | 1 + 3 files changed, 53 insertions(+), 30 deletions(-) diff --git a/app/scripts/controllers/detect-tokens.js b/app/scripts/controllers/detect-tokens.js index c1c7efea07e4..86c8cc222856 100644 --- a/app/scripts/controllers/detect-tokens.js +++ b/app/scripts/controllers/detect-tokens.js @@ -28,7 +28,6 @@ export default class DetectTokensController { * @param config.interval * @param config.preferences * @param config.network - * @param config.keyringMemStore * @param config.tokenList * @param config.tokensController * @param config.assetsContractController @@ -89,13 +88,8 @@ export default class DetectTokensController { this.restartTokenDetection({ chainId: this.chainId }); } }); - messenger.subscribe('KeyringController:unlock', () => { - this.isUnlocked = true; - this.restartTokenDetection(); - }); - messenger.subscribe('KeyringController:lock', () => { - this.isUnlocked = false; - }); + + this.#registerKeyringHandlers(); } /** @@ -260,4 +254,22 @@ export default class DetectTokensController { return this.isOpen && this.isUnlocked; } /* eslint-enable accessor-pairs */ + + /** + * Constructor helper to register listeners on the keyring + * locked state changes + */ + #registerKeyringHandlers() { + const { isUnlocked } = this.messenger.call('KeyringController:getState'); + this.isUnlocked = isUnlocked; + + this.messenger.subscribe('KeyringController:unlock', () => { + this.isUnlocked = true; + this.restartTokenDetection(); + }); + + this.messenger.subscribe('KeyringController:lock', () => { + this.isUnlocked = false; + }); + } } diff --git a/app/scripts/controllers/detect-tokens.test.js b/app/scripts/controllers/detect-tokens.test.js index 25a959cf9102..0db3be07e5c4 100644 --- a/app/scripts/controllers/detect-tokens.test.js +++ b/app/scripts/controllers/detect-tokens.test.js @@ -15,17 +15,6 @@ import { toChecksumHexAddress } from '../../../shared/modules/hexstring-utils'; import DetectTokensController from './detect-tokens'; import PreferencesController from './preferences'; -function buildMessenger() { - return new ControllerMessenger().getRestricted({ - name: 'DetectTokensController', - allowedEvents: [ - 'NetworkController:stateChange', - 'KeyringController:lock', - 'KeyringController:unlock', - ], - }); -} - describe('DetectTokensController', function () { let sandbox, assetsContractController, @@ -33,10 +22,23 @@ describe('DetectTokensController', function () { preferences, provider, tokensController, - tokenListController; + tokenListController, + messenger; const noop = () => undefined; + const getRestrictedMessenger = () => { + return messenger.getRestricted({ + name: 'DetectTokensController', + allowedActions: ['KeyringController:getState'], + allowedEvents: [ + 'NetworkController:stateChange', + 'KeyringController:lock', + 'KeyringController:unlock', + ], + }); + }; + const networkControllerProviderConfig = { getAccounts: noop, }; @@ -200,6 +202,11 @@ describe('DetectTokensController', function () { .reply(200, { error: 'ChainId 3 is not supported' }) .persist(); + messenger = new ControllerMessenger(); + messenger.registerActionHandler('KeyringController:getState', () => ({ + isUnlocked: true, + })); + const networkControllerMessenger = new ControllerMessenger(); network = new NetworkController({ messenger: networkControllerMessenger, @@ -264,7 +271,11 @@ describe('DetectTokensController', function () { it('should poll on correct interval', async function () { const stub = sinon.stub(global, 'setInterval'); - new DetectTokensController({ messenger: buildMessenger(), interval: 1337 }); // eslint-disable-line no-new + // eslint-disable-next-line no-new + new DetectTokensController({ + messenger: getRestrictedMessenger(), + interval: 1337, + }); assert.strictEqual(stub.getCall(0).args[1], 1337); stub.restore(); }); @@ -273,7 +284,7 @@ describe('DetectTokensController', function () { const clock = sandbox.useFakeTimers(); await network.setProviderType(NETWORK_TYPES.MAINNET); const controller = new DetectTokensController({ - messenger: buildMessenger(), + messenger: getRestrictedMessenger(), preferences, network, tokenList: tokenListController, @@ -309,7 +320,7 @@ describe('DetectTokensController', function () { }); await tokenListController.start(); const controller = new DetectTokensController({ - messenger: buildMessenger(), + messenger: getRestrictedMessenger(), preferences, network, tokenList: tokenListController, @@ -332,7 +343,7 @@ describe('DetectTokensController', function () { sandbox.useFakeTimers(); await network.setProviderType(NETWORK_TYPES.MAINNET); const controller = new DetectTokensController({ - messenger: buildMessenger(), + messenger: getRestrictedMessenger(), preferences, network, tokenList: tokenListController, @@ -383,7 +394,7 @@ describe('DetectTokensController', function () { sandbox.useFakeTimers(); await network.setProviderType(NETWORK_TYPES.MAINNET); const controller = new DetectTokensController({ - messenger: buildMessenger(), + messenger: getRestrictedMessenger(), preferences, network, tokenList: tokenListController, @@ -441,7 +452,7 @@ describe('DetectTokensController', function () { it('should trigger detect new tokens when change address', async function () { sandbox.useFakeTimers(); const controller = new DetectTokensController({ - messenger: buildMessenger(), + messenger: getRestrictedMessenger(), preferences, network, tokenList: tokenListController, @@ -459,9 +470,8 @@ describe('DetectTokensController', function () { it('should trigger detect new tokens when submit password', async function () { sandbox.useFakeTimers(); - const messenger = buildMessenger(); const controller = new DetectTokensController({ - messenger: buildMessenger(), + messenger: getRestrictedMessenger(), preferences, network, tokenList: tokenListController, @@ -481,7 +491,7 @@ describe('DetectTokensController', function () { const clock = sandbox.useFakeTimers(); await network.setProviderType(NETWORK_TYPES.MAINNET); const controller = new DetectTokensController({ - messenger: buildMessenger(), + messenger: getRestrictedMessenger(), preferences, network, tokenList: tokenListController, @@ -502,7 +512,7 @@ describe('DetectTokensController', function () { const clock = sandbox.useFakeTimers(); await network.setProviderType(NETWORK_TYPES.MAINNET); const controller = new DetectTokensController({ - messenger: buildMessenger(), + messenger: getRestrictedMessenger(), preferences, network, tokensController, diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index fa8080932bae..fd5c6427755f 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -1142,6 +1142,7 @@ export default class MetamaskController extends EventEmitter { const detectTokensControllerMessenger = this.controllerMessenger.getRestricted({ name: 'DetectTokensController', + allowedActions: ['KeyringController:getState'], allowedEvents: [ 'NetworkController:stateChange', 'KeyringController:lock', From c3ca98cecdf7ac05ac69f37c5acf0355e43ba271 Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Tue, 5 Sep 2023 16:00:25 +0200 Subject: [PATCH 3/3] test: add case for lock event --- app/scripts/controllers/detect-tokens.test.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/app/scripts/controllers/detect-tokens.test.js b/app/scripts/controllers/detect-tokens.test.js index 0db3be07e5c4..c7306dc83295 100644 --- a/app/scripts/controllers/detect-tokens.test.js +++ b/app/scripts/controllers/detect-tokens.test.js @@ -485,6 +485,25 @@ describe('DetectTokensController', function () { messenger.publish('KeyringController:unlock'); sandbox.assert.called(stub); + assert.equal(controller.isUnlocked, true); + }); + + it('should not be active after lock event is emitted', async function () { + sandbox.useFakeTimers(); + const controller = new DetectTokensController({ + messenger: getRestrictedMessenger(), + preferences, + network, + tokenList: tokenListController, + tokensController, + assetsContractController, + }); + controller.isOpen = true; + + messenger.publish('KeyringController:lock'); + + assert.equal(controller.isUnlocked, false); + assert.equal(controller.isActive, false); }); it('should not trigger detect new tokens when not unlocked', async function () {