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: remove selectIdentities in favour of selectInternalAccounts #9724

Merged
merged 17 commits into from
Jul 3, 2024

Conversation

owencraston
Copy link
Contributor

@owencraston owencraston commented May 22, 2024

Description

  1. What is the reason for the change?
    In an effort to make MetaMask multi chain we are migrating from an address based accounts to account id based account. In feat: Accounts controller integration #8759 previous PR we integrated the accounts controller which stores metadata like the account name, address and supported methods. This pr migrates all the use of selectIdentities to use the accounts controller as the source of truth for all accounts information (address, name etc).
  2. What is the improvement/solution?
  • find all use of selectIdentities and replace it with selectInternalAccounts and/or selectSelectedInternalAccountChecksummedAddress
  • These selectors are already in use in main and are defined here.
  • tests for these utils can be found here.
  • Note that ENS resolution in the send flow is not working but has been broken in production for a while. ENS resolution in the accounts list and dapp transactions are working as expected.

Related issues

Fixes: https://github.com/MetaMask/accounts-planning/issues/410

Q.A/Review check list

  • QA builds
  • Verify that there is no use of selectIdentities in the app
  • Verify that no functions are expecting identities as params

Manual testing steps

Fresh install

  1. Launch wallet
  2. create a new account
  3. once on the wallet view, add a new account
  4. create a custom name for your account(s)
  5. swipe away the app and re open
  6. login
  7. the selected account should be same one that was previously selected
  8. all addresses should be in checksum format
  9. connect the portfolio dapp via the portfolio button on the home screen.
  10. Click connect on the portfolio dapp
  11. The first address in the address picker should be match the selected address on the wallet view and be in checksum format

Upgrade path

  1. Use an existing wallet that has multiple accounts created/imported
  2. Change the names of your accounts so that they have custom names
  3. "upgrade" your wallet to this branch
  4. the selected account should be the same as it was on the previous branch
  5. all of the account data should remain the same (names and balances)

Send flow

  1. Start a send transaction
  2. Verify that the account names and addresses are consistent in the relevant screens of the flow

Hardware wallet connection and confirmation

  1. Connect a hardware wallet device
  2. Go to the MetaMask test dapp and try to connect an account
  3. Verify that the account names and addresses are consistent

Screenshots/Recordings

After

Basic usage

Untitled.mov

Hardware account name preserved when connecting and confirming with dapps

screen-20240703-155002.mp4

Complex usage

  • Ledger account connected
  • Imported account with ens (orangefox.eth)
  • custom account name
  • derived account
  • Tested....
    • send flow has the correct names ✅
    • wallet list has the correct names, even after a force reload (and correct selected account) ✅
    • dapp connection request has the correct names for all accounts including hardware and ENS account ✅
    • dapp transactions have correct names for all account types
Untitled.mp4

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.

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 base branch from main to feat/accounts-controller-selectors May 22, 2024 22:22
@owencraston owencraston force-pushed the feat/accounts-controller-selectors branch 6 times, most recently from 71a2b86 to ef5c955 Compare May 29, 2024 18:26
@github-actions github-actions bot locked and limited conversation to collaborators May 29, 2024
@owencraston owencraston reopened this May 29, 2024
@owencraston owencraston force-pushed the feat/accounts-controller-selectors branch from ef5c955 to 4a74a9a Compare May 30, 2024 20:26
@owencraston owencraston force-pushed the feat/accounts-controller-selectors branch 4 times, most recently from 64f888b to d6fca2b Compare June 6, 2024 05:32
@owencraston owencraston force-pushed the feat/internal-accounts branch 2 times, most recently from 89852ba to 6764d97 Compare June 6, 2024 07:05
@owencraston owencraston marked this pull request as ready for review June 6, 2024 07:07
@owencraston owencraston requested review from a team as code owners June 6, 2024 07:07
@owencraston owencraston added team-accounts needs-qa Any New Features that needs a full manual QA prior to being added to a release. Run Smoke E2E Triggers smoke e2e on Bitrise labels Jun 6, 2024
@github-actions github-actions bot unlocked this conversation Jun 6, 2024
Copy link
Contributor

github-actions bot commented Jun 6, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 6764d97
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/77d961a3-c226-43e1-a38b-ac2e942e1dbd

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

@owencraston owencraston added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 3, 2024
Copy link
Contributor

github-actions bot commented Jul 3, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 263856b
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/cd931912-b532-4bed-89fb-786ed6c249e1

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

Copy link

sonarcloud bot commented Jul 3, 2024

@plasmacorral
Copy link
Contributor

plasmacorral commented Jul 3, 2024

Accounts Tested:

  • 3 HD accounts (1 has ENS)
  • 2 imported accounts (1 has ENS)
  • 4 QR accounts (1 has ENS and another has custom name)
  • 1 Ledger account (has ENS)

