From 5528c63ec59d87db9c4f5ea4c2f118848681a7ab Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Tue, 14 Jan 2025 18:12:26 +0800 Subject: [PATCH 1/3] feat: add reveal srp to hd accounts. --- app/_locales/en/messages.json | 3 + .../app/srp-quiz-modal/SRPQuiz/SRPQuiz.tsx | 8 +- .../account-details-display.js | 38 +++++++- .../account-details/account-details.test.js | 43 +++++++-- ...account-details.js => account-details.tsx} | 63 +++++++++---- ui/helpers/utils/util.js | 22 +++++ ui/helpers/utils/util.test.js | 93 +++++++++++++++++++ .../security-tab/reveal-srp-list/index.ts | 2 +- ui/store/actions.ts | 2 +- yarn.lock | 38 ++------ 10 files changed, 249 insertions(+), 63 deletions(-) rename ui/components/multichain/account-details/{account-details.js => account-details.tsx} (76%) diff --git a/app/_locales/en/messages.json b/app/_locales/en/messages.json index ab755e35f01b..39e3d2c188ee 100644 --- a/app/_locales/en/messages.json +++ b/app/_locales/en/messages.json @@ -5123,6 +5123,9 @@ "showPrivateKey": { "message": "Show private key" }, + "showSRP": { + "message": "Show secret recovery phrase" + }, "showTestnetNetworks": { "message": "Show test networks" }, diff --git a/ui/components/app/srp-quiz-modal/SRPQuiz/SRPQuiz.tsx b/ui/components/app/srp-quiz-modal/SRPQuiz/SRPQuiz.tsx index aa57f4b6b150..65517638014e 100644 --- a/ui/components/app/srp-quiz-modal/SRPQuiz/SRPQuiz.tsx +++ b/ui/components/app/srp-quiz-modal/SRPQuiz/SRPQuiz.tsx @@ -63,6 +63,7 @@ export type SRPQuizProps = { accountId: string; // The account id will be used to determine which HD keyring to use. isOpen: boolean; onClose: () => void; + closeAfterCompleting?: boolean; }; export default function SRPQuiz(props: SRPQuizProps): JSX.Element { @@ -227,7 +228,12 @@ export default function SRPQuiz(props: SRPQuizProps): JSX.Element { buttons={[ { label: t('continue'), - onClick: () => history.push(`${REVEAL_SEED_ROUTE}/${typeIndex}`), + onClick: () => { + history.push(`${REVEAL_SEED_ROUTE}/${typeIndex}`); + if (props.closeAfterCompleting) { + props.onClose(); + } + }, variant: ButtonVariant.Primary, size: ButtonSize.Lg, 'data-testid': 'srp-quiz-continue', diff --git a/ui/components/multichain/account-details/account-details-display.js b/ui/components/multichain/account-details/account-details-display.js index d5af76004887..2a8ea69805df 100644 --- a/ui/components/multichain/account-details/account-details-display.js +++ b/ui/components/multichain/account-details/account-details-display.js @@ -9,8 +9,12 @@ import { setAccountLabel } from '../../../store/actions'; import { getHardwareWalletType, getInternalAccountByAddress, + getMetaMaskKeyrings, } from '../../../selectors'; -import { isAbleToExportAccount } from '../../../helpers/utils/util'; +import { + isAbleToExportAccount, + isAbleToRevealSrp, +} from '../../../helpers/utils/util'; import { Box, ButtonSecondary, @@ -40,11 +44,16 @@ export const AccountDetailsDisplay = ({ const dispatch = useDispatch(); const trackEvent = useContext(MetaMetricsContext); const t = useI18nContext(); + const keyrings = useSelector(getMetaMaskKeyrings); + const account = useSelector((state) => + getInternalAccountByAddress(state, address), + ); const { metadata: { keyring }, - } = useSelector((state) => getInternalAccountByAddress(state, address)); + } = account; const exportPrivateKeyFeatureEnabled = isAbleToExportAccount(keyring?.type); + const exportSRPFeatureaEnabled = isAbleToRevealSrp(account, keyrings); const chainId = useSelector(getCurrentChainId); const deviceName = useSelector(getHardwareWalletType); @@ -74,9 +83,11 @@ export const AccountDetailsDisplay = ({ {exportPrivateKeyFeatureEnabled ? ( { trackEvent({ category: MetaMetricsEventCategory.Accounts, @@ -86,12 +97,33 @@ export const AccountDetailsDisplay = ({ location: 'Account Details Modal', }, }); - onExportClick(); + onExportClick('PrivateKey'); }} > {t('showPrivateKey')} ) : null} + {exportSRPFeatureaEnabled ? ( + { + trackEvent({ + category: MetaMetricsEventCategory.Accounts, + event: MetaMetricsEventName.KeyExportSelected, + properties: { + key_type: MetaMetricsEventKeyType.Pkey, + location: 'Account Details Modal', + }, + }); + onExportClick('SRP'); + }} + > + {t('showSRP')} + + ) : null} ); }; diff --git a/ui/components/multichain/account-details/account-details.test.js b/ui/components/multichain/account-details/account-details.test.js index ac87f0027dac..dcde1dde616d 100644 --- a/ui/components/multichain/account-details/account-details.test.js +++ b/ui/components/multichain/account-details/account-details.test.js @@ -1,11 +1,14 @@ import { LavaDomeDebug } from '@lavamoat/lavadome-core'; -import { fireEvent, screen } from '@testing-library/react'; +import { fireEvent, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import React from 'react'; -// TODO: Remove restricted import -// eslint-disable-next-line import/no-restricted-paths -import { showPrivateKey } from '../../../../app/_locales/en/messages.json'; +import { + showSRP, + // TODO: Remove restricted import + // eslint-disable-next-line import/no-restricted-paths +} from '../../../../app/_locales/en/messages.json'; import mockState from '../../../../test/data/mock-state.json'; + import { renderWithProvider } from '../../../../test/jest'; import { shortenAddress } from '../../../helpers/utils/util'; import { @@ -66,8 +69,10 @@ describe('AccountDetails', () => { }); it('shows export private key contents and password field when clicked', () => { - const { queryByText, queryByPlaceholderText } = render(); - const exportPrivateKeyButton = queryByText(showPrivateKey.message); + const { queryByText, queryByPlaceholderText, getByTestId } = render(); + const exportPrivateKeyButton = getByTestId( + 'account-details-display-export-private-key', + ); fireEvent.click(exportPrivateKeyButton); expect( @@ -81,8 +86,10 @@ describe('AccountDetails', () => { it('attempts to validate password when submitted', async () => { const password = 'password'; - const { queryByPlaceholderText, queryByText } = render(); - const exportPrivateKeyButton = queryByText(showPrivateKey.message); + const { queryByPlaceholderText, queryByText, getByTestId } = render(); + const exportPrivateKeyButton = getByTestId( + 'account-details-display-export-private-key', + ); fireEvent.click(exportPrivateKeyButton); queryByPlaceholderText('Password').focus(); @@ -132,4 +139,24 @@ describe('AccountDetails', () => { expect(accountName).toBeInTheDocument(); }); + + it("shows the `Show Secret Recovery Phrase` button when the account's type is a HD Keyring", () => { + render(); + + const showSRPButton = screen.getByText(showSRP.message); + + expect(showSRPButton).toBeInTheDocument(); + }); + + it('shows srp flow when the `Show Secret Recovery Phrase` button is clicked', async () => { + render(); + + const showSRPButton = screen.getByText(showSRP.message); + fireEvent.click(showSRPButton); + + const securityQuizTitle = screen.getByTestId('srp-quiz-header'); + await waitFor(() => { + expect(securityQuizTitle).toBeInTheDocument(); + }); + }); }); diff --git a/ui/components/multichain/account-details/account-details.js b/ui/components/multichain/account-details/account-details.tsx similarity index 76% rename from ui/components/multichain/account-details/account-details.js rename to ui/components/multichain/account-details/account-details.tsx index c04070e523e2..4dd1f08f9a2c 100644 --- a/ui/components/multichain/account-details/account-details.js +++ b/ui/components/multichain/account-details/account-details.tsx @@ -38,21 +38,34 @@ import { ModalBody, } from '../../component-library'; import { AddressCopyButton } from '../address-copy-button'; +import SRPQuiz from '../../app/srp-quiz-modal'; import { AccountDetailsAuthenticate } from './account-details-authenticate'; import { AccountDetailsDisplay } from './account-details-display'; import { AccountDetailsKey } from './account-details-key'; -export const AccountDetails = ({ address }) => { +export enum AttemptExportState { + None = 'None', + PrivateKey = 'PrivateKey', + SRP = 'SRP', +} + +type AccountDetailsProps = { address: string }; + +export const AccountDetails = ({ address }: AccountDetailsProps) => { const dispatch = useDispatch(); const t = useI18nContext(); const trackEvent = useContext(MetaMetricsContext); const useBlockie = useSelector(getUseBlockie); const accounts = useSelector(getMetaMaskAccountsOrdered); const { + id: accountId, metadata: { name }, } = useSelector((state) => getInternalAccountByAddress(state, address)); const [showHoldToReveal, setShowHoldToReveal] = useState(false); - const [attemptingExport, setAttemptingExport] = useState(false); + const [attemptingExport, setAttemptingExport] = useState( + AttemptExportState.None, + ); + const [srpQuizModalVisible, setSrpQuizModalVisible] = useState(false); // This is only populated when the user properly authenticates const [privateKey, setPrivateKey] = useState(''); @@ -80,7 +93,7 @@ export const AccountDetails = ({ address }) => { <> {/* This is the Modal that says "Show private key" on top and has a few states */} @@ -89,18 +102,32 @@ export const AccountDetails = ({ address }) => { { - dispatch(hideWarning()); - setPrivateKey(''); - setAttemptingExport(false); - }) + attemptingExport + ? () => { + dispatch(hideWarning()); + setPrivateKey(''); + setAttemptingExport(AttemptExportState.None); + } + : undefined } > {attemptingExport ? t('showPrivateKey') : avatar} - {attemptingExport ? ( + {attemptingExport === AttemptExportState.None && ( + { + if (attemptExportMode === AttemptExportState.SRP) { + setSrpQuizModalVisible(true); + } + setAttemptingExport(attemptExportMode); + }} + /> + )} + {attemptingExport === AttemptExportState.PrivateKey && ( <> { /> )} - ) : ( - setAttemptingExport(true)} - /> )} @@ -164,6 +184,15 @@ export const AccountDetails = ({ address }) => { }} holdToRevealType="PrivateKey" /> + { + setSrpQuizModalVisible(false); + onClose(); + }} + closeAfterCompleting + /> ); }; diff --git a/ui/helpers/utils/util.js b/ui/helpers/utils/util.js index d687d9b82338..5f05a8696e4e 100644 --- a/ui/helpers/utils/util.js +++ b/ui/helpers/utils/util.js @@ -13,6 +13,7 @@ import bowser from 'bowser'; import { WALLET_SNAP_PERMISSION_KEY } from '@metamask/snaps-rpc-methods'; import { stripSnapPrefix } from '@metamask/snaps-utils'; import { isObject, isStrictHexString } from '@metamask/utils'; +import { KeyringTypes } from '@metamask/keyring-controller'; import { CHAIN_IDS, NETWORK_TYPES } from '../../../shared/constants/network'; import { logErrorWithMessage } from '../../../shared/modules/error'; import { @@ -724,6 +725,27 @@ export const isAbleToExportAccount = (keyringType = '') => { return !keyringType.includes('Hardware') && !keyringType.includes('Snap'); }; +export const isAbleToRevealSrp = (accountToExport, keyrings) => { + if ( + accountToExport.type !== KeyringTypes.hd && + accountToExport.type !== KeyringTypes.snap + ) { + return false; + } + + // All hd keyrings can reveal their srp. + if (accountToExport.type === KeyringTypes.hd) { + return true; + } + + // For snap accounts we must check if the entropy source was from a HD keyring. + const keyringId = accountToExport.metadata.keyring.id; + const hdKeyringsIds = keyrings + .filter((keyring) => keyring.type === KeyringTypes.hd) + .map((keyring) => keyring.id); + return hdKeyringsIds.includes(keyringId); +}; + /** * Checks if a tokenId in Hex or decimal format already exists in an object. * diff --git a/ui/helpers/utils/util.test.js b/ui/helpers/utils/util.test.js index bdf5c9dd9b98..5f449368d245 100644 --- a/ui/helpers/utils/util.test.js +++ b/ui/helpers/utils/util.test.js @@ -1309,4 +1309,97 @@ describe('util', () => { expect(sortedAccount).toStrictEqual([]); }); }); + + describe('isAbleToRevealSrp', () => { + const mockHDKeyring = { + id: 'hd-keyring-id', + type: 'HD Key Tree', + }; + + const mockSnapKeyring = { + id: 'snap-keyring-id', + type: 'Snap Keyring', + }; + + const mockLedgerKeyring = { + id: 'ledger-keyring-id', + type: 'Ledger Hardware', + }; + + it('should return true for HD Key Tree accounts', () => { + const hdAccount = { + type: 'HD Key Tree', + address: '0x123', + metadata: { + keyring: { + id: 'hd-keyring-id', + }, + }, + }; + + expect(util.isAbleToRevealSrp(hdAccount, [mockHDKeyring])).toBe(true); + }); + + it('should return true for Snap accounts derived from HD keyring', () => { + const snapAccount = { + type: 'Snap', + address: '0x123', + metadata: { + keyring: { + id: 'hd-keyring-id', + }, + }, + }; + + expect( + util.isAbleToRevealSrp(snapAccount, [mockHDKeyring, mockSnapKeyring]), + ).toBe(true); + }); + + it('should return false for Snap accounts not derived from HD keyring', () => { + const snapAccount = { + type: 'Snap', + address: '0x123', + metadata: { + keyring: { + id: 'some-other-id', + }, + }, + }; + + expect( + util.isAbleToRevealSrp(snapAccount, [mockHDKeyring, mockSnapKeyring]), + ).toBe(false); + }); + + it('should return false for hardware wallet accounts', () => { + const ledgerAccount = { + type: 'Ledger Hardware', + address: '0x123', + metadata: { + keyring: { + id: 'ledger-keyring-id', + }, + }, + }; + + expect(util.isAbleToRevealSrp(ledgerAccount, [mockLedgerKeyring])).toBe( + false, + ); + }); + + it('should return false for any other account type', () => { + const otherAccount = { + type: 'Simple Key Pair', + address: '0x123', + metadata: { + keyring: { + id: 'simple-keyring-id', + }, + }, + }; + + expect(util.isAbleToRevealSrp(otherAccount, [mockHDKeyring])).toBe(false); + }); + }); }); diff --git a/ui/pages/settings/security-tab/reveal-srp-list/index.ts b/ui/pages/settings/security-tab/reveal-srp-list/index.ts index 0344efee03d4..695ae793c401 100644 --- a/ui/pages/settings/security-tab/reveal-srp-list/index.ts +++ b/ui/pages/settings/security-tab/reveal-srp-list/index.ts @@ -1 +1 @@ -export { RevealSRPList } from './reveal-srp-list'; \ No newline at end of file +export { RevealSRPList } from './reveal-srp-list'; diff --git a/ui/store/actions.ts b/ui/store/actions.ts index 1ea4fdfc95e3..6b71b1e056c7 100644 --- a/ui/store/actions.ts +++ b/ui/store/actions.ts @@ -3380,7 +3380,7 @@ export function toggleNetworkMenu(payload?: { }; } -export function setAccountDetailsAddress(address: string[]) { +export function setAccountDetailsAddress(address: string) { return { type: actionConstants.SET_ACCOUNT_DETAILS_ADDRESS, payload: address, diff --git a/yarn.lock b/yarn.lock index 5e7c04283d20..85f834e9bda4 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5634,9 +5634,9 @@ __metadata: languageName: node linkType: hard -"@metamask/keyring-controller@file:.yalc/@metamask/keyring-controller::locator=metamask-crx%40workspace%3A.": - version: 19.0.2 - resolution: "@metamask/keyring-controller@file:.yalc/@metamask/keyring-controller#.yalc/@metamask/keyring-controller::hash=5a1aca&locator=metamask-crx%40workspace%3A." +"@metamask/keyring-controller@npm:@metamask-previews/keyring-controller@19.0.2-preview-e96ca40c": + version: 19.0.2-preview-e96ca40c + resolution: "@metamask-previews/keyring-controller@npm:19.0.2-preview-e96ca40c" dependencies: "@ethereumjs/util": "npm:^8.1.0" "@keystonehq/metamask-airgapped-keyring": "npm:^0.14.1" @@ -5652,8 +5652,7 @@ __metadata: async-mutex: "npm:^0.5.0" ethereumjs-wallet: "npm:^1.0.1" immer: "npm:^9.0.6" - ulid: "npm:^2.3.0" - checksum: 10/05df32c92dbbf3d2db103985d3ea08a0be69ce78c51c0e6471fa228da923f5d3c08b1f664cc32bc4ddb725d2aa7917d90768744a60c007e43abb6573b486fe0b + checksum: 10/6a638de01cb8a93ba0cfd82b0bd37e745bec074421a55e304d9f16ac3853a1a820f14145e3655f343f5120531f9e61fe900873b9882ac385833f1a82af4df714 languageName: node linkType: hard @@ -6277,22 +6276,6 @@ __metadata: languageName: node linkType: hard -"@metamask/snaps-rpc-methods@file:.yalc/@metamask/snaps-rpc-methods::locator=metamask-crx%40workspace%3A.": - version: 11.7.0 - resolution: "@metamask/snaps-rpc-methods@file:.yalc/@metamask/snaps-rpc-methods#.yalc/@metamask/snaps-rpc-methods::hash=74bdbd&locator=metamask-crx%40workspace%3A." - dependencies: - "@metamask/key-tree": "npm:^10.0.1" - "@metamask/permission-controller": "npm:^11.0.3" - "@metamask/rpc-errors": "npm:^7.0.1" - "@metamask/snaps-sdk": "npm:^6.13.0" - "@metamask/snaps-utils": "npm:^8.6.1" - "@metamask/superstruct": "npm:^3.1.0" - "@metamask/utils": "npm:^10.0.0" - "@noble/hashes": "npm:^1.3.1" - checksum: 10/0e8d6c9865c55e61ad9d6b8704e12caa11640710a18cb68d97a39637c385b0bf8ef9e7995cc0236c4770f9429b0a022f9cdb006fb24dec40757763ba8ab861d3 - languageName: node - linkType: hard - "@metamask/snaps-rpc-methods@npm:^11.7.0": version: 11.7.0 resolution: "@metamask/snaps-rpc-methods@npm:11.7.0" @@ -26506,7 +26489,7 @@ __metadata: "@metamask/json-rpc-engine": "npm:^10.0.0" "@metamask/json-rpc-middleware-stream": "npm:^8.0.4" "@metamask/keyring-api": "npm:^10.1.0" - "@metamask/keyring-controller": "file:.yalc/@metamask/keyring-controller" + "@metamask/keyring-controller": "npm:@metamask-previews/keyring-controller@19.0.2-preview-e96ca40c" "@metamask/logging-controller": "npm:^6.0.0" "@metamask/logo": "npm:^3.1.2" "@metamask/message-manager": "npm:^11.0.0" @@ -26539,7 +26522,7 @@ __metadata: "@metamask/smart-transactions-controller": "npm:^15.0.0" "@metamask/snaps-controllers": "npm:^9.15.0" "@metamask/snaps-execution-environments": "npm:^6.10.0" - "@metamask/snaps-rpc-methods": "file:.yalc/@metamask/snaps-rpc-methods" + "@metamask/snaps-rpc-methods": "npm:^11.7.0" "@metamask/snaps-sdk": "npm:^6.13.0" "@metamask/snaps-utils": "npm:^8.6.1" "@metamask/solana-wallet-snap": "npm:^0.1.9" @@ -35960,15 +35943,6 @@ __metadata: languageName: node linkType: hard -"ulid@npm:^2.3.0": - version: 2.3.0 - resolution: "ulid@npm:2.3.0" - bin: - ulid: ./bin/cli.js - checksum: 10/11d7dd35072b863effb1249f66fb03070142a625610f00e5afd99af7e909b5de9cc7ebca6ede621a6bb1b7479b2489d6f064db6742b55c14bff6496ac60f290f - languageName: node - linkType: hard - "umd@npm:3.0.3, umd@npm:^3.0.0": version: 3.0.3 resolution: "umd@npm:3.0.3" From a2d038f0093f3154af21552316ba985536b1d2c8 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Thu, 16 Jan 2025 11:38:36 +0800 Subject: [PATCH 2/3] fix: multisrp fence --- app/scripts/metamask-controller.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index a011fa3d5b3e..b1daa5a8eb78 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -22,7 +22,9 @@ import { providerAsMiddleware } from '@metamask/eth-json-rpc-middleware'; import { debounce, throttle, memoize, wrap } from 'lodash'; import { KeyringController, + ///: BEGIN:ONLY_INCLUDE_IF(multi-srp) KeyringTypes, + ///: END:ONLY_INCLUDE_IF keyringBuilderFactory, } from '@metamask/keyring-controller'; import createFilterMiddleware from '@metamask/eth-json-rpc-filters'; @@ -5152,7 +5154,12 @@ export default class MetamaskController extends EventEmitter { * @param {string} keyringIndex * @returns {Promise} The address of the newly-created account. */ - async addNewAccount(accountCount, keyringIndex) { + async addNewAccount( + accountCount, + ///: BEGIN:ONLY_INCLUDE_IF(multi-srp) + keyringIndex, + ///: END:ONLY_INCLUDE_IF + ) { const oldAccounts = await this.keyringController.getAccounts(); const addedAccountAddress = await this.keyringController.addNewAccount( From 94413862c8d38ce598abe52e3306744f7a13834d Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Thu, 16 Jan 2025 11:39:14 +0800 Subject: [PATCH 3/3] fix: dedupe --- yarn.lock | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/yarn.lock b/yarn.lock index 85f834e9bda4..3eb0858c33eb 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5720,22 +5720,7 @@ __metadata: languageName: node linkType: hard -"@metamask/message-manager@npm:^11.0.0, @metamask/message-manager@npm:^11.0.1": - version: 11.0.1 - resolution: "@metamask/message-manager@npm:11.0.1" - dependencies: - "@metamask/base-controller": "npm:^7.0.2" - "@metamask/controller-utils": "npm:^11.4.2" - "@metamask/eth-sig-util": "npm:^8.0.0" - "@metamask/utils": "npm:^10.0.0" - "@types/uuid": "npm:^8.3.0" - jsonschema: "npm:^1.2.4" - uuid: "npm:^8.3.2" - checksum: 10/a63a79bbddfd06ca729ca1ca2a1f2fd9b5c930efaaee1d8bc077f7352ae058ff7f99c9b6bdfd7f7d3df4328a3c3606922d961d45c8d39c2b7afb4a893ec1cf6b - languageName: node - linkType: hard - -"@metamask/message-manager@npm:^11.0.3": +"@metamask/message-manager@npm:^11.0.0, @metamask/message-manager@npm:^11.0.1, @metamask/message-manager@npm:^11.0.3": version: 11.0.3 resolution: "@metamask/message-manager@npm:11.0.3" dependencies: @@ -24703,7 +24688,7 @@ __metadata: languageName: node linkType: hard -"jsonschema@npm:^1.2.4, jsonschema@npm:^1.4.1": +"jsonschema@npm:^1.4.1": version: 1.4.1 resolution: "jsonschema@npm:1.4.1" checksum: 10/d7a188da7a3100a2caa362b80e98666d46607b7a7153aac405b8e758132961911c6df02d444d4700691330874e21a62639f550e856b21ddd28423690751ca9c6