Skip to content

feat: Add new metadata to confirmation controllers#6473

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Gudahtt merged 3 commits into from
Sep 10, 2025
Merged

feat: Add new metadata to confirmation controllers #6473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a7fab7
feat: Add new metadata to confirmation controllers
Gudahtt Sep 4, 2025
ee776cf
Update changelogs
Gudahtt Sep 4, 2025
5e4b883
Fix lint error
Gudahtt Sep 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/address-book-controller/CHANGELOG.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added

- Add two new controller state metadata properties: `includeInStateLogs` and `usedInUi` ([#6473](https://github.com/MetaMask/core/pull/6473))

### Changed

- Bump `@metamask/base-controller` from `^8.0.1` to `^8.3.0` ([#6284](https://github.com/MetaMask/core/pull/6284), [#6355](https://github.com/MetaMask/core/pull/6355), [#6465](https://github.com/MetaMask/core/pull/6465))
Expand Down
64 changes: 63 additions & 1 deletion packages/address-book-controller/src/AddressBookController.test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
import { Messenger } from '@metamask/base-controller';
import { Messenger, deriveStateFromMetadata } from '@metamask/base-controller';
import { toHex } from '@metamask/controller-utils';
import type { Hex } from '@metamask/utils';

Expand DownExpand Up@@ -618,4 +618,66 @@ describe('AddressBookController', () => {
expect(chain2Contacts).toHaveLength(2);
expect(chain137Contacts).toHaveLength(1);
});

describe('metadata', () => {
it('includes expected state in debug snapshots', () => {
const { controller } = arrangeMocks();

expect(
deriveStateFromMetadata(
controller.state,
controller.metadata,
'anonymous',
),
).toMatchInlineSnapshot(`Object {}`);
});

it('includes expected state in state logs', () => {
const { controller } = arrangeMocks();

expect(
deriveStateFromMetadata(
controller.state,
controller.metadata,
'includeInStateLogs',
),
).toMatchInlineSnapshot(`
Object {
"addressBook": Object {},
}
`);
});

it('persists expected state', () => {
const { controller } = arrangeMocks();

expect(
deriveStateFromMetadata(
controller.state,
controller.metadata,
'persist',
),
).toMatchInlineSnapshot(`
Object {
"addressBook": Object {},
}
`);
});

it('exposes expected state to UI', () => {
const { controller } = arrangeMocks();

expect(
deriveStateFromMetadata(
controller.state,
controller.metadata,
'usedInUi',
),
).toMatchInlineSnapshot(`
Object {
"addressBook": Object {},
}
`);
});
});
});
7 changes: 6 additions & 1 deletion packages/address-book-controller/src/AddressBookController.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -147,7 +147,12 @@ export type AddressBookControllerEvents =
| AddressBookControllerContactDeletedEvent;

const addressBookControllerMetadata = {
addressBook: { persist: true, anonymous: false },
addressBook: {
includeInStateLogs: true,

@matthewwalsh0matthewwalsh0Sep 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, very pedantic, but how are deciding the order of these properties if we're defining them in every controller? Alphabetical? Importance?

@GudahttGudahttSep 10, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been putting them in alphabetical order, leaving the order of pre-existing properties alone to minimize churn. I intended to move anonymous to be alphabetical as part of the rename

persist: true,
anonymous: false,

@matthewwalsh0matthewwalsh0Sep 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we plan to rename this to includeInDebugLogs, is that a future PR?

@GudahttGudahttSep 10, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes exactly, it will happen as part of the migration to base-controller/next, tracked here: #5626

usedInUi: true,

@matthewwalsh0matthewwalsh0Sep 10, 2025•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, will this be used to filter the background RPC patches sent to the UI in extension?

Will it exclude properties from the Redux store in mobile?

@GudahttGudahttSep 10, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's the idea! We won't start using it to filter in practice without another round of review though, so we'll have an opportunity to catch mistakes still.

Initially the plan is just to filter the UI state in extension. The mobile Redux store is also used for persistence, so we can't really do it there. But there is work underway to change that. After the storage changes on mobile, I hope we can filter the UI Redux state using this value on mobile as well.

},
};

/**
Expand Down
4 changes: 4 additions & 0 deletions packages/approval-controller/CHANGELOG.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added

- Add two new controller state metadata properties: `includeInStateLogs` and `usedInUi` ([#6473](https://github.com/MetaMask/core/pull/6473))

### Changed

- Bump `@metamask/utils` from `^11.2.0` to `^11.4.2` ([#6054](https://github.com/MetaMask/core/pull/6054))
Expand Down
60 changes: 59 additions & 1 deletion packages/approval-controller/src/ApprovalController.test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
/* eslint-disable jest/expect-expect */

import { Messenger } from '@metamask/base-controller';
import { deriveStateFromMetadata, Messenger } from '@metamask/base-controller';
import { errorCodes, JsonRpcError } from '@metamask/rpc-errors';
import { nanoid } from 'nanoid';

import { flushPromises } from '../../../tests/helpers';

Check warning on line 7 in packages/approval-controller/src/ApprovalController.test.ts

View workflow job for this annotation

GitHub Actions/ Lint, build, and test / Lint (22.x)

`../../../tests/helpers` import should occur after import of `./errors`
import type {
AddApprovalOptions,
ApprovalControllerActions,
Expand DownExpand Up@@ -353,7 +353,7 @@
const id = Object.keys(
approvalController.state[PENDING_APPROVALS_STORE_KEY],
)[0];
expect(id && typeof id === 'string').toBe(true);

Check warning on line 356 in packages/approval-controller/src/ApprovalController.test.ts

View workflow job for this annotation

GitHub Actions/ Lint, build, and test / Lint (22.x)

Avoid having conditionals in tests
});

it('adds correctly specified entry with request data', () => {
Expand DownExpand Up@@ -404,12 +404,12 @@
).not.toThrow();

expect(
approvalController.has({ id: 'foo1' }) &&

Check warning on line 407 in packages/approval-controller/src/ApprovalController.test.ts

View workflow job for this annotation

GitHub Actions/ Lint, build, and test / Lint (22.x)

Avoid having conditionals in tests
approvalController.has({ id: 'foo2' }),
).toBe(true);

expect(
approvalController.has({ origin: ORIGIN }) &&

Check warning on line 412 in packages/approval-controller/src/ApprovalController.test.ts

View workflow job for this annotation

GitHub Actions/ Lint, build, and test / Lint (22.x)

Avoid having conditionals in tests

Check warning on line 412 in packages/approval-controller/src/ApprovalController.test.ts

View workflow job for this annotation

GitHub Actions/ Lint, build, and test / Lint (22.x)

Avoid having conditionals in tests
approvalController.has({ origin: ORIGIN, type: 'myType1' }) &&
approvalController.has({ origin: ORIGIN, type: 'myType2' }),
).toBe(true);
Expand DownExpand Up@@ -946,7 +946,7 @@
approvalController.accept('foo');

expect(
!approvalController.has({ id: 'foo' }) &&

Check warning on line 949 in packages/approval-controller/src/ApprovalController.test.ts

View workflow job for this annotation

GitHub Actions/ Lint, build, and test / Lint (22.x)

Avoid having conditionals in tests

Check warning on line 949 in packages/approval-controller/src/ApprovalController.test.ts

View workflow job for this annotation

GitHub Actions/ Lint, build, and test / Lint (22.x)

Avoid having conditionals in tests

Check warning on line 949 in packages/approval-controller/src/ApprovalController.test.ts

View workflow job for this annotation

GitHub Actions/ Lint, build, and test / Lint (22.x)

Avoid having conditionals in tests
!approvalController.has({ type: 'type' }) &&
!approvalController.has({ origin: 'bar.baz' }) &&
!approvalController.state[PENDING_APPROVALS_STORE_KEY].foo,
Expand All@@ -966,7 +966,7 @@
approvalController.accept('fizz');

expect(
!approvalController.has({ id: 'fizz' }) &&

Check warning on line 969 in packages/approval-controller/src/ApprovalController.test.ts

View workflow job for this annotation

GitHub Actions/ Lint, build, and test / Lint (22.x)

Avoid having conditionals in tests
!approvalController.has({ origin: 'bar.baz', type: 'type2' }),
).toBe(true);

Expand DownExpand Up@@ -1712,4 +1712,62 @@
});
});
});

