-
-
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
feat: Accounts controller integration #8759
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. |
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring: Next stepsTake a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with |
f88a93f
to
a06fa48
Compare
@SocketSecurity ignore npm/@metamask/obs-store@9.0.0 |
8830508
to
903851d
Compare
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@metamask/snaps-controllers@5.0.0, npm/@metamask/snaps-sdk@2.0.0, npm/@metamask/snaps-utils@6.0.0, npm/fast-xml-parser@4.2.4, npm/nan@2.18.0 |
23a9263
to
6532b10
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8759 +/- ##
==========================================
+ Coverage 45.50% 45.56% +0.05%
==========================================
Files 1275 1276 +1
Lines 31202 31296 +94
Branches 3184 3200 +16
==========================================
+ Hits 14199 14259 +60
- Misses 16163 16191 +28
- Partials 840 846 +6 ☔ View full report in Codecov by Sentry. |
dc02c6a
to
7f40dcd
Compare
d77bf86
to
56b6b96
Compare
Bitrise❌❌❌ Commit hash: 56b6b96 Note
|
d638145
to
8cc9c49
Compare
f00960c
to
91981b1
Compare
Bitrise✅✅✅ Commit hash: 2384808 Note
|
Bitrise✅✅✅ Commit hash: daadd0c Note
|
Bitrise✅✅✅ Commit hash: 9db203a Note
|
Bitrise✅✅✅ Commit hash: 032b30c Note
|
Quality Gate passedIssues Measures |
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
Description
032.ts
for more information.setSelectedAddress
that updates both the AccountsController and PreferencesController. this method ensures that the selected address is always up to date and in sync. There should beNO MORE direct calls to PreferencesController.setSelectedAddress()
in the U.I.Related issues
Fixes: https://github.com/MetaMask/accounts-planning/issues/305
Equivalent extension PR: MetaMask/metamask-extension#21437
Manual testing steps
Setup
accounts-controller
yarn setup && yarn watch:clean
yarn start:ios
or inside xcode/android studio5. add the following logs to app/components/Nav/App/index.js
Testing
accounts
and the preferences controller should have no selectedAddress or identities
selectedAddress
inside the PreferencesController should match the address for the selectedAccount inside the AccountsController. Since the accounts controller uses account id's instead of the address as a key, you will need to cross reference the account id its properties such as account name and account address.Caveats
internalState
object within the PreferencesController as there are other weird state values that are out of sync and do not reflect the true state of the application.Screenshots/Recordings
Before
After
720.Screen.Recording.2024-03-06.at.8.22.46.PM.mov
QA
I personally QA'd this build and tested the following flows. For these test I used this QA build.
Import wallet on a fresh build
Video:
Untitled.mp4
Wallet Update Path (testing migration)
Video:
1000000327.mp4
Dapp connection and send flow
Video:
1000000329.mp4
Import account, remove account, re import account
Video:
1000000330.2.mp4
screen-20240320-191514.mp4
Pre-merge author checklist
Pre-merge reviewer checklist