From 614647e9a87c27bd2f7bd6fd01ce364bc6fb2c15 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Thu, 21 Aug 2025 14:40:10 -0230 Subject: [PATCH 01/10] feat: Add additional controller metadata Add the `includeInStateLogs` and `usedInUi` state metadata properties, and rename `anonymous` to `includeInDebugSnapshot`. This is the implementation of the second option of this ADR: https://github.com/MetaMask/decisions/pull/101 Related to https://github.com/MetaMask/decisions/pull/101 Fixes #643 --- .../src/next/BaseController.test.ts | 688 +++++++++++++----- .../src/next/BaseController.ts | 63 +- 2 files changed, 542 insertions(+), 209 deletions(-) diff --git a/packages/base-controller/src/next/BaseController.test.ts b/packages/base-controller/src/next/BaseController.test.ts index a7b6d37bcf1..d6231c2b3ec 100644 --- a/packages/base-controller/src/next/BaseController.test.ts +++ b/packages/base-controller/src/next/BaseController.test.ts @@ -1,5 +1,6 @@ /* eslint-disable jest/no-export */ import { Messenger } from '@metamask/messenger'; +import type { Json } from '@metamask/utils'; import type { Draft, Patch } from 'immer'; import * as sinon from 'sinon'; @@ -8,13 +9,16 @@ import type { ControllerEvents, ControllerGetStateAction, ControllerStateChangeEvent, + StatePropertyMetadata, } from './BaseController'; import { BaseController, getAnonymizedState, getPersistentState, + deriveStateFromMetadata, } from './BaseController'; + export const countControllerName = 'CountController'; type CountControllerState = { @@ -33,8 +37,10 @@ export type CountControllerEvent = ControllerStateChangeEvent< export const countControllerStateMetadata = { count: { + includeInDebugSnapshot: true, + includeInStateLogs: true, persist: true, - anonymous: true, + usedInUi: true, }, }; @@ -104,8 +110,10 @@ type MessagesControllerEvent = ControllerStateChangeEvent< const messagesControllerStateMetadata = { messages: { + includeInDebugSnapshot: true, + includeInStateLogs: true, persist: true, - anonymous: true, + usedInUi: true, }, }; @@ -573,6 +581,204 @@ describe('BaseController', () => { expect(listener1.callCount).toBe(0); expect(listener2.callCount).toBe(0); }); + + describe('inter-controller communication', () => { + // These two contrived mock controllers are setup to test with. + // The 'VisitorController' records strings that represent visitors. + // The 'VisitorOverflowController' monitors the 'VisitorController' to ensure the number of + // visitors doesn't exceed the maximum capacity. If it does, it will clear out all visitors. + + const visitorName = 'VisitorController'; + + type VisitorControllerState = { + visitors: string[]; + }; + type VisitorControllerClearAction = { + type: `${typeof visitorName}:clear`; + handler: () => void; + }; + type VisitorExternalActions = VisitorOverflowUpdateMaxAction; + type VisitorControllerActions = + | VisitorControllerClearAction + | ControllerActions; + type VisitorControllerStateChangeEvent = ControllerEvents< + typeof visitorName, + VisitorControllerState + >; + type VisitorExternalEvents = VisitorOverflowStateChangeEvent; + type VisitorControllerEvents = VisitorControllerStateChangeEvent; + + const visitorControllerStateMetadata = { + visitors: { + includeInDebugSnapshot: true, + includeInStateLogs: true, + persist: true, + usedInUi: true, + }, + }; + + type VisitorMessenger = Messenger< + typeof visitorName, + VisitorControllerActions | VisitorExternalActions, + VisitorControllerEvents | VisitorExternalEvents + >; + class VisitorController extends BaseController< + typeof visitorName, + VisitorControllerState, + VisitorMessenger + > { + constructor(messenger: VisitorMessenger) { + super({ + messenger, + metadata: visitorControllerStateMetadata, + name: visitorName, + state: { visitors: [] }, + }); + + messenger.registerActionHandler('VisitorController:clear', this.clear); + } + + clear = () => { + this.update(() => { + return { visitors: [] }; + }); + }; + + addVisitor(visitor: string) { + this.update(({ visitors }) => { + return { visitors: [...visitors, visitor] }; + }); + } + + destroy() { + super.destroy(); + } + } + + const visitorOverflowName = 'VisitorOverflowController'; + + type VisitorOverflowControllerState = { + maxVisitors: number; + }; + type VisitorOverflowUpdateMaxAction = { + type: `${typeof visitorOverflowName}:updateMax`; + handler: (max: number) => void; + }; + type VisitorOverflowExternalActions = VisitorControllerClearAction; + type VisitorOverflowControllerActions = + | VisitorOverflowUpdateMaxAction + | ControllerActions< + typeof visitorOverflowName, + VisitorOverflowControllerState + >; + type VisitorOverflowStateChangeEvent = ControllerEvents< + typeof visitorOverflowName, + VisitorOverflowControllerState + >; + type VisitorOverflowExternalEvents = VisitorControllerStateChangeEvent; + type VisitorOverflowControllerEvents = VisitorOverflowStateChangeEvent; + + const visitorOverflowControllerMetadata = { + maxVisitors: { + includeInDebugSnapshot: true, + includeInStateLogs: true, + persist: false, + usedInUi: true, + }, + }; + + type VisitorOverflowMessenger = Messenger< + typeof visitorOverflowName, + VisitorOverflowControllerActions | VisitorOverflowExternalActions, + VisitorOverflowControllerEvents | VisitorOverflowExternalEvents + >; + + class VisitorOverflowController extends BaseController< + typeof visitorOverflowName, + VisitorOverflowControllerState, + VisitorOverflowMessenger + > { + constructor(messenger: VisitorOverflowMessenger) { + super({ + messenger, + metadata: visitorOverflowControllerMetadata, + name: visitorOverflowName, + state: { maxVisitors: 5 }, + }); + + messenger.registerActionHandler( + 'VisitorOverflowController:updateMax', + this.updateMax, + ); + + messenger.subscribe('VisitorController:stateChange', this.onVisit); + } + + onVisit = ({ visitors }: VisitorControllerState) => { + if (visitors.length > this.state.maxVisitors) { + this.messenger.call('VisitorController:clear'); + } + }; + + updateMax = (max: number) => { + this.update(() => { + return { maxVisitors: max }; + }); + }; + + destroy() { + super.destroy(); + } + } + + it('should allow messaging between controllers', () => { + // Construct root messenger + const rootMessenger = new Messenger< + 'Root', + VisitorControllerActions | VisitorOverflowControllerActions, + VisitorControllerEvents | VisitorOverflowControllerEvents + >({ namespace: 'Root' }); + // Construct controller messengers, delegating to parent + const visitorControllerMessenger = new Messenger< + typeof visitorName, + VisitorControllerActions | VisitorOverflowUpdateMaxAction, + VisitorControllerEvents | VisitorOverflowStateChangeEvent, + typeof rootMessenger + >({ namespace: visitorName, parent: rootMessenger }); + const visitorOverflowControllerMessenger = new Messenger< + typeof visitorOverflowName, + VisitorOverflowControllerActions | VisitorControllerClearAction, + VisitorOverflowControllerEvents | VisitorControllerStateChangeEvent, + typeof rootMessenger + >({ namespace: visitorOverflowName, parent: rootMessenger }); + // Delegate external actions/events to controller messengers + rootMessenger.delegate({ + actions: ['VisitorController:clear'], + events: ['VisitorController:stateChange'], + messenger: visitorOverflowControllerMessenger, + }); + rootMessenger.delegate({ + actions: ['VisitorOverflowController:updateMax'], + events: ['VisitorOverflowController:stateChange'], + messenger: visitorControllerMessenger, + }); + // Construct controllers + const visitorController = new VisitorController( + visitorControllerMessenger, + ); + const visitorOverflowController = new VisitorOverflowController( + visitorOverflowControllerMessenger, + ); + + rootMessenger.call('VisitorOverflowController:updateMax', 2); + visitorController.addVisitor('A'); + visitorController.addVisitor('B'); + visitorController.addVisitor('C'); // this should trigger an overflow + + expect(visitorOverflowController.state.maxVisitors).toBe(2); + expect(visitorController.state.visitors).toHaveLength(0); + }); + }); }); describe('getAnonymizedState', () => { @@ -587,7 +793,14 @@ describe('getAnonymizedState', () => { it('should return empty state when no properties are anonymized', () => { const anonymizedState = getAnonymizedState( { count: 1 }, - { count: { anonymous: false, persist: false } }, + { + count: { + includeInDebugSnapshot: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + }, + }, ); expect(anonymizedState).toStrictEqual({}); }); @@ -602,20 +815,28 @@ describe('getAnonymizedState', () => { }, { password: { - anonymous: false, + includeInDebugSnapshot: false, + includeInStateLogs: false, persist: false, + usedInUi: false, }, privateKey: { - anonymous: false, + includeInDebugSnapshot: false, + includeInStateLogs: false, persist: false, + usedInUi: false, }, network: { - anonymous: true, + includeInDebugSnapshot: true, + includeInStateLogs: false, persist: false, + usedInUi: false, }, tokens: { - anonymous: true, + includeInDebugSnapshot: true, + includeInStateLogs: false, persist: false, + usedInUi: false, }, }, ); @@ -636,8 +857,10 @@ describe('getAnonymizedState', () => { }, { transactionHash: { - anonymous: anonymizeTransactionHash, + includeInDebugSnapshot: anonymizeTransactionHash, + includeInStateLogs: false, persist: false, + usedInUi: false, }, }, ); @@ -659,8 +882,10 @@ describe('getAnonymizedState', () => { }, { txMeta: { - anonymous: anonymizeTxMeta, + includeInDebugSnapshot: anonymizeTxMeta, + includeInStateLogs: false, persist: false, + usedInUi: false, }, }, ); @@ -697,8 +922,10 @@ describe('getAnonymizedState', () => { }, { txMeta: { - anonymous: anonymizeTxMeta, + includeInDebugSnapshot: anonymizeTxMeta, + includeInStateLogs: false, persist: false, + usedInUi: false, }, }, ); @@ -715,8 +942,10 @@ describe('getAnonymizedState', () => { }, { count: { - anonymous: (count) => Number(count), + includeInDebugSnapshot: (count) => Number(count), + includeInStateLogs: false, persist: false, + usedInUi: false, }, }, ); @@ -735,12 +964,16 @@ describe('getAnonymizedState', () => { // @ts-expect-error Intentionally testing invalid state { privateKey: { - anonymous: true, + includeInDebugSnapshot: true, + includeInStateLogs: true, persist: true, + usedInUi: true, }, network: { - anonymous: false, + includeInDebugSnapshot: false, + includeInStateLogs: false, persist: false, + usedInUi: false, }, }, ); @@ -765,7 +998,14 @@ describe('getPersistentState', () => { it('should return empty state when no properties are persistent', () => { const persistentState = getPersistentState( { count: 1 }, - { count: { anonymous: false, persist: false } }, + { + count: { + includeInDebugSnapshot: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + }, + }, ); expect(persistentState).toStrictEqual({}); }); @@ -780,20 +1020,28 @@ describe('getPersistentState', () => { }, { password: { - anonymous: false, + includeInDebugSnapshot: false, + includeInStateLogs: false, persist: true, + usedInUi: false, }, privateKey: { - anonymous: false, + includeInDebugSnapshot: false, + includeInStateLogs: false, persist: true, + usedInUi: false, }, network: { - anonymous: false, + includeInDebugSnapshot: false, + includeInStateLogs: false, persist: false, + usedInUi: false, }, tokens: { - anonymous: false, + includeInDebugSnapshot: false, + includeInStateLogs: false, persist: false, + usedInUi: false, }, }, ); @@ -814,8 +1062,10 @@ describe('getPersistentState', () => { }, { transactionHash: { - anonymous: false, + includeInDebugSnapshot: false, + includeInStateLogs: false, persist: normalizeTransacitonHash, + usedInUi: false, }, }, ); @@ -837,8 +1087,10 @@ describe('getPersistentState', () => { }, { txMeta: { - anonymous: false, + includeInDebugSnapshot: false, + includeInStateLogs: false, persist: getPersistentTxMeta, + usedInUi: false, }, }, ); @@ -875,8 +1127,10 @@ describe('getPersistentState', () => { }, { txMeta: { - anonymous: false, + includeInDebugSnapshot: false, + includeInStateLogs: false, persist: getPersistentTxMeta, + usedInUi: false, }, }, ); @@ -893,8 +1147,10 @@ describe('getPersistentState', () => { }, { count: { - anonymous: false, + includeInDebugSnapshot: false, + includeInStateLogs: false, persist: (count) => Number(count), + usedInUi: false, }, }, ); @@ -913,12 +1169,16 @@ describe('getPersistentState', () => { // @ts-expect-error Intentionally testing invalid state { privateKey: { - anonymous: false, + includeInDebugSnapshot: false, + includeInStateLogs: false, persist: true, + usedInUi: false, }, network: { - anonymous: false, + includeInDebugSnapshot: false, + includeInStateLogs: false, persist: false, + usedInUi: true, }, }, ); @@ -929,198 +1189,240 @@ describe('getPersistentState', () => { const onTimeout = setTimeoutStub.firstCall.args[0]; expect(() => onTimeout()).toThrow(`No metadata found for 'extraState'`); }); +}); - describe('inter-controller communication', () => { - // These two contrived mock controllers are setup to test with. - // The 'VisitorController' records strings that represent visitors. - // The 'VisitorOverflowController' monitors the 'VisitorController' to ensure the number of - // visitors doesn't exceed the maximum capacity. If it does, it will clear out all visitors. +describe('deriveStateFromMetadata', () => { + afterEach(() => { + sinon.restore(); + }); - const visitorName = 'VisitorController'; + describe.each([ + 'includeInDebugSnapshot', + 'includeInStateLogs', + 'persist', + 'usedInUi', + ] as const)('%s', (property: keyof StatePropertyMetadata) => { + it('should return empty state', () => { + expect(deriveStateFromMetadata({}, {}, property)).toStrictEqual({}); + }); - type VisitorControllerState = { - visitors: string[]; - }; - type VisitorControllerClearAction = { - type: `${typeof visitorName}:clear`; - handler: () => void; - }; - type VisitorExternalActions = VisitorOverflowUpdateMaxAction; - type VisitorControllerActions = - | VisitorControllerClearAction - | ControllerActions; - type VisitorControllerStateChangeEvent = ControllerEvents< - typeof visitorName, - VisitorControllerState - >; - type VisitorExternalEvents = VisitorOverflowStateChangeEvent; - type VisitorControllerEvents = VisitorControllerStateChangeEvent; + it('should return empty state when no properties are enabled', () => { + const derivedState = deriveStateFromMetadata( + { count: 1 }, + { + count: { + includeInDebugSnapshot: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: false, + }, + }, + property, + ); - const visitorControllerStateMetadata = { - visitors: { - persist: true, - anonymous: true, - }, - }; + expect(derivedState).toStrictEqual({}); + }); - type VisitorMessenger = Messenger< - typeof visitorName, - VisitorControllerActions | VisitorExternalActions, - VisitorControllerEvents | VisitorExternalEvents - >; - class VisitorController extends BaseController< - typeof visitorName, - VisitorControllerState, - VisitorMessenger - > { - constructor(messenger: VisitorMessenger) { - super({ - messenger, - metadata: visitorControllerStateMetadata, - name: visitorName, - state: { visitors: [] }, - }); + it('should return derived state', () => { + const derivedState = deriveStateFromMetadata( + { + password: 'secret password', + privateKey: '123', + network: 'mainnet', + tokens: ['DAI', 'USDC'], + }, + { + password: { + includeInDebugSnapshot: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: true, + }, + privateKey: { + includeInDebugSnapshot: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: true, + }, + network: { + includeInDebugSnapshot: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: false, + }, + tokens: { + includeInDebugSnapshot: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: false, + }, + }, + property, + ); - messenger.registerActionHandler('VisitorController:clear', this.clear); - } + expect(derivedState).toStrictEqual({ + password: 'secret password', + privateKey: '123', + }); + }); - clear = () => { - this.update(() => { - return { visitors: [] }; - }); - }; + if (property !== 'usedInUi') { + it('should use function to derive state', () => { + const normalizeTransactionHash = (hash: string) => { + return hash.toLowerCase(); + }; - addVisitor(visitor: string) { - this.update(({ visitors }) => { - return { visitors: [...visitors, visitor] }; - }); - } + const derivedState = deriveStateFromMetadata( + { + transactionHash: '0X1234', + }, + { + transactionHash: { + includeInDebugSnapshot: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: normalizeTransactionHash, + }, + }, + property, + ); - destroy() { - super.destroy(); - } - } + expect(derivedState).toStrictEqual({ transactionHash: '0x1234' }); + }); - const visitorOverflowName = 'VisitorOverflowController'; + it('should allow returning a partial object from a deriver', () => { + const getDerivedTxMeta = (txMeta: { hash: string; value: number }) => { + return { value: txMeta.value }; + }; - type VisitorOverflowControllerState = { - maxVisitors: number; - }; - type VisitorOverflowUpdateMaxAction = { - type: `${typeof visitorOverflowName}:updateMax`; - handler: (max: number) => void; - }; - type VisitorOverflowExternalActions = VisitorControllerClearAction; - type VisitorOverflowControllerActions = - | VisitorOverflowUpdateMaxAction - | ControllerActions< - typeof visitorOverflowName, - VisitorOverflowControllerState - >; - type VisitorOverflowStateChangeEvent = ControllerEvents< - typeof visitorOverflowName, - VisitorOverflowControllerState - >; - type VisitorOverflowExternalEvents = VisitorControllerStateChangeEvent; - type VisitorOverflowControllerEvents = VisitorOverflowStateChangeEvent; + const derivedState = deriveStateFromMetadata( + { + txMeta: { + hash: '0x123', + value: 10, + }, + }, + { + txMeta: { + includeInDebugSnapshot: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: getDerivedTxMeta, + }, + }, + property, + ); - const visitorOverflowControllerMetadata = { - maxVisitors: { - persist: false, - anonymous: true, - }, - }; + expect(derivedState).toStrictEqual({ txMeta: { value: 10 } }); + }); - type VisitorOverflowMessenger = Messenger< - typeof visitorOverflowName, - VisitorOverflowControllerActions | VisitorOverflowExternalActions, - VisitorOverflowControllerEvents | VisitorOverflowExternalEvents - >; + it('should allow returning a nested partial object from a deriver', () => { + const getDerivedTxMeta = (txMeta: { + hash: string; + value: number; + history: { hash: string; value: number }[]; + }) => { + return { + history: txMeta.history.map((entry) => { + return { value: entry.value }; + }), + value: txMeta.value, + }; + }; + + const derivedState = deriveStateFromMetadata( + { + txMeta: { + hash: '0x123', + history: [ + { + hash: '0x123', + value: 9, + }, + ], + value: 10, + }, + }, + { + txMeta: { + includeInDebugSnapshot: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: getDerivedTxMeta, + }, + }, + property, + ); - class VisitorOverflowController extends BaseController< - typeof visitorOverflowName, - VisitorOverflowControllerState, - VisitorOverflowMessenger - > { - constructor(messenger: VisitorOverflowMessenger) { - super({ - messenger, - metadata: visitorOverflowControllerMetadata, - name: visitorOverflowName, - state: { maxVisitors: 5 }, + expect(derivedState).toStrictEqual({ + txMeta: { history: [{ value: 9 }], value: 10 }, }); + }); - messenger.registerActionHandler( - 'VisitorOverflowController:updateMax', - this.updateMax, + it('should allow transforming types in a deriver', () => { + const derivedState = deriveStateFromMetadata( + { + count: '1', + }, + { + count: { + includeInDebugSnapshot: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: (count: string) => Number(count), + }, + }, + property, ); - messenger.subscribe('VisitorController:stateChange', this.onVisit); - } - - onVisit = ({ visitors }: VisitorControllerState) => { - if (visitors.length > this.state.maxVisitors) { - this.messenger.call('VisitorController:clear'); - } - }; - - updateMax = (max: number) => { - this.update(() => { - return { maxVisitors: max }; - }); - }; - - destroy() { - super.destroy(); - } + expect(derivedState).toStrictEqual({ count: 1 }); + }); } - it('should allow messaging between controllers', () => { - // Construct root messenger - const rootMessenger = new Messenger< - 'Root', - VisitorControllerActions | VisitorOverflowControllerActions, - VisitorControllerEvents | VisitorOverflowControllerEvents - >({ namespace: 'Root' }); - // Construct controller messengers, delegating to parent - const visitorControllerMessenger = new Messenger< - typeof visitorName, - VisitorControllerActions | VisitorOverflowUpdateMaxAction, - VisitorControllerEvents | VisitorOverflowStateChangeEvent, - typeof rootMessenger - >({ namespace: visitorName, parent: rootMessenger }); - const visitorOverflowControllerMessenger = new Messenger< - typeof visitorOverflowName, - VisitorOverflowControllerActions | VisitorControllerClearAction, - VisitorOverflowControllerEvents | VisitorControllerStateChangeEvent, - typeof rootMessenger - >({ namespace: visitorOverflowName, parent: rootMessenger }); - // Delegate external actions/events to controller messengers - rootMessenger.delegate({ - actions: ['VisitorController:clear'], - events: ['VisitorController:stateChange'], - messenger: visitorOverflowControllerMessenger, - }); - rootMessenger.delegate({ - actions: ['VisitorOverflowController:updateMax'], - events: ['VisitorOverflowController:stateChange'], - messenger: visitorControllerMessenger, - }); - // Construct controllers - const visitorController = new VisitorController( - visitorControllerMessenger, - ); - const visitorOverflowController = new VisitorOverflowController( - visitorOverflowControllerMessenger, + it('should suppress errors thrown when deriving state', () => { + const setTimeoutStub = sinon.stub(globalThis, 'setTimeout'); + const derivedState = deriveStateFromMetadata( + { + extraState: 'extraState', + privateKey: '123', + network: 'mainnet', + }, + // @ts-expect-error Intentionally testing invalid state + { + privateKey: { + includeInDebugSnapshot: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: true, + }, + network: { + includeInDebugSnapshot: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: false, + }, + }, + property, ); - rootMessenger.call('VisitorOverflowController:updateMax', 2); - visitorController.addVisitor('A'); - visitorController.addVisitor('B'); - visitorController.addVisitor('C'); // this should trigger an overflow + expect(derivedState).toStrictEqual({ + privateKey: '123', + }); - expect(visitorOverflowController.state.maxVisitors).toBe(2); - expect(visitorController.state.visitors).toHaveLength(0); + expect(setTimeoutStub.callCount).toBe(1); + const onTimeout = setTimeoutStub.firstCall.args[0]; + expect(() => onTimeout()).toThrow(`No metadata found for 'extraState'`); }); }); }); diff --git a/packages/base-controller/src/next/BaseController.ts b/packages/base-controller/src/next/BaseController.ts index 6e25ee9aefb..793b59c22a7 100644 --- a/packages/base-controller/src/next/BaseController.ts +++ b/packages/base-controller/src/next/BaseController.ts @@ -58,20 +58,47 @@ export type StateMetadata = { /** * Metadata for a single state property - * - * @property persist - Indicates whether this property should be persisted - * (`true` for persistent, `false` for transient), or is set to a function - * that derives the persistent state from the state. - * @property anonymous - Indicates whether this property is already anonymous, - * (`true` for anonymous, `false` if it has potential to be personally - * identifiable), or is set to a function that returns an anonymized - * representation of this state. */ -// TODO: Either fix this lint violation or explain why it's necessary to ignore. -// eslint-disable-next-line @typescript-eslint/naming-convention -export type StatePropertyMetadata = { - persist: boolean | StateDeriver; - anonymous: boolean | StateDeriver; +export type StatePropertyMetadata = { + /** + * Indicates whether this property should be included in debug snapshots attached to Sentry + * errors. + * + * Set this to false if the state may contain personally identifiable information, or if it's + * too large to include in a debug snapshot. + */ + includeInDebugSnapshot: boolean | StateDeriver; + /** + * Indicates whether this property should be included in state logs. + * + * Set this to false if the data should be kept hidden from support agents (e.g. if it contains + * secret keys, or personally-identifiable information that is not useful for debugging). + * + * We do allow state logs to contain some personally identifiable information to assist with + * diagnosing errors (e.g. transaction hashes, addresses), but we still attempt to limit the + * data we expose to what is most useful for helping users. + */ + includeInStateLogs: boolean | StateDeriver; + /** + * Indicates whether this property should be persisted. + * + * If true, the property will be persisted and saved between sessions. + * If false, the property will not be saved between sessions, and it will always be missing from the `state` constructor parameter. + */ + persist: boolean | StateDeriver; + /** + * Indicates whether this property is used by the UI. + * + * If true, the property will be accessible from the UI. + * If false, it will be inaccessible from the UI. + * + * Making a property accessible to the UI has a performance overhead, so it's better to set this + * to `false` if it's not used in the UI, especially for properties that can be large in size. + * + * Note that we disallow the use of a state derivation function here to preserve type information + * for the UI (the state deriver type always returns `Json`). + */ + usedInUi: boolean; }; /** @@ -337,6 +364,7 @@ export class BaseController< * By "anonymized" we mean that it should not contain any information that could be personally * identifiable. * + * @deprecated use `deriveStateFromMetadata` instead. * @param state - The controller state. * @param metadata - The controller state metadata, which describes how to derive the * anonymized state. @@ -346,12 +374,13 @@ export function getAnonymizedState( state: ControllerState, metadata: StateMetadata, ): Record { - return deriveStateFromMetadata(state, metadata, 'anonymous'); + return deriveStateFromMetadata(state, metadata, 'includeInDebugSnapshot'); } /** * Returns the subset of state that should be persisted. * + * @deprecated use `deriveStateFromMetadata` instead. * @param state - The controller state. * @param metadata - The controller state metadata, which describes which pieces of state should be persisted. * @returns The subset of controller state that should be persisted. @@ -371,10 +400,12 @@ export function getPersistentState( * @param metadataProperty - The metadata property to use to derive state. * @returns The metadata-derived controller state. */ -function deriveStateFromMetadata( +export function deriveStateFromMetadata< + ControllerState extends StateConstraint, +>( state: ControllerState, metadata: StateMetadata, - metadataProperty: 'anonymous' | 'persist', + metadataProperty: keyof StatePropertyMetadata, ): Record { return (Object.keys(state) as (keyof ControllerState)[]).reduce< Record From f1fa8ba20dad8ce992e9894731bd1af9b83c5fce Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Thu, 21 Aug 2025 14:45:58 -0230 Subject: [PATCH 02/10] Update changelog --- packages/base-controller/CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/base-controller/CHANGELOG.md b/packages/base-controller/CHANGELOG.md index 145ed16d2f0..061c53265ed 100644 --- a/packages/base-controller/CHANGELOG.md +++ b/packages/base-controller/CHANGELOG.md @@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Under the `next` export, expand the controller metadata properties ([#6359](https://github.com/MetaMask/core/pull/6359)) + - Note that this is a breaking change, but it's not labelled as such because it only impacts the experimental `next` export. + - The `anonymous` metadata property has been renamed to `includeInDebugSnapshot` + - Two additional metadata properties have been added: `includeInStateLogs` and `usedInUi`. State derivation is disallowed for `usedInUi`. + ## [8.2.0] ### Added From c9dcda44be49921eb71760cd4a7ea50ae73e8aec Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Mon, 25 Aug 2025 12:01:19 -0230 Subject: [PATCH 03/10] Fix lint errors --- eslint-warning-thresholds.json | 5 +---- packages/base-controller/src/next/BaseController.test.ts | 1 - 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json index ddccc5cb383..9a63314bd50 100644 --- a/eslint-warning-thresholds.json +++ b/eslint-warning-thresholds.json @@ -113,10 +113,7 @@ "jsdoc/check-tag-names": 2 }, "packages/base-controller/src/next/BaseController.test.ts": { - "import-x/namespace": 16 - }, - "packages/base-controller/src/next/BaseController.ts": { - "jsdoc/check-tag-names": 2 + "import-x/namespace": 18 }, "packages/build-utils/src/transforms/remove-fenced-code.test.ts": { "import-x/order": 1 diff --git a/packages/base-controller/src/next/BaseController.test.ts b/packages/base-controller/src/next/BaseController.test.ts index d6231c2b3ec..f18ab15dbda 100644 --- a/packages/base-controller/src/next/BaseController.test.ts +++ b/packages/base-controller/src/next/BaseController.test.ts @@ -18,7 +18,6 @@ import { deriveStateFromMetadata, } from './BaseController'; - export const countControllerName = 'CountController'; type CountControllerState = { From 630c803b8f0bca56fd77ed29ec989687c7112004 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Tue, 2 Sep 2025 13:34:52 -0230 Subject: [PATCH 04/10] Update changelog --- packages/base-controller/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/base-controller/CHANGELOG.md b/packages/base-controller/CHANGELOG.md index 061c53265ed..2579858d787 100644 --- a/packages/base-controller/CHANGELOG.md +++ b/packages/base-controller/CHANGELOG.md @@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Note that this is a breaking change, but it's not labelled as such because it only impacts the experimental `next` export. - The `anonymous` metadata property has been renamed to `includeInDebugSnapshot` - Two additional metadata properties have been added: `includeInStateLogs` and `usedInUi`. State derivation is disallowed for `usedInUi`. + - Add `deriveStateFromMetadata` export, which can derive state for any metadata property + - Deprecate `getPersistentState` and `getAnonymizedState`, recommending `deriveStateFromMetadata` instead ## [8.2.0] From 6e03837c0a600d8a2416385598c9742dd4fdd616 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Tue, 2 Sep 2025 14:53:43 -0230 Subject: [PATCH 05/10] Update StatePropertyMetadataConstraint --- packages/base-controller/src/next/BaseController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/base-controller/src/next/BaseController.ts b/packages/base-controller/src/next/BaseController.ts index 793b59c22a7..105a61e496c 100644 --- a/packages/base-controller/src/next/BaseController.ts +++ b/packages/base-controller/src/next/BaseController.ts @@ -112,7 +112,7 @@ export type StateDeriverConstraint = (value: never) => Json; * This type can be assigned to any `StatePropertyMetadata` type. */ export type StatePropertyMetadataConstraint = { - [P in 'anonymous' | 'persist']: boolean | StateDeriverConstraint; + [P in keyof StatePropertyMetadata]: boolean | StateDeriverConstraint; }; /** From e75c7312fda67045e5eee4cb0b65163964fc070f Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 3 Sep 2025 14:16:18 -0230 Subject: [PATCH 06/10] Undo `anonymous` rename, make new properties optional, apply to main export --- eslint-warning-thresholds.json | 5 +- packages/base-controller/CHANGELOG.md | 16 +- .../src/BaseController.test.ts | 434 ++++++++++++------ .../base-controller/src/BaseController.ts | 63 ++- .../src/next/BaseController.test.ts | 93 ++-- .../src/next/BaseController.ts | 12 +- 6 files changed, 405 insertions(+), 218 deletions(-) diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json index 9a63314bd50..44a46f64a62 100644 --- a/eslint-warning-thresholds.json +++ b/eslint-warning-thresholds.json @@ -107,10 +107,7 @@ "jsdoc/tag-lines": 2 }, "packages/base-controller/src/BaseController.test.ts": { - "import-x/namespace": 16 - }, - "packages/base-controller/src/BaseController.ts": { - "jsdoc/check-tag-names": 2 + "import-x/namespace": 18 }, "packages/base-controller/src/next/BaseController.test.ts": { "import-x/namespace": 18 diff --git a/packages/base-controller/CHANGELOG.md b/packages/base-controller/CHANGELOG.md index 2579858d787..3f95de97b59 100644 --- a/packages/base-controller/CHANGELOG.md +++ b/packages/base-controller/CHANGELOG.md @@ -7,14 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add `deriveStateFromMetadata` export, which can derive state for any metadata property ([#6359](https://github.com/MetaMask/core/pull/6359)) + - This change has also been made to the experimental `next` export. +- Add optional `includeInStateLogs` and `usedInUi` metadata properties ([#6359](https://github.com/MetaMask/core/pull/6359)) + - State derivation is disallowed for `usedInUi`. + - This change has also been made to the experimental `next` export. + ### Changed -- Under the `next` export, expand the controller metadata properties ([#6359](https://github.com/MetaMask/core/pull/6359)) - - Note that this is a breaking change, but it's not labelled as such because it only impacts the experimental `next` export. - - The `anonymous` metadata property has been renamed to `includeInDebugSnapshot` - - Two additional metadata properties have been added: `includeInStateLogs` and `usedInUi`. State derivation is disallowed for `usedInUi`. - - Add `deriveStateFromMetadata` export, which can derive state for any metadata property - - Deprecate `getPersistentState` and `getAnonymizedState`, recommending `deriveStateFromMetadata` instead +- Deprecate `getPersistentState` and `getAnonymizedState`, recommending `deriveStateFromMetadata` instead ([#6359](https://github.com/MetaMask/core/pull/6359)) + - This change has also been made to the experimental `next` export. ## [8.2.0] diff --git a/packages/base-controller/src/BaseController.test.ts b/packages/base-controller/src/BaseController.test.ts index 9fe417892c2..77bd661793c 100644 --- a/packages/base-controller/src/BaseController.test.ts +++ b/packages/base-controller/src/BaseController.test.ts @@ -1,13 +1,16 @@ /* eslint-disable jest/no-export */ +import type { Json } from '@metamask/utils'; import type { Draft, Patch } from 'immer'; import * as sinon from 'sinon'; import type { ControllerGetStateAction, ControllerStateChangeEvent, + StatePropertyMetadata, } from './BaseController'; import { BaseController, + deriveStateFromMetadata, getAnonymizedState, getPersistentState, isBaseController, @@ -631,7 +634,14 @@ describe('getAnonymizedState', () => { it('should return empty state when no properties are anonymized', () => { const anonymizedState = getAnonymizedState( { count: 1 }, - { count: { anonymous: false, persist: false } }, + { + count: { + anonymous: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + }, + }, ); expect(anonymizedState).toStrictEqual({}); }); @@ -647,19 +657,27 @@ describe('getAnonymizedState', () => { { password: { anonymous: false, + includeInStateLogs: false, persist: false, + usedInUi: false, }, privateKey: { anonymous: false, + includeInStateLogs: false, persist: false, + usedInUi: false, }, network: { anonymous: true, + includeInStateLogs: false, persist: false, + usedInUi: false, }, tokens: { anonymous: true, + includeInStateLogs: false, persist: false, + usedInUi: false, }, }, ); @@ -681,7 +699,9 @@ describe('getAnonymizedState', () => { { transactionHash: { anonymous: anonymizeTransactionHash, + includeInStateLogs: false, persist: false, + usedInUi: false, }, }, ); @@ -704,7 +724,9 @@ describe('getAnonymizedState', () => { { txMeta: { anonymous: anonymizeTxMeta, + includeInStateLogs: false, persist: false, + usedInUi: false, }, }, ); @@ -742,7 +764,9 @@ describe('getAnonymizedState', () => { { txMeta: { anonymous: anonymizeTxMeta, + includeInStateLogs: false, persist: false, + usedInUi: false, }, }, ); @@ -760,7 +784,9 @@ describe('getAnonymizedState', () => { { count: { anonymous: (count) => Number(count), + includeInStateLogs: false, persist: false, + usedInUi: false, }, }, ); @@ -780,11 +806,15 @@ describe('getAnonymizedState', () => { { privateKey: { anonymous: true, + includeInStateLogs: true, persist: true, + usedInUi: true, }, network: { anonymous: false, + includeInStateLogs: false, persist: false, + usedInUi: false, }, }, ); @@ -809,7 +839,14 @@ describe('getPersistentState', () => { it('should return empty state when no properties are persistent', () => { const persistentState = getPersistentState( { count: 1 }, - { count: { anonymous: false, persist: false } }, + { + count: { + anonymous: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + }, + }, ); expect(persistentState).toStrictEqual({}); }); @@ -825,19 +862,27 @@ describe('getPersistentState', () => { { password: { anonymous: false, + includeInStateLogs: false, persist: true, + usedInUi: false, }, privateKey: { anonymous: false, + includeInStateLogs: false, persist: true, + usedInUi: false, }, network: { anonymous: false, + includeInStateLogs: false, persist: false, + usedInUi: false, }, tokens: { anonymous: false, + includeInStateLogs: false, persist: false, + usedInUi: false, }, }, ); @@ -859,7 +904,9 @@ describe('getPersistentState', () => { { transactionHash: { anonymous: false, + includeInStateLogs: false, persist: normalizeTransacitonHash, + usedInUi: false, }, }, ); @@ -882,7 +929,9 @@ describe('getPersistentState', () => { { txMeta: { anonymous: false, + includeInStateLogs: false, persist: getPersistentTxMeta, + usedInUi: false, }, }, ); @@ -920,7 +969,9 @@ describe('getPersistentState', () => { { txMeta: { anonymous: false, + includeInStateLogs: false, persist: getPersistentTxMeta, + usedInUi: false, }, }, ); @@ -938,7 +989,9 @@ describe('getPersistentState', () => { { count: { anonymous: false, + includeInStateLogs: false, persist: (count) => Number(count), + usedInUi: false, }, }, ); @@ -958,11 +1011,15 @@ describe('getPersistentState', () => { { privateKey: { anonymous: false, + includeInStateLogs: false, persist: true, + usedInUi: false, }, network: { anonymous: false, + includeInStateLogs: false, persist: false, + usedInUi: true, }, }, ); @@ -973,176 +1030,257 @@ describe('getPersistentState', () => { const onTimeout = setTimeoutStub.firstCall.args[0]; expect(() => onTimeout()).toThrow(`No metadata found for 'extraState'`); }); +}); - describe('inter-controller communication', () => { - // These two contrived mock controllers are setup to test with. - // The 'VisitorController' records strings that represent visitors. - // The 'VisitorOverflowController' monitors the 'VisitorController' to ensure the number of - // visitors doesn't exceed the maximum capacity. If it does, it will clear out all visitors. - - const visitorName = 'VisitorController'; - - type VisitorControllerState = { - visitors: string[]; - }; - type VisitorControllerAction = { - type: `${typeof visitorName}:clear`; - handler: () => void; - }; - type VisitorControllerEvent = { - type: `${typeof visitorName}:stateChange`; - payload: [VisitorControllerState, Patch[]]; - }; +describe('deriveStateFromMetadata', () => { + afterEach(() => { + sinon.restore(); + }); - const visitorControllerStateMetadata = { - visitors: { - persist: true, - anonymous: true, + it('returns an empty object when deriving state for an unset property', () => { + const derivedState = deriveStateFromMetadata( + { count: 1 }, + { + count: { + anonymous: false, + includeInStateLogs: false, + persist: false, + // usedInUi is not set + }, }, - }; + 'usedInUi', + ); - type VisitorMessenger = RestrictedMessenger< - typeof visitorName, - VisitorControllerAction | VisitorOverflowControllerAction, - VisitorControllerEvent | VisitorOverflowControllerEvent, - never, - never - >; - class VisitorController extends BaseController< - typeof visitorName, - VisitorControllerState, - VisitorMessenger - > { - constructor(messagingSystem: VisitorMessenger) { - super({ - messenger: messagingSystem, - metadata: visitorControllerStateMetadata, - name: visitorName, - state: { visitors: [] }, - }); + expect(derivedState).toStrictEqual({}); + }); - messagingSystem.registerActionHandler( - 'VisitorController:clear', - this.clear, - ); - } + describe.each([ + 'anonymous', + 'includeInStateLogs', + 'persist', + 'usedInUi', + ] as const)('%s', (property: keyof StatePropertyMetadata) => { + it('should return empty state', () => { + expect(deriveStateFromMetadata({}, {}, property)).toStrictEqual({}); + }); - clear = () => { - this.update(() => { - return { visitors: [] }; - }); - }; + it('should return empty state when no properties are enabled', () => { + const derivedState = deriveStateFromMetadata( + { count: 1 }, + { + count: { + anonymous: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: false, + }, + }, + property, + ); - addVisitor(visitor: string) { - this.update(({ visitors }) => { - return { visitors: [...visitors, visitor] }; - }); - } + expect(derivedState).toStrictEqual({}); + }); - destroy() { - super.destroy(); - } - } + it('should return derived state', () => { + const derivedState = deriveStateFromMetadata( + { + password: 'secret password', + privateKey: '123', + network: 'mainnet', + tokens: ['DAI', 'USDC'], + }, + { + password: { + anonymous: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: true, + }, + privateKey: { + anonymous: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: true, + }, + network: { + anonymous: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: false, + }, + tokens: { + anonymous: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: false, + }, + }, + property, + ); - const visitorOverflowName = 'VisitorOverflowController'; + expect(derivedState).toStrictEqual({ + password: 'secret password', + privateKey: '123', + }); + }); - type VisitorOverflowControllerState = { - maxVisitors: number; - }; - type VisitorOverflowControllerAction = { - type: `${typeof visitorOverflowName}:updateMax`; - handler: (max: number) => void; - }; - type VisitorOverflowControllerEvent = { - type: `${typeof visitorOverflowName}:stateChange`; - payload: [VisitorOverflowControllerState, Patch[]]; - }; + if (property !== 'usedInUi') { + it('should use function to derive state', () => { + const normalizeTransactionHash = (hash: string) => { + return hash.toLowerCase(); + }; - const visitorOverflowControllerMetadata = { - maxVisitors: { - persist: false, - anonymous: true, - }, - }; + const derivedState = deriveStateFromMetadata( + { + transactionHash: '0X1234', + }, + { + transactionHash: { + anonymous: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: normalizeTransactionHash, + }, + }, + property, + ); - type VisitorOverflowMessenger = RestrictedMessenger< - typeof visitorOverflowName, - VisitorControllerAction | VisitorOverflowControllerAction, - VisitorControllerEvent | VisitorOverflowControllerEvent, - `${typeof visitorName}:clear`, - `${typeof visitorName}:stateChange` - >; - - class VisitorOverflowController extends BaseController< - typeof visitorOverflowName, - VisitorOverflowControllerState, - VisitorOverflowMessenger - > { - constructor(messagingSystem: VisitorOverflowMessenger) { - super({ - messenger: messagingSystem, - metadata: visitorOverflowControllerMetadata, - name: visitorOverflowName, - state: { maxVisitors: 5 }, - }); + expect(derivedState).toStrictEqual({ transactionHash: '0x1234' }); + }); - messagingSystem.registerActionHandler( - 'VisitorOverflowController:updateMax', - this.updateMax, - ); + it('should allow returning a partial object from a deriver', () => { + const getDerivedTxMeta = (txMeta: { hash: string; value: number }) => { + return { value: txMeta.value }; + }; - messagingSystem.subscribe( - 'VisitorController:stateChange', - this.onVisit, + const derivedState = deriveStateFromMetadata( + { + txMeta: { + hash: '0x123', + value: 10, + }, + }, + { + txMeta: { + anonymous: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: getDerivedTxMeta, + }, + }, + property, ); - } - onVisit = ({ visitors }: VisitorControllerState) => { - if (visitors.length > this.state.maxVisitors) { - this.messagingSystem.call('VisitorController:clear'); - } - }; + expect(derivedState).toStrictEqual({ txMeta: { value: 10 } }); + }); - updateMax = (max: number) => { - this.update(() => { - return { maxVisitors: max }; + it('should allow returning a nested partial object from a deriver', () => { + const getDerivedTxMeta = (txMeta: { + hash: string; + value: number; + history: { hash: string; value: number }[]; + }) => { + return { + history: txMeta.history.map((entry) => { + return { value: entry.value }; + }), + value: txMeta.value, + }; + }; + + const derivedState = deriveStateFromMetadata( + { + txMeta: { + hash: '0x123', + history: [ + { + hash: '0x123', + value: 9, + }, + ], + value: 10, + }, + }, + { + txMeta: { + anonymous: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: getDerivedTxMeta, + }, + }, + property, + ); + + expect(derivedState).toStrictEqual({ + txMeta: { history: [{ value: 9 }], value: 10 }, }); - }; + }); - destroy() { - super.destroy(); - } - } + it('should allow transforming types in a deriver', () => { + const derivedState = deriveStateFromMetadata( + { + count: '1', + }, + { + count: { + anonymous: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: (count: string) => Number(count), + }, + }, + property, + ); - it('should allow messaging between controllers', () => { - const messenger = new Messenger< - VisitorControllerAction | VisitorOverflowControllerAction, - VisitorControllerEvent | VisitorOverflowControllerEvent - >(); - const visitorControllerMessenger = messenger.getRestricted({ - name: visitorName, - allowedActions: [], - allowedEvents: [], - }); - const visitorController = new VisitorController( - visitorControllerMessenger, - ); - const visitorOverflowControllerMessenger = messenger.getRestricted({ - name: visitorOverflowName, - allowedActions: ['VisitorController:clear'], - allowedEvents: ['VisitorController:stateChange'], + expect(derivedState).toStrictEqual({ count: 1 }); }); - const visitorOverflowController = new VisitorOverflowController( - visitorOverflowControllerMessenger, + } + + it('should suppress errors thrown when deriving state', () => { + const setTimeoutStub = sinon.stub(globalThis, 'setTimeout'); + const derivedState = deriveStateFromMetadata( + { + extraState: 'extraState', + privateKey: '123', + network: 'mainnet', + }, + // @ts-expect-error Intentionally testing invalid state + { + privateKey: { + anonymous: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: true, + }, + network: { + anonymous: false, + includeInStateLogs: false, + persist: false, + usedInUi: false, + [property]: false, + }, + }, + property, ); - messenger.call('VisitorOverflowController:updateMax', 2); - visitorController.addVisitor('A'); - visitorController.addVisitor('B'); - visitorController.addVisitor('C'); // this should trigger an overflow + expect(derivedState).toStrictEqual({ + privateKey: '123', + }); - expect(visitorOverflowController.state.maxVisitors).toBe(2); - expect(visitorController.state.visitors).toHaveLength(0); + expect(setTimeoutStub.callCount).toBe(1); + const onTimeout = setTimeoutStub.firstCall.args[0]; + expect(() => onTimeout()).toThrow(`No metadata found for 'extraState'`); }); }); }); diff --git a/packages/base-controller/src/BaseController.ts b/packages/base-controller/src/BaseController.ts index 8e3b8cdc2d6..70f2c616420 100644 --- a/packages/base-controller/src/BaseController.ts +++ b/packages/base-controller/src/BaseController.ts @@ -80,20 +80,47 @@ export type StateMetadata = { /** * Metadata for a single state property - * - * @property persist - Indicates whether this property should be persisted - * (`true` for persistent, `false` for transient), or is set to a function - * that derives the persistent state from the state. - * @property anonymous - Indicates whether this property is already anonymous, - * (`true` for anonymous, `false` if it has potential to be personally - * identifiable), or is set to a function that returns an anonymized - * representation of this state. */ -// TODO: Either fix this lint violation or explain why it's necessary to ignore. -// eslint-disable-next-line @typescript-eslint/naming-convention -export type StatePropertyMetadata = { - persist: boolean | StateDeriver; - anonymous: boolean | StateDeriver; +export type StatePropertyMetadata = { + /** + * Indicates whether this property should be included in debug snapshots attached to Sentry + * errors. + * + * Set this to false if the state may contain personally identifiable information, or if it's + * too large to include in a debug snapshot. + */ + anonymous: boolean | StateDeriver; + /** + * Indicates whether this property should be included in state logs. + * + * Set this to false if the data should be kept hidden from support agents (e.g. if it contains + * secret keys, or personally-identifiable information that is not useful for debugging). + * + * We do allow state logs to contain some personally identifiable information to assist with + * diagnosing errors (e.g. transaction hashes, addresses), but we still attempt to limit the + * data we expose to what is most useful for helping users. + */ + includeInStateLogs?: boolean | StateDeriver; + /** + * Indicates whether this property should be persisted. + * + * If true, the property will be persisted and saved between sessions. + * If false, the property will not be saved between sessions, and it will always be missing from the `state` constructor parameter. + */ + persist: boolean | StateDeriver; + /** + * Indicates whether this property is used by the UI. + * + * If true, the property will be accessible from the UI. + * If false, it will be inaccessible from the UI. + * + * Making a property accessible to the UI has a performance overhead, so it's better to set this + * to `false` if it's not used in the UI, especially for properties that can be large in size. + * + * Note that we disallow the use of a state derivation function here to preserve type information + * for the UI (the state deriver type always returns `Json`). + */ + usedInUi?: boolean; }; /** @@ -107,7 +134,7 @@ export type StateDeriverConstraint = (value: never) => Json; * This type can be assigned to any `StatePropertyMetadata` type. */ export type StatePropertyMetadataConstraint = { - [P in 'anonymous' | 'persist']: boolean | StateDeriverConstraint; + [P in keyof StatePropertyMetadata]: boolean | StateDeriverConstraint; }; /** @@ -321,6 +348,7 @@ export class BaseController< * By "anonymized" we mean that it should not contain any information that could be personally * identifiable. * + * @deprecated Use `deriveStateFromMetadata` instead. * @param state - The controller state. * @param metadata - The controller state metadata, which describes how to derive the * anonymized state. @@ -336,6 +364,7 @@ export function getAnonymizedState( /** * Returns the subset of state that should be persisted. * + * @deprecated Use `deriveStateFromMetadata` instead. * @param state - The controller state. * @param metadata - The controller state metadata, which describes which pieces of state should be persisted. * @returns The subset of controller state that should be persisted. @@ -355,10 +384,12 @@ export function getPersistentState( * @param metadataProperty - The metadata property to use to derive state. * @returns The metadata-derived controller state. */ -function deriveStateFromMetadata( +export function deriveStateFromMetadata< + ControllerState extends StateConstraint, +>( state: ControllerState, metadata: StateMetadata, - metadataProperty: 'anonymous' | 'persist', + metadataProperty: keyof StatePropertyMetadata, ): Record { return (Object.keys(state) as (keyof ControllerState)[]).reduce< Record diff --git a/packages/base-controller/src/next/BaseController.test.ts b/packages/base-controller/src/next/BaseController.test.ts index f18ab15dbda..7761b849302 100644 --- a/packages/base-controller/src/next/BaseController.test.ts +++ b/packages/base-controller/src/next/BaseController.test.ts @@ -36,7 +36,7 @@ export type CountControllerEvent = ControllerStateChangeEvent< export const countControllerStateMetadata = { count: { - includeInDebugSnapshot: true, + anonymous: true, includeInStateLogs: true, persist: true, usedInUi: true, @@ -109,7 +109,7 @@ type MessagesControllerEvent = ControllerStateChangeEvent< const messagesControllerStateMetadata = { messages: { - includeInDebugSnapshot: true, + anonymous: true, includeInStateLogs: true, persist: true, usedInUi: true, @@ -609,7 +609,7 @@ describe('BaseController', () => { const visitorControllerStateMetadata = { visitors: { - includeInDebugSnapshot: true, + anonymous: true, includeInStateLogs: true, persist: true, usedInUi: true, @@ -679,7 +679,7 @@ describe('BaseController', () => { const visitorOverflowControllerMetadata = { maxVisitors: { - includeInDebugSnapshot: true, + anonymous: true, includeInStateLogs: true, persist: false, usedInUi: true, @@ -794,7 +794,7 @@ describe('getAnonymizedState', () => { { count: 1 }, { count: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: false, @@ -814,25 +814,25 @@ describe('getAnonymizedState', () => { }, { password: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: false, }, privateKey: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: false, }, network: { - includeInDebugSnapshot: true, + anonymous: true, includeInStateLogs: false, persist: false, usedInUi: false, }, tokens: { - includeInDebugSnapshot: true, + anonymous: true, includeInStateLogs: false, persist: false, usedInUi: false, @@ -856,7 +856,7 @@ describe('getAnonymizedState', () => { }, { transactionHash: { - includeInDebugSnapshot: anonymizeTransactionHash, + anonymous: anonymizeTransactionHash, includeInStateLogs: false, persist: false, usedInUi: false, @@ -881,7 +881,7 @@ describe('getAnonymizedState', () => { }, { txMeta: { - includeInDebugSnapshot: anonymizeTxMeta, + anonymous: anonymizeTxMeta, includeInStateLogs: false, persist: false, usedInUi: false, @@ -921,7 +921,7 @@ describe('getAnonymizedState', () => { }, { txMeta: { - includeInDebugSnapshot: anonymizeTxMeta, + anonymous: anonymizeTxMeta, includeInStateLogs: false, persist: false, usedInUi: false, @@ -941,7 +941,7 @@ describe('getAnonymizedState', () => { }, { count: { - includeInDebugSnapshot: (count) => Number(count), + anonymous: (count) => Number(count), includeInStateLogs: false, persist: false, usedInUi: false, @@ -963,13 +963,13 @@ describe('getAnonymizedState', () => { // @ts-expect-error Intentionally testing invalid state { privateKey: { - includeInDebugSnapshot: true, + anonymous: true, includeInStateLogs: true, persist: true, usedInUi: true, }, network: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: false, @@ -999,7 +999,7 @@ describe('getPersistentState', () => { { count: 1 }, { count: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: false, @@ -1019,25 +1019,25 @@ describe('getPersistentState', () => { }, { password: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: true, usedInUi: false, }, privateKey: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: true, usedInUi: false, }, network: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: false, }, tokens: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: false, @@ -1061,7 +1061,7 @@ describe('getPersistentState', () => { }, { transactionHash: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: normalizeTransacitonHash, usedInUi: false, @@ -1086,7 +1086,7 @@ describe('getPersistentState', () => { }, { txMeta: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: getPersistentTxMeta, usedInUi: false, @@ -1126,7 +1126,7 @@ describe('getPersistentState', () => { }, { txMeta: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: getPersistentTxMeta, usedInUi: false, @@ -1146,7 +1146,7 @@ describe('getPersistentState', () => { }, { count: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: (count) => Number(count), usedInUi: false, @@ -1168,13 +1168,13 @@ describe('getPersistentState', () => { // @ts-expect-error Intentionally testing invalid state { privateKey: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: true, usedInUi: false, }, network: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: true, @@ -1195,8 +1195,25 @@ describe('deriveStateFromMetadata', () => { sinon.restore(); }); + it('returns an empty object when deriving state for an unset property', () => { + const derivedState = deriveStateFromMetadata( + { count: 1 }, + { + count: { + anonymous: false, + includeInStateLogs: false, + persist: false, + // usedInUi is not set + }, + }, + 'usedInUi', + ); + + expect(derivedState).toStrictEqual({}); + }); + describe.each([ - 'includeInDebugSnapshot', + 'anonymous', 'includeInStateLogs', 'persist', 'usedInUi', @@ -1210,7 +1227,7 @@ describe('deriveStateFromMetadata', () => { { count: 1 }, { count: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: false, @@ -1233,28 +1250,28 @@ describe('deriveStateFromMetadata', () => { }, { password: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: false, [property]: true, }, privateKey: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: false, [property]: true, }, network: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: false, [property]: false, }, tokens: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: false, @@ -1282,7 +1299,7 @@ describe('deriveStateFromMetadata', () => { }, { transactionHash: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: false, @@ -1309,7 +1326,7 @@ describe('deriveStateFromMetadata', () => { }, { txMeta: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: false, @@ -1351,7 +1368,7 @@ describe('deriveStateFromMetadata', () => { }, { txMeta: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: false, @@ -1373,7 +1390,7 @@ describe('deriveStateFromMetadata', () => { }, { count: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: false, @@ -1398,14 +1415,14 @@ describe('deriveStateFromMetadata', () => { // @ts-expect-error Intentionally testing invalid state { privateKey: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: false, [property]: true, }, network: { - includeInDebugSnapshot: false, + anonymous: false, includeInStateLogs: false, persist: false, usedInUi: false, diff --git a/packages/base-controller/src/next/BaseController.ts b/packages/base-controller/src/next/BaseController.ts index 105a61e496c..af52816fba0 100644 --- a/packages/base-controller/src/next/BaseController.ts +++ b/packages/base-controller/src/next/BaseController.ts @@ -67,7 +67,7 @@ export type StatePropertyMetadata = { * Set this to false if the state may contain personally identifiable information, or if it's * too large to include in a debug snapshot. */ - includeInDebugSnapshot: boolean | StateDeriver; + anonymous: boolean | StateDeriver; /** * Indicates whether this property should be included in state logs. * @@ -78,7 +78,7 @@ export type StatePropertyMetadata = { * diagnosing errors (e.g. transaction hashes, addresses), but we still attempt to limit the * data we expose to what is most useful for helping users. */ - includeInStateLogs: boolean | StateDeriver; + includeInStateLogs?: boolean | StateDeriver; /** * Indicates whether this property should be persisted. * @@ -98,7 +98,7 @@ export type StatePropertyMetadata = { * Note that we disallow the use of a state derivation function here to preserve type information * for the UI (the state deriver type always returns `Json`). */ - usedInUi: boolean; + usedInUi?: boolean; }; /** @@ -364,7 +364,7 @@ export class BaseController< * By "anonymized" we mean that it should not contain any information that could be personally * identifiable. * - * @deprecated use `deriveStateFromMetadata` instead. + * @deprecated Use `deriveStateFromMetadata` instead. * @param state - The controller state. * @param metadata - The controller state metadata, which describes how to derive the * anonymized state. @@ -374,13 +374,13 @@ export function getAnonymizedState( state: ControllerState, metadata: StateMetadata, ): Record { - return deriveStateFromMetadata(state, metadata, 'includeInDebugSnapshot'); + return deriveStateFromMetadata(state, metadata, 'anonymous'); } /** * Returns the subset of state that should be persisted. * - * @deprecated use `deriveStateFromMetadata` instead. + * @deprecated Use `deriveStateFromMetadata` instead. * @param state - The controller state. * @param metadata - The controller state metadata, which describes which pieces of state should be persisted. * @returns The subset of controller state that should be persisted. From e67660bcc98c0df9ee45b4f299044cf1aea30464 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 3 Sep 2025 14:48:02 -0230 Subject: [PATCH 07/10] Update constraint type to disallow usedInUi state deriver --- packages/base-controller/src/BaseController.ts | 2 +- packages/base-controller/src/next/BaseController.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/base-controller/src/BaseController.ts b/packages/base-controller/src/BaseController.ts index 70f2c616420..f5f5797034a 100644 --- a/packages/base-controller/src/BaseController.ts +++ b/packages/base-controller/src/BaseController.ts @@ -134,7 +134,7 @@ export type StateDeriverConstraint = (value: never) => Json; * This type can be assigned to any `StatePropertyMetadata` type. */ export type StatePropertyMetadataConstraint = { - [P in keyof StatePropertyMetadata]: boolean | StateDeriverConstraint; + [P in keyof StatePropertyMetadata]: StatePropertyMetadata[P]; }; /** diff --git a/packages/base-controller/src/next/BaseController.ts b/packages/base-controller/src/next/BaseController.ts index af52816fba0..70c713e2d8b 100644 --- a/packages/base-controller/src/next/BaseController.ts +++ b/packages/base-controller/src/next/BaseController.ts @@ -112,7 +112,7 @@ export type StateDeriverConstraint = (value: never) => Json; * This type can be assigned to any `StatePropertyMetadata` type. */ export type StatePropertyMetadataConstraint = { - [P in keyof StatePropertyMetadata]: boolean | StateDeriverConstraint; + [P in keyof StatePropertyMetadata]: StatePropertyMetadata[P]; }; /** From 49bfed24bb96dfea4add67e4f4becc2f61fe8612 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 3 Sep 2025 15:07:12 -0230 Subject: [PATCH 08/10] Revert back to broader constraint type for state deriver, use hardcoded metadata instead as a way to disallow deriver for usedInUi --- packages/base-controller/src/BaseController.ts | 5 ++++- packages/base-controller/src/next/BaseController.ts | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/base-controller/src/BaseController.ts b/packages/base-controller/src/BaseController.ts index f5f5797034a..2346db8fc44 100644 --- a/packages/base-controller/src/BaseController.ts +++ b/packages/base-controller/src/BaseController.ts @@ -134,7 +134,10 @@ export type StateDeriverConstraint = (value: never) => Json; * This type can be assigned to any `StatePropertyMetadata` type. */ export type StatePropertyMetadataConstraint = { - [P in keyof StatePropertyMetadata]: StatePropertyMetadata[P]; + anonymous: boolean | StateDeriverConstraint; + includeInStateLogs?: boolean | StateDeriverConstraint; + persist: boolean | StateDeriverConstraint; + usedInUi?: boolean; }; /** diff --git a/packages/base-controller/src/next/BaseController.ts b/packages/base-controller/src/next/BaseController.ts index 70c713e2d8b..23cb5d973c2 100644 --- a/packages/base-controller/src/next/BaseController.ts +++ b/packages/base-controller/src/next/BaseController.ts @@ -112,7 +112,10 @@ export type StateDeriverConstraint = (value: never) => Json; * This type can be assigned to any `StatePropertyMetadata` type. */ export type StatePropertyMetadataConstraint = { - [P in keyof StatePropertyMetadata]: StatePropertyMetadata[P]; + anonymous: boolean | StateDeriverConstraint; + includeInStateLogs?: boolean | StateDeriverConstraint; + persist: boolean | StateDeriverConstraint; + usedInUi?: boolean; }; /** From ce550023229c51fcaee1c3f308c5c7e253525fbe Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 3 Sep 2025 15:25:36 -0230 Subject: [PATCH 09/10] Export deriveStateFromMetadata from index --- packages/base-controller/src/index.ts | 1 + packages/base-controller/src/next/index.ts | 1 + 2 files changed, 2 insertions(+) diff --git a/packages/base-controller/src/index.ts b/packages/base-controller/src/index.ts index 56da39acadc..24bce186892 100644 --- a/packages/base-controller/src/index.ts +++ b/packages/base-controller/src/index.ts @@ -13,6 +13,7 @@ export type { } from './BaseController'; export { BaseController, + deriveStateFromMetadata, getAnonymizedState, getPersistentState, isBaseController, diff --git a/packages/base-controller/src/next/index.ts b/packages/base-controller/src/next/index.ts index 3d6bb4276fc..74fdaced86e 100644 --- a/packages/base-controller/src/next/index.ts +++ b/packages/base-controller/src/next/index.ts @@ -13,6 +13,7 @@ export type { } from './BaseController'; export { BaseController, + deriveStateFromMetadata, getAnonymizedState, getPersistentState, } from './BaseController'; From 50859debac536129e0604e8858966c1b978c020d Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 3 Sep 2025 15:29:35 -0230 Subject: [PATCH 10/10] Put deprecation under Deprecated category instead of Changed Co-authored-by: Elliot Winkler --- packages/base-controller/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/base-controller/CHANGELOG.md b/packages/base-controller/CHANGELOG.md index 3f95de97b59..416f53eb67d 100644 --- a/packages/base-controller/CHANGELOG.md +++ b/packages/base-controller/CHANGELOG.md @@ -15,7 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - State derivation is disallowed for `usedInUi`. - This change has also been made to the experimental `next` export. -### Changed +### Deprecated - Deprecate `getPersistentState` and `getAnonymizedState`, recommending `deriveStateFromMetadata` instead ([#6359](https://github.com/MetaMask/core/pull/6359)) - This change has also been made to the experimental `next` export.