Add Accounts Controller #20344

montelaidev · 2023-08-01T16:14:08Z

Explanation

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.

On the other hand, the accounts-controller takes up the following responsibilities:

Ensuring the list of accounts remains up-to-date.
Supplying the UI with relevant account-related details.

This PR depends on MetaMask/core#1637

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
removes backup of PreferencesController

PreferencesController

removed setAddresses, removeAddress, addAddresses, syncAddresses, setSelectedAddress, getSelectedAddress, setAccountLabel

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:

099 migration file, this migration converts every existing identity to an InternalAccount and selectedAddress to selectedAccount on the AccoumtsController
New KEYRING_API_ENABLED environment variable to enable the usage of snap accounts when using metamask flask
Adds AccountsController to the store and memStore
Adds setSelectedInternalAccount and setAccountName to the api

Manual Testing Steps

Test hardware wallets that are not covered by the existing e2e tests.

Connect to Ledger/ Trezor/ QR airgapped wallet / Lattice
Go to the test-dapp
Test all of the signing methods

Pre-merge author checklist

I've clearly explained:
- What problem this PR is solving
- How this problem was solved
- How reviewers can test my changes
Sufficient automated test coverage has been added

Pre-merge reviewer checklist

Manual testing (e.g. pull and build branch, run in browser, test code being changed)
PR is linked to the appropriate GitHub issue
IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

github-actions · 2023-08-01T16:14:21Z

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.

…e.error warnings in test

…roller

metamaskbot · 2023-09-26T15:43:23Z

Builds ready [acbb8df]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): 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
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (916 ± 360 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	83	130	99	15	7
		domContentLoaded	68	132	93	18	9
		load	79	1760	916	750	360
		domInteractive	68	132	93	18	9

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

background: 272.43 KiB (7.67%)
ui: 14.87 KiB (0.18%)
common: 33.6 KiB (0.72%)

…roller

montelaidev · 2023-09-26T17:08:05Z

@metamaskbot update-policies

metamaskbot · 2023-09-26T17:21:48Z

Policies updated

app/scripts/metamask-controller.js

test/e2e/snaps/test-snap-manageAccount.spec.js

test/e2e/tests/import-flow.spec.js

…roller

montelaidev · 2023-09-28T08:43:42Z

@metamaskbot update-policies

metamaskbot · 2023-09-28T08:56:17Z

Policies updated

…roller

…t snap

montelaidev · 2023-09-28T11:47:58Z

@metamaskbot update-policies

metamaskbot · 2023-09-28T12:01:47Z

Policies updated

danroc · 2023-09-28T12:52:37Z

app/scripts/metamask-controller.actions.test.js

    });

    it('two successive calls with different accountCount give different results', async function () {
      await metamaskController.createNewVaultAndKeychain('test@123');
      const addNewAccountResult1 = await metamaskController.addNewAccount(1);
      const addNewAccountResult2 = await metamaskController.addNewAccount(2);
-      assert.notEqual(addNewAccountResult1, addNewAccountResult2);
+      console.log(addNewAccountResult1, addNewAccountResult2);


Remove console.log

hmalik88 · 2023-09-28T16:09:14Z

app/scripts/controllers/permissions/specifications.js

 * }} options - Options bag.
 * @param options.getAllAccounts - A function that returns all Ethereum accounts
 * in the current MetaMask instance.
- * @param options.getIdentities - A function that returns the
+ * @param options.getInternalAccounts - A function that returns the


Does the description need updating?

@raulmono

