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

refactor: refactor network picker #30433

Merged
merged 91 commits into from
Feb 25, 2025
Merged

Conversation

gantunesr
Copy link
Member

@gantunesr gantunesr commented Feb 19, 2025

Description

Open in GitHub Codespaces

Related issues

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

Manual testing steps

Note: UX changes should only be present in Flask/Beta

  1. Go to the experimental settings view and enable Solana
  2. See that the Solana network is now added to the network picker
  3. Select the network
  4. An account creation flow should be displayed
  5. Complete the flow and add a Solana account
  6. Change multiple times between accounts and networks
  7. Verify that the relation network-account is always correct
    • Solana Account <> Solana Network
    • EVM Account <> EVM Network

Screenshots/Recordings

Before

Screen.Recording.2025-02-24.at.9.55.07.AM.mov

After

Screen.Recording.2025-02-24.at.10.01.45.AM.mov

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.

@metamaskbot
Copy link
Collaborator

Builds ready [edf1786]
Page Load Metrics (2008 ± 96 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint163523242011209100
domContentLoaded16292255197719292
load16342325200820196
domInteractive27124482613
backgroundConnect10140363215
firstReactRender157729199
getState56419188
initialActions01000
loadScripts12101776150216177
setupStore858262010
uiStartup190326352308215103
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 2.24 KiB (0.04%)
  • ui: 1.6 MiB (21.56%)
  • common: 4.41 KiB (0.05%)

@metamaskbot
Copy link
Collaborator

Builds ready [27c0a3d]
Page Load Metrics (1716 ± 66 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint34420191643324155
domContentLoaded14661989169413565
load15122024171613866
domInteractive25129472914
backgroundConnect95921147
firstReactRender1476372311
getState459172010
initialActions0485147
loadScripts10511539124812460
setupStore77612157
uiStartup17192274194014469
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 2.26 KiB (0.04%)
  • ui: 1.6 MiB (21.55%)
  • common: 4.41 KiB (0.05%)

Copy link
Member

@NidhiKJha NidhiKJha left a comment

Choose a reason for hiding this comment

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

Great work! Before I approve:

  1. UX bug: I believe, this is not a network picker bug but while creating Solana accounts from settings page, the modal appears behind the page which seems confusing.
Screen.Recording.2025-02-25.at.10.05.07.AM.mov
  1. Are we not adding the support to edit RPC URL for Solana network like other EVM networks?

@gantunesr
Copy link
Member Author

Good catch @NidhiKJha!

UX bug: I believe, this is not a network picker bug but while creating Solana accounts from settings page, the modal appears behind the page which seems confusing.

Indeed this is not a bug introduced in this PR, I'll open a ticket for us to fix it

Are we not adding the support to edit RPC URL for Solana network like other EVM networks?

No, since the snap abstracts this away from the client and there is no snap API that allows the client to get/update this information

ccharly
ccharly previously approved these changes Feb 25, 2025
Copy link
Contributor

@ccharly ccharly left a comment

Choose a reason for hiding this comment

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

Code LGTM (thanks for all the changes).

Here are some screenshots from my testing:

  • The migration
    Screenshot 2025-02-25 at 12 48 37

  • Enabling bitcoin support (needs to enable this in the builds.yml)
    Screenshot 2025-02-25 at 13 31 18

  • Logic works fine when selecting an account/network (the other part is switching automatically)


Warning

Small issue with the Bitcoin testnet support (which is OK for now, since we've disabled Bitcoin support temporarily)

Now, the only issue I have found so far, is with "Bitcoin testnet". We don't have this network yet (and the logic to associate accounts and their networks is a bit incorrect on the AccountsController: https://github.com/MetaMask/core/blob/main/packages/accounts-controller/src/AccountsController.ts#L1017-L1019, I'm currently fixing this now).

Back to the problem, if we use an account that uses a scopes that is not associated with one of our non-EVM network, the wallet will be kinda blocked with this screen:

Screenshot 2025-02-25 at 11 35 28

We can fix this issue in another PR, cause I think part of the MultichainNetworkController logic might have to be updated for this.

@metamaskbot
Copy link
Collaborator

Builds ready [40cd6de]
Page Load Metrics (1753 ± 118 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint27222601516545262
domContentLoaded142221631722250120
load146521781753245118
domInteractive24119442412
backgroundConnect1282362010
firstReactRender1488302110
getState578252311
initialActions01000
loadScripts101816431293211101
setupStore75914136
uiStartup167028492034338163
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 2.26 KiB (0.04%)
  • ui: 9.1 KiB (0.12%)
  • common: 4.41 KiB (0.04%)

Copy link
Member

@NidhiKJha NidhiKJha left a comment

Choose a reason for hiding this comment

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

Solana setting is missing in experimental settings. By default it's being added in the network list.
https://github.com/user-attachments/assets/fd77cedc-061b-407c-bf87-1092c80f6748

Copy link
Member

@NidhiKJha NidhiKJha left a comment

Choose a reason for hiding this comment

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

LGTM

@owencraston owencraston added this pull request to the merge queue Feb 25, 2025
Merged via the queue into main with commit 3726202 Feb 25, 2025
79 checks passed
@owencraston owencraston deleted the gar/refactor-network-picker branch February 25, 2025 20:05
@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.

7 participants