metamask-mobile icon indicating copy to clipboard operation
metamask-mobile copied to clipboard

Published 20 hours ago • verified publisher icon MetaMask

feat: multiple accounts support in ledger

Open dawnseeker8 opened this issue 1 year ago • 5 comments
trafficstars

Description

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

  1. use @metamask/[email protected] to replace old @consensys/[email protected]
  2. add LedgerSelectAccount component to allow user select multiple accounts from ledger devices. (screen is similiar to QR code select account screen)
  3. add remove accounts for all hardware wallet accounts in AccountActions.tsx file.
  4. add some metric logging for remove accounts and connect accounts
  5. modify the engine.ts code and ledger.ts to allow intialise the new ledger keyring and its middleware and transport object.
  6. 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

  1. Launch wallet
  2. Create a new account
  3. Once on the wallet view, add a new account or hardware wallet
  4. Add hardware wallet
  5. Ledger
  6. Once device is found, click Continue
  7. Tick the selected Ledger accounts to be imported
  8. Click Unlock

Forget Multiple Ledger accounts

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

Forget Individual Ledger Account

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

Forget Individual QR Code Wallet Account

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

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • [x] 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.

dawnseeker8 avatar Jun 25 '24 12:06 dawnseeker8

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.

github-actions[bot] avatar Jun 25 '24 12:06 github-actions[bot]

Codecov Report

Attention: Patch coverage is 60.28369% with 56 lines in your changes missing coverage. Please review.

Project coverage is 49.78%. Comparing base (b2cce87) to head (7c77ce1). Report is 9 commits behind head on main.

Files Patch % Lines
app/components/Views/LedgerSelectAccount/index.tsx 41.66% 34 Missing and 1 partial :warning:
...components/Views/AccountActions/AccountActions.tsx 60.86% 12 Missing and 6 partials :warning:
...nts/Views/ConnectHardware/SelectHardware/index.tsx 0.00% 1 Missing :warning:
...mponents/Views/LedgerSelectAccount/index.styles.ts 66.66% 0 Missing and 1 partial :warning:
app/components/hooks/Ledger/useLedgerBluetooth.ts 0.00% 1 Missing :warning:
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #10109      +/-   ##
==========================================
+ Coverage   49.67%   49.78%   +0.10%     
==========================================
  Files        1450     1453       +3     
  Lines       34888    34997     +109     
  Branches     3950     3968      +18     
==========================================
+ Hits        17330    17422      +92     
  Misses      16456    16456              
- Partials     1102     1119      +17

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jun 26 '24 06:06 codecov-commenter

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 449fab396733ebcc44add614461ab41bd636381a Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/e6045017-3886-4834-8e99-f92887f5c3fb

[!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[bot] avatar Jun 26 '24 07:06 github-actions[bot]

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: a710a749883a7f214dd90844df577b4e7c2beaa2 Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/e2b14473-dda4-49b7-abbb-ddd8db777946

[!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[bot] avatar Jun 28 '24 07:06 github-actions[bot]

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 073260cecfcda427a635efc588716dbef85cd850 Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/0d70df48-4213-4fcf-9603-908cf5bce2a4

[!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[bot] avatar Jul 01 '24 08:07 github-actions[bot]

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@metamask/[email protected] None +6 7.26 MB metamaskbot
npm/[email protected] None +2 24.7 kB ryanzim

🚮 Removed packages: npm/@consensys/[email protected], npm/@ledgerhq/[email protected]

View full report↗︎

socket-security[bot] avatar Jul 02 '24 08:07 socket-security[bot]

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: ef50cb922f03c5e5a31f4ff0b31e380862a14bec Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/e5027c93-e01e-4fa3-b515-500b1dd36437

[!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[bot] avatar Jul 02 '24 11:07 github-actions[bot]

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: e3e46f9baf0ec084539ee4a5b60200814ce3fd8e Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/50992526-d8d6-42d4-9b5f-09315c8c5c90

[!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[bot] avatar Jul 05 '24 07:07 github-actions[bot]

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 9982b72bced84ba45aa88d14d9b568329c3af6c3 Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/1103934e-a395-4d2c-8141-a0f7fcfd7afd

[!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[bot] avatar Jul 08 '24 09:07 github-actions[bot]

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 2276ce7f8e15ba3504c3a857618f409949931719 Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c6b59eaa-aacd-4177-acbb-acac8342db20

[!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[bot] avatar Jul 08 '24 16:07 github-actions[bot]

Final e2e tests are all green even Bitrise E2E Status display failure. https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c6b59eaa-aacd-4177-acbb-acac8342db20

dawnseeker8 avatar Jul 09 '24 04:07 dawnseeker8

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 368de0f427f19528a0f5f7f634adadf76be61a85 Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/49117b82-f925-4015-b3d0-21bd5bffb841

[!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[bot] avatar Jul 12 '24 17:07 github-actions[bot]

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: c355be4f4b8390dd7eb350dd029f089b1c0b329d Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/4e5f48d7-a226-4fca-b1f6-987e5b5804fa

[!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[bot] avatar Jul 15 '24 06:07 github-actions[bot]

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: fe4d92e4027157610dd9fa88c4478fe9a785a5cb Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/0d27c56a-e5f3-4ba6-906e-b1f4fd4dd4b2

[!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[bot] avatar Jul 15 '24 11:07 github-actions[bot]

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.

dawnseeker8 avatar Jul 16 '24 02:07 dawnseeker8

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: c1a24b2c2ac04ea02690a271d6e5d9823d17acfd 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[bot] avatar Jul 16 '24 02:07 github-actions[bot]

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 90d86ab2970eb86ae20474abc1523437e260b555 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

github-actions[bot] avatar Jul 16 '24 04:07 github-actions[bot]

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

vivek-consensys avatar Jul 16 '24 07:07 vivek-consensys

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

dawnseeker8 avatar Jul 16 '24 08:07 dawnseeker8

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: df0bd74d77787f392cf737bb4fc9e86f434c5972 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

github-actions[bot] avatar Jul 16 '24 12:07 github-actions[bot]

Quality Gate Passed 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

sonarqubecloud[bot] avatar Jul 16 '24 12:07 sonarqubecloud[bot]

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

dawnseeker8 avatar Jul 16 '24 13:07 dawnseeker8