## Explanation Progresses MetaMask/accounts-planning#14 Designs: https://www.figma.com/file/XzaR4r04pMXIE8ft1T2hmJ/Add%2Fremove-snap-accounts%2Fkeyring-snaps-%2313-and-%2314?type=design&node-id=604-2586&mode=design - this PR works in conjunction with changes in the [eth-snap-keyring](MetaMask/eth-snap-keyring#99) to intercept account additions from a snap and get approval from the user. - This PR heavily leverages existing patterns with the approval controller/confirmations flow and for that reason, differs slightly from the original designs. - Since we are waiting on the [accounts controller PR](#20344) to support custom names, I have removed the TextField from the creation screen. - I have also opted to use the out of the box approval success/error screens with custom messages instead of the designs that @raulmono provided above. These standardized success/error screens are almost the same as the designs but lack the description and translations from the design. It is not currently possible to have custom success/error screens without a massive overhaul to the confirmations flow which would effect many teams. If changes are needed we should work with @matthewwalsh0 to make those changes upstream at a later date. ## Screenshots/Screencaps  <img width="384" alt="Screenshot 2023-09-15 at 2 46 30 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/7d119dc5-6a7c-4e83-ae9d-52390217a52d"> <img width="363" alt="Screenshot 2023-09-15 at 4 57 09 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/f20a63a6-9e16-40f7-b269-d697424454f0"> <img width="371" alt="Screenshot 2023-09-15 at 4 57 17 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/c00c2baa-8aa4-403f-aec1-7164a103792c"> <img width="370" alt="Screenshot 2023-09-20 at 3 54 27 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/8adc01ed-6138-43e7-aa93-7514d35f8a5e"> <img width="367" alt="Screenshot 2023-09-20 at 3 54 34 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/b246f7a6-05b1-4f5c-a2e5-d90b99310588"> ### Before https://github.com/MetaMask/metamask-extension/assets/22918444/465e7833-6ed1-4c38-b618-81723ed54928 ### After #### Happy Path https://github.com/MetaMask/metamask-extension/assets/22918444/049566e1-a121-4803-9166-1f26de2eba6f #### Error case https://github.com/MetaMask/metamask-extension/assets/22918444/fb628db4-4781-4bab-9efd-b755e011ff26 ## Manual Testing Steps This change requires the manual installing of the snaps rpc methods handlers and building metamask flask. Here are the steps. #### Setup 1. checkout this branch and run `yarn start --build-type flask` 2. in your browser, load the extension by clicking `Load unpacked` in the extension settings and using the `dist/chrome/` (or whatever browser you are using) as the source 3. create your account and login #### Testing the snap account creation 1. open this test snap: https://metamask.github.io/snap-simple-keyring/latest/ 4. connect the dapp 5. click create account. Custom account names are not supported at this time <img width="1182" alt="Screenshot 2023-09-20 at 4 13 31 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/1383cfed-c060-4d6f-b065-db7c9d63e3c2"> 6. click create 7. a new account should be created on the snap 8. open metamask and click the accounts dropdown, a new snap account should be added #### Testing the denial flow 1. open this test snap: https://metamask.github.io/snap-simple-keyring/latest/ 2. connect the dapp 3. click create account <img width="1182" alt="Screenshot 2023-09-20 at 4 13 31 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/1383cfed-c060-4d6f-b065-db7c9d63e3c2"> 4. A modal should open on extension 5. click cancel 9. the modal should close and the dapp should throw an error `User denied account creation` <img width="1158" alt="Screenshot 2023-09-20 at 4 14 25 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/7eee4396-a762-4c97-a680-2e03b358596d"> #### Testing the error case - luckily for us, the snap simple keyring had a bug in version 0.2.1 that we can use to test this flow. To test the error flow, use the following steps 1. open this test snap: https://metamask.github.io/snap-simple-keyring/0.2.1/ 2. connect dapp (uninstall and reinstall if you were using a different version before) 3. click create account 4. the account creation modal should pop up 5. click create, an error should be shown with the message `Method not supported: event:accountCreated` 6. create another account 10. click cancel, the modal should close and the dapp should throw an error `User denied account addition` #### Testing account removal 1. follow the previous mentioned steps to create 2 or more snap accounts with this [snap](https://metamask.github.io/snap-simple-keyring/latest/) 2. copy the account `id` from the side bar and paste it into the remove account section of the snap 3. click remove account <img width="1184" alt="Screenshot 2023-09-20 at 4 17 09 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/e8fb4891-a59d-436f-995c-02d145e1b3e9"> 4. A modal should show up indicating what you are trying to do 5. click the `share` icon beside the public address and it should open that account on etherscan. It should work with different networks as well. 6. click `remove` 7. a success screen should be shown and the snap should update with the state and the account no longer visible in the side bar 8. open the extension and verify that the account is no longer in the account list 9. repeat steps 1 and 2 10. this time click cancel 11. the extension should close and an error on the snap should appear with the text `User denied account removal` <img width="1168" alt="Screenshot 2023-09-20 at 4 20 03 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/e85e8ab1-aec0-4ac7-8a56-5253c82af53b"> 12. verify that the account was not removed from the extension or the snap ## Pre-merge author checklist - [x] I've clearly explained: - [x] What problem this PR is solving - [x] How this problem was solved - [x] How reviewers can test my changes - [x] Sufficient automated test coverage has been added ## Pre-merge reviewer checklist - [x] Manual testing (e.g. pull and build branch, run in browser, test code being changed) - [x] PR is linked to the appropriate GitHub issue - [ ] **IF** this PR fixes a bug in the release milestone, add this PR to the release milestone If further QA is required (e.g. new feature, complex testing steps, large refactor), add the `Extension QA Board` label. In this case, a QA Engineer approval will be be required. --------- Co-authored-by: Monte Lai <[email protected]> Co-authored-by: Matthew Walsh <[email protected]> Co-authored-by: Daniel Rocha <[email protected]> Co-authored-by: Howard Braham <[email protected]> Co-authored-by: MetaMask Bot <[email protected]>

