Skip to content

Commit

Permalink
feat: preferences controller to base controller v2 (#27398)
Browse files Browse the repository at this point in the history
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**
In this PR, we want to bring PreferencesController up to date with our
latest controller patterns by upgrading to BaseControllerV2.
<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27398?quickstart=1)

## **Related issues**

Fixes: #25917 

## **Manual testing steps**
Use case 1
1. Install the previous release
2. Complete user onboarding
3. Go to settings and change couple of user settings. For example
language, currency and theme.
4. Close and disable MM in the extension
5. Checkout the version with these changes
6. Build and login
7. Make sure, the user preferences set earlier are still there

Use case 2
1. Disable all the MM extensions
2. Install the version with these changes
3. When you click on MM, the default language should be English
4. Complete user onboarding
5. Go to settings and change couple of user settings. For example
language, currency and theme.
6. Close and disable and enable the MM in extension. which forces user
to login MM in the extension
7. Once you login again, make sure, the user preferences set earlier are
still there

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: Michele Esposito <34438276+mikesposito@users.noreply.github.com>
Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com>
Co-authored-by: cryptodev-2s <109512101+cryptodev-2s@users.noreply.github.com>
  • Loading branch information
4 people authored Oct 14, 2024
1 parent acbb17e commit cedabc6
Show file tree
Hide file tree
Showing 33 changed files with 1,147 additions and 646 deletions.
2 changes: 1 addition & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ module.exports = {
'app/scripts/controllers/swaps/**/*.test.ts',
'app/scripts/controllers/metametrics.test.js',
'app/scripts/controllers/permissions/**/*.test.js',
'app/scripts/controllers/preferences.test.js',
'app/scripts/controllers/preferences-controller.test.ts',
'app/scripts/lib/**/*.test.js',
'app/scripts/metamask-controller.test.js',
'app/scripts/migrations/*.test.js',
Expand Down
5 changes: 2 additions & 3 deletions app/scripts/background.js
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ function maybeDetectPhishing(theController) {
return {};
}

const prefState = theController.preferencesController.store.getState();
const prefState = theController.preferencesController.state;
if (!prefState.usePhishDetect) {
return {};
}
Expand Down Expand Up @@ -758,8 +758,7 @@ export function setupController(
controller.preferencesController,
),
getUseAddressBarEnsResolution: () =>
controller.preferencesController.store.getState()
.useAddressBarEnsResolution,
controller.preferencesController.state.useAddressBarEnsResolution,
provider: controller.provider,
});

Expand Down
11 changes: 3 additions & 8 deletions app/scripts/controllers/account-tracker-controller.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { BlockTracker, Provider } from '@metamask/network-controller';

