Skip to content

Commit 78adf4f

Browse files
gantunesrgantunesr
authored
fix: account list in SRP reveal flow (#39005)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/39005?quickstart=1) The account list displayed on the SRP reveal flow was outdated by showing the UI pre-BIP44. This PR updates the UI to show the correct multichain account list. ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: Update SRP flow to display multichain accounts ## **Related issues** Fixes: #35731 Fixes: #38115 Fixes: https://consensyssoftware.atlassian.net/browse/MUL-1363 ## **Manual testing steps** 1. Import 2 or more SRPs 2. Create multiple accounts for each SRP (3 or more) 3. Go to the reveal SRP flow 4. Check that the account name, icon, and balance is correct when toggling the "Show X accounts" option 5. Check that the revealed SRP is the correct one ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** Review linked issues for before screenshots ### **After** https://github.com/user-attachments/assets/a1980e71-11b3-4174-8112-e981355c0e28 ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/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-extension/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. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Modernizes the SRP reveal flow to correctly display multichain accounts and balances. > > - Introduces `SrpCard` and simplifies `SrpList` to source wallet IDs via `getWalletIdsByType(AccountWalletType.Entropy)` and render accounts from `useWalletInfo` > - Simplifies `SrpListItem` API to `accountId/accountName/balance`, uses `PreferredAvatar` seeded by `getIconSeedAddressByAccountGroupId`, and displays provided fiat balance > - Adds selector `getWalletIdsByType` in `selectors/multichain-accounts/account-tree` > - Updates/adds tests: `srp-list.test.tsx`, `srp-card.test.tsx`, and `srp-list-item.test.tsx` to match new data flow and UI > - E2E: stabilizes multichain account creation by waiting for "Adding account..." to clear; fixes SRP ownership check and expected account label > - Minor UI tweak: increases `srp-list__account-name` max width to 120px and exports `SrpListItem` > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit e09bda0. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 61f700c commit 78adf4f

File tree

11 files changed

+457
-312
lines changed

11 files changed

+457
-312
lines changed

‎test/e2e/flask/multi-srp/import-srp.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ describe('Multi SRP - Import SRP', function (this: Suite) {
4040
},
4141
async (driver: Driver) => {
4242
const accountListPage = new AccountListPage(driver);
43-
await accountListPage.checkAccountBelongsToSrp('Account 2', 2);
43+
await accountListPage.checkAccountBelongsToSrp('Account 1', 2);
4444
},
4545
);
4646
});

‎test/e2e/page-objects/pages/account-list-page.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -404,7 +404,15 @@ class AccountListPage {
404404
const createMultichainAccountButtons = await this.driver.findElements(
405405
this.addMultichainAccountButton,
406406
);
407-
await createMultichainAccountButtons[options?.srpIndex ?? 0].click();
407+
const buttonIndex = options?.srpIndex ?? 0;
408+
await createMultichainAccountButtons[buttonIndex].click();
409+
410+
// Wait for the account creation to complete by waiting for loading state to finish
411+
// The button shows "Adding account..." during loading and "Add account" when done
412+
await this.driver.assertElementNotPresent({
413+
css: this.addMultichainAccountButton,
414+
text: 'Adding account...',
415+
});
408416
}
409417