@raulmono

Progresses https://github.com/MetaMask/accounts-planning/issues/14 Designs: https://www.figma.com/file/XzaR4r04pMXIE8ft1T2hmJ/Add%2Fremove-snap-accounts%2Fkeyring-snaps-%2313-and-%2314?type=design&node-id=604-2586&mode=design - this PR works in conjunction with changes in the [eth-snap-keyring](MetaMask/eth-snap-keyring#99) to intercept account additions from a snap and get approval from the user. - This PR heavily leverages existing patterns with the approval controller/confirmations flow and for that reason, differs slightly from the original designs. - Since we are waiting on the [accounts controller PR](#20344) to support custom names, I have removed the TextField from the creation screen. - I have also opted to use the out of the box approval success/error screens with custom messages instead of the designs that @raulmono provided above. These standardized success/error screens are almost the same as the designs but lack the description and translations from the design. It is not currently possible to have custom success/error screens without a massive overhaul to the confirmations flow which would effect many teams. If changes are needed we should work with @matthewwalsh0 to make those changes upstream at a later date.  <img width="384" alt="Screenshot 2023-09-15 at 2 46 30 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/7d119dc5-6a7c-4e83-ae9d-52390217a52d"> <img width="363" alt="Screenshot 2023-09-15 at 4 57 09 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/f20a63a6-9e16-40f7-b269-d697424454f0"> <img width="371" alt="Screenshot 2023-09-15 at 4 57 17 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/c00c2baa-8aa4-403f-aec1-7164a103792c"> <img width="370" alt="Screenshot 2023-09-20 at 3 54 27 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/8adc01ed-6138-43e7-aa93-7514d35f8a5e"> <img width="367" alt="Screenshot 2023-09-20 at 3 54 34 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/b246f7a6-05b1-4f5c-a2e5-d90b99310588"> https://github.com/MetaMask/metamask-extension/assets/22918444/465e7833-6ed1-4c38-b618-81723ed54928 https://github.com/MetaMask/metamask-extension/assets/22918444/049566e1-a121-4803-9166-1f26de2eba6f https://github.com/MetaMask/metamask-extension/assets/22918444/fb628db4-4781-4bab-9efd-b755e011ff26 This change requires the manual installing of the snaps rpc methods handlers and building metamask flask. Here are the steps. 1. checkout this branch and run `yarn start --build-type flask` 2. in your browser, load the extension by clicking `Load unpacked` in the extension settings and using the `dist/chrome/` (or whatever browser you are using) as the source 3. create your account and login 1. open this test snap: https://metamask.github.io/snap-simple-keyring/latest/ 4. connect the dapp 5. click create account. Custom account names are not supported at this time <img width="1182" alt="Screenshot 2023-09-20 at 4 13 31 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/1383cfed-c060-4d6f-b065-db7c9d63e3c2"> 6. click create 7. a new account should be created on the snap 8. open metamask and click the accounts dropdown, a new snap account should be added 1. open this test snap: https://metamask.github.io/snap-simple-keyring/latest/ 2. connect the dapp 3. click create account <img width="1182" alt="Screenshot 2023-09-20 at 4 13 31 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/1383cfed-c060-4d6f-b065-db7c9d63e3c2"> 4. A modal should open on extension 5. click cancel 9. the modal should close and the dapp should throw an error `User denied account creation` <img width="1158" alt="Screenshot 2023-09-20 at 4 14 25 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/7eee4396-a762-4c97-a680-2e03b358596d"> - luckily for us, the snap simple keyring had a bug in version 0.2.1 that we can use to test this flow. To test the error flow, use the following steps 1. open this test snap: https://metamask.github.io/snap-simple-keyring/0.2.1/ 2. connect dapp (uninstall and reinstall if you were using a different version before) 3. click create account 4. the account creation modal should pop up 5. click create, an error should be shown with the message `Method not supported: event:accountCreated` 6. create another account 10. click cancel, the modal should close and the dapp should throw an error `User denied account addition` 1. follow the previous mentioned steps to create 2 or more snap accounts with this [snap](https://metamask.github.io/snap-simple-keyring/latest/) 2. copy the account `id` from the side bar and paste it into the remove account section of the snap 3. click remove account <img width="1184" alt="Screenshot 2023-09-20 at 4 17 09 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/e8fb4891-a59d-436f-995c-02d145e1b3e9"> 4. A modal should show up indicating what you are trying to do 5. click the `share` icon beside the public address and it should open that account on etherscan. It should work with different networks as well. 6. click `remove` 7. a success screen should be shown and the snap should update with the state and the account no longer visible in the side bar 8. open the extension and verify that the account is no longer in the account list 9. repeat steps 1 and 2 10. this time click cancel 11. the extension should close and an error on the snap should appear with the text `User denied account removal` <img width="1168" alt="Screenshot 2023-09-20 at 4 20 03 PM" src="https://github.com/MetaMask/metamask-extension/assets/22918444/e85e8ab1-aec0-4ac7-8a56-5253c82af53b"> 12. verify that the account was not removed from the extension or the snap - [x] I've clearly explained: - [x] What problem this PR is solving - [x] How this problem was solved - [x] How reviewers can test my changes - [x] Sufficient automated test coverage has been added - [x] Manual testing (e.g. pull and build branch, run in browser, test code being changed) - [x] PR is linked to the appropriate GitHub issue - [ ] **IF** this PR fixes a bug in the release milestone, add this PR to the release milestone If further QA is required (e.g. new feature, complex testing steps, large refactor), add the `Extension QA Board` label. In this case, a QA Engineer approval will be be required. --------- Co-authored-by: Monte Lai <[email protected]> Co-authored-by: Matthew Walsh <[email protected]> Co-authored-by: Daniel Rocha <[email protected]> Co-authored-by: Howard Braham <[email protected]> Co-authored-by: MetaMask Bot <[email protected]>

montelaidev · 2023-11-09T05:49:25Z

Breaking up this pr into multiple prs

Accounts Controller PR Sequence
#21437
#21554
#21553
#21709
#21758

montelaidev added 14 commits August 3, 2023 09:23

fix: account-details-menu test

e169619

feat: update import-nfts-modal to use internal account and fix consol…

0cd626a

…e.error warnings in test

fix: hardware selector test

71877f2

feat: add snap keyring to switch case

dfd0f13

fix: tests for ui/pages ui/hooks ui/components

2a19e39

fix: lint

3a015b1

fix: code fence

355c5dd

fix: removable keyrings

ba4618c

fix: fix unit tests

061bb76

fix: send test

3eb0a18

fix: use getAccountExpect

fbeda3c

fix: eth-overview test

f3a17c5

fix: add fence to snap code in accountscontroller

fe9a421

chore: update yarn.lock

84d9670

montelaidev force-pushed the feat/accounts-controller branch from ef4ee14 to 84d9670 Compare August 3, 2023 04:46

montelaidev added 14 commits August 3, 2023 21:32

fix: add and remove account logic on keyring state change

b524093

fix: metamask-controller.actions tests

d1daf9a

fix: remove the use of identities

3228730

feat: add new tests internal accounts in metamask-controller

dd18f59