import { flushPromises } from '../../../test/lib/timer-helpers';
import { createTestProviderTools } from '../../../test/stub/provider';
import PreferencesController from './preferences-controller';
import type {
AccountTrackerControllerOptions,
AllowedActions,
Expand Down Expand Up @@ -166,13 +165,9 @@ function withController<ReturnValue>(
provider: provider as Provider,
blockTracker: blockTrackerStub as unknown as BlockTracker,
getNetworkIdentifier: jest.fn(),
preferencesController: {
store: {
getState: () => ({
useMultiAccountBalanceChecker,
}),
},
} as PreferencesController,
preferencesControllerState: {
useMultiAccountBalanceChecker,
},
messenger: controllerMessenger.getRestricted({
name: 'AccountTrackerController',
allowedActions: [
Expand Down
18 changes: 8 additions & 10 deletions app/scripts/controllers/account-tracker-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ import type {
OnboardingControllerGetStateAction,
OnboardingControllerStateChangeEvent,
} from './onboarding';
import PreferencesController from './preferences-controller';
import { PreferencesControllerState } from './preferences-controller';

// Unique name for the controller
const controllerName = 'AccountTrackerController';
Expand Down Expand Up @@ -170,7 +170,7 @@ export type AccountTrackerControllerOptions = {
provider: Provider;
blockTracker: BlockTracker;
getNetworkIdentifier: (config?: NetworkClientConfiguration) => string;
preferencesController: PreferencesController;
preferencesControllerState: Partial<PreferencesControllerState>;
};

/**
Expand Down Expand Up @@ -198,7 +198,7 @@ export default class AccountTrackerController extends BaseController<

#getNetworkIdentifier: AccountTrackerControllerOptions['getNetworkIdentifier'];

#preferencesController: AccountTrackerControllerOptions['preferencesController'];
#preferencesControllerState: AccountTrackerControllerOptions['preferencesControllerState'];

#selectedAccount: InternalAccount;

Expand All @@ -209,7 +209,7 @@ export default class AccountTrackerController extends BaseController<
* @param options.provider - An EIP-1193 provider instance that uses the current global network
* @param options.blockTracker - A block tracker, which emits events for each new block
* @param options.getNetworkIdentifier - A function that returns the current network or passed network configuration
* @param options.preferencesController - The preferences controller
* @param options.preferencesControllerState - The state of preferences controller
*/
constructor(options: AccountTrackerControllerOptions) {
super({
Expand All @@ -226,7 +226,7 @@ export default class AccountTrackerController extends BaseController<
this.#blockTracker = options.blockTracker;

this.#getNetworkIdentifier = options.getNetworkIdentifier;
this.#preferencesController = options.preferencesController;
this.#preferencesControllerState = options.preferencesControllerState;

// subscribe to account removal
this.messagingSystem.subscribe(
Expand Down Expand Up @@ -257,7 +257,7 @@ export default class AccountTrackerController extends BaseController<
'AccountsController:selectedEvmAccountChange',
(newAccount) => {
const { useMultiAccountBalanceChecker } =
this.#preferencesController.store.getState();
this.#preferencesControllerState;

if (
this.#selectedAccount.id !== newAccount.id &&
Expand Down Expand Up @@ -672,8 +672,7 @@ export default class AccountTrackerController extends BaseController<

const { chainId, provider, identifier } =
this.#getCorrectNetworkClient(networkClientId);
const { useMultiAccountBalanceChecker } =
this.#preferencesController.store.getState();
const { useMultiAccountBalanceChecker } = this.#preferencesControllerState;

let addresses = [];
if (useMultiAccountBalanceChecker) {
Expand Down Expand Up @@ -724,8 +723,7 @@ export default class AccountTrackerController extends BaseController<
provider: Provider,
chainId: Hex,
): Promise<void> {
const { useMultiAccountBalanceChecker } =
this.#preferencesController.store.getState();
const { useMultiAccountBalanceChecker } = this.#preferencesControllerState;

let balance = '0x0';

Expand Down
23 changes: 15 additions & 8 deletions app/scripts/controllers/app-state.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export default class AppStateController extends EventEmitter {
isUnlocked,
initState,
onInactiveTimeout,
preferencesStore,
preferencesController,
messenger,
extension,
} = opts;
Expand Down Expand Up @@ -86,12 +86,18 @@ export default class AppStateController extends EventEmitter {
this.waitingForUnlock = [];
addUnlockListener(this.handleUnlock.bind(this));

preferencesStore.subscribe(({ preferences }) => {
const currentState = this.store.getState();
if (currentState.timeoutMinutes !== preferences.autoLockTimeLimit) {
this._setInactiveTimeout(preferences.autoLockTimeLimit);
}
});
messenger.subscribe(
'PreferencesController:stateChange',
({ preferences }) => {
const currentState = this.store.getState();
if (
preferences &&
currentState.timeoutMinutes !== preferences.autoLockTimeLimit
) {
this._setInactiveTimeout(preferences.autoLockTimeLimit);
}
},
);

messenger.subscribe(
'KeyringController:qrKeyringStateChange',
Expand All @@ -101,7 +107,8 @@ export default class AppStateController extends EventEmitter {
}),
);

const { preferences } = preferencesStore.getState();
const { preferences } = preferencesController.state;

this._setInactiveTimeout(preferences.autoLockTimeLimit);

this.messagingSystem = messenger;
Expand Down
7 changes: 3 additions & 4 deletions app/scripts/controllers/app-state.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,12 @@ describe('AppStateController', () => {
initState,
onInactiveTimeout: jest.fn(),
showUnlockRequest: jest.fn(),
preferencesStore: {
subscribe: jest.fn(),
getState: jest.fn(() => ({
preferencesController: {
state: {
preferences: {
autoLockTimeLimit: 0,
},
})),
},
},
messenger: {
call: jest.fn(() => ({
Expand Down
17 changes: 9 additions & 8 deletions app/scripts/controllers/metametrics.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,9 @@ export default class MetaMetricsController {
* @param {object} options
* @param {object} options.segment - an instance of analytics for tracking
* events that conform to the new MetaMetrics tracking plan.
* @param {object} options.preferencesStore - The preferences controller store, used
* to access and subscribe to preferences that will be attached to events
* @param {object} options.preferencesControllerState - The state of preferences controller
* @param {Function} options.onPreferencesStateChange - Used to attach a listener to the
* stateChange event emitted by the PreferencesController
* @param {Function} options.onNetworkDidChange - Used to attach a listener to the
* networkDidChange event emitted by the networkController
* @param {Function} options.getCurrentChainId - Gets the current chain id from the
Expand All @@ -132,7 +133,8 @@ export default class MetaMetricsController {
*/
constructor({
segment,
preferencesStore,
preferencesControllerState,
onPreferencesStateChange,
onNetworkDidChange,
getCurrentChainId,
version,
Expand All @@ -148,16 +150,15 @@ export default class MetaMetricsController {
captureException(err);
}
};
const prefState = preferencesStore.getState();
this.chainId = getCurrentChainId();
this.locale = prefState.currentLocale.replace('_', '-');
this.locale = preferencesControllerState.currentLocale.replace('_', '-');
this.version =
environment === 'production' ? version : `${version}-${environment}`;
this.extension = extension;
this.environment = environment;

///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
this.selectedAddress = prefState.selectedAddress;
this.selectedAddress = preferencesControllerState.selectedAddress;
///: END:ONLY_INCLUDE_IF

const abandonedFragments = omitBy(initState?.fragments, 'persist');
Expand All @@ -181,8 +182,8 @@ export default class MetaMetricsController {
},
});

preferencesStore.subscribe(({ currentLocale }) => {
this.locale = currentLocale.replace('_', '-');
onPreferencesStateChange(({ currentLocale }) => {
this.locale = currentLocale?.replace('_', '-');
});

onNetworkDidChange(() => {
Expand Down
49 changes: 24 additions & 25 deletions app/scripts/controllers/metametrics.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,22 +74,6 @@ const DEFAULT_PAGE_PROPERTIES = {
...DEFAULT_SHARED_PROPERTIES,
};

function getMockPreferencesStore({ currentLocale = LOCALE } = {}) {
let preferencesStore = {
currentLocale,
};
const subscribe = jest.fn();
const updateState = (newState) => {
preferencesStore = { ...preferencesStore, ...newState };
subscribe.mock.calls[0][0](preferencesStore);
};
return {
getState: jest.fn().mockReturnValue(preferencesStore),
updateState,
subscribe,
};
}

const SAMPLE_PERSISTED_EVENT = {
id: 'testid',
persist: true,
Expand Down Expand Up @@ -117,7 +101,10 @@ function getMetaMetricsController({
participateInMetaMetrics = true,
metaMetricsId = TEST_META_METRICS_ID,
marketingCampaignCookieId = null,
preferencesStore = getMockPreferencesStore(),
preferencesControllerState = { currentLocale: LOCALE },
onPreferencesStateChange = () => {
// do nothing
},
getCurrentChainId = () => FAKE_CHAIN_ID,
onNetworkDidChange = () => {
// do nothing
Expand All @@ -128,7 +115,8 @@ function getMetaMetricsController({
segment: segmentInstance || segment,
getCurrentChainId,
onNetworkDidChange,
preferencesStore,
preferencesControllerState,
onPreferencesStateChange,
version: '0.0.1',
environment: 'test',
initState: {
Expand Down Expand Up @@ -209,11 +197,16 @@ describe('MetaMetricsController', function () {
});

it('should update when preferences changes', function () {
const preferencesStore = getMockPreferencesStore();
let subscribeListener;
const onPreferencesStateChange = (listener) => {
subscribeListener = listener;
};
const metaMetricsController = getMetaMetricsController({
preferencesStore,
preferencesControllerState: { currentLocale: LOCALE },
onPreferencesStateChange,
});
preferencesStore.updateState({ currentLocale: 'en_UK' });

subscribeListener({ currentLocale: 'en_UK' });
expect(metaMetricsController.locale).toStrictEqual('en-UK');
});
});
Expand Down Expand Up @@ -732,9 +725,11 @@ describe('MetaMetricsController', function () {

it('should track a page view if isOptInPath is true and user not yet opted in', function () {
const metaMetricsController = getMetaMetricsController({
preferencesStore: getMockPreferencesStore({
preferencesControllerState: {
currentLocale: LOCALE,
participateInMetaMetrics: null,
}),
},
onPreferencesStateChange: jest.fn(),
});
const spy = jest.spyOn(segment, 'page');
metaMetricsController.trackPage(
Expand All @@ -746,6 +741,7 @@ describe('MetaMetricsController', function () {
},
{ isOptInPath: true },
);

expect(spy).toHaveBeenCalledTimes(1);
expect(spy).toHaveBeenCalledWith(
{
Expand All @@ -765,9 +761,11 @@ describe('MetaMetricsController', function () {

it('multiple trackPage call with same actionId should result in same messageId being sent to segment', function () {
const metaMetricsController = getMetaMetricsController({
preferencesStore: getMockPreferencesStore({
preferencesControllerState: {
currentLocale: LOCALE,
participateInMetaMetrics: null,
}),
},
onPreferencesStateChange: jest.fn(),
});
const spy = jest.spyOn(segment, 'page');
metaMetricsController.trackPage(
Expand All @@ -790,6 +788,7 @@ describe('MetaMetricsController', function () {
},
{ isOptInPath: true },
);

expect(spy).toHaveBeenCalledTimes(2);
expect(spy).toHaveBeenCalledWith(
{
Expand Down
13 changes: 6 additions & 7 deletions app/scripts/controllers/mmi-controller.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,10 +203,10 @@ describe('MMIController', function () {
});

metaMetricsController = new MetaMetricsController({
preferencesStore: {
getState: jest.fn().mockReturnValue({ currentLocale: 'en' }),
subscribe: jest.fn(),
preferencesControllerState: {
currentLocale: 'en'
},
onPreferencesStateChange: jest.fn(),
getCurrentChainId: jest.fn(),
onNetworkDidChange: jest.fn(),
});
Expand Down Expand Up @@ -245,13 +245,12 @@ describe('MMIController', function () {
initState: {},
onInactiveTimeout: jest.fn(),
showUnlockRequest: jest.fn(),
preferencesStore: {
subscribe: jest.fn(),
getState: jest.fn(() => ({
preferencesController: {
state: {
preferences: {
autoLockTimeLimit: 0,
},
})),
},
},
messenger: mockMessenger,
}),
Expand Down
2 changes: 1 addition & 1 deletion app/scripts/controllers/mmi-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ import {
import { getCurrentChainId } from '../../../ui/selectors';
import MetaMetricsController from './metametrics';
import { getPermissionBackgroundApiMethods } from './permissions';
import { PreferencesController } from './preferences-controller';
import AccountTrackerController from './account-tracker-controller';
import PreferencesController from './preferences-controller';
import { AppStateController } from './app-state';

type UpdateCustodianTransactionsParameters = {
Expand Down
Loading

0 comments on commit cedabc6

Please sign in to comment.