Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: multisrp ui #29816

Closed
wants to merge 241 commits into from
Closed

feat: multisrp ui #29816

wants to merge 241 commits into from

Conversation

montelaidev
Copy link
Contributor

@montelaidev montelaidev commented Jan 21, 2025

Description

This PR introduces multisrp to the UI. It enables the importing of a SRP, creating accounts associated to a SRP and revealing the associated SRP.

Related issues

Depends on #29942

Manual testing steps

Importing a new SRP

  1. Click on the account selector
  2. Click on connect more
  3. Click on Import secret recovery phrase
  4. Paste in a SRP
  5. Continue
  6. See a new account is created
  7. Click on the account selector and see new pills indicating the SRP numbers

Creating an account for a specific SRP

  1. Import a new SRP
  2. Click on add new account
  3. Select the SRP you want to create the account to.
  4. See a new account is created

Revealing SRP

  1. Import a new SRP
  2. Click on the account menu
  3. Go to settings --> security and privacy
  4. Click on reveal srp
  5. Select the srp that you want to reveal
  6. Go through SRP quiz and click. to reveal
  7. See the SRP

Screenshots/Recordings

After

Importing a new SRP

Screen.Recording.2025-02-14.at.17.26.32.mov

Creating accounts associated to an srp

Screen.Recording.2025-02-14.at.17.26.53.mov

Revealing SRPs

Screen.Recording.2025-02-14.at.17.26.32.mov

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.

PatrykLucka and others added 30 commits December 11, 2024 13:59
@danroc
Copy link
Contributor

danroc commented Feb 26, 2025

Is this part of the design? It's strange to say that a SRP can also be 15, 18, or 21 words but the only options are 12 or 24.

image

@montelaidev
Copy link
Contributor Author

Is this part of the design? It's strange to say that a SRP can also be 15, 18, or 21 words but the only options are 12 or 24.

Updated this on 4418a35

<Box marginBottom={3}>
<SelectSRP
onClick={onSelectSRP}
srpName={`Secret Phrase ${selectedKeyringIndex + 1}`}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a transalated message here