fix: send-asset-row story

b7b6d9b

fix: add account

a919a0b

fix: snapshot

a4fc477

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

f3625a1

…roller

fix: update actions

e10b25a

fix: update permission to use accounts

38c12eb

fix: actions tests

2d353bc

Merge remote-tracking branch 'origin' into feat/accounts-controller

4741972

fix: merge usage of identities

e8cc01a

fix: import flow e2e

9d087d6

fix: add addressExists callback to snapkeyring

acbb8df

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

3be04ad

…roller

Update LavaMoat policies

37739e8

danroc reviewed Sep 27, 2023

View reviewed changes

app/scripts/metamask-controller.js Outdated Show resolved Hide resolved

danroc reviewed Sep 27, 2023

View reviewed changes

test/e2e/snaps/test-snap-manageAccount.spec.js Outdated Show resolved Hide resolved

danroc reviewed Sep 27, 2023

View reviewed changes

test/e2e/tests/import-flow.spec.js Outdated Show resolved Hide resolved

montelaidev added 4 commits September 28, 2023 11:48

fix: addNewAccount implementation to match newer version

88e5656

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

9a99e78

…roller

fix: remove unused getAccountType

b7a4338

fix: unit tests

cd22f62

Update LavaMoat policies

7315df9

montelaidev added 3 commits September 28, 2023 18:12

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

35be64e

…roller

chore: update policy

3006deb

fix: test-snap-account e2e test to search for SSK name instead of jus…

7121faf

…t snap

Update LavaMoat policies

7aed4f5

albertolive approved these changes Sep 28, 2023

View reviewed changes

danroc reviewed Sep 28, 2023

View reviewed changes

hmalik88 reviewed Sep 28, 2023

View reviewed changes

montelaidev closed this Nov 9, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Accounts Controller #20344

Add Accounts Controller #20344

montelaidev commented Aug 1, 2023 •

edited

Loading

github-actions bot commented Aug 1, 2023

metamaskbot commented Sep 26, 2023

montelaidev commented Sep 26, 2023

metamaskbot commented Sep 26, 2023

montelaidev commented Sep 28, 2023

metamaskbot commented Sep 28, 2023

montelaidev commented Sep 28, 2023

metamaskbot commented Sep 28, 2023

danroc Sep 28, 2023

hmalik88 Sep 28, 2023

montelaidev commented Nov 9, 2023

Add Accounts Controller #20344

Add Accounts Controller #20344

Conversation

montelaidev commented Aug 1, 2023 • edited Loading

Explanation

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Aug 1, 2023

metamaskbot commented Sep 26, 2023

montelaidev commented Sep 26, 2023

metamaskbot commented Sep 26, 2023

montelaidev commented Sep 28, 2023

metamaskbot commented Sep 28, 2023

montelaidev commented Sep 28, 2023

metamaskbot commented Sep 28, 2023

danroc Sep 28, 2023

Choose a reason for hiding this comment

hmalik88 Sep 28, 2023

Choose a reason for hiding this comment

montelaidev commented Nov 9, 2023

montelaidev commented Aug 1, 2023 •

edited

Loading