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

Conversation

danroc
Copy link
Contributor

@danroc danroc commented Feb 24, 2025

Description

This PR handles the displayAccountNameSuggestion flag that can be set by pre-installed Snaps in the accountCreated event.

Open in GitHub Codespaces

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 --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

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.

Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@danroc danroc changed the title feat(snap-keyring): handle displayAccountNameSuggestion flag feat(snap-keyring)!: handle displayAccountNameSuggestion flag Feb 24, 2025
Comment on lines 403 to 404
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.

@danroc danroc changed the title feat(snap-keyring)!: handle displayAccountNameSuggestion flag feat(snap-keyring): handle displayAccountNameSuggestion flag Feb 25, 2025
@danroc danroc marked this pull request as ready for review February 25, 2025 07:54
@danroc danroc requested review from a team as code owners February 25, 2025 07:54
@danroc danroc enabled auto-merge February 25, 2025 07:54
@metamaskbot
Copy link
Collaborator

Builds ready [d05e715]
Page Load Metrics (1526 ± 59 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30417111456282135
domContentLoaded13111861150712460
load13181865152612459
domInteractive177232168
backgroundConnect85325167
firstReactRender1375252010
getState4441094
initialActions01000
loadScripts9511394112110651
setupStore7491094
uiStartup15092159173314871
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 598 Bytes (0.01%)
  • ui: 52 Bytes (0.00%)
  • common: 1.6 MiB (18.99%)

@ccharly
Copy link
Contributor

ccharly commented Feb 25, 2025

Just to complement my approval, here's a video of my testing:

Screen.Recording.2025-02-25.at.10.26.30.mov

@danroc danroc added this pull request to the merge queue Feb 25, 2025
Merged via the queue into main with commit 32c2077 Feb 25, 2025
79 checks passed
@danroc danroc deleted the dr/skip-name-dialog branch February 25, 2025 10:46
@github-actions github-actions bot locked and limited conversation to collaborators Feb 25, 2025
@metamaskbot metamaskbot added the release-12.14.0 Issue or pull request that will be included in release 12.14.0 label Feb 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.14.0 Issue or pull request that will be included in release 12.14.0 team-accounts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants