feat: multiple accounts support in ledger #10109

dawnseeker8 · 2024-06-25T12:25:46Z

Description

This PR will enable the multiple accounts supports for ledger devices.
Following changes has been made in this PR:

use @metamask/eth-ledger-keyring-bridge@4.0.0 to replace old @consensys/ledgerhq-metamask-keyring@0.0.9
add LedgerSelectAccount component to allow user select multiple accounts from ledger devices. (screen is similiar to QR code select account screen)
add remove accounts for all hardware wallet accounts in AccountActions.tsx file.
add some metric logging for remove accounts and connect accounts
modify the engine.ts code and ledger.ts to allow intialise the new ledger keyring and its middleware and transport object.
Modify the BlockingActionModel to support onAnimationCompleted event so that we can have better smooth model animation than before. (very lagging animation when some heavy operations like import multiple accounts happened in the background)

Related issues

Fixes:

Manual testing steps

Connect Multiple Ledger accounts

Launch wallet
Create a new account
Once on the wallet view, add a new account or hardware wallet
Add hardware wallet
Ledger
Once device is found, click Continue
Tick the selected Ledger accounts to be imported
Click Unlock

Forget Multiple Ledger accounts

Use an existing wallet that has multiple Ledger accounts imported
Once on the wallet view, add a new account or hardware wallet
Add hardware wallet
Ledger
Once device is found, click 'Continue'
Click 'Forget this device' button
User will be returned to account list with all Ledger accounts removed

Forget Individual Ledger Account

Use an existing wallet that has multiple Ledger accounts imported
Once on the wallet view, click on the three dots beside the Address section
Click 'Remove hardware account'
Click 'Remove'
User will be returned to account list with the individual Ledger account removed

Forget Individual QR Code Wallet Account

Use an existing wallet that has multiple QR Code accounts imported
Once on the wallet view, click on the three dots beside the Address section
Click 'Remove hardware account'
Click 'Remove'
User will be returned to account list with the individual QR Code account removed

Screenshots/Recordings

Before

After

Pre-merge author checklist

I’ve followed MetaMask Contributor Docs and MetaMask Mobile 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.

…the library with local change. 1. modify the Engine.ts to use new library 2. modify Ledger.ts to use new library will expect some errors during build, will fix this error and unit tests in next commit.

# Conflicts: # ios/MetaMask.xcodeproj/project.pbxproj

# Conflicts: # app/core/Engine.ts

…the need of release of library. also fix some issue of missing deviceId in library cause the bluetooth unable to connect.

… Still need to refactory the code to remove unused functions.

…ksum not match. also improve the unit tests for ledger.ts to cover all functions.

# Conflicts: # app/components/Nav/App/index.js # app/core/Engine.ts # ios/MetaMask.xcodeproj/project.pbxproj # package.json # yarn.lock

# Conflicts: # app/core/Engine.ts # app/core/Ledger/Ledger.test.ts # app/core/Ledger/Ledger.ts # patches/@MetaMask+keyring-controller+9.0.0.patch # yarn.lock

…ocken code. Enhance in BlockingActionModal with smooth animation to provide better user friendly interaction. add code to remove single accounts in actions. Still need to fix unit tests, however because MM mobile team will upgrade keyringController to 15, will start to fix unit tests after rerbase.

# Conflicts: # app/components/Views/ConnectHardware/SelectHardware/index.tsx # app/components/Views/LedgerAccountInfo/index.tsx # app/core/Ledger/Ledger.test.ts # app/core/Ledger/Ledger.ts # package.json # patches/@MetaMask+keyring-controller+13.0.0.patch # yarn.lock

This version will be first version which ledger work after keyring controller 16 upgrade.

dawnseeker8 · 2024-07-16T02:34:44Z

LGTM! I just left one final comment that was not resolved yet in the previous reviews

hi, @gantunesr i have done the change to AccountSelector component to make onCheck (toggleAccount) optional.

github-actions · 2024-07-16T02:37:32Z

Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: c1a24b2
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/83827214-d37b-4d3d-bf90-a0119c1a913c

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

github-actions · 2024-07-16T04:05:06Z

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 90d86ab
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/942d85f4-ec3c-4bab-936f-333db0f572cd

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

vivek-consensys · 2024-07-16T07:01:02Z

Flows tested on iPhone 13/iOS 17.5.1 and Samsung S21/Android 14 and screen recordings attached below:-

Connect Multiple Ledger accounts

Private Zenhub Video

Forget Multiple Ledger accounts

Private Zenhub Video

Forget Individual Ledger Account

Private Zenhub Video

Forget Individual QR Code Wallet Account

Private Zenhub Video

dawnseeker8 · 2024-07-16T08:39:22Z

Final e2e tests has passed
https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/942d85f4-ec3c-4bab-936f-333db0f572cd

…t` component to include a close icon, which will minic how the `ConnectQRHardware` component structure. hide the navigation header for `ledger Connect` screen.

github-actions · 2024-07-16T12:04:45Z

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: df0bd74
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/219562a6-6f21-4700-afbc-3a0502159d3e

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

sonarcloud · 2024-07-16T12:12:51Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
61.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

tommasini

LGTM!

dawnseeker8 · 2024-07-16T13:59:21Z

E2e smoke tests pass. https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/92c7ec10-e428-40d5-b64c-41cc42c0c577

dawnseeker8 added 30 commits April 8, 2024 09:01

feat: add library @metamask/eth-ledger-bridge-keyring, and the patch …

1ff5849

…the library with local change. 1. modify the Engine.ts to use new library 2. modify Ledger.ts to use new library will expect some errors during build, will fix this error and unit tests in next commit.

Merge branch 'main' into feat/ledger-multiple-accounts-support

15434b2

feat: make a backup of the change to switch to another branch.

c997d0c

Merge branch 'main' into feat/ledger-multiple-accounts-support

a83521d

# Conflicts: # ios/MetaMask.xcodeproj/project.pbxproj

Merge branch 'main' into feat/ledger-multiple-accounts-support

9be9588

# Conflicts: # ios/MetaMask.xcodeproj/project.pbxproj

fix: Downgrade the library

9e9e90b

Merge branch 'main' into feat/ledger-multiple-accounts-support

9ddf32c

Merge branch 'main' into feat/ledger-multiple-accounts-support

990e0b0

# Conflicts: # app/core/Engine.ts

fix: Rebase from main

d376691

feat: add package.tgz to project so that project can build without …

5eb781c

…the need of release of library. also fix some issue of missing deviceId in library cause the bluetooth unable to connect.

feat: revert the ledger.ts code in hardwareWallet folder.

ee7efd4

feat: workable version with add multiple accounts and forget devices.…

56f9229

… Still need to refactory the code to remove unused functions.

feat: workable version with add multiple accounts and forget devices.…

f22b625

… Still need to refactory the code to remove unused functions.

feat: Fix the unit tests in LedgerConnect

fe9a06a

feat: Fix the lint error and redundant imports

0cd5536

feat: Try to fix the hash unmatch issue in @metamask/eth-ledger-keyring

bd47961

feat: Fix the build issue after the package.tgz file change.

52b8967

feat: Try to fix the yarn setup integrity check failed because chec…

d5be2ce

…ksum not match. also improve the unit tests for ledger.ts to cover all functions.

feat: make a backup before carry out the rebase.

aa4f2ed

Merge branch 'main' into feat/ledger-multiple-accounts-support

6f7721c

# Conflicts: # app/components/Nav/App/index.js # app/core/Engine.ts # ios/MetaMask.xcodeproj/project.pbxproj # package.json # yarn.lock

feat: forget to push yarn.lock

dc848aa

feat: Fix the build issue after rebase.

aa461e2

feat: add remove hardware wallet from AccountActions.tsx

078cc28

feat: add loading mask when remove hardware account devices.

ebfae11

Merge branch 'main' into feat/ledger-multiple-accounts-support

5cf3c9d

# Conflicts: # app/core/Engine.ts # app/core/Ledger/Ledger.test.ts # app/core/Ledger/Ledger.ts # patches/@MetaMask+keyring-controller+9.0.0.patch # yarn.lock

fix: Backup new metric change.

a0c2b16

fix: Rebase from main to upgrade to keyring-controller 16.

f9941c0

This version will be first version which ledger work after keyring controller 16 upgrade.

feat: Add unit tests to cover all functions in ledger core.

192fdb4

dawnseeker8 requested a review from gantunesr July 16, 2024 02:31

dawnseeker8 added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 16, 2024

gantunesr previously approved these changes Jul 16, 2024

View reviewed changes

Merge branch 'main' into feat/ledger-multiple-accounts-support-rebase

90d86ab

dawnseeker8 added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 16, 2024

vivek-consensys added the QA Passed A successful QA run through has been done label Jul 16, 2024

dawnseeker8 added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 16, 2024

feat: Change the structure of LedgerSelectAccount and `LedgerConnec…

df0bd74

…t` component to include a close icon, which will minic how the `ConnectQRHardware` component structure. hide the navigation header for `ledger Connect` screen.

dawnseeker8 dismissed gantunesr’s stale review via df0bd74 July 16, 2024 12:00

dawnseeker8 added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jul 16, 2024

dawnseeker8 requested a review from gantunesr July 16, 2024 12:04

gantunesr approved these changes Jul 16, 2024

View reviewed changes

tommasini approved these changes Jul 16, 2024

View reviewed changes

dawnseeker8 merged commit 38cf842 into main Jul 16, 2024
37 of 38 checks passed

dawnseeker8 deleted the feat/ledger-multiple-accounts-support-rebase branch July 16, 2024 13:59

github-actions bot locked and limited conversation to collaborators Jul 16, 2024

github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jul 16, 2024

metamaskbot added the release-7.28.0 Issue or pull request that will be included in release 7.28.0 label Jul 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: multiple accounts support in ledger #10109

feat: multiple accounts support in ledger #10109

dawnseeker8 commented Jun 25, 2024 •

edited

Loading

dawnseeker8 commented Jul 16, 2024

github-actions bot commented Jul 16, 2024 •

edited by metamaskbot

Loading

github-actions bot commented Jul 16, 2024 •

edited by metamaskbot

Loading

vivek-consensys commented Jul 16, 2024 •

edited

Loading

dawnseeker8 commented Jul 16, 2024

github-actions bot commented Jul 16, 2024 •

edited by metamaskbot

Loading

sonarcloud bot commented Jul 16, 2024

tommasini left a comment

dawnseeker8 commented Jul 16, 2024

feat: multiple accounts support in ledger #10109

feat: multiple accounts support in ledger #10109

Conversation

dawnseeker8 commented Jun 25, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

dawnseeker8 commented Jul 16, 2024

github-actions bot commented Jul 16, 2024 • edited by metamaskbot Loading

Bitrise

github-actions bot commented Jul 16, 2024 • edited by metamaskbot Loading

Bitrise

vivek-consensys commented Jul 16, 2024 • edited Loading

dawnseeker8 commented Jul 16, 2024

github-actions bot commented Jul 16, 2024 • edited by metamaskbot Loading

Bitrise

sonarcloud bot commented Jul 16, 2024

Quality Gate passed

tommasini left a comment

Choose a reason for hiding this comment

dawnseeker8 commented Jul 16, 2024

dawnseeker8 commented Jun 25, 2024 •

edited

Loading

github-actions bot commented Jul 16, 2024 •

edited by metamaskbot

Loading

github-actions bot commented Jul 16, 2024 •

edited by metamaskbot

Loading

vivek-consensys commented Jul 16, 2024 •

edited

Loading

github-actions bot commented Jul 16, 2024 •

edited by metamaskbot

Loading