Skip to content

Conversation

@montelaidev
Copy link
Contributor

@montelaidev montelaidev commented Oct 26, 2023

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:

This PR creates new selectors for Internal Accounts and and updates some ui elements to use the selectors.

Depends on:
#21437

Related issues

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

Manual testing steps

Screenshots/Recordings

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines).
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • 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.

@github-actions
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.

@socket-security
Copy link

socket-security bot commented Oct 26, 2023

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

👍 No dependency changes detected in pull request

@AlexJupiter
Copy link
Contributor

https://airgap.it/ can be used to replicate this if a Keystone device isn't available @k-g-j

@gantunesr gantunesr dismissed stale reviews from k-g-j, danroc, and owencraston via f46249b December 13, 2023 15:42
@gantunesr
Copy link
Member

@plasmacorral @AlexJupiter this is fixed in the latest commit

@plasmacorral plasmacorral added needs-qa Label will automate into QA workspace needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Dec 18, 2023
k-g-j
k-g-j previously approved these changes Dec 18, 2023
@gantunesr gantunesr force-pushed the feat/update-selectors-to-include-internal-accounts branch from b02f424 to 7f5ffd6 Compare December 18, 2023 23:58
@metamaskbot
Copy link
Collaborator

Builds ready [936937f]
Page Load Metrics (1492 ± 114 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1032911635325
domContentLoaded10202405125
load98118751492237114
domInteractive10202405125
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 102 Bytes (0.00%)
  • ui: 699 Bytes (0.01%)
  • common: 0 Bytes (0.00%)

@gantunesr gantunesr removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Dec 19, 2023
@plasmacorral plasmacorral added QA Passed and removed DO-NOT-MERGE Pull requests that should not be merged needs-qa Label will automate into QA workspace labels Dec 19, 2023
@plasmacorral
Copy link
Contributor

plasmacorral commented Dec 19, 2023

Manually tested chrome 117.0.5938.92 and firefox 120.0.1 and migrating from 11.5.2. Tested imported, Ledger, Trezor, QR, Lattice and Snap accounts with no remaining issues. Tests included NFT details, Account selection, Account connection, signature requests, transactions/activity detail, wallet overview, and editing approvals.

@plasmacorral plasmacorral merged commit 23b5b88 into develop Dec 19, 2023
@plasmacorral plasmacorral deleted the feat/update-selectors-to-include-internal-accounts branch December 19, 2023 03:54
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2023
@metamaskbot metamaskbot added the release-11.9.0 Issue or pull request that will be included in release 11.9.0 label Dec 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

QA Passed release-11.9.0 Issue or pull request that will be included in release 11.9.0 team-accounts-framework Accounts Framework team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants