-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mahdi
5833a66
to
028e13d
Compare
ffe4904
to
1c91e19
Compare
Codecov ReportAttention: Patch coverage is
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. |
dd5e2d9
to
23b13d3
Compare
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/ae949224-4ee9-45fc-a686-29446f28ccad |
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this 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
fb06ee6
fb06ee6
to
365e928
Compare
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/810408dd-838f-4dba-81ab-e8fc00bd745b |
Quality Gate passedIssues Measures |
Description
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.
state.engine.backgroundState.PreferencesController
in favour of the respective redux selectors. I also migrated the Wallet screen to use theuseAccounts
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
backgroundState.PreferencesController
selectedAddress
andidentities
selectIdentities
and theselectSelectedAddress
selectorsManual testing steps
There should be no visible changes after this PR
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
Pre-merge reviewer checklist