metamask-extension icon indicating copy to clipboard operation
metamask-extension copied to clipboard

Published 20 hours ago • verified publisher icon MetaMask

Update getMetaMaskIdentities with getInternalAccounts

Open montelaidev opened this issue 2 years ago • 16 comments

Description

Currently, MetaMask faces tight coupling between its UI and the keyring-controller. The UI heavily relies on the keyring-controller's state while also amalgamating information from various sources, such as preferences and balances.

However, this approach presents some challenges. The keyring-controller must be aware of the UI's limitations, like the need for instant account list provision to avoid lag. Moreover, it takes on the responsibility of adding metadata to accounts, such as the associated keyring type, required for displaying account details.

To address these issues, the introduction of the accounts-controller comes into play as a new abstraction layer between the UI and the keyring-controller. This separation of responsibilities allows the keyring-controller to focus on two main tasks:

  • Routing requests to the appropriate keyring.
  • Persisting the state of the keyrings.

This PR replaces the usage of getMetaMaskIdentities with getInternalAccounts

Depends On: https://github.com/MetaMask/metamask-extension/pull/21553

Related issues

Resolves https://github.com/MetaMask/accounts-planning/issues/134

Manual testing steps

  1. Use a hardware wallet to test all the flows

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • [x] I’ve followed MetaMask Coding Standards.
  • [x] I've clearly explained:
    • [x] What problem this PR is solving.
    • [x] How this problem was solved.
    • [x] How reviewers can test my changes.
  • [x] I’ve indicated what issue this PR is linked to: Fixes #???
  • [x] I’ve included tests if applicable.
  • [x] I’ve documented any added code.
  • [x] I’ve applied the right labels on the PR (see labeling guidelines).
  • [x] I’ve properly set the pull request status:
    • [x] In case it's not yet "ready for review", I've set it to "draft".
    • [x] In case it's "ready for review", I've changed it from "draft" to "non-draft".

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.

montelaidev avatar Nov 07 '23 08:11 montelaidev

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 Nov 07 '23 08:11 github-actions[bot]

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

github-actions[bot] avatar Jan 25 '24 14:01 github-actions[bot]

@metamaskbot update-policies

montelaidev avatar Jan 26 '24 08:01 montelaidev

Policies updated

metamaskbot avatar Jan 26 '24 08:01 metamaskbot