410418
/**
@@ -1058,6 +1066,7 @@ class AccountListPage {
10581066
if (srpIndex === 0) {
10591067
throw new Error('SRP index must be > 0');
10601068
}
1069+
10611070
const srps = await this.driver.findElements('.select-srp__container');
10621071
const selectedSrp = srps[srpIndex - 1];
10631072
const showAccountsButton = await this.driver.waitForSelector(

‎ui/components/multichain/multi-srp/srp-list/index.scss‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,6 @@
2828
&__account-name {
2929
overflow: hidden;
3030
white-space: nowrap;
31-
max-width: 80px;
31+
max-width: 120px;
3232
}
3333
}

‎ui/components/multichain/multi-srp/srp-list/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
11
export { SrpList } from './srp-list';
2+
export { SrpListItem } from './srp-list-item';

‎ui/components/multichain/multi-srp/srp-list/srp-card.test.tsx‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
import React from 'react';
2+
import configureMockStore from 'redux-mock-store';
3+
import thunk from 'redux-thunk';
4+
import { fireEvent } from '@testing-library/react';
5+
6+
import { KeyringTypes } from '@metamask/keyring-controller';
7+
import type { AccountGroupId, AccountWalletId } from '@metamask/account-api';
8+
9+
import { renderWithProvider } from '../../../../../test/lib/render-helpers-navigate';
10+
import mockState from '../../../../../test/data/mock-state.json';
11+
import { FirstTimeFlowType } from '../../../../../shared/constants/onboarding';
12+
import { SrpCard } from './srp-card';
13+
14+
const mockWalletId = 'mock-wallet-id' as AccountWalletId;
15+
const mockTotalFiatBalance = '$100.00';
16+
17+
const mocks = {
18+
useSingleWalletAccountsBalanceCallback: jest
19+
.fn()
20+
.mockReturnValue((_: AccountGroupId) => mockTotalFiatBalance),
21+
onActionComplete: jest.fn(),
22+
useWalletInfoCallback: jest.fn().mockReturnValue({
23+
multichainAccounts: [
24+
{
25+
id: 'mock-account-id-1' as AccountGroupId,
26+
metadata: {
27+
name: 'Mock Account 1',
28+
},
29+
},
30+
],
31+
keyringId: '01JKAF3DSGM3AB87EM9N0K41AJ',
32+
isSRPBackedUp: true,
33+
}),
34+
};
35+
36+
jest.mock('../../../../hooks/multichain-accounts/useWalletBalance', () => ({
37+
useSingleWalletAccountsBalanceCallback: (walletId: AccountWalletId) =>
38+
mocks.useSingleWalletAccountsBalanceCallback(walletId),
39+
}));
40+
41+
jest.mock('../../../../hooks/multichain-accounts/useWalletInfo', () => ({
42+
useWalletInfo: (walletId: AccountWalletId) =>
43+
mocks.useWalletInfoCallback(walletId),
44+
}));
45+
46+
const mockSecondHdKeyring = {
47+
accounts: [],
48+
type: KeyringTypes.hd,
49+
metadata: {
50+
id: '01JN31PKMJ3ANWYFJZM3Z8MYT4',
51+
name: '',
52+
},
53+
};
54+
55+
const render = (shouldTriggerBackup: boolean) => {
56+
const store = configureMockStore([thunk])({
57+
...mockState,
58+
metamask: {
59+
...mockState.metamask,
60+
keyrings: [...mockState.metamask.keyrings, mockSecondHdKeyring],
61+
firstTimeFlowType: FirstTimeFlowType.create,
62+
seedPhraseBackedUp: false,
63+
},
64+
});
65+
66+
return renderWithProvider(
67+
<SrpCard
68+
index={0}
69+
walletId={mockWalletId}
70+
shouldTriggerBackup={shouldTriggerBackup}
71+
onActionComplete={mocks.onActionComplete}
72+
/>,
73+
store,
74+
);
75+
};
76+
77+
describe('SrpCard', () => {
78+
it('renders the secret recovery phrases card', () => {
79+
const { getByText } = render(false);
80+
expect(getByText('Secret Recovery Phrase 1')).toBeInTheDocument();
81+
});
82+
83+
it('shows/hides accounts when clicking show/hide text', () => {
84+
const { getByText } = render(false);
85+
const showAccountsButton = getByText('Show 1 account');
86+
fireEvent.click(showAccountsButton);
87+
expect(getByText('Hide 1 account')).toBeInTheDocument();
88+
});
89+
90+
it('calls onActionComplete when clicking a keyring', () => {
91+
const { getByTestId } = render(true);
92+
const firstKeyringId = mockState.metamask.keyrings[0].metadata.id;
93+
94+
const keyring = getByTestId(`hd-keyring-${firstKeyringId}`);
95+
fireEvent.click(keyring);
96+
97+
expect(mocks.onActionComplete).toHaveBeenCalledWith(firstKeyringId, true);
98+
});
99+
});

‎ui/components/multichain/multi-srp/srp-list/srp-card.tsx‎

Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,174 @@
1+
import React, { useState, useContext, useCallback } from 'react';
2+
import type { AccountWalletId } from '@metamask/account-api';
3+
4+
import { MetaMetricsContext } from '../../../../contexts/metametrics';
5+
import { useWalletInfo } from '../../../../hooks/multichain-accounts/useWalletInfo';
6+
import { useI18nContext } from '../../../../hooks/useI18nContext';
7+
import { useSingleWalletAccountsBalanceCallback } from '../../../../hooks/multichain-accounts/useWalletBalance';
8+
import {
9+
MetaMetricsEventCategory,
10+
MetaMetricsEventName,
11+
} from '../../../../../shared/constants/metametrics';
12+
13+
import Card from '../../../ui/card';
14+
import {
15+
Box,
16+
IconName,
17+
Icon,
18+
Text,
19+
IconSize,
20+
} from '../../../component-library';
21+
import {
22+
JustifyContent,
23+
Display,
24+
TextColor,
25+
FlexDirection,
26+
AlignItems,
27+
BlockSize,
28+
TextVariant,
29+
IconColor,
30+
} from '../../../../helpers/constants/design-system';
31+
import { SrpListItem } from './srp-list-item';
32+
33+
/**
34+
* Props for the SrpCard component.
35+
*/
36+
type SrpCardProps = {
37+
index: number;
38+
walletId: AccountWalletId;
39+
shouldTriggerBackup: boolean;
40+
onActionComplete: (id: string, triggerBackup?: boolean) => void;
41+
isSettingsPage?: boolean;
42+
hideShowAccounts?: boolean;
43+
};
44+
45+
export const SrpCard = ({
46+
index,
47+
walletId,
48+
shouldTriggerBackup,
49+
onActionComplete,
50+
isSettingsPage = false,
51+
hideShowAccounts = false,
52+
}: SrpCardProps) => {
53+
const t = useI18nContext();
54+
const trackEvent = useContext(MetaMetricsContext);
55+
const { multichainAccounts, keyringId } = useWalletInfo(walletId);
56+
const [showAccounts, setShowAccounts] = useState<boolean>(false);
57+
const walletAccountBalance = useSingleWalletAccountsBalanceCallback(walletId);
58+
59+
const showHideText = useCallback(
60+
(numberOfAccounts: number): string => {
61+
if (numberOfAccounts > 1) {
62+
return showAccounts
63+
? t('SrpListHideAccounts', [numberOfAccounts])
64+
: t('SrpListShowAccounts', [numberOfAccounts]);
65+
}
66+
return showAccounts
67+
? t('SrpListHideSingleAccount', [numberOfAccounts])
68+
: t('SrpListShowSingleAccount', [numberOfAccounts]);
69+
},
70+
[showAccounts, t],
71+
);
72+
73+
return (
74+
<Card
75+
key={`srp-${index}-${keyringId}`}
76+
data-testid={`hd-keyring-${keyringId}`}
77+
onClick={() => {
78+
trackEvent({
79+
category: MetaMetricsEventCategory.Accounts,
80+
event: MetaMetricsEventName.SecretRecoveryPhrasePickerClicked,
81+
properties: {
82+
// TODO: Fix in https://github.com/MetaMask/metamask-extension/issues/31860
83+
// eslint-disable-next-line @typescript-eslint/naming-convention
84+
button_type: 'srp_select',
85+
},
86+
});
87+
keyringId && onActionComplete(keyringId, shouldTriggerBackup);
88+
}}
89+
className="select-srp__container"
90+
marginBottom={3}
91+
>
92+
<Box
93+
display={Display.Flex}
94+
flexDirection={FlexDirection.Row}
95+
alignItems={AlignItems.center}
96+
justifyContent={JustifyContent.spaceBetween}
97+
>
98+
<Box>
99+
<Text variant={TextVariant.bodyMdMedium}>
100+
{t('srpListName', [index + 1])}
101+
</Text>
102+
{!hideShowAccounts && (
103+
<Text
104+
variant={TextVariant.bodySm}
105+
color={TextColor.primaryDefault}
106+
className="srp-list__show-accounts"
107+
data-testid={`srp-list-show-accounts-${index}`}
108+
onClick={(event: React.MouseEvent) => {
109+
event.stopPropagation();
110+
trackEvent({
111+
category: MetaMetricsEventCategory.Accounts,
112+
event: MetaMetricsEventName.SecretRecoveryPhrasePickerClicked,
113+
properties: {
114+
// TODO: Fix in https://github.com/MetaMask/metamask-extension/issues/31860
115+
// eslint-disable-next-line @typescript-eslint/naming-convention
116+
button_type: 'details',
117+
},
118+
});
119+
setShowAccounts((prevState) => !prevState);
120+
}}
121+
>
122+
{showHideText(multichainAccounts.length)}
123+
</Text>
124+
)}
125+
</Box>
126+
<Box display={Display.Flex} alignItems={AlignItems.center} gap={1}>
127+
{isSettingsPage && (
128+
<Text
129+
variant={TextVariant.bodyMdMedium}
130+
color={
131+
shouldTriggerBackup
132+
? TextColor.errorDefault
133+
: TextColor.textAlternative
134+
}
135+
>
136+
{shouldTriggerBackup
137+
? t('srpListStateNotBackedUp')
138+
: t('srpListStateBackedUp')}
139+
</Text>
140+
)}
141+
<Icon
142+
name={IconName.ArrowRight}
143+
size={IconSize.Sm}
144+
color={
145+
shouldTriggerBackup && isSettingsPage
146+
? IconColor.errorDefault
147+
: IconColor.iconAlternative
148+
}
149+
/>
150+
</Box>
151+
</Box>
152+
{showAccounts && (
153+
<Box>
154+
<Box
155+
width={BlockSize.Full}
156+
className="srp-list__divider"
157+
marginTop={2}
158+
marginBottom={2}
159+
/>
160+
{multichainAccounts.map((group) => {
161+
return (
162+
<SrpListItem
163+
key={`account-${group.id}`}
164+
accountId={group.id}
165+
accountName={group.metadata.name}
166+
balance={walletAccountBalance(group.id) ?? ''}
167+
/>
168+
);
169+
})}
170+
</Box>
171+
)}
172+
</Card>
173+
);
174+
};

0 commit comments

Comments
 (0)