feat: add `scopes` field to `KeyringAccount` #29195

ccharly · 2024-12-13T11:22:54Z

Description

Testing the new scopes added on the KeyringAccount.

Related issues

Requires this PR to be merged first:

refactor: use new @metamask/keyring-api layout (split packages) #28861

Related to:

Manual testing steps

Use a previous stable version

git checkout Version-v12.10.0 # Or use a release build

Run it:

yarn
yarn start:flask

Create a bunch of accounts (EVM, non-EVM (Solana/Bitcoin), hardware-wallet, Snap EVM accounts like the SSK)

Save your extension logs (Settings > Advanced)
Now disable your extension (chrome://extensions)
Stops your yarn start:flask
Now, update your extension by going back to this PR (or by using the latest RC if you're validating an RC version)

git checkout feat/keyring-account-scopes

Re-run it:

yarn
yarn start:flask

Re-enable your extension (chrome://extensions)
Open up your console logs (looking at the service worker)
You should now see some migrations running like:

You should also see some other migrations from the Snap keyring this like so:

Save your extension logs again (Settings > Advanced, and use a different filename)
Now, compare your 2 .json files

Screenshots/Recordings

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
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.

github-actions · 2024-12-13T11:23:08Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

socket-security · 2024-12-13T11:24:09Z

New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@metamask/[email protected] 🔁 npm/@metamask/[email protected]	None	`+1`	985 kB	metamaskbot
npm/@metamask/[email protected] 🔁 npm/@metamask/[email protected]	None	`0`	1.48 MB	metamaskbot
npm/@metamask/[email protected] 🔁 npm/@metamask/[email protected]	None	`0`	360 kB	metamaskbot
npm/@metamask/[email protected] 🔁 npm/@metamask/[email protected]	None	`0`	32.2 kB	metamaskbot
npm/@metamask/[email protected] 🔁 npm/@metamask/[email protected]	None	`0`	59.2 kB	metamaskbot
npm/@metamask/[email protected] 🔁 npm/@metamask/[email protected]	None	`0`	995 kB	metamaskbot
npm/@scure/[email protected] 🔁 npm/@scure/[email protected]	None	`0`	122 kB	paulmillr
npm/[email protected] 🔁 npm/[email protected]	None	`0`	13.3 kB	sheetjs
npm/[email protected]	None	`0`	5 kB	sheetjs
npm/[email protected]	None	`0`	113 kB	sheetjs

🚮 Removed packages: npm/@metamask/[email protected], npm/@metamask/[email protected]

View full report↗︎

metamaskbot · 2025-01-14T16:40:35Z

Builds ready [3da5933]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1794 ± 102 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	381	2182	1660	470	226
		domContentLoaded	1415	2173	1760	210	101
		load	1425	2181	1794	213	102
		domInteractive	24	216	45	41	20
		backgroundConnect	8	68	33	21	10
		firstReactRender	18	100	51	30	14
		getState	5	98	21	25	12
		initialActions	0	1	0	0	0
		loadScripts	1051	1664	1318	175	84
		setupStore	7	52	13	11	6
		uiStartup	1785	2950	2158	321	154

Bundle size diffs [🚀 Bundle size reduced!]

background: -38.04 KiB (-0.65%)
ui: 0 Bytes (0.00%)
common: -74.95 KiB (-0.89%)

metamaskbot · 2025-01-15T09:47:03Z

Builds ready [c512ad5]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1914 ± 78 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	246	2162	1669	581	279
		domContentLoaded	1504	2144	1891	160	77
		load	1513	2166	1914	163	78
		domInteractive	27	153	62	33	16
		backgroundConnect	7	75	23	22	11
		firstReactRender	16	104	46	27	13
		getState	5	72	23	21	10
		initialActions	0	1	0	0	0
		loadScripts	1049	1625	1426	139	67
		setupStore	6	65	18	19	9
		uiStartup	1752	3093	2246	269	129

Bundle size diffs [🚀 Bundle size reduced!]

background: -38.04 KiB (-0.65%)
ui: 0 Bytes (0.00%)
common: -74.95 KiB (-0.89%)

danroc · 2025-01-15T10:09:11Z

app/scripts/migrations/105.test.ts

 import { ETH_EOA_METHODS } from '../../../shared/constants/eth-methods';
 import { migrate } from './105';
+import type { Identity, InternalAccount } from './105';


Why is it imported from the migration? Shouldn't it be defined somewhere else?

Got it from the next file, but I wouldn't expect InternalAccount to be defined there.

Well.. given that the migration was done for a previous "state version", I thought that keeping the "old InternalAccount representation" for that migration was better than relying on the new one.

I honestly don't know what's the best strategy here.

So yes, the test import locally-defined type of the InternalAccount and Identity was duplicated in both the test and the migration files, so I just refactored slightly (I can revert this).

danroc · 2025-01-15T10:18:41Z

ui/selectors/accounts.test.ts

@@ -59,6 +59,7 @@ describe('Accounts Selectors', () => {
          'eth_signTypedData_v4',
        ],
        type: 'eip155:eoa',
+        scopes: ['eip155'],


Suggested change

scopes: ['eip155'],

scopes: [EthScopes.Mainnet],

montelaidev · 2025-01-15T12:02:08Z

ui/hooks/accounts/useMultichainWalletSnapClient.test.ts

@@ -48,6 +54,7 @@ describe('useMultichainWalletSnapClient', () => {
        id: '22b44d7c-1e57-4b5b-8502-02698e686fd1',
        options: {},
        methods: ['someMethod'],
+        scopes: [SolScopes.Devnet],


Nit: It would be nice to update this account type back to SolanaAccountType.DataAccount

Good catch! The scopes is not really correct either (compared to what we do on the migration and on the Solana Snap, I'll update that now)

@ccharly

# **Description** The primary purpose of this PR is to update the accounts controller and the assets-controller to the latest version. In doing that, there were several other package updates that were needed. Here is a summary of the updates... # 🔴 Major updates 🔴 ### "@metamask/accounts-controller": "^20.0.1" -> ^21.0.0 - [changelog](https://github.com/MetaMask/core/blob/main/packages/accounts-controller/CHANGELOG.md#2100) - Breaking changes - BREAKING: Add scopes field to KeyringAccount (MetaMask/core#5066), (MetaMask/core#5136) This field is now required and will be used to identify the supported chains (using CAIP-2 chain IDs) for every accounts. - Changes - Bump @metamask/base-controller from ^7.0.0 to ^7.1.1 (MetaMask/core#5079), (MetaMask/core#5135) - Bump @metamask/utils to ^11.0.1 (MetaMask/core#5080) - Bump @metamask/rpc-errors to ^7.0.2 (MetaMask/core#5080) - Use new @metamask/keyring-internal-api@^1.0.0 (MetaMask/core#4695) This package has been split out from the Keyring API. - Bump @metamask/keyring-api from ^10.1.0 to ^12.0.0 (MetaMask/core#4695) - Bump @metamask/eth-snap-keyring from ^5.0.1 to ^7.0.0 (MetaMask/core#4695) ESM/CommonJS support. ### "@metamask/assets-controllers": "^45.1.1", -> ^46.0.0", - [changelog](https://github.com/MetaMask/core/blob/main/packages/assets-controllers/CHANGELOG.md#4600) - Breaking changes - BREAKING: Bump @metamask/accounts-controller peer dependency from ^20.0.0 to ^21.0.0 (MetaMask/core#5140) - Changes - Remove use of @metamask/keyring-api (MetaMask/core#4695) @metamask/providers and webextension-polyfill peer dependencies are no longer required. - Use new @metamask/keyring-internal-api@^1.0.0 (MetaMask/core#4695) This package has been split out from the Keyring API. Its types are compatible with the @metamask/keyring-api package used previously. - Bump @metamask/base-controller from ^7.0.0 to ^7.1.1 (MetaMask/core#5079), (MetaMask/core#5135) - Bump @metamask/keyring-api from ^12.0.0 to ^13.0.0 (MetaMask/core#5066) - Bump @metamask/utils to ^11.0.1 (MetaMask/core#5080) - Bump @metamask/rpc-errors to ^7.0.2 (MetaMask/core#5080) - Added - Add new MultichainBalancesController (MetaMask/core#4965) This controller has been migrated from the MetaMask extension codebase. ### "@metamask/utils": "^10.0.1" -> ^11.0.1" - [changelog](https://github.com/MetaMask/utils/blob/main/CHANGELOG.md#1101) - breaking changes - BREAKING: generateRandomMnemonic now returns Promise<void> instead of void (MetaMask/utils#222) ### Added "@metamask/keyring-internal-api": "^2.0.0", - [changelog](https://github.com/MetaMask/accounts/blob/main/packages/keyring-internal-api/CHANGELOG.md#200) - This package was a peer dep on the latest accounts controller and assets controller. Given this I figured we should add it now anyway. - Changes needed - update imports from `@metamask/keyring-api` to `@metamask/keyring-internal-api` - add support for scopes in the InternalAccount object - added migration (066.ts) to backfill the scopes to existing accounts. - This change is based off a [similar change made in the extension](MetaMask/metamask-extension#29195) made by @ccharly ### Added @metamask/keyring-snap-client: "^2.0.0" - [changelog](https://github.com/MetaMask/accounts/blob/main/packages/keyring-snap-client/CHANGELOG.md#200) - Added because KeyringClient is now exported from `@metamask/keyring-snap-client` instead of `@metamask/keyring-api'`. See `app/components/Views/AddAccountActions/AddAccountActions.tsx` for changes. ### "@metamask/keyring-api": "^10.1.0", -> ^13.0.0" - [changelog](https://github.com/MetaMask/accounts/blob/main/packages/keyring-api/CHANGELOG.md#1300) - Breaking changes - BREAKING: Add scopes field to KeyringAccount (MetaMask/accounts#101) - BREAKING: Split into several smaller packages (MetaMask/accounts#24) - This should improve dependencies management. - Internal related types (internal to both clients) have been moved to keyring-internal-* packages. - Keyring API clients (mainly used by dapps) have been moved to keyring-snap-client package. - Common utils have been moevd to keyring-utils package. ### "@metamask/eth-snap-keyring": "^5.0.1" -> ^7.0.0" - [changelog](https://github.com/MetaMask/accounts/blob/main/packages/keyring-eth-simple/CHANGELOG.md#700) - breaking changes - BREAKING: Increase minimum Node.js version to 16 (MetaMask/eth-simple-keyring#152) - BREAKING: Bump @metamask/eth-sig-util from ^6.0.1 to ^7.0.0 (MetaMask/eth-simple-keyring#156) - Bump @metamask/utils from ^5.0.0 to ^8.1.0 (MetaMask/eth-simple-keyring#153) - Bump ethereum-cryptography from ^1.2.0 to ^2.1.2 (MetaMask/eth-simple-keyring#153) - BREAKING: Bump @metamask/eth-sig-util dependency from ^7.0.3 to ^8.0.0 (MetaMask/accounts#79) - signTypedData no longer support number for addresses, see [here](https://github.com/MetaMask/eth-sig-util/blob/main/CHANGELOG.md#800). ## 🟡 Minor updates 🟡 ### "@metamask/base-controller": "^7.0.1", -> ^7.1.1 - [changelog](https://github.com/MetaMask/core/blob/main/packages/base-controller/CHANGELOG.md#711) - Changes - Bump @metamask/utils from ^10.0.0 to ^11.0.1 - Rename ControllerMessenger to Messenger (MetaMask/core#5050) - ControllerMessenger has been renamed to Messenger - RestrictedControllerMessengerConstraint has been renamed to RestrictedMessengerConstraint - RestrictedControllerMessenger has been renamed to RestrictedMessenger - The RestrictedMessenger constructor parameter controllerMessenger has been renamed to messenger, though the old name is still accepted - The old names remain exported as deprecated aliases of the new names, so this is not a breaking change. ### "@metamask/providers": "^18.1.0" -> ^18.3.1" - [changelog](https://github.com/MetaMask/providers/blob/main/CHANGELOG.md#1831) - Changes - Bump @metamask/json-rpc-engine from ^10.0.1 to ^10.0.2 (MetaMask/providers#397) - Bump @metamask/json-rpc-middleware-stream from ^8.0.5 to ^8.0.6 (MetaMask/providers#397) - Bump @metamask/rpc-errors from ^7.0.1 to ^7.0.2 (MetaMask/providers#397) - Bump @metamask/utils from ^10.0.0 to ^11.0.1 (MetaMask/providers#397) ## 🟢 Patch updates 🟢 ### "@metamask/json-rpc-engine": "^10.0.0" -> ^10.0.2", - [changelog](https://github.com/MetaMask/core/blob/main/packages/json-rpc-engine/CHANGELOG.md#1002) ### "@metamask/json-rpc-middleware-stream": "^8.0.2" -> 8.0.6" - [changelog](https://github.com/MetaMask/core/blob/main/packages/json-rpc-middleware-stream/CHANGELOG.md#806) - changed - Bump @metamask/json-rpc-engine from ^10.0.1 to ^10.0.2 (MetaMask/core#5082) - Bump @metamask/utils from ^10.0.0 to ^11.0.1 (MetaMask/core#5080) ### "@metamask/rpc-errors": "^7.0.1" -> ^7.0.2" - [changelog](https://github.com/MetaMask/rpc-errors/blob/main/CHANGELOG.md#702) - changes - Bump @metamask/utils from ^10.0.0 to ^11.0.1 (MetaMask/rpc-errors#166) ## **Related issues** Fixes: #12967 Fixes: #12966 Fixes: MetaMask/accounts-planning#758 Unblocks: #12599 ## **Manual testing steps** #### Basic import account flow with tokens https://github.com/user-attachments/assets/1a8d3e59-34e4-413e-a3e8-7dbdc5f7424a ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] 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.

metamaskbot · 2025-01-17T13:30:55Z

Builds ready [9e94e59]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1782 ± 107 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1544	2446	1781	228	110
		domContentLoaded	1533	2373	1752	219	105
		load	1545	2447	1782	224	107
		domInteractive	21	204	55	45	21
		backgroundConnect	7	98	29	25	12
		firstReactRender	16	98	41	27	13
		getState	5	93	20	24	12
		initialActions	0	0	0	0	0
		loadScripts	1121	1787	1308	176	84
		setupStore	6	66	13	15	7
		uiStartup	1753	2720	2032	263	126

Bundle size diffs [🚀 Bundle size reduced!]

background: -38.65 KiB (-0.65%)
ui: 0 Bytes (0.00%)
common: -73.09 KiB (-0.84%)

ccharly and others added 21 commits December 3, 2024 17:00

refactor: use new accounts packages

f6d5942

chore: lavamoat:auto

cbd9f36

chore: yarn dedupe

bf4405f

chore: update resolutions

de54690

Merge branch 'main' into refactor/use-new-accounts-packages

356eab7

chore: update resolutions

0584a61

Merge branch 'main' into refactor/use-new-accounts-packages

0fb1493

chore: dedupe + lavamoat:auto

a2b7d0b

chore: update deps

a39c3df

chore: lint

6dce397

chore: update deps

238f522

chore: remove preview builds

ab65f1f

Merge branch 'main' into refactor/use-new-accounts-packages

7cf8701

chore: dedupe

dfa88e1

Merge branch 'main' into refactor/use-new-accounts-packages

9584e9e

Merge branch 'main' into refactor/use-new-accounts-packages

dc10d94

chore: lavamoat:auto

5ea8323

chore: dedupe

45784bb

Merge branch 'main' into refactor/use-new-accounts-packages

50035f3

Merge branch 'main' into refactor/use-new-accounts-packages

384c9e9

ci: add flags.circleci.timeoutMinutes

281c5bf

metamaskbot added the team-accounts label Dec 13, 2024

feat: use new scopes from KeyringAccount

5a28bd5

ccharly force-pushed the feat/keyring-account-scopes branch from d1cc825 to 81c5c8f Compare December 13, 2024 11:36

ccharly changed the title ~~Feat/keyring account scopes~~ feat(keyring-api)!: add scopes field to KeyringAccount Dec 13, 2024

ccharly mentioned this pull request Dec 13, 2024

feat!: add scopes field to KeyringAccount MetaMask/core#5066

Merged

4 tasks

ccharly force-pushed the feat/keyring-account-scopes branch from ecb5bc1 to 3a72085 Compare December 13, 2024 17:12

fix: add resolution for keyring-snap-client

98e9e2d

ccharly requested review from a team as code owners January 14, 2025 16:39

ccharly marked this pull request as draft January 14, 2025 16:39

ccharly added 3 commits January 15, 2025 09:59

refactor(migration): 137 -> 138

7fc4b83

refactor(migration): remove forced migration

0183026

Merge branch 'main' into feat/keyring-account-scopes

c512ad5

test(solana): mock new network calls

784dec7

danroc reviewed Jan 15, 2025

View reviewed changes

refactor(migration/105): rename InternalAccount -> InternalAccountV1

d61277d

danroc reviewed Jan 15, 2025

View reviewed changes

montelaidev reviewed Jan 15, 2025

View reviewed changes

owencraston mentioned this pull request Jan 15, 2025

chore: Update assets and accounts packages MetaMask/metamask-mobile#12984

Merged

7 tasks

ccharly mentioned this pull request Jan 16, 2025

fix(accounts-controller): re-use current internal state in updateAccounts MetaMask/core#5161

Draft

4 tasks

refactor: remove migration

e671cc3

ccharly added 4 commits January 16, 2025 22:45

chore: bump eth-snap-keyring to 8.1.0

734de66

Merge branch 'main' into feat/keyring-account-scopes

08a1d11

chore: dedupe + lavamoat:auto

14977ad

chore: bump accounts-controller to 21.0.1

c70c99a

ccharly force-pushed the feat/keyring-account-scopes branch from f8e088a to c70c99a Compare January 17, 2025 11:42

ccharly added 3 commits January 17, 2025 13:50

Merge branch 'main' into feat/keyring-account-scopes

739071f

refactor: use account type and scopes constants

ca66450

refactor: use account type/scopes/methods constants for solana

9e94e59

ccharly marked this pull request as ready for review January 17, 2025 12:58

darioAnongba approved these changes Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `scopes` field to `KeyringAccount` #29195

feat: add `scopes` field to `KeyringAccount` #29195

ccharly commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024

socket-security bot commented Dec 13, 2024 •

edited

Loading

metamaskbot commented Jan 14, 2025

metamaskbot commented Jan 15, 2025

danroc Jan 15, 2025 •

edited

Loading

ccharly Jan 15, 2025

danroc Jan 15, 2025

ccharly Jan 17, 2025

montelaidev Jan 15, 2025

ccharly Jan 17, 2025

ccharly Jan 17, 2025

metamaskbot commented Jan 17, 2025

feat: add scopes field to KeyringAccount #29195

Are you sure you want to change the base?

feat: add scopes field to KeyringAccount #29195

Conversation

ccharly commented Dec 13, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Dec 13, 2024

socket-security bot commented Dec 13, 2024 • edited Loading

metamaskbot commented Jan 14, 2025

metamaskbot commented Jan 15, 2025

danroc Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

ccharly Jan 15, 2025

Choose a reason for hiding this comment

danroc Jan 15, 2025

Choose a reason for hiding this comment

ccharly Jan 17, 2025

Choose a reason for hiding this comment

montelaidev Jan 15, 2025

Choose a reason for hiding this comment

ccharly Jan 17, 2025

Choose a reason for hiding this comment

ccharly Jan 17, 2025

Choose a reason for hiding this comment

metamaskbot commented Jan 17, 2025

feat: add `scopes` field to `KeyringAccount` #29195

feat: add `scopes` field to `KeyringAccount` #29195

ccharly commented Dec 13, 2024 •

edited

Loading

socket-security bot commented Dec 13, 2024 •

edited

Loading

danroc Jan 15, 2025 •

edited

Loading