Add Accounts Controller #21437

montelaidev · 2023-10-18T16:02:42Z

Description

Currently, MetaMask faces tight coupling between its UI and the keyring-controller. The UI heavily relies on the keyring-controller's state while also amalgamating information from various sources, such as preferences and balances.

However, this approach presents some challenges. The keyring-controller must be aware of the UI's limitations, like the need for instant account list provision to avoid lag. Moreover, it takes on the responsibility of adding metadata to accounts, such as the associated keyring type, required for displaying account details.

To address these issues, the introduction of the accounts-controller comes into play as a new abstraction layer between the UI and the keyring-controller. This separation of responsibilities allows the keyring-controller to focus on two main tasks:

Routing requests to the appropriate keyring.
Persisting the state of the keyrings.

This PR introduces @metamask/accounts-controller and InternalAccounts to the background script. It is one of several PRs that will be switching the usage of identities to internal accounts.

Changes:

PermissionsController

getCaveatSpecifications and getPermissionSpecifications uses getInternalAccounts instead of getIdentities
captureKeyringTypesWithMissingIdentities uses the InternalAccount model instead of an identity

IncomingTransactionHelper

getAccount now returns an InternalAccount, and uses the AccountsController's getSelectedAccount

AlertController

Removed the preferenceStore
Added the accountsController and controllerMessenger
adds the subscription of selectedAccountChange from the AccountsController

DetectTokenController

removes subscription of selectedAddress from the PreferencesController
adds the subscription of selectedAccountChange from the AccountsController

AccountTracker

removes subscription of selectedAddress from the PreferencesController
adds the subscription of selectedAccountChange from the AccountsController

Backup

adds backup of AccountsController

MetamaskController

modifies TokensController's onPreferencesStateChange to subscribe to selectedAccountChange from the AccountsController
modifies the KeyringController constructor arguments
modifies metamaskMiddleware's getSelectedAddress to getSelectedAccount
modifies setupControllerEventSubscriptions to subscribe to selectedAccountChange from the accounts controller and removes subscribing to selectedAddress from the PreferencesController
modifies setAccountLabel to use setAccountName from the AccountsController
createNewVaultAndKeychain - awaits for updateAccounts and uses setSelectedAccount from the AccountsController

Additions:

105 migration file, this migration creates an InternalAccount for every identity and selectedAddress to selectedAccount on the AccountsController
Adds AccountsController to the store and memStore
Adds setSelectedInternalAccount and setAccountName to the api
Changes selectFirstIdentity to selectFirstAccount
Unlock hardware wallet to return internal accounts
resetAccount to use getSelectedAccount
Adds a new subscription, selectedAccountChange to change accounts in setupControllerEventSubscription

Related issues

Resolves https://github.com/MetaMask/accounts-planning/issues/133

Manual testing steps

Use a hardware wallet to test all the flows

Screenshots/Recordings

No UI changes

Pre-merge author checklist

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 · 2023-10-18T16:02:55Z

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 · 2023-10-18T16:38:18Z

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

Packages	Version	New capabilities	Transitives	Size	Publisher
@metamask/accounts-controller	4.0.0	None	`+1`	179 kB	metamaskbot
@metamask/eth-snap-keyring	1.0.0...2.0.0	None	`+0/-0`	96.8 kB	metamaskbot

package.json

…roller-2

socket-security · 2023-10-24T16:45:12Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

…roller-2

…etrics

montelaidev · 2023-10-25T14:19:01Z

@metamaskbot update-policies

metamaskbot · 2023-10-25T14:34:53Z

Policy update failed. You can review the logs or retry the policy update here

montelaidev · 2023-10-25T14:36:32Z

@metamaskbot update-policies

metamaskbot · 2023-10-25T14:50:26Z

Policies updated

montelaidev · 2023-10-25T16:52:41Z

@metamaskbot update-policies

…ask/metamask-extension into feat/accounts-controller-2

into feat/accounts-controller-2

metamaskbot · 2023-11-25T00:03:00Z

Builds ready [1060e98]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (649 ± 271 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	83	131	104	15	7
		domContentLoaded	66	123	93	16	8
		load	78	1496	649	565	271
		domInteractive	66	123	93	16	8

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 17.59 KiB (0.48%)
ui: 0 Bytes (0.00%)
common: 71 Bytes (0.00%)

metamaskbot · 2023-11-26T14:00:04Z

Builds ready [9184101]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (279 ± 187 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	84	114	91	7	3
		domContentLoaded	69	105	77	9	4
		load	80	1251	279	389	187
		domInteractive	69	104	77	9	4

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 17.59 KiB (0.48%)
ui: 0 Bytes (0.00%)
common: 71 Bytes (0.00%)

metamaskbot · 2023-11-28T11:56:57Z

Builds ready [d9f20a9]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (465 ± 251 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	87	118	92	7	4
		domContentLoaded	68	115	80	12	6
		load	80	1260	465	522	251
		domInteractive	68	115	80	12	6

mcmire

Looks good!

plasmacorral · 2023-11-30T17:02:49Z

Tested commit d9f20a9 with Mac Sonoma 14.0 and primarily in chrome 117.0.5938.92 with some additional test on Firefox 119.0.1 and 120.0.0

Accounts tested:
Ledger Nano X with firmware 2.1.0
Keystone v3 with firmware 1.1.2
Trezor Model T with firmware 2.6.0
Trezor Classic with firmware 1.12.1
Lattice v2 with firmware 0.14.1 and router 0.49.0
Snap account in flask and stable with SSK v1.1.0
Imported private key
HD

Signing, sending, deploying contracts, changing account names, removing individual accounts, forgetting devices, re-adding and signing some more all functioned as expected. Testing with throttled 3G network speeds was conducted. Migration testing confirms that user edited account names persist and migrated accounts can sign and send post-migration with no issue.

MajorLift reviewed Oct 18, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

montelaidev added 2 commits October 19, 2023 14:49

feat: initial accounts controller

0fa9405

fix: use preview build

593186a

montelaidev force-pushed the feat/accounts-controller-2 branch from 6edd2a6 to 593186a Compare October 24, 2023 16:25

montelaidev added 2 commits October 25, 2023 00:30

Merge remote-tracking branch 'origin/develop' into feat/accounts-cont…

5b6a727

…roller-2

fix: detect-tokens test

4660b70

montelaidev added the team-accounts label Oct 24, 2023

montelaidev added 12 commits October 25, 2023 01:02

fix: lint

344aca0

fix: metrics test

a4399ff

fix: remvoe getSelectedAddress from getSnapAndHardwareInfoForMetrics

919e940

Merge remote-tracking branch 'origin/develop' into feat/accounts-cont…

f608783

…roller-2

fix: remove getSelectedAddres from args of getSnapAndHardwareInfoForM…

549b1d5

…etrics

fix: lint

f8dadfa

fix: e2e tests

263ab05

fix: update snapshot

22afde4

fix: add migration

1c5e827

fix: update eth_account test

b4e92e0

feat: use release accounts controller 4.0.0 and keyring-controller 8.1.0

a6e54d3

fix: restore preference controller to snapkeyring callbacks

6629fae

Merge branch 'develop' into feat/accounts-controller-2

0b18c65

Update LavaMoat policies

0ec77b7

feat: update eth-snap-keyring to 2.0.0

7007d2f

gantunesr and others added 11 commits November 22, 2023 22:54

Merge branch 'develop' into feat/accounts-controller-2

54d4c19

fix: update syncIdentities to synchronous

5a0b319

Merge branch 'feat/accounts-controller-2' of https://github.com/MetaM…

c78a9cd

…ask/metamask-extension into feat/accounts-controller-2

Merge branch 'develop' into feat/accounts-controller-2

233c94a

Merge branch 'develop' into feat/accounts-controller-2

525ba5a

Merge branch 'develop' into feat/accounts-controller-2

741649b

chore: update migration to 105

b4befa9

fix: lint

bbe4e19

test: fix

766ac73

Merge branch 'develop' of https://github.com/MetaMask/metamask-extension

87d1b77

into feat/accounts-controller-2

chore: patch accounts-controller

1060e98

Merge branch 'develop' into feat/accounts-controller-2

9184101

Merge branch "develop" into feat/accounts-controller-2

d9f20a9

danroc approved these changes Nov 28, 2023

View reviewed changes

mcmire approved these changes Nov 28, 2023

View reviewed changes

plasmacorral added QA Passed DO-NOT-MERGE Pull requests that should not be merged and removed DO-NOT-MERGE Pull requests that should not be merged needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) QA Passed labels Nov 29, 2023

plasmacorral added QA Passed and removed DO-NOT-MERGE Pull requests that should not be merged labels Nov 30, 2023

plasmacorral merged commit 9487e82 into develop Nov 30, 2023
69 of 75 checks passed

plasmacorral deleted the feat/accounts-controller-2 branch November 30, 2023 17:07

github-actions bot locked and limited conversation to collaborators Nov 30, 2023

metamaskbot added the release-11.8.0 Issue or pull request that will be included in release 11.8.0 label Nov 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Accounts Controller #21437

Add Accounts Controller #21437

montelaidev commented Oct 18, 2023 •

edited by plasmacorral

Loading

github-actions bot commented Oct 18, 2023

socket-security bot commented Oct 18, 2023 •

edited

Loading

socket-security bot commented Oct 24, 2023 •

edited

Loading

montelaidev commented Oct 25, 2023

metamaskbot commented Oct 25, 2023

montelaidev commented Oct 25, 2023

metamaskbot commented Oct 25, 2023

montelaidev commented Oct 25, 2023

metamaskbot commented Nov 25, 2023

metamaskbot commented Nov 26, 2023

metamaskbot commented Nov 28, 2023

mcmire left a comment

plasmacorral commented Nov 30, 2023 •

edited

Loading

Add Accounts Controller #21437

Add Accounts Controller #21437

Conversation

montelaidev commented Oct 18, 2023 • edited by plasmacorral Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Oct 18, 2023

socket-security bot commented Oct 18, 2023 • edited Loading

socket-security bot commented Oct 24, 2023 • edited Loading

montelaidev commented Oct 25, 2023

metamaskbot commented Oct 25, 2023

montelaidev commented Oct 25, 2023

metamaskbot commented Oct 25, 2023

montelaidev commented Oct 25, 2023

metamaskbot commented Nov 25, 2023

metamaskbot commented Nov 26, 2023

metamaskbot commented Nov 28, 2023

mcmire left a comment

Choose a reason for hiding this comment

plasmacorral commented Nov 30, 2023 • edited Loading

montelaidev commented Oct 18, 2023 •

edited by plasmacorral

Loading

socket-security bot commented Oct 18, 2023 •

edited

Loading

socket-security bot commented Oct 24, 2023 •

edited

Loading

plasmacorral commented Nov 30, 2023 •

edited

Loading