describe('metadata', () => {
it('includes expected state in debug snapshots', () => {
expect(
deriveStateFromMetadata(
approvalController.state,
approvalController.metadata,
'anonymous',
),
).toMatchInlineSnapshot(`
Object {
"pendingApprovals": Object {},
}
`);
});

it('includes expected state in state logs', () => {
expect(
deriveStateFromMetadata(
approvalController.state,
approvalController.metadata,
'includeInStateLogs',
),
).toMatchInlineSnapshot(`
Object {
"approvalFlows": Array [],
"pendingApprovalCount": 0,
"pendingApprovals": Object {},
}
`);
});

it('persists expected state', () => {
expect(
deriveStateFromMetadata(
approvalController.state,
approvalController.metadata,
'persist',
),
).toMatchInlineSnapshot(`Object {}`);
});

it('exposes expected state to UI', () => {
expect(
deriveStateFromMetadata(
approvalController.state,
approvalController.metadata,
'usedInUi',
),
).toMatchInlineSnapshot(`
Object {
"approvalFlows": Array [],
"pendingApprovalCount": 0,
"pendingApprovals": Object {},
}
`);
});
});
});
21 changes: 18 additions & 3 deletions packages/approval-controller/src/ApprovalController.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -27,9 +27,24 @@ export const APPROVAL_TYPE_RESULT_SUCCESS = 'result_success';
const controllerName = 'ApprovalController';

const stateMetadata = {
pendingApprovals: { persist: false, anonymous: true },
pendingApprovalCount: { persist: false, anonymous: false },
approvalFlows: { persist: false, anonymous: false },
pendingApprovals: {
includeInStateLogs: true,
persist: false,
anonymous: true,
usedInUi: true,
},
pendingApprovalCount: {
includeInStateLogs: true,
persist: false,
anonymous: false,
usedInUi: true,
},
approvalFlows: {
includeInStateLogs: true,
persist: false,
anonymous: false,
usedInUi: true,
},
};

const getAlreadyPendingMessage = (origin: string, type: string) =>
Expand Down
4 changes: 4 additions & 0 deletions packages/ens-controller/CHANGELOG.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added

- Add two new controller state metadata properties: `includeInStateLogs` and `usedInUi` ([#6473](https://github.com/MetaMask/core/pull/6473))

### Changed

- Bump `@metamask/base-controller` from `^8.0.1` to `^8.3.0` ([#6284](https://github.com/MetaMask/core/pull/6284), [#6355](https://github.com/MetaMask/core/pull/6355), [#6465](https://github.com/MetaMask/core/pull/6465))
Expand Down
Loading
Loading