From 32c2077d3a4e52d6470e20951bd333d47a298ce0 Mon Sep 17 00:00:00 2001 From: Daniel Rocha <68558152+danroc@users.noreply.github.com> Date: Tue, 25 Feb 2025 11:11:50 +0100 Subject: [PATCH] feat(snap-keyring): handle `displayAccountNameSuggestion` flag (#30531) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR handles the `displayAccountNameSuggestion` flag that can be set by pre-installed Snaps in the `accountCreated` event. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30531?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/accounts-planning/issues/829 ## **Manual testing steps** 1. Update a pre-installed Snap to set `displayAccountNameSuggestion` to false: ```diff diff --git a/packages/snap/src/core/handlers/onKeyringRequest/Keyring.ts b/packages/snap/src/core/handlers/onKeyringRequest/Keyring.ts index 27238e7..6edabc7 100644 --- a/packages/snap/src/core/handlers/onKeyringRequest/Keyring.ts +++ b/packages/snap/src/core/handlers/onKeyringRequest/Keyring.ts @@ -224,7 +224,8 @@ export class SolanaKeyring implements Keyring { * and the snaps sdk does not allow extra properties. */ account: keyringAccount, - accountNameSuggestion: `Solana Account ${index + 1}`, + accountNameSuggestion: `Solana Account`, + displayAccountNameSuggestion: false, }); return keyringAccount; ``` 2. Build the Snap and add a resolution in the extension's `package.json`: ``` "@metamask/solana-wallet-snap@^1.2.0": "file:../snap-solana-wallet/packages/snap" ``` 3. Start the extension: `yarn && yarn start:flask` 4. Create an account 5. The account name dialog should not be shown ## **Screenshots/Recordings** ### **Before** ### **After** ## **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. --- app/scripts/lib/snap-keyring/snap-keyring.ts | 86 ++++++++++++++----- app/scripts/lib/snap-keyring/types.ts | 2 + app/scripts/metamask-controller.js | 1 + package.json | 2 +- shared/lib/accounts/accounts.ts | 27 ++++++ shared/lib/accounts/index.ts | 4 + .../create-named-snap-account.tsx | 23 +---- yarn.lock | 4 +- 8 files changed, 105 insertions(+), 44 deletions(-) create mode 100644 shared/lib/accounts/accounts.ts create mode 100644 shared/lib/accounts/index.ts diff --git a/app/scripts/lib/snap-keyring/snap-keyring.ts b/app/scripts/lib/snap-keyring/snap-keyring.ts index 43491bc54b1b..4325c597d13b 100644 --- a/app/scripts/lib/snap-keyring/snap-keyring.ts +++ b/app/scripts/lib/snap-keyring/snap-keyring.ts @@ -14,6 +14,7 @@ import MetamaskController from '../../metamask-controller'; // eslint-disable-next-line import/no-restricted-paths import { IconName } from '../../../../ui/components/component-library/icon'; import MetaMetricsController from '../../controllers/metametrics-controller'; +import { getUniqueAccountName } from '../../../../shared/lib/accounts'; import { isBlockedUrl } from './utils/isBlockedUrl'; import { showError, showSuccess } from './utils/showResult'; import { SnapKeyringBuilderMessenger } from './types'; @@ -201,23 +202,62 @@ class SnapKeyringImpl implements SnapKeyringCallbacks { } } + /** + * Get the account name from the user through a dialog. + * + * @param snapId - ID of the Snap that created the account. + * @param accountNameSuggestion - Suggested name for the account. + * @returns The name that should be used for the account. + */ + async #getAccountNameFromDialog( + snapId: SnapId, + accountNameSuggestion: string, + ): Promise<{ success: boolean; accountName?: string }> { + const { success, name: accountName } = + await showAccountNameSuggestionDialog( + snapId, + this.#messenger, + accountNameSuggestion, + ); + + return { success, accountName }; + } + + /** + * Use the account name suggestion to decide the name of the account. + * + * @param accountNameSuggestion - Suggested name for the account. + * @returns The name that should be used for the account. + */ + async #getAccountNameFromSuggestion( + accountNameSuggestion: string, + ): Promise<{ success: boolean; accountName?: string }> { + const accounts = await this.#messenger.call( + 'AccountsController:listMultichainAccounts', + ); + const accountName = getUniqueAccountName(accounts, accountNameSuggestion); + return { success: true, accountName }; + } + async #addAccountConfirmations({ snapId, - skipConfirmation, + skipConfirmationDialog, + skipAccountNameSuggestionDialog, handleUserInput, accountNameSuggestion, }: { snapId: SnapId; - skipConfirmation: boolean; + skipConfirmationDialog: boolean; + skipAccountNameSuggestionDialog: boolean; accountNameSuggestion: string; handleUserInput: (accepted: boolean) => Promise; }): Promise<{ accountName?: string }> { return await this.#withApprovalFlow(async (_) => { - // 1. Show the account **creation** confirmation dialog. + // 1. Show the account CREATION confirmation dialog. { // If confirmation dialog are skipped, we consider the account creation to be confirmed until the account name dialog is closed const success = - skipConfirmation || + skipConfirmationDialog || (await showAccountCreationDialog(snapId, this.#messenger)); if (!success) { @@ -228,24 +268,19 @@ class SnapKeyringImpl implements SnapKeyringCallbacks { } } - // 2. Show the account **renaming** confirmation dialog. + // 2. Show the account RENAMING confirmation dialog. Note that + // pre-installed Snaps can skip this dialog. { - const { success, name: accountName } = - await showAccountNameSuggestionDialog( - snapId, - this.#messenger, - accountNameSuggestion, - ); + const { success, accountName } = skipAccountNameSuggestionDialog + ? await this.#getAccountNameFromSuggestion(accountNameSuggestion) + : await this.#getAccountNameFromDialog(snapId, accountNameSuggestion); - if (!success) { - // User has cancelled account creation - await handleUserInput(success); + await handleUserInput(success); + if (!success) { throw new Error('User denied account creation'); } - await handleUserInput(success); - return { accountName }; } }); @@ -254,13 +289,13 @@ class SnapKeyringImpl implements SnapKeyringCallbacks { async #addAccountFinalize({ address, snapId, - skipConfirmation, + skipConfirmationDialog, accountName, onceSaved, }: { address: string; snapId: SnapId; - skipConfirmation: boolean; + skipConfirmationDialog: boolean; onceSaved: Promise; accountName?: string; }) { @@ -305,7 +340,7 @@ class SnapKeyringImpl implements SnapKeyringCallbacks { ); } - if (!skipConfirmation) { + if (!skipConfirmationDialog) { // TODO: Add events tracking to the dialog itself, so that events are more // "linked" to UI actions // User should now see the "Successfuly added account" page @@ -366,17 +401,24 @@ class SnapKeyringImpl implements SnapKeyringCallbacks { onceSaved: Promise, accountNameSuggestion: string = '', displayConfirmation: boolean = false, + displayAccountNameSuggestion: boolean = true, ) { assertIsValidSnapId(snapId); // If Snap is preinstalled and does not request confirmation, skip the confirmation dialog. - const skipConfirmation = isSnapPreinstalled(snapId) && !displayConfirmation; + const skipConfirmationDialog = + isSnapPreinstalled(snapId) && !displayConfirmation; + + // Only pre-installed Snaps can skip the account name suggestion dialog. + const skipAccountNameSuggestionDialog = + isSnapPreinstalled(snapId) && !displayAccountNameSuggestion; // First part of the flow, which includes confirmation dialogs (if not skipped). // Once confirmed, we resume the Snap execution. const { accountName } = await this.#addAccountConfirmations({ snapId, - skipConfirmation, + skipConfirmationDialog, + skipAccountNameSuggestionDialog, accountNameSuggestion, handleUserInput, }); @@ -388,7 +430,7 @@ class SnapKeyringImpl implements SnapKeyringCallbacks { void this.#addAccountFinalize({ address, snapId, - skipConfirmation, + skipConfirmationDialog, accountName, onceSaved, }); diff --git a/app/scripts/lib/snap-keyring/types.ts b/app/scripts/lib/snap-keyring/types.ts index db1e4e6c56f1..6e1314f7a6e8 100644 --- a/app/scripts/lib/snap-keyring/types.ts +++ b/app/scripts/lib/snap-keyring/types.ts @@ -4,6 +4,7 @@ import type { KeyringControllerGetAccountsAction } from '@metamask/keyring-contr import { GetSubjectMetadata } from '@metamask/permission-controller'; import { AccountsControllerGetAccountByAddressAction, + AccountsControllerListMultichainAccountsAction, AccountsControllerSetAccountNameAction, AccountsControllerSetSelectedAccountAction, } from '@metamask/accounts-controller'; @@ -34,6 +35,7 @@ export type SnapKeyringBuilderAllowActions = | AccountsControllerSetSelectedAccountAction | AccountsControllerGetAccountByAddressAction | AccountsControllerSetAccountNameAction + | AccountsControllerListMultichainAccountsAction | HandleSnapRequest | GetSnap | PreferencesControllerGetStateAction; diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index b021ce2ae41c..a5cd248b49e7 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -1125,6 +1125,7 @@ export default class MetamaskController extends EventEmitter { 'AccountsController:setSelectedAccount', 'AccountsController:getAccountByAddress', 'AccountsController:setAccountName', + 'AccountsController:listMultichainAccounts', 'SnapController:handleRequest', 'SnapController:get', 'PreferencesController:getState', diff --git a/package.json b/package.json index 232bc7d46962..89c36e385136 100644 --- a/package.json +++ b/package.json @@ -273,7 +273,7 @@ "@metamask/eth-json-rpc-middleware": "^15.1.2", "@metamask/eth-ledger-bridge-keyring": "^8.0.3", "@metamask/eth-sig-util": "^7.0.1", - "@metamask/eth-snap-keyring": "^11.0.0", + "@metamask/eth-snap-keyring": "^11.1.0", "@metamask/eth-token-tracker": "^10.0.2", "@metamask/eth-trezor-keyring": "^6.0.0", "@metamask/etherscan-link": "^3.0.0", diff --git a/shared/lib/accounts/accounts.ts b/shared/lib/accounts/accounts.ts new file mode 100644 index 000000000000..a6385a44f5a8 --- /dev/null +++ b/shared/lib/accounts/accounts.ts @@ -0,0 +1,27 @@ +import { InternalAccount } from '@metamask/keyring-internal-api'; + +/** + * Get the next available account name based on the suggestion and the list of + * accounts. + * + * @param accounts - The list of accounts to check for name availability + * @param nameSuggestion - The suggested name for the account + * @returns The next available account name based on the suggestion + */ +export function getUniqueAccountName( + accounts: InternalAccount[], + nameSuggestion: string, +): string { + let suffix = 1; + let candidateName = nameSuggestion; + + const isNameTaken = (name: string) => + accounts.some((account) => account.metadata.name === name); + + while (isNameTaken(candidateName)) { + suffix += 1; + candidateName = `${nameSuggestion} ${suffix}`; + } + + return candidateName; +} diff --git a/shared/lib/accounts/index.ts b/shared/lib/accounts/index.ts new file mode 100644 index 000000000000..5fcdc110a743 --- /dev/null +++ b/shared/lib/accounts/index.ts @@ -0,0 +1,4 @@ +export * from './accounts'; +export * from './bitcoin-wallet-snap'; +export * from './snaps'; +export * from './solana-wallet-snap'; diff --git a/ui/components/multichain/create-named-snap-account/create-named-snap-account.tsx b/ui/components/multichain/create-named-snap-account/create-named-snap-account.tsx index d04763902247..fd6ae6eb7cdb 100644 --- a/ui/components/multichain/create-named-snap-account/create-named-snap-account.tsx +++ b/ui/components/multichain/create-named-snap-account/create-named-snap-account.tsx @@ -8,6 +8,7 @@ import { Box, ModalHeader } from '../../component-library'; import { useI18nContext } from '../../../hooks/useI18nContext'; import { getMostRecentOverviewPage } from '../../../ducks/history/history'; import { getNextAvailableAccountName } from '../../../store/actions'; +import { getUniqueAccountName } from '../../../../shared/lib/accounts'; export type CreateNamedSnapAccountProps = { /** @@ -44,25 +45,9 @@ export const CreateNamedSnapAccount: React.FC = ({ const getNextAccountName = useCallback( async (accounts: InternalAccount[]): Promise => { // If a snap-suggested account name exists, use it as a base - if (snapSuggestedAccountName) { - let suffix = 1; - let candidateName = snapSuggestedAccountName; - - // Check if the name is already taken - const isNameTaken = (name: string) => - accounts.some((account) => account.metadata.name === name); - - // Keep incrementing suffix until we find an available name - while (isNameTaken(candidateName)) { - suffix += 1; - candidateName = `${snapSuggestedAccountName} ${suffix}`; - } - - return candidateName; - } - - // If no snap-suggested name, use the next available account name - return getNextAvailableAccountName(KeyringTypes.snap); + return snapSuggestedAccountName + ? getUniqueAccountName(accounts, snapSuggestedAccountName) + : getNextAvailableAccountName(KeyringTypes.snap); }, [], ); diff --git a/yarn.lock b/yarn.lock index 5e71c764f82d..5b99cead6c52 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5329,7 +5329,7 @@ __metadata: languageName: node linkType: hard -"@metamask/eth-snap-keyring@npm:^11.0.0, @metamask/eth-snap-keyring@npm:^11.1.0": +"@metamask/eth-snap-keyring@npm:^11.1.0": version: 11.1.0 resolution: "@metamask/eth-snap-keyring@npm:11.1.0" dependencies: @@ -26953,7 +26953,7 @@ __metadata: "@metamask/eth-json-rpc-provider": "npm:^4.1.6" "@metamask/eth-ledger-bridge-keyring": "npm:^8.0.3" "@metamask/eth-sig-util": "npm:^7.0.1" - "@metamask/eth-snap-keyring": "npm:^11.0.0" + "@metamask/eth-snap-keyring": "npm:^11.1.0" "@metamask/eth-token-tracker": "npm:^10.0.2" "@metamask/eth-trezor-keyring": "npm:^6.0.0" "@metamask/etherscan-link": "npm:^3.0.0"