Builds ready [f8dd3c1]
Page Load Metrics (691 ± 13 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint841279784
domContentLoaded9171411
load6597436912613
domInteractive9171411
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 1.19 KiB (0.02%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Jan 29 '24 08:01 metamaskbot

Builds ready [69d2f43]
Page Load Metrics (735 ± 14 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint9012610384
domContentLoaded9171331
load6778087353014
domInteractive9171331
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 1.19 KiB (0.02%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Jan 29 '24 09:01 metamaskbot

Builds ready [e8b48f4]
Page Load Metrics (796 ± 20 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint91138114115
domContentLoaded10471884
load7098907964120
domInteractive10471884
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 1.2 KiB (0.02%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Jan 31 '24 13:01 metamaskbot

I see that this PR also changes the file ui/pages/snaps/snap-view/snap-settings.js.

This PR is quite big so the chance of being blocked is high, I strongly recommend opening a new PR with just that change to remove that review dependency.

gantunesr avatar Feb 06 '24 22:02 gantunesr

Codecov Report

Attention: Patch coverage is 52.87356% with 41 lines in your changes are missing coverage. Please review.

Project coverage is 68.56%. Comparing base (f893b45) to head (fb4478e).

:exclamation: Current head fb4478e differs from pull request most recent head 9082b46. Consider uploading reports for the commit 9082b46 to get more accurate results

Files Patch % Lines
app/scripts/controllers/mmi-controller.js 5.88% 16 Missing :warning:
...ent/permission-page-container-content.component.js 0.00% 6 Missing :warning:
...e-container/permission-page-container.container.js 0.00% 4 Missing :warning:
...ct-list-tab/view-contact/view-contact.container.js 0.00% 4 Missing :warning:
app/scripts/metamask-controller.js 25.00% 3 Missing :warning:
...ct-list-tab/edit-contact/edit-contact.container.js 0.00% 3 Missing :warning:
ui/selectors/selectors.js 85.71% 3 Missing :warning:
...e-container/permission-page-container.component.js 0.00% 1 Missing :warning:
...d-content/add-recipient/add-recipient.component.js 0.00% 1 Missing :warning:
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #21709      +/-   ##
===========================================
- Coverage    68.60%   68.56%   -0.04%     
===========================================
  Files         1101     1100       -1     
  Lines        43196    43143      -53     
  Branches     11564    11531      -33     
===========================================
- Hits         29632    29577      -55     
- Misses       13564    13566       +2

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

codecov[bot] avatar Feb 07 '24 05:02 codecov[bot]

Builds ready [6656aec]
Page Load Metrics (781 ± 30 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint84137104136
domContentLoaded11231732
load6959777816230
domInteractive11231732
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 1.64 KiB (0.02%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Feb 07 '24 06:02 metamaskbot

I do not have a hardware wallet, but I tested on the E2E test dapp and the SSK with a snap account and EOA all flows and did not notice any bugs or errors in sending or signing.

k-g-j avatar Feb 08 '24 22:02 k-g-j

Builds ready [3d74b68]
Page Load Metrics (990 ± 95 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint881861262914
domContentLoaded95726126
load749135799019995
domInteractive95726126
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 1.74 KiB (0.03%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Feb 09 '24 09:02 metamaskbot

Builds ready [37411e7]
Page Load Metrics (1015 ± 50 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1213051874321
domContentLoaded990282311
load7881298101510450
domInteractive990282311
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 1.74 KiB (0.03%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Feb 15 '24 15:02 metamaskbot

@montelaidev I found a minor bug in the send flow

https://github.com/MetaMask/metamask-extension/assets/17601467/1d4ad918-9b62-4fb1-b1c1-6c56fd602dfd

gantunesr avatar Feb 15 '24 17:02 gantunesr

@montelaidev I found a minor bug in the send flow

Screen.Recording.2024-02-15.at.14.32.29.mov

Looks like its the same bug as this https://github.com/MetaMask/metamask-extension/issues/22933, its due to the petnames overriding the names being passed to the component.

montelaidev avatar Feb 15 '24 17:02 montelaidev

True, let's try to fix it in another PR

gantunesr avatar Feb 15 '24 23:02 gantunesr

Tested on Mac in Sonoma 14.3.1 with Chrome 117.0.5938.92 using Ledger, Lattice, QR, Trezor and Snap accounts. Also spot checked in Firefox 123.0 Lattice, QR, Trezor and Snap accounts (Ledger not supported >v114)

plasmacorral avatar Feb 27 '24 21:02 plasmacorral

I have encountered a situation where the test dapp can no longer be used to connect an account, after the one that was connected is removed. It initially struck me as similar to what is reported here, but occurs with connecting an account rather than signatures and I want to be confident its not related to the changes in this PR before we merge.

Reproduction steps:

  1. Import SRP
  2. Add Hardware wallet (Ledger tested but could not reproduce with a Snap account from SSK v1.1.1)
  3. Connect to test dapp and open console
  4. Conduct a V4 typed data signature and verify recovery result
  5. Return to add hardware flow and forget device
  6. Go back to test dapp from full screen add hardware
  7. Then open pop-up again by tapping on fox
  8. Tap Connect to connect Account 1 or whatever account is now active after removing Ledger
  9. Note error Something went wrong, and we were unable to complete this action
  10. Tap Dismiss
  11. Note console error in test dapp MetaMask: 'eth_accounts' unexpectedly updated accounts. Please report this bug.
  12. Connect button in test dapp is no longer responsive, but Use MetaMask Use Window.Ethereum and Wallet Connect buttons do all remain interactive

Recording: https://recordit.co/bm4XY9l9QO

EDIT: Reproduced this with a dist build of 11.11.0 using the test dapp, issue is unrelated to changes in this PR.

plasmacorral avatar Feb 28 '24 00:02 plasmacorral

Builds ready [f8d99ec]
Page Load Metrics (868 ± 425 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint721911203416
domContentLoaded106226157
load542169868884425
domInteractive106226157
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 1.74 KiB (0.02%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Feb 28 '24 13:02 metamaskbot

Builds ready [fb4478e]
Page Load Metrics (1099 ± 400 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint722691184220
domContentLoaded106731188
load6720391099833400
domInteractive106731188
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 1.74 KiB (0.02%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Mar 04 '24 13:03 metamaskbot

My comments are to be addressed in a future PR.

danroc avatar Mar 05 '24 14:03 danroc

Builds ready [9082b46]
Page Load Metrics (1074 ± 395 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint692221193919
domContentLoaded96328168
load5619051074822395
domInteractive96328168
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 1.74 KiB (0.02%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar Mar 05 '24 14:03 metamaskbot