metamask-extension icon indicating copy to clipboard operation
metamask-extension copied to clipboard
verified publisher icon MetaMask

feat: define account name during creation

Open k-g-j opened this issue 1 year ago • 7 comments

Description

Changes the add snap account flow to include a final modal that allows snaps to suggest a name for the new account or for the user to input a name for their new account. The new snap account is now created with a name either inputted by the user, suggested by the snap, or the default fallback account name.

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/accounts-planning/issues/261

Manual testing steps

Testing with preinstalled snap

You will need to temporarily add @metamask/account-watcher as a preinstalled snap

  1. Comment out these lines from the getSnapsList selector
   // Comment these lines out below so that preinstalled snaps aren't hidden from snaps list
    if (snap.preinstalled) {
      return snap.hidden === false;
    }
  1. Add the Account Watcher snap with yarn add @metamask/account-watcher
  2. Replace preinstalled-snaps.ts with this:
import type { PreinstalledSnap } from '@metamask/snaps-controllers';
import MessageSigningSnap from '@metamask/message-signing-snap/dist/preinstalled-snap.json';
import AccountWatcherSnap from '@metamask/account-watcher/dist/preinstalled-snap.json';

const PREINSTALLED_SNAPS: readonly PreinstalledSnap[] = Object.freeze([
  MessageSigningSnap as PreinstalledSnap,
  AccountWatcherSnap as PreinstalledSnap,
]);

export default PREINSTALLED_SNAPS;
  1. Build the extension and go to vertical three dot menu > Snaps > Account Watcher
  2. Go through the flow to watch an account without inputting a name, with inputting a name, and canceling adding the account
Testing with account management snap
  1. Go to the SSK and connect the snap
  2. Go through the flow to create an account without inputting a name, with inputting a name, and canceling adding the account

Screenshots/Recordings

Demos

Preinstalled snap flow

https://github.com/MetaMask/metamask-extension/assets/91970214/3782a417-4fd0-4b56-b4b7-30f64e75c6e8

Account management snap flow

https://github.com/MetaMask/metamask-extension/assets/91970214/9f53a67d-13c5-44ac-b286-78eddbfb89ae

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.

k-g-j avatar Jun 10 '24 22:06 k-g-j

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.

github-actions[bot] avatar Jun 10 '24 22:06 github-actions[bot]

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

socket-security[bot] avatar Jun 10 '24 22:06 socket-security[bot]

Question for @ccharly and @gantunesr Do you think we should throw an error after this line if a user cancels creating the account via the naming modal?

I noticed in the current implementation we do throw an error if a user cancels account creation. I have not been doing so, but maybe it is necessary?

k-g-j avatar Jun 21 '24 15:06 k-g-j

@k-g-j

Question for @ccharly and @gantunesr Do you think we should throw an error after this line if a user cancels creating the account via the naming modal?

I noticed in the current implementation we do throw an error if a user cancels account creation. I have not been doing so, but maybe it is necessary?

IMO, yes, we should throw here too. This error seems to bubble up to the caller (when testing with the SSK, the dapp got the error message back), since cancelling the account renaming will trigger account removal, we should keep this behavior consistent and also returns an error here.

Throwing an error here, might have an impact on the extension when we trigger this kind of flow from the extension itself, but we would have to adapt the behavior of the extension in this case.

ccharly avatar Jun 24 '24 09:06 ccharly

IMO, yes, we should throw here too.

Okay, I applied your suggestion here 203e9ac and will test it in the extension

k-g-j avatar Jun 25 '24 15:06 k-g-j

Will test it in the extension

Update: It seems to work much better when throwing the error and being consistent here. As @ccharly said, when testing with the SSK, the dapp got the error message back 😄

k-g-j avatar Jun 25 '24 15:06 k-g-j

Codecov Report

Attention: Patch coverage is 41.17647% with 10 lines in your changes missing coverage. Please review.

Project coverage is 69.94%. Comparing base (b27dd2b) to head (36c7403).

Files Patch % Lines
.../multichain/account-list-menu/account-list-menu.js 20.00% 8 Missing :warning:
...ltichain/create-btc-account/create-btc-account.tsx 75.00% 1 Missing :warning:
ui/store/actions.ts 0.00% 1 Missing :warning:
Additional details and impacted files 
@@           Coverage Diff            @@
##           develop   #25191   +/-   ##
========================================
  Coverage    69.94%   69.94%           
========================================
  Files         1409     1409           
  Lines        49794    49771   -23     
  Branches     13773    13762   -11     
========================================
- Hits         34826    34811   -15     
+ Misses       14968    14960    -8

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jul 23 '24 19:07 codecov[bot]

Builds ready [3bc5ff7]
Page Load Metrics (181 ± 214 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint703591197034
domContentLoaded10155353215
load482114181446214
domInteractive10155353215
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 573 Bytes (0.02%)
  • ui: -507.66 KiB (-6.73%)
  • common: 1.68 KiB (0.02%)

metamaskbot avatar Jul 23 '24 19:07 metamaskbot

Builds ready [a3fcb3a]
Page Load Metrics (71 ± 10 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint78143101188
domContentLoaded117231178
load50118712010
domInteractive117231178
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 573 Bytes (0.02%)
  • ui: -483.32 KiB (-6.42%)
  • common: 1.68 KiB (0.02%)

metamaskbot avatar Jul 25 '24 20:07 metamaskbot

Builds ready [36c7403]
Page Load Metrics (174 ± 188 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint742341163517
domContentLoaded11154343215
load501874174391188
domInteractive11154343215
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 573 Bytes (0.02%)
  • ui: -483.32 KiB (-6.42%)
  • common: 1.68 KiB (0.02%)

metamaskbot avatar Jul 26 '24 09:07 metamaskbot

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

sonarqubecloud[bot] avatar Jul 26 '24 15:07 sonarqubecloud[bot]

Builds ready [1f02535]
Page Load Metrics (56 ± 6 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint7011987136
domContentLoaded95523115
load409156126
domInteractive95523115
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 573 Bytes (0.02%)
  • ui: -483.32 KiB (-6.42%)
  • common: 1.68 KiB (0.02%)

metamaskbot avatar Jul 26 '24 16:07 metamaskbot