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: consolidate accounts references to a single source of truth #8664

Merged
merged 14 commits into from
Feb 27, 2024

Conversation

owencraston
Copy link
Contributor

@owencraston owencraston commented Feb 21, 2024

Description

  1. What is the reason for the change?
    The accounts team is working towards the integration of the new accounts controller. This controller will become the source of truth for all things accounts (name address, methods KeyringType etc). In order to make this integration easier we must first centralize the source of truth for accounts.
  2. What is the improvement/solution?
  • Currently account information is being derived from the preferences controller. In this PR I removed direct use of state.engine.backgroundState.PreferencesController in favour of the respective redux selectors. I also migrated the Wallet screen to use the useAccounts hook which achieves the same result as before but without custom logic.

There should be no visible or functional changes to the app.

Related issues

Fixes: #8482

Notes for code reviewers

  • 🔴 No use of backgroundState.PreferencesController selectedAddress and identities
  • 🟢 Instead they should all be using the selectIdentities and the selectSelectedAddress selectors

Manual testing steps

There should be no visible changes after this PR

  1. create/import a wallet
  2. click the 3 dots on the the account selector
  3. change the account name
  4. add an account
  5. open the browser
  6. navigate to a dapp: https://uniswap.org/
  7. connect the dapp
  8. ensure that the account names are the same as the ones you input previously

Screenshots/Recordings

Before

After

Creation flow

Screen.Recording.2024-02-26.at.7.01.50.PM.mov

Import flow

Screen.Recording.2024-02-26.at.7.15.26.PM.mov

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). Not required for external contributors.
  • 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.

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.

@owencraston owencraston changed the title migrate WalleAccount component refactor: Consolidate accounts references to a single source of truth Feb 21, 2024
Copy link

@M111111111r M111111111r left a comment

Choose a reason for hiding this comment

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

Mahdi

@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2024

Codecov Report

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

Project coverage is 41.37%. Comparing base (4411edc) to head (23b13d3).

Files Patch % Lines
app/components/Views/Wallet/index.tsx 50.00% 2 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8664      +/-   ##
==========================================
- Coverage   41.38%   41.37%   -0.01%     
==========================================
  Files        1266     1266              
  Lines       30769    30768       -1     
  Branches     3028     3031       +3     
==========================================
- Hits        12734    12731       -3     
- Misses      17275    17276       +1     
- Partials      760      761       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@owencraston owencraston marked this pull request as ready for review February 27, 2024 05:17
@owencraston owencraston requested a review from a team as a code owner February 27, 2024 05:17
@github-actions github-actions bot added the Run Smoke E2E Triggers smoke e2e on Bitrise label Feb 27, 2024
Copy link
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/ae949224-4ee9-45fc-a686-29446f28ccad
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

@gantunesr gantunesr changed the title refactor: Consolidate accounts references to a single source of truth refactor: consolidate accounts references to a single source of truth Feb 27, 2024
Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

Very cool logic abstraction, it will indeed bring value, just left a question

app/components/Views/Wallet/index.tsx Show resolved Hide resolved
tommasini
tommasini previously approved these changes Feb 27, 2024
Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

LGTM!

gantunesr
gantunesr previously approved these changes Feb 27, 2024
Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

lgtm, just left a couple of extreme nitpicks

app/components/UI/WalletAccount/WalletAccount.test.tsx Outdated Show resolved Hide resolved
app/components/Views/Wallet/index.tsx Outdated Show resolved Hide resolved
app/components/Views/Wallet/index.tsx Show resolved Hide resolved
@owencraston owencraston dismissed stale reviews from gantunesr and tommasini via fb06ee6 February 27, 2024 22:54
@owencraston owencraston added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Feb 27, 2024
Copy link
Contributor

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/810408dd-838f-4dba-81ab-e8fc00bd745b
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

Copy link

sonarcloud bot commented Feb 27, 2024

@owencraston owencraston merged commit 7ef00e1 into main Feb 27, 2024
31 checks passed
@owencraston owencraston deleted the consolidate-accounts branch February 27, 2024 23:14
@github-actions github-actions bot locked and limited conversation to collaborators Feb 27, 2024
@metamaskbot metamaskbot added the release-7.18.0 Issue or pull request that will be included in release 7.18.0 label Feb 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-7.18.0 Issue or pull request that will be included in release 7.18.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-accounts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Mobile Account Snaps] Migrate all accounts references to selectors
6 participants