Devices tested:

  • Samsung a515f running Android 12 with security 1/1/2024
  • Pixel 5a running Android 14 with security Apr 5, 2024
  • iPhone 13 mini running iOS 15.6.1
  • iPhone Xs running iOS 17.5.1

Hardware wallets tested:

  • LNX running 2.2.3 with Eth App 1.10.4
  • Keystone v3 with fw1.5.2

Successful tests:

  • Migration from QA build of v7.24.3 to QA build of commit 263856b
    • Accounts and Names persisted
  • Adding accounts
    • HD
    • Imported
    • Ledger
    • QR
  • Remove account/Forget device
    • Ledger
    • QR
    • Imported
  • Send and confirm screens in send flow
    • Imported and HD accounts display the ENS name in both the account list and send flows when they are the sender
  • Dapp connect accounts shows correct names
  • Lock/unlock

Observations:

ALL 4 OBSERVATIONS BELOW CAN BE REPRODUCED IN PRODUCTION AND ARE NOT INTRODUCED HERE

1. Send Flow: 4:42 in recording below
Reported in production issue 10257

  • Send to and Confirm screens do not display the ENS name of the recipient address, even when it is shown in the account list.
  • EDIT: I can confirm that I can reproduce this on a production build of v7.24.4

2. ENS Names on Hardware Addresses: 4:25 in recording below
Reported in production under 10256

  • Ledger and QR accounts that resolve to an ENS address do not have their account names updated in the account list. - I can confirm that I can reproduce this on a QA build of v7.24.3

3. ENS Name Mismatch (Related to Issue 2): 4:25 in recording below

  • For Ledger or QR accounts, if the address resolves to an ENS name and is the sender, the ENS name is shown in the Send to and Confirm screens, causing a mismatch with the account list where the account name is not updated.
  • EDIT: I can confirm that I can reproduce this on a production build of v7.24.4

4. ENS Resolution Delay: 5:33 in recording below
Reported in production under 10258

  • When switching between accounts with ENS names, the non-ENS account name is sometimes displayed before the ENS name is resolved. This may be a production issue that I am just noticing today.
  • EDIT: I can confirm that I can reproduce this on a production build of v7.24.4

Recording:

https://www.loom.com/share/75e854462d3748ceb065d83d860b15db?sid=cba2e632-bd39-4475-8df5-7d4b2b4beb1b

@plasmacorral plasmacorral added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed QA'd but questions A QA run through has been done but you need clarification on minor issues you found and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jul 3, 2024
@owencraston
Copy link
Contributor Author

@plasmacorral I went back to version 7.21.0 which was long before any accounts controller changes were added and noticed that the ens resolution in the send flow has been broken for some time.

screen-20240703-145605.1.mp4

@owencraston
Copy link
Contributor Author

owencraston commented Jul 3, 2024

Tests regarding [Bug] Hardware wallet account name not reflecting on signing transaction screen #10079

  • I have confirmed that this PR addresses this bug

Steps:
Connect QR/Ledger device on Metamask Mobile
Go to test dapp
Connect using hd wallet
Personal sign using hd wallet
Verify the sign transaction page displays
Expected: Hardware wallet account name to reflect with account list from homepage
Actual: Hardware wallet account name to reflect with account list from homepage

Before:

RPReplay_Final1719230238.MP4

After:

screen-20240703-154132.mp4

cc @vivek-consensys

@plasmacorral plasmacorral added QA Passed A successful QA run through has been done and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed QA'd but questions A QA run through has been done but you need clarification on minor issues you found labels Jul 3, 2024
@plasmacorral
Copy link
Contributor

plasmacorral commented Jul 3, 2024

These three issues could potentially be fixed by this PR:

These issues are resolved in this PR, and my observations above were also able to be reproduced in production v7.24.3 and 7.24.4.

Copy link
Member

@SamuelSalas SamuelSalas left a comment

Choose a reason for hiding this comment

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

LGTM

@owencraston owencraston merged commit 466f57c into main Jul 3, 2024
58 of 64 checks passed
@owencraston owencraston deleted the feat/internal-accounts branch July 3, 2024 23:33
@github-actions github-actions bot locked and limited conversation to collaborators Jul 3, 2024
@metamaskbot metamaskbot added release-7.27.1 Issue or pull request that will be included in release 7.27.1 release-7.26.1 Issue or pull request that will be included in release 7.26.1 and removed release-7.27.1 Issue or pull request that will be included in release 7.27.1 labels Jul 3, 2024
@metamaskbot
Copy link
Collaborator

Missing release label release-7.26.1 on PR. Adding release label release-7.26.1 on PR and removing other release labels(release-7.27.1), as PR was cherry-picked in branch 7.26.1.

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.26.1 Issue or pull request that will be included in release 7.26.1 Run Smoke E2E Triggers smoke e2e on Bitrise team-accounts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Hardware wallet account name not reflecting on signing transaction screen