@@ -4331,7 +4331,11 @@ export default class MetamaskController extends EventEmitter {
this.accountsController.setSelectedAccount(account.id);

// TODO: Find a way to encapsulate this logic in the KeyringController itself.
const keyringId = findKeyringIdByAddress(newAccountAddress);
const { keyrings, keyringsMetadata } = this.keyringController.state;
const keyringId = findKeyringId(keyrings, keyringsMetadata, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isnt findKeyringId going to fail if not included on the code fence?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the keyring controller will have the KeyringMetadata.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but the import of findKeyringId is fenced

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The importMnemonicToVault using findKeyringId is also fenced.

Comment on lines +73 to +79

export function setShowNewSRPAddedToast(value: boolean) {
return {
type: SET_SHOW_NEW_SRP_ADDED_TOAST,
payload: value,
};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing code-fence?

@@ -51,3 +51,4 @@ export { ReceiveModal } from './receive-modal';
export { EditNetworksModal } from './edit-networks-modal';
export { EditAccountsModal } from './edit-accounts-modal';
export { Carousel } from './carousel';
export { ImportSRP, SelectSRP, SRPList } from './multi-srp';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably code fence here

Comment on lines +69 to +101
let newSrpError = '';
const joinedDraftSrp = newDraftSrp.join(' ').trim();
let invalidWords = Array(newDraftSrp.length).fill(false);

if (newDraftSrp.some((word) => word !== '')) {
invalidWords = newDraftSrp.map((word) => !wordlist.includes(word));

if (newDraftSrp.some((word) => word === '')) {
newSrpError = t('importSRPNumberOfWordsError');
} else if (hasUpperCase(joinedDraftSrp)) {
newSrpError = t('invalidSeedPhraseCaseSensitive');
} else if (invalidWords.some((word) => word === true)) {
const invalidWordIndex = invalidWords.reduce((acc, word, index) => {
if (word) {
// We add 1 to the index because the index is 0-based
// This is displayed to the user to show which word in the list is incorrect.
acc.push(index + 1);
}
return acc;
}, []);
if (invalidWordIndex.length === 1) {
newSrpError = t('importSRPWordError', [invalidWordIndex.pop()]);
} else if (invalidWordIndex.length >= 2) {
const firstPartOfError = invalidWordIndex.slice(0, -1).join(', ');
newSrpError = t('importSRPWordErrorAlternative', [
firstPartOfError,
invalidWordIndex.pop(),
]);
}
} else if (!isValidMnemonic(joinedDraftSrp)) {
newSrpError = t('invalidSeedPhrase');
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably move this somewhere and make it a bit more readable

@@ -47,3 +47,4 @@
@import 'notifications-tag-counter';
@import 'toast';
@import "ramps-card";
@import "multi-srp";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be code fenced?

@metamaskbot
Copy link
Collaborator

Builds ready [945c285]
Page Load Metrics (1794 ± 104 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint147524431794227109
domContentLoaded146423661766216104
load147723731794217104
domInteractive26115402512
backgroundConnect1174292010
firstReactRender1581322311
getState563212010
initialActions01000
loadScripts10841882132718991
setupStore762242110
uiStartup170826902056237114
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 18.16 KiB (0.24%)
  • common: 1.78 KiB (0.02%)

@@ -0,0 +1,28 @@
import React, { useState } from 'react';
import { Box } from '../../../../components/component-library';
import SRPQuizModal from '../../../../components/app/srp-quiz-modal/SRPQuiz';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filename looks wrong now. Looking at other modals, it probably should be:
components/app/srp-quiz-modal/srp-quiz-modal.tsx

@@ -77,6 +87,10 @@ const mapStateToProps = (state) => {
securityAlertsEnabled: getIsSecurityAlertsEnabled(state),
useTransactionSimulations: metamask.useTransactionSimulations,
metaMetricsDataDeletionId: getMetaMetricsDataDeletionId(state),
///: BEGIN:ONLY_INCLUDE_IF(multi-srp)
selectedAccount: getSelectedInternalAccount(state),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this? I don't see it being declared on ui/pages/settings/security-tab/security-tab.component.js props?

@@ -77,6 +87,10 @@ const mapStateToProps = (state) => {
securityAlertsEnabled: getIsSecurityAlertsEnabled(state),
useTransactionSimulations: metamask.useTransactionSimulations,
metaMetricsDataDeletionId: getMetaMetricsDataDeletionId(state),
///: BEGIN:ONLY_INCLUDE_IF(multi-srp)
selectedAccount: getSelectedInternalAccount(state),
hasMultipleHDKeyrings,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Minor, but other "HD" names, uses "Hd" (not a real issue, but just wanted to note that)

const accountBalance = getSelectedInternalAccount(state)
? getCurrentEthBalance(state)
: 0;
const accountBalance = selectedAccount ? getCurrentEthBalance(state) : 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to flag this here, but what if someone uses MM with only Solana accounts? We might wanna also look for any non-EVM balances now? (I know this was the initial logic, but we might need to keep note of that somewhere)

Comment on lines +15 to +26
if (params.address && params.type) {
return (
keyring.accounts.includes(params.address) &&
keyring.type === params.type
);
}
if (params.address) {
return keyring.accounts.includes(params.address);
}
if (params.type) {
return keyring.type === params.type;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably toLowerCase the address here?

keyring.type === KeyringTypes.hd &&
keyring.metadata.id === keyringId,
)
: keyrings[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could re-use this one:

Suggested change
: keyrings[0];
: defaultPrimaryKeyring;

Comment on lines +531 to +551
const keyrings = getMetaMaskHdKeyrings(getState());
const [defaultPrimaryKeyring] = keyrings;
let oldAccounts = defaultPrimaryKeyring.accounts;

///: BEGIN:ONLY_INCLUDE_IF(multi-srp)
const hdKeyring = keyringId
? keyrings.find(
(keyring) =>
keyring.type === KeyringTypes.hd &&
keyring.metadata.id === keyringId,
)
: keyrings[0];

if (!hdKeyring) {
console.log('Should never reach this. There is always a keyring');
throw new Error('Keyring not found');
}

oldAccounts = hdKeyring.accounts;
///: END:ONLY_INCLUDE_IF

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small suggestion to make this a bit more "direct":

Suggested change
const keyrings = getMetaMaskHdKeyrings(getState());
const [defaultPrimaryKeyring] = keyrings;
let oldAccounts = defaultPrimaryKeyring.accounts;
///: BEGIN:ONLY_INCLUDE_IF(multi-srp)
const hdKeyring = keyringId
? keyrings.find(
(keyring) =>
keyring.type === KeyringTypes.hd &&
keyring.metadata.id === keyringId,
)
: keyrings[0];
if (!hdKeyring) {
console.log('Should never reach this. There is always a keyring');
throw new Error('Keyring not found');
}
oldAccounts = hdKeyring.accounts;
///: END:ONLY_INCLUDE_IF
const keyrings = getMetaMaskHdKeyrings(getState());
const [defaultPrimaryKeyring] = keyrings;
// The HD keyring to add the account for.
let hdKeyring = defaultPrimaryKeyring;
///: BEGIN:ONLY_INCLUDE_IF(multi-srp)
if (keyringId) {
hdKeyring = keyrings.find(
(keyring) => keyring.metadata.id === keyringId,
);
}
///: END:ONLY_INCLUDE_IF
// Fail-safe in case we could not find the associated HD keyring.
if (!hdKeyring) {
console.error('Should never reach this. There is always a keyring');
throw new Error('Keyring not found');
}
const oldAccounts = hdKeyring.accounts;

NOTE: I've also used console.warn (is console.error better here) rather than a normal log.

Comment on lines +167 to +171
"@metamask/keyring-controller": {
"packages": {
"ulid": true
}
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we really needs this override? Since we already have the next one that is strictly related to "@metamask/keyring-controller>ulid"?

Has this been added manually?

I've tried to remove it, and it also removes the policy changes (which also seems unnecessary to me, but I could be wrong here..)

import { SECOND_TEST_E2E_SRP, withMultiSRP } from './common-multi-srp';

describe('Multi SRP - Reveal Imported SRP', function (this: Suite) {
const testPassword = 'correct horse battery staple';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just import WALLET_PASSWORD from test/e2e/helpers.js instead.

import { SECOND_TEST_E2E_SRP, withMultiSRP } from './common-multi-srp';

describe('Multi SRP - Import SRP', function (this: Suite) {
const testPassword = 'correct horse battery staple';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just import WALLET_PASSWORD from test/e2e/helpers.js instead.

Comment on lines +9 to +49
describe('Multi SRP - Reveal Imported SRP', function (this: Suite) {
const testPassword = 'correct horse battery staple';
const firstSRPIndex = 1;
const secondSRPIndex = 2;

it('successfully exports the default SRP', async function () {
await withMultiSRP(
{ title: this.test?.fullTitle() },
async (driver: Driver) => {
await new HeaderNavbar(driver).openSettingsPage();
const settingsPage = new SettingsPage(driver);
await settingsPage.check_pageIsLoaded();
await settingsPage.goToPrivacySettings();

const privacySettings = new PrivacySettings(driver);
await privacySettings.openRevealSrpQuiz(firstSRPIndex);
await privacySettings.completeRevealSrpQuiz();
await privacySettings.fillPasswordToRevealSrp(testPassword);
await privacySettings.check_srpTextIsDisplayed(FIRST_TEST_E2E_SRP);
},
);
});

it('successfully exports the imported SRP', async function () {
await withMultiSRP(
{ title: this.test?.fullTitle() },
async (driver: Driver) => {
await new HeaderNavbar(driver).openSettingsPage();
const settingsPage = new SettingsPage(driver);
await settingsPage.check_pageIsLoaded();
await settingsPage.goToPrivacySettings();

const privacySettings = new PrivacySettings(driver);
await privacySettings.openRevealSrpQuiz(secondSRPIndex);
await privacySettings.completeRevealSrpQuiz();
await privacySettings.fillPasswordToRevealSrp(testPassword);
await privacySettings.check_srpTextIsDisplayed(SECOND_TEST_E2E_SRP);
},
);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
describe('Multi SRP - Reveal Imported SRP', function (this: Suite) {
const testPassword = 'correct horse battery staple';
const firstSRPIndex = 1;
const secondSRPIndex = 2;
it('successfully exports the default SRP', async function () {
await withMultiSRP(
{ title: this.test?.fullTitle() },
async (driver: Driver) => {
await new HeaderNavbar(driver).openSettingsPage();
const settingsPage = new SettingsPage(driver);
await settingsPage.check_pageIsLoaded();
await settingsPage.goToPrivacySettings();
const privacySettings = new PrivacySettings(driver);
await privacySettings.openRevealSrpQuiz(firstSRPIndex);
await privacySettings.completeRevealSrpQuiz();
await privacySettings.fillPasswordToRevealSrp(testPassword);
await privacySettings.check_srpTextIsDisplayed(FIRST_TEST_E2E_SRP);
},
);
});
it('successfully exports the imported SRP', async function () {
await withMultiSRP(
{ title: this.test?.fullTitle() },
async (driver: Driver) => {
await new HeaderNavbar(driver).openSettingsPage();
const settingsPage = new SettingsPage(driver);
await settingsPage.check_pageIsLoaded();
await settingsPage.goToPrivacySettings();
const privacySettings = new PrivacySettings(driver);
await privacySettings.openRevealSrpQuiz(secondSRPIndex);
await privacySettings.completeRevealSrpQuiz();
await privacySettings.fillPasswordToRevealSrp(testPassword);
await privacySettings.check_srpTextIsDisplayed(SECOND_TEST_E2E_SRP);
},
);
});
});
const exportSrp = async (driver: Driver, srpIndex: number, srp: string) => {
await new HeaderNavbar(driver).openSettingsPage();
const settingsPage = new SettingsPage(driver);
await settingsPage.check_pageIsLoaded();
await settingsPage.goToPrivacySettings();
const privacySettings = new PrivacySettings(driver);
await privacySettings.openRevealSrpQuiz(srpIndex);
await privacySettings.completeRevealSrpQuiz();
await privacySettings.fillPasswordToRevealSrp(testPassword);
await privacySettings.check_srpTextIsDisplayed(srp);
};
describe('Multi SRP - Reveal Imported SRP', function (this: Suite) {
const testPassword = 'correct horse battery staple';
it('successfully exports the default SRP', async function () {
await withMultiSRP(
{ title: this.test?.fullTitle() },
async (driver: Driver) => {
await new exportSrp(driver, 1, FIRST_TEST_E2E_SRP);
},
);
});
it('successfully exports the imported SRP', async function () {
await withMultiSRP(
{ title: this.test?.fullTitle() },
async (driver: Driver) => {
await new exportSrp(driver, 2, SECOND_TEST_E2E_SRP);
},
);
});
});

});
});

it('should log an error and not call onActionComplete on import failure', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it('should log an error and not call onActionComplete on import failure', async () => {
it('logs an error and not call onActionComplete on import failure', async () => {

expect(getByText('Import wallet')).not.toBeEnabled();
});

it('should call addNewMnemonicToVault and showAlert on successful import', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it('should call addNewMnemonicToVault and showAlert on successful import', async () => {
it('calls addNewMnemonicToVault and showAlert on successful import', async () => {

});
});

it('should not enable the "Import wallet" button when the secret recovery phrase is empty', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it('should not enable the "Import wallet" button when the secret recovery phrase is empty', async () => {
it('does not enable the "Import wallet" button when the secret recovery phrase is empty', async () => {

jest.restoreAllMocks();
});

it('should enable the "Import wallet" button when a valid secret recovery phrase is entered', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it('should enable the "Import wallet" button when a valid secret recovery phrase is entered', async () => {
it('enables the "Import wallet" button when a valid secret recovery phrase is entered', async () => {

@montelaidev montelaidev mentioned this pull request Feb 27, 2025
7 tasks
@montelaidev montelaidev added the DO-NOT-MERGE Pull requests that should not be merged label Mar 5, 2025
github-merge-queue bot pushed a commit that referenced this pull request Mar 5, 2025
<!--
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**

This PR introduces importing srps as a new feature. It is split from a
previous [pr](#29816)



<!--
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/30598?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

**Importing a new SRP**
1. Click on the account selector
2. Click on connect more
3. Click on `Import secret recovery phrase`
4. Paste in a SRP
5. Continue
6. See a new account is created
7. Click on the account selector and see new pills indicating the SRP
numbers

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

### **After**

Importing a new SRP


https://github.com/user-attachments/assets/ac349e02-1dd4-4e87-86a7-3e21c7b2cd7b

## **Pre-merge author checklist**

- [x] 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).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.

---------

Co-authored-by: Charly Chevalier <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Mar 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
DO-NOT-MERGE Pull requests that should not be merged team-accounts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants