Skip to content

Commit

Permalink
feat: remove selectSelectedAddress in favour of selectSelectedInterna…
Browse files Browse the repository at this point in the history
…lAccount (#9070)

## **Description**
1. What is the reason for the change?
In an effort to make MetaMask multi chain we are migrating from an
address based accounts to account id based account. In
[this](#8759) previous
PR we integrated the accounts controller which stores metadata like the
account name, address and supported methods. This pr migrates all the
use of `selectSelectedAddress` to use the accounts controller as the
source of truth for the selected address.
2. What is the improvement/solution?
- created `selectSelectedInternalAccount` which returns the entire
account object for the currently selected address
- created a `selectSelectedInternalAccountAddressAsChecksum` that
returns just the selected address instead of the entire accounts object
- deprecated `selectSelectedAddress`
- removed all use of `selectSelectedAddress` in favour of
`selectSelectedInternalAccountAddressAsChecksum` or
`selectSelectedInternalAccount`
- For screens that only use the address then we will use the
`selectSelectedInternalAccountAddressAsChecksum`
- If the screen is using more than just the address (the account name
for example) then we will select the entire account with
`selectSelectedInternalAccount`
- modified all the screen tests to mock the accounts controller state
- I created some test helpers to make mocking the accounts controller
- `createMockInternalAccount` and `createMockUUIDFromAddress` in
`app/selectors/accountsController.test.ts`

## **Related issues**

Fixes: MetaMask/MetaMask-planning#2548

## **Manual testing steps**

#### Fresh install
1. Launch wallet
2. create a new account
4. once on the wallet view, add a new account
5. create a custom name for your account(s)
6. swipe away the app and re open
7. login
8. the selected account should be same one that was previously selected
9. all addresses should be in [checksum
format](https://coincodex.com/article/2078/ethereum-address-checksum-explained/)
10. connect the portfolio dapp via the portfolio button on the home
screen.
11. Click connect on the portfolio dapp
12. The first address in the address picker should be match the selected
address on the wallet view and be in checksum format

#### Upgrade path

1. Use an existing wallet that has multiple accounts created/imported
2. Ideally change the names of your accounts so that they have custom
names
3. "upgrade" your wallet to this branch
4. the selected account should be the same as it was on the previous
branch
5. all of the account data should remain the same (names and balances)

## **Screenshots/Recordings**

### **Before**

N/A

### **After**


https://github.com/MetaMask/metamask-mobile/assets/22918444/52938128-9fbc-48a8-a8bf-0a0f4d7b2b14


## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask 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**

- [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.
  • Loading branch information
owencraston authored Jun 13, 2024
1 parent 76299fb commit 3adefb4
Show file tree
Hide file tree
Showing 90 changed files with 1,212 additions and 837 deletions.
6 changes: 3 additions & 3 deletions app/components/Nav/Main/RootRPCMethodsUI.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ import FlowLoaderModal from '../../Approvals/FlowLoaderModal';
import TemplateConfirmationModal from '../../Approvals/TemplateConfirmationModal';
import { selectTokenList } from '../../../selectors/tokenListController';
import { selectTokens } from '../../../selectors/tokensController';
import { selectSelectedAddress } from '../../../selectors/preferencesController';
import { getDeviceId } from '../../../core/Ledger/Ledger';
import { selectSelectedInternalAccountChecksummedAddress } from '../../../selectors/accountsController';
import { createLedgerTransactionModalNavDetails } from '../../UI/LedgerModals/LedgerTransactionModal';
import ExtendedKeyringTypes from '../../../constants/keyringTypes';
import { useMetrics } from '../../../components/hooks/useMetrics';
Expand Down Expand Up @@ -197,8 +197,8 @@ const RootRPCMethodsUI = (props) => {
[
props.selectedAddress,
props.shouldUseSmartTransaction,
trackEvent,
trackAnonymousEvent,
trackEvent,
],
);

Expand Down Expand Up @@ -482,7 +482,7 @@ RootRPCMethodsUI.propTypes = {
};

const mapStateToProps = (state) => ({
selectedAddress: selectSelectedAddress(state),
selectedAddress: selectSelectedInternalAccountChecksummedAddress(state),
chainId: selectChainId(state),
tokens: selectTokens(state),
swapsTransactions:
Expand Down

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions app/components/UI/AccountApproval/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
} from '../../../selectors/networkController';
import { selectTokensLength } from '../../../selectors/tokensController';
import { selectAccountsLength } from '../../../selectors/accountTrackerController';
import { selectSelectedAddress } from '../../../selectors/preferencesController';
import { selectSelectedInternalAccountChecksummedAddress } from '../../../selectors/accountsController';
import AppConstants from '../../../../app/core/AppConstants';
import { shuffle } from 'lodash';
import SDKConnect from '../../../core/SDKConnect/SDKConnect';
Expand Down Expand Up @@ -376,7 +376,7 @@ class AccountApproval extends PureComponent {
const mapStateToProps = (state) => ({
accountsLength: selectAccountsLength(state),
tokensLength: selectTokensLength(state),
selectedAddress: selectSelectedAddress(state),
selectedAddress: selectSelectedInternalAccountChecksummedAddress(state),
networkType: selectProviderType(state),
chainId: selectChainId(state),
});
Expand Down
9 changes: 9 additions & 0 deletions app/components/UI/AccountApproval/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React from 'react';
import AccountApproval from '.';
import initialBackgroundState from '../../../util/test/initial-background-state.json';
import renderWithProvider from '../../../util/test/renderWithProvider';
import { MOCK_ACCOUNTS_CONTROLLER_STATE } from '../../../util/test/accountsControllerTestUtils';

jest.mock('../../../core/Engine', () => ({
context: {
Expand All @@ -14,6 +15,13 @@ jest.mock('../../../core/Engine', () => ({
},
KeyringController: {
getAccountKeyringType: () => Promise.resolve('HD Key Tree'),
state: {
keyrings: [
{
accounts: ['0xC4955C0d639D99699Bfd7Ec54d9FaFEe40e4D272'],
},
],
},
},
},
}));
Expand All @@ -22,6 +30,7 @@ const mockInitialState = {
engine: {
backgroundState: {
...initialBackgroundState,
AccountsController: MOCK_ACCOUNTS_CONTROLLER_STATE,
},
},
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,15 @@ import { Transaction } from './AccountFromToInfoCard.types';
import AccountFromToInfoCard from '.';
import Engine from '../../../core/Engine';
import initialBackgroundState from '../../../util/test/initial-background-state.json';
import { createMockAccountsControllerState } from '../../../util/test/accountsControllerTestUtils';

const MOCK_ADDRESS_1 = '0xe64dD0AB5ad7e8C5F2bf6Ce75C34e187af8b920A';
const MOCK_ADDRESS_2 = '0x519d2CE57898513F676a5C3b66496c3C394c9CC7';

const MOCK_ACCOUNTS_CONTROLLER_STATE = createMockAccountsControllerState([
MOCK_ADDRESS_1,
MOCK_ADDRESS_2,
]);

const mockInitialState = {
settings: {},
Expand All @@ -17,10 +26,10 @@ const mockInitialState = {
...initialBackgroundState,
AccountTrackerController: {
accounts: {
'0xe64dD0AB5ad7e8C5F2bf6Ce75C34e187af8b920A': {
[MOCK_ADDRESS_1]: {
balance: 200,
},
'0x519d2CE57898513F676a5C3b66496c3C394c9CC7': {
[MOCK_ADDRESS_2]: {
balance: 200,
},
},
Expand All @@ -31,18 +40,19 @@ const mockInitialState = {
},
},
PreferencesController: {
selectedAddress: '0xe64dD0AB5ad7e8C5F2bf6Ce75C34e187af8b920A',
selectedAddress: MOCK_ADDRESS_1,
identities: {
'0xe64dD0AB5ad7e8C5F2bf6Ce75C34e187af8b920A': {
address: '0xe64dD0AB5ad7e8C5F2bf6Ce75C34e187af8b920A',
[MOCK_ADDRESS_1]: {
address: MOCK_ADDRESS_1,
name: 'Account 1',
},
'0x519d2CE57898513F676a5C3b66496c3C394c9CC7': {
address: '0x519d2CE57898513F676a5C3b66496c3C394c9CC7',
[MOCK_ADDRESS_2]: {
address: MOCK_ADDRESS_2,
name: 'Account 2',
},
},
},
AccountsController: MOCK_ACCOUNTS_CONTROLLER_STATE,
},
},
};
Expand Down Expand Up @@ -70,6 +80,7 @@ jest.mock('../../../core/Engine', () => ({
],
},
},
AccountsController: MOCK_ACCOUNTS_CONTROLLER_STATE,
},
}));

Expand Down
14 changes: 6 additions & 8 deletions app/components/UI/AccountInfoCard/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ import React from 'react';
import AccountInfoCard from './';
import renderWithProvider from '../../../util/test/renderWithProvider';
import initialBackgroundState from '../../../util/test/initial-background-state.json';
import {
MOCK_ACCOUNTS_CONTROLLER_STATE,
MOCK_ADDRESS_1,
} from '../../../util/test/accountsControllerTestUtils';

jest.mock('../../../core/Engine', () => ({
resetState: jest.fn(),
Expand All @@ -26,18 +30,12 @@ const mockInitialState = {
...initialBackgroundState,
AccountTrackerController: {
accounts: {
'0xC4955C0d639D99699Bfd7Ec54d9FaFEe40e4D272': {
[MOCK_ADDRESS_1]: {
balance: '0x2',
},
},
},
PreferencesController: {
selectedAddress: '0xC4955C0d639D99699Bfd7Ec54d9FaFEe40e4D272',
identities: {
address: '0xC4955C0d639D99699Bfd7Ec54d9FaFEe40e4D272',
name: 'Account 1',
},
},
AccountsController: MOCK_ACCOUNTS_CONTROLLER_STATE,
CurrencyRateController: {
currentCurrency: 'inr',
currencyRates: {
Expand Down
8 changes: 3 additions & 5 deletions app/components/UI/AccountOverview/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,8 @@ import AppConstants from '../../../core/AppConstants';
import Engine from '../../../core/Engine';
import { selectChainId } from '../../../selectors/networkController';
import { selectCurrentCurrency } from '../../../selectors/currencyRateController';
import {
selectIdentities,
selectSelectedAddress,
} from '../../../selectors/preferencesController';
import { selectIdentities } from '../../../selectors/preferencesController';
import { selectSelectedInternalAccountChecksummedAddress } from '../../../selectors/accountsController';
import { createAccountSelectorNavDetails } from '../../Views/AccountSelector';
import Text, {
TextVariant,
Expand Down Expand Up @@ -462,7 +460,7 @@ class AccountOverview extends PureComponent {
}

const mapStateToProps = (state) => ({
selectedAddress: selectSelectedAddress(state),
selectedAddress: selectSelectedInternalAccountChecksummedAddress(state),
identities: selectIdentities(state),
currentCurrency: selectCurrentCurrency(state),
chainId: selectChainId(state),
Expand Down
10 changes: 8 additions & 2 deletions app/components/UI/AccountOverview/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,13 @@ import AccountOverview from './';
import initialBackgroundState from '../../../util/test/initial-background-state.json';

import Engine from '../../../core/Engine';
import {
MOCK_ACCOUNTS_CONTROLLER_STATE,
MOCK_ADDRESS_1,
} from '../../../util/test/accountsControllerTestUtils';

const mockedEngine = Engine;

jest.mock('../../../core/Engine.ts', () => ({
init: () => mockedEngine.init({}),
context: {
Expand All @@ -14,7 +19,7 @@ jest.mock('../../../core/Engine.ts', () => ({
state: {
keyrings: [
{
accounts: ['0x76cf1CdD1fcC252442b50D6e97207228aA4aefC3'],
accounts: ['0xC4955C0d639D99699Bfd7Ec54d9FaFEe40e4D272'],
index: 0,
type: 'HD Key Tree',
},
Expand All @@ -30,8 +35,9 @@ const mockInitialState = {
backgroundState: {
...initialBackgroundState,
PreferencesController: {
selectedAddress: '0x76cf1CdD1fcC252442b50D6e97207228aA4aefC3',
selectedAddress: MOCK_ADDRESS_1,
},
AccountsController: MOCK_ACCOUNTS_CONTROLLER_STATE,
},
},
};
Expand Down
20 changes: 10 additions & 10 deletions app/components/UI/AccountSelectorList/AccountSelector.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@ import { View } from 'react-native';
import { ACCOUNT_BALANCE_BY_ADDRESS_TEST_ID } from '../../../../wdio/screen-objects/testIDs/Components/AccountListComponent.testIds';
import initialBackgroundState from '../../../util/test/initial-background-state.json';
import { regex } from '../../../../app/util/regex';
import { MOCK_ACCOUNTS_CONTROLLER_STATE } from '../../../util/test/accountsControllerTestUtils';
import { toChecksumAddress } from 'ethereumjs-util';
import { createMockAccountsControllerState } from '../../../util/test/accountsControllerTestUtils';

const MOCK_ACCOUNT_ADDRESSES = Object.values(
MOCK_ACCOUNTS_CONTROLLER_STATE.internalAccounts.accounts,
).map((account) => account.address);
const BUSINESS_ACCOUNT = '0xC4955C0d639D99699Bfd7Ec54d9FaFEe40e4D272';
const PERSONAL_ACCOUNT = '0xd018538C87232FF95acbCe4870629b75640a78E7';

const BUSINESS_ACCOUNT = toChecksumAddress(MOCK_ACCOUNT_ADDRESSES[0]);
const PERSONAL_ACCOUNT = toChecksumAddress(MOCK_ACCOUNT_ADDRESSES[1]);
const MOCK_ACCOUNTS_CONTROLLER_STATE = createMockAccountsControllerState([
BUSINESS_ACCOUNT,
PERSONAL_ACCOUNT,
]);

jest.mock('../../../util/address', () => {
const actual = jest.requireActual('../../../util/address');
Expand Down Expand Up @@ -172,12 +172,12 @@ describe('AccountSelectorList', () => {
expect(accounts.length).toBe(1);

const businessAccountItem = await queryByTestId(
`${ACCOUNT_BALANCE_BY_ADDRESS_TEST_ID}-${PERSONAL_ACCOUNT}`,
`${ACCOUNT_BALANCE_BY_ADDRESS_TEST_ID}-${BUSINESS_ACCOUNT}`,
);

expect(within(businessAccountItem).getByText(regex.eth(2))).toBeDefined();
expect(within(businessAccountItem).getByText(regex.eth(1))).toBeDefined();
expect(
within(businessAccountItem).getByText(regex.usd(6400)),
within(businessAccountItem).getByText(regex.usd(3200)),
).toBeDefined();

expect(toJSON()).toMatchSnapshot();
Expand Down

Large diffs are not rendered by default.

6 changes: 4 additions & 2 deletions app/components/UI/AddCustomCollectible/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
NFT_IDENTIFIER_INPUT_BOX_ID,
} from '../../../../wdio/screen-objects/testIDs/Screens/NFTImportScreen.testIds';
import { selectChainId } from '../../../selectors/networkController';
import { selectSelectedAddress } from '../../../selectors/preferencesController';
import { selectSelectedInternalAccountChecksummedAddress } from '../../../selectors/accountsController';
import { getDecimalChainId } from '../../../util/networks';
import { useMetrics } from '../../../components/hooks/useMetrics';

Expand Down Expand Up @@ -85,7 +85,9 @@ const AddCustomCollectible = ({
const { trackEvent } = useMetrics();
const styles = createStyles(colors);

const selectedAddress = useSelector(selectSelectedAddress);
const selectedAddress = useSelector(
selectSelectedInternalAccountChecksummedAddress,
);
const chainId = useSelector(selectChainId);

useEffect(() => {
Expand Down
10 changes: 5 additions & 5 deletions app/components/UI/AddressCopy/AddressCopy.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,8 @@ import generateTestId from '../../../../wdio/utils/generateTestId';
// Internal dependencies
import styleSheet from './AddressCopy.styles';
import { AddressCopyProps } from './AddressCopy.types';
import {
selectIdentities,
selectSelectedAddress,
} from '../../../selectors/preferencesController';
import { selectIdentities } from '../../../selectors/preferencesController';
import { selectSelectedInternalAccountChecksummedAddress } from '../../../selectors/accountsController';
import { useMetrics } from '../../../components/hooks/useMetrics';

const AddressCopy = ({ formatAddressType = 'full' }: AddressCopyProps) => {
Expand All @@ -51,7 +49,9 @@ const AddressCopy = ({ formatAddressType = 'full' }: AddressCopyProps) => {
/**
* A string that represents the selected address
*/
const selectedAddress = useSelector(selectSelectedAddress);
const selectedAddress = useSelector(
selectSelectedInternalAccountChecksummedAddress,
);

/**
* An object containing each identity in the format address => account
Expand Down
6 changes: 4 additions & 2 deletions app/components/UI/AssetOverview/AssetOverview.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {
import { selectContractExchangeRates } from '../../../selectors/tokenRatesController';
import { selectAccountsByChainId } from '../../../selectors/accountTrackerController';
import { selectContractBalances } from '../../../selectors/tokenBalancesController';
import { selectSelectedAddress } from '../../../selectors/preferencesController';
import { selectSelectedInternalAccountChecksummedAddress } from '../../../selectors/accountsController';
import Logger from '../../../util/Logger';
import { safeToChecksumAddress } from '../../../util/address';
import {
Expand Down Expand Up @@ -70,7 +70,9 @@ const AssetOverview: React.FC<AssetOverviewProps> = ({
const primaryCurrency = useSelector(
(state: RootStateOrAny) => state.settings.primaryCurrency,
);
const selectedAddress = useSelector(selectSelectedAddress);
const selectedAddress = useSelector(
selectSelectedInternalAccountChecksummedAddress,
);
const tokenExchangeRates = useSelector(selectContractExchangeRates);
const tokenBalances = useSelector(selectContractBalances);
const chainId = useSelector((state: RootStateOrAny) => selectChainId(state));
Expand Down
4 changes: 2 additions & 2 deletions app/components/UI/CollectibleContractElement/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { removeFavoriteCollectible } from '../../../actions/collectibles';
import { collectibleContractsSelector } from '../../../reducers/collectibles';
import { useTheme } from '../../../util/theme';
import { selectChainId } from '../../../selectors/networkController';
import { selectSelectedAddress } from '../../../selectors/preferencesController';
import { selectSelectedInternalAccountChecksummedAddress } from '../../../selectors/accountsController';
import Icon, {
IconName,
IconColor,
Expand Down Expand Up @@ -306,7 +306,7 @@ CollectibleContractElement.propTypes = {
const mapStateToProps = (state) => ({
collectibleContracts: collectibleContractsSelector(state),
chainId: selectChainId(state),
selectedAddress: selectSelectedAddress(state),
selectedAddress: selectSelectedInternalAccountChecksummedAddress(state),
});

const mapDispatchToProps = (dispatch) => ({
Expand Down
4 changes: 2 additions & 2 deletions app/components/UI/CollectibleContracts/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ import {
import {
selectDisplayNftMedia,
selectIsIpfsGatewayEnabled,
selectSelectedAddress,
selectUseNftDetection,
} from '../../../selectors/preferencesController';
import { selectSelectedInternalAccountChecksummedAddress } from '../../../selectors/accountsController';
import {
IMPORT_NFT_BUTTON_ID,
NFT_TAB_CONTAINER_ID,
Expand Down Expand Up @@ -422,7 +422,7 @@ CollectibleContracts.propTypes = {
const mapStateToProps = (state) => ({
networkType: selectProviderType(state),
chainId: selectChainId(state),
selectedAddress: selectSelectedAddress(state),
selectedAddress: selectSelectedInternalAccountChecksummedAddress(state),
useNftDetection: selectUseNftDetection(state),
collectibleContracts: collectibleContractsSelector(state),
collectibles: collectiblesSelector(state),
Expand Down
Loading

0 comments on commit 3adefb4

Please sign in to comment.