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(snap-keyring): handle displayAccountNameSuggestion flag #30531

Merged
merged 10 commits into from
Feb 25, 2025
69 changes: 55 additions & 14 deletions app/scripts/lib/snap-keyring/snap-keyring.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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/accounts';
import { isBlockedUrl } from './utils/isBlockedUrl';
import { showError, showSuccess } from './utils/showResult';
import { SnapKeyringBuilderMessenger } from './types';
Expand Down Expand Up @@ -201,19 +202,58 @@ 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,
skipAccountNameSuggestion,
handleUserInput,
accountNameSuggestion,
}: {
snapId: SnapId;
skipConfirmation: boolean;
skipAccountNameSuggestion: boolean;
accountNameSuggestion: string;
handleUserInput: (accepted: boolean) => Promise<void>;
}): 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 =
Expand All @@ -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 } = skipAccountNameSuggestion
? 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 };
}
});
Expand Down Expand Up @@ -365,18 +400,24 @@ class SnapKeyringImpl implements SnapKeyringCallbacks {
handleUserInput: (accepted: boolean) => Promise<void>,
onceSaved: Promise<string>,
accountNameSuggestion: string = '',
displayConfirmation: boolean = false,
displayConfirmation: boolean = true,
displayAccountNameSuggestion: boolean = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

For future reviewers: The displayConfirmation was wrongly set to false. Now with this change, all pre-installed Snaps will have to send this flag while emitting the AccountCreated event.

Copy link
Contributor Author

@danroc danroc Feb 25, 2025

Choose a reason for hiding this comment

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

Note: The change mentioned in the comment above was reverted because it requires a change in snap-watch-only. It will be handled in a separate PR once the Snap is updated.

) {
assertIsValidSnapId(snapId);

// If Snap is preinstalled and does not request confirmation, skip the confirmation dialog.
const skipConfirmation = isSnapPreinstalled(snapId) && !displayConfirmation;

// Only pre-installed Snaps can skip the account name suggestion dialog.
const skipAccountNameSuggestion =
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,
skipAccountNameSuggestion,
accountNameSuggestion,
handleUserInput,
});
Expand Down
2 changes: 2 additions & 0 deletions app/scripts/lib/snap-keyring/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -34,6 +35,7 @@ export type SnapKeyringBuilderAllowActions =
| AccountsControllerSetSelectedAccountAction
| AccountsControllerGetAccountByAddressAction
| AccountsControllerSetAccountNameAction
| AccountsControllerListMultichainAccountsAction
| HandleSnapRequest
| GetSnap
| PreferencesControllerGetStateAction;
Expand Down
1 change: 1 addition & 0 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -1124,6 +1124,7 @@ export default class MetamaskController extends EventEmitter {
'AccountsController:setSelectedAccount',
'AccountsController:getAccountByAddress',
'AccountsController:setAccountName',
'AccountsController:listMultichainAccounts',
'SnapController:handleRequest',
'SnapController:get',
'PreferencesController:getState',
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
27 changes: 27 additions & 0 deletions shared/lib/accounts/accounts.ts
Original file line number Diff line number Diff line change
@@ -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;
}
Original file line number Diff line number Diff line change
Expand Up @@ -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/accounts';

export type CreateNamedSnapAccountProps = {
/**
Expand Down Expand Up @@ -44,25 +45,9 @@ export const CreateNamedSnapAccount: React.FC<CreateNamedSnapAccountProps> = ({
const getNextAccountName = useCallback(
async (accounts: InternalAccount[]): Promise<string> => {
// 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);
},
[],
);
Expand Down
4 changes: 2 additions & 2 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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"
Expand Down
Loading