Skip to content

Commit

Permalink
Add Accounts Controller (#21437)
Browse files Browse the repository at this point in the history
## **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**:
1. PermissionsController

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

2. IncomingTransactionHelper
- `getAccount` now returns an InternalAccount, and uses the
AccountsController's `getSelectedAccount`

3. AlertController

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

4. DetectTokenController

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

5. AccountTracker

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

6. Backup

- adds backup of AccountsController

7. 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**:
1. 103 migration file, this migration creates an InternalAccount for
every identity and selectedAddress to selectedAccount on the
AccountsController
2. Adds AccountsController to the store and memStore
3. Adds `setSelectedInternalAccount` and `setAccountName` to the api
4. Changes `selectFirstIdentity` to `selectFirstAccount`
5. Unlock hardware wallet to return internal accounts 
6. `resetAccount` to use getSelectedAccount
7. Adds a new subscription, `selectedAccountChange` to change accounts
in `setupControllerEventSubscription`


## **Related issues**

Resolves MetaMask/accounts-planning#133

## **Manual testing steps**

1. Use a hardware wallet to test all the flows

## **Screenshots/Recordings**

No UI changes

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [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] I’ve indicated what issue this PR is linked to: Fixes #???
- [x] I’ve included tests if applicable.
- [x] I’ve documented any added code.
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
- [x] I’ve properly set the pull request status:
  - [x] In case it's not yet "ready for review", I've set it to "draft".
- [x] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

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

---------

Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com>
Co-authored-by: gantunesr <gustavoantunesrod@gmail.com>
Co-authored-by: Howard Braham <howrad@gmail.com>
Co-authored-by: Gustavo Antunes <17601467+gantunesr@users.noreply.github.com>
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
  • Loading branch information
6 people committed Nov 30, 2023
1 parent b99a88b commit 9487e82
Show file tree
Hide file tree
Showing 40 changed files with 1,877 additions and 334 deletions.
111 changes: 110 additions & 1 deletion .storybook/test-data.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { draftTransactionInitialState } from '../ui/ducks/send';
import { KeyringType } from '../shared/constants/keyring';
import { NetworkType } from '@metamask/controller-utils';
import { NetworkStatus } from '@metamask/network-controller';
import { EthAccountType, EthMethod } from '@metamask/keyring-api';
import { CHAIN_IDS } from '../shared/constants/network';

const state = {
Expand Down Expand Up @@ -296,6 +297,63 @@ const state = {
isUnlocked: true,
isAccountMenuOpen: false,
rpcUrl: 'https://rawtestrpc.metamask.io/',
internalAccounts: {
accounts: {
'cf8dace4-9439-4bd4-b3a8-88c821c8fcb3': {
address: '0x64a845a5b02460acf8a3d84503b0d68d028b4bb4',
id: 'cf8dace4-9439-4bd4-b3a8-88c821c8fcb3',
metadata: {
name: 'This is a Really Long Account Name',
keyring: {
type: 'HD Key Tree',
},
},
options: {},
methods: [...Object.values(EthMethod)],
type: EthAccountType.Eoa,
},
'07c2cfec-36c9-46c4-8115-3836d3ac9047': {
address: '0xb19ac54efa18cc3a14a5b821bfec73d284bf0c5e',
id: '07c2cfec-36c9-46c4-8115-3836d3ac9047',
metadata: {
name: 'Account 2',
keyring: {
type: 'HD Key Tree',
},
},
options: {},
methods: [...Object.values(EthMethod)],
type: EthAccountType.Eoa,
},
'15e69915-2a1a-4019-93b3-916e11fd432f': {
address: '0x9d0ba4ddac06032527b140912ec808ab9451b788',
id: '15e69915-2a1a-4019-93b3-916e11fd432f',
metadata: {
name: 'Account 3',
keyring: {
type: 'HD Key Tree',
},
},
options: {},
methods: [...Object.values(EthMethod)],
type: EthAccountType.Eoa,
},
'784225f4-d30b-4e77-a900-c8bbce735b88': {
address: '0xeb9e64b93097bc15f01f13eae97015c57ab64823',
id: '784225f4-d30b-4e77-a900-c8bbce735b88',
metadata: {
name: 'Account 4',
keyring: {
type: 'HD Key Tree',
},
},
options: {},
methods: [...Object.values(EthMethod)],
type: EthAccountType.Eoa,
},
},
selectedAccount: 'cf8dace4-9439-4bd4-b3a8-88c821c8fcb3',
},
identities: {
'0x64a845a5b02460acf8a3d84503b0d68d028b4bb4': {
name: 'This is a Really Long Account Name',
Expand Down Expand Up @@ -449,6 +507,57 @@ const state = {
],
},
],
'0x64a845a5b02460acf8a3d84503b0d68d028b4bb4': [
{
address: '0x514910771AF9Ca656af840dff83E8264EcF986CA',
decimals: 18,
symbol: 'LINK',
image:
'https://crypto.com/price/coin-data/icon/LINK/color_icon.png',
aggregators: [
'coinGecko',
'oneInch',
'paraswap',
'zapper',
'zerion',
],
},
{
address: '0xc00e94Cb662C3520282E6f5717214004A7f26888',
decimals: 18,
symbol: 'COMP',
image:
'https://crypto.com/price/coin-data/icon/COMP/color_icon.png',
aggregators: [
'bancor',
'cmc',
'cryptocom',
'coinGecko',
'oneInch',
'paraswap',
'pmm',
'zapper',
'zerion',
'zeroEx',
],
},
{
address: '0xfffffffFf15AbF397dA76f1dcc1A1604F45126DB',
decimals: 18,
symbol: 'FSW',
image:
'https://assets.coingecko.com/coins/images/12256/thumb/falconswap.png?1598534184',
aggregators: [
'aave',
'cmc',
'coinGecko',
'oneInch',
'paraswap',
'zapper',
'zerion',
],
},
],
},
},
detectedTokens: [
Expand Down Expand Up @@ -619,7 +728,7 @@ const state = {
path: '/txParams/nonce',
timestamp: 1629582711220,
value: '0x15b',
}
},
],
[
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
diff --git a/dist/utils.js b/dist/utils.js
index 810f229841ffff83f7a28191bc558862b1809e01..2daf3b12cf8a77aad9a5e0a6167734540805ecfc 100644
--- a/dist/utils.js
+++ b/dist/utils.js
@@ -36,6 +36,13 @@ function keyringTypeToName(keyringType) {
case keyring_controller_1.KeyringTypes.custody: {
return 'Custody';
}
+ /**
+ * PATCH INFORMATION - The keyring type used for custody account has been changed from 'Custody' to 'Custody - JSONRPC'.
+ * To do: Develop a better solution to keep all keyring types in sync.
+ */
+ case 'Custody - JSONRPC': {
+ return 'Custody';
+ }
default: {
throw new Error(`Unknown keyring ${keyringType}`);
}
30 changes: 18 additions & 12 deletions app/scripts/controllers/alert.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export default class AlertController {
* @param {AlertControllerOptions} [opts] - Controller configuration parameters
*/
constructor(opts = {}) {
const { initState = {}, preferencesStore } = opts;
const { initState = {}, controllerMessenger } = opts;
const state = {
...defaultState,
alertEnabledness: {
Expand All @@ -48,19 +48,25 @@ export default class AlertController {
};

this.store = new ObservableStore(state);
this.controllerMessenger = controllerMessenger;

this.selectedAddress = preferencesStore.getState().selectedAddress;
this.selectedAddress = this.controllerMessenger.call(
'AccountsController:getSelectedAccount',
);

preferencesStore.subscribe(({ selectedAddress }) => {
const currentState = this.store.getState();
if (
currentState.unconnectedAccountAlertShownOrigins &&
this.selectedAddress !== selectedAddress
) {
this.selectedAddress = selectedAddress;
this.store.updateState({ unconnectedAccountAlertShownOrigins: {} });
}
});
this.controllerMessenger.subscribe(
'AccountsController:selectedAccountChange',
(account) => {
const currentState = this.store.getState();
if (
currentState.unconnectedAccountAlertShownOrigins &&
this.selectedAddress !== account.address
) {
this.selectedAddress = account.address;
this.store.updateState({ unconnectedAccountAlertShownOrigins: {} });
}
},
);
}

setAlertEnabledness(alertId, enabledness) {
Expand Down
35 changes: 27 additions & 8 deletions app/scripts/controllers/detect-tokens.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export default class DetectTokensController extends PollingControllerOnly {
* @param config.assetsContractController
* @param config.trackMetaMetricsEvent
* @param config.messenger
* @param config.getCurrentSelectedAccount
* @param config.getNetworkClientById
* @param config.disableLegacyInterval
*/
Expand All @@ -46,6 +47,7 @@ export default class DetectTokensController extends PollingControllerOnly {
tokensController,
assetsContractController = null,
trackMetaMetricsEvent,
getCurrentSelectedAccount,
getNetworkClientById,
disableLegacyInterval = false,
} = {}) {
Expand All @@ -62,7 +64,7 @@ export default class DetectTokensController extends PollingControllerOnly {
this.tokenList = tokenList;
this.useTokenDetection =
this.preferences?.store.getState().useTokenDetection;
this.selectedAddress = this.preferences?.store.getState().selectedAddress;
this.selectedAddress = getCurrentSelectedAccount().address;
this.tokenAddresses = this.tokensController?.state.tokens.map((token) => {
return token.address;
});
Expand All @@ -72,16 +74,33 @@ export default class DetectTokensController extends PollingControllerOnly {
this.chainId = this.getChainIdFromNetworkStore();
this._trackMetaMetricsEvent = trackMetaMetricsEvent;

preferences?.store.subscribe(({ selectedAddress, useTokenDetection }) => {
if (
this.selectedAddress !== selectedAddress ||
this.useTokenDetection !== useTokenDetection
) {
this.selectedAddress = selectedAddress;
messenger.subscribe(
'AccountsController:selectedAccountChange',
(account) => {
const useTokenDetection =
this.preferences?.store.getState().useTokenDetection;
if (
this.selectedAddress !== account.address ||
this.useTokenDetection !== useTokenDetection
) {
this.selectedAddress = account.address;
this.useTokenDetection = useTokenDetection;
this.restartTokenDetection({
selectedAddress: this.selectedAddress,
});
}
},
);

preferences?.store.subscribe(({ useTokenDetection }) => {
if (this.useTokenDetection !== useTokenDetection) {
this.useTokenDetection = useTokenDetection;
this.restartTokenDetection({ selectedAddress });
this.restartTokenDetection({
selectedAddress: this.selectedAddress,
});
}
});

tokensController?.subscribe(
({ tokens = [], ignoredTokens = [], detectedTokens = [] }) => {
this.tokenAddresses = tokens.map((token) => {
Expand Down
Loading

0 comments on commit 9487e82

Please sign in to comment.