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

feat: bump controllers related accounts logic #9508

Merged
merged 57 commits into from
May 3, 2024

Conversation

gantunesr
Copy link
Member

@gantunesr gantunesr commented May 2, 2024

Description

The original PR is #9318. It was reverted and now the changes are being introduced in this new PR.

This PR updates several controllers that handle logic related to the wallet and accounts.

  • Keyring controller to v13
  • Accounts Controller to v11
  • Preferences controller v8

Some noteworthy points for this changes,

  • Downgrade controller utils to interact with linea sepolia, only function used by preferences controller of controller-utils is toChecksumHexAddress and the function is the same on v^5 and on v^8
  • Updated flow of create account and import account to update the ecnryptionLib that way on the future we can create or import and we will not run the flow [recreateVaultWithSamePassword](https://github.com/MetaMask/metamask-mobile/blob/fcfa9d1bd03e16ba18f3e9919b85a854614c2377/app/core/Authentication/Authentication.ts#L110), since it was running unnecessarily
  • Updated controllers initialization that had preferencesOnStateChange argument on their constructor (Mostly assets-controllers and PPOM), to have the address checksummed, on this current version PreferencesController starts to listen to KeyringController and that address is now on lower case format, which means that we need to update to a checksummed format for those specific scenarios for the assets controllers keep their data and tokens never disappear from the user for missmatching user address format
  • Updated keyring controller patch with the same as extension to prevent the HD wallet disappear, please check the details on this core branch: extension-keyring-controller-v13-patch

Related issues

Manual testing steps

  1. Create new wallet
  2. Import existing wallet
  3. Import private key
  4. Upgrade testing

Screenshots/Recordings

Upgrade from main with Binance Smart Chain Selected, imported account with private key and tokens imported (Multiple chains added, and tokens imported on them also NFT imported on mainnet)

Screen.Recording.2024-04-24.at.13.01.08.mov

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • 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.

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.

gantunesr and others added 30 commits March 26, 2024 11:57
…Accounts Controller to v^11, this is still missing to check some breaking changes around the keyring controller v13
…r on engine and introduce the key for prefereces state change event on EngineService since now Preferences Controller extends BaseController v2
…es, patch preferences controller to export the etherscan supported chain ids to have sepolia and to be exported to the mobile app
@gantunesr gantunesr added team-accounts team-mobile-platform QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed QA in Progress QA has started on the feature. labels May 2, 2024
@gantunesr
Copy link
Member Author

gantunesr commented May 2, 2024

Bug reported in original PR,

Ran migration tests with a QA build of v7.18.0 (1268) to a QA build of commit 869cd17 and noted some accounts that were lost: Have before and after state logs available on 3 unique devices. All three devices lost the Ledger account, and 2 of 3 devices(both android and iOS) lost multiple QR accounts.

Follow up notes:
Before and After Statelogs here

This seems to be sporadic, as I have not been able to reproduce it with the same devices and the same 7.18.0 QA build like the initial observation.

@gantunesr gantunesr added the DO-NOT-MERGE Pull requests that should not be merged label May 2, 2024
Copy link

sonarcloud bot commented May 2, 2024

@plasmacorral
Copy link
Contributor

plasmacorral commented May 2, 2024

CONFIRMED NOT RELATED TO CHANGES IN THIS PR:

When importing a valid private key by QR code, user is shown an error: "Invalid deeplink Invalid URL:" and the private key string is displayed in the error message. The account is successfully imported, but the error message is not appropriate in this scenario.

Was observed with QA build from PR 9318, with multiple devices:

  • Pixel 5a- Android 14
  • Samsung a515f- Android 12
  • iPhone Xs- iOS 17.4.1

Steps:

  1. Import an SRP and get to wallet view
  2. Follow add account flow for importing private key
  3. Scan QR of valid private key
  4. Observe error

UPDATE: This is present on main commit 6cc76b2 and is not related to the changes in this PR. This is not present on 7.21.0 build (production) but is present on the RC for 7.22.0. Opened a fresh issue.

@plasmacorral
Copy link
Contributor

plasmacorral commented May 3, 2024

Tested migrations on Android 12, 14 and iOS 17.4.1 with Imported, Ledger and QR accounts present from both 7.18.0 (1268) as well as 7.19.0 (1292). Custom labels were preserved. Able to add and remove accounts as expected, change password and biometric auth, create new wallet, import an existing SRP in setup, import private keys after setup, and confirm SRP backup demand when value is added.

Had some observations, but was not able to reproduce outside of the initial occurrence and looks like there have been subsequent changes in both 7.19.0 and 7.20.0 that may resolve the issues observed.

  • Observed accounts disappear once updating from 7.18.0 to the branch under test
    • There were two independent fixes for both Ledger and QR wallets both going missing, within 7.19.0
  • Observed tokens disappear once on an imported account updating from 7.19.0 to the branch under test
    • There was a fix for disappearing tokens within 7.20.0
  • Observed private key input appearing in an error message when importing by QR
    • was present on main and looks like it may have been introduced within 7.22.0, so filed a fresh report here

@plasmacorral plasmacorral added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. DO-NOT-MERGE Pull requests that should not be merged QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels May 3, 2024
@legobeat legobeat mentioned this pull request May 3, 2024
7 tasks
@cortisiko cortisiko added the Run Smoke E2E Triggers smoke e2e on Bitrise label May 3, 2024
Copy link
Contributor

github-actions bot commented May 3, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 23506e2
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/009ba7f0-021f-44d4-9e05-64121d2fed65

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@Cal-L
Copy link
Contributor

Cal-L commented May 3, 2024

E2E failure not related to this branch since it's failing because of notification prompt

Copy link
Contributor

@Cal-L Cal-L 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 merged commit f02bb1a into main May 3, 2024
47 of 49 checks passed
@gantunesr gantunesr deleted the feat/bump-controllers-re-accounts branch May 3, 2024 21:01
@github-actions github-actions bot locked and limited conversation to collaborators May 3, 2024
@metamaskbot metamaskbot added the release-7.23.0 Issue or pull request that will be included in release 7.23.0 label May 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-7.23.0 Issue or pull request that will be included in release 7.23.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-accounts team-mobile-platform
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

9 participants