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

fix(11215): show error message when user selects a new ledger device #12127

Merged
merged 4 commits into from
Nov 20, 2024

Conversation

Akaryatrh
Copy link
Contributor

@Akaryatrh Akaryatrh commented Oct 31, 2024

Description

As discussed with @AlexJupiter, to prevent users to add multiple ledger devices, we decided to show an error message when deviceId would not match with the device already added.
This is a temporary solution until proper mutliple devices support is implemented.

Related issues

Fixes: #11215

Manual testing steps

⚠️ These steps require at least two different ledger devices

  1. Add a new hardware account, select Ledger device n°1 and import an account
  2. Add a new hardware account, select Ledger device n°2
  3. See that error message is displayed
  4. Select Ledger device n°1, see that error message is not displayed

Screenshots/Recordings

Before

After

video_2024-11-04_14-29-58.mp4

Pre-merge author checklist

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.

@angelcheung22 angelcheung22 added the needs-content Needs content / copy support. label Nov 4, 2024
@Akaryatrh Akaryatrh added the ready-for-translation This is used in a github action to trigger a branch as ready for translation team to pick up label Nov 4, 2024
@Akaryatrh Akaryatrh marked this pull request as ready for review November 4, 2024 13:39
@Akaryatrh Akaryatrh requested a review from a team as a code owner November 4, 2024 13:39
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 56.20%. Comparing base (9f29292) to head (0c8ff66).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
app/components/Views/LedgerConnect/index.tsx 84.61% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12127      +/-   ##
==========================================
+ Coverage   55.78%   56.20%   +0.42%     
==========================================
  Files        1786     1786              
  Lines       40362    40373      +11     
  Branches     5054     5058       +4     
==========================================
+ Hits        22516    22693     +177     
+ Misses      16297    16126     -171     
- Partials     1549     1554       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vivek-consensys vivek-consensys added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Nov 6, 2024
@vivek-consensys vivek-consensys added the QA in Progress QA has started on the feature. label Nov 6, 2024
github-merge-queue bot pushed a commit to MetaMask/accounts that referenced this pull request Nov 6, 2024
…orgetting a device (#86)

<!--
Thanks for your contribution! Take a moment to answer these questions so
that reviewers have the information they need to properly understand
your changes:

* What is the current state of things and why does it need to change?
* What is the solution your changes offer and how does it work?

Are there any issues or other links reviewers should consult to
understand this pull request better? For instance:

* Fixes #12345
* See: #67890
-->

Currently when we call forgetDevice function from ledger keyring, it
doesn't clear up the stored `deviceId`. This could be a blocker when
checking for existing stored `deviceId` client side.

Then, this will unblock a [fix for multiple devices
handling](MetaMask/metamask-mobile#12127) as [an
issue has been found during QA
testing](MetaMask/metamask-mobile#11215 (comment))
related to `deviceId`.
@angelcheung22 angelcheung22 removed the needs-content Needs content / copy support. label Nov 8, 2024
Copy link

sonarcloud bot commented Nov 13, 2024

@vivek-consensys
Copy link
Contributor

Tested using bitrise build on iOS 18.0.1/Android 14 and error is displayed correctly.

ScreenRecording_11-13-2024.16-02-53_1.MP4

@vivek-consensys vivek-consensys added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Nov 13, 2024
@Akaryatrh Akaryatrh added this pull request to the merge queue Nov 20, 2024
Merged via the queue into main with commit e186299 Nov 20, 2024
42 of 43 checks passed
@Akaryatrh Akaryatrh deleted the fix/11215 branch November 20, 2024 13:20
@github-actions github-actions bot locked and limited conversation to collaborators Nov 20, 2024
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Nov 20, 2024
@metamaskbot metamaskbot added the release-7.37.0 Issue or pull request that will be included in release 7.37.0 label Nov 20, 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 ready-for-translation This is used in a github action to trigger a branch as ready for translation team to pick up release-7.37.0 Issue or pull request that will be included in release 7.37.0 team-hardware-wallets
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Uniswap transaction stuck - seems to be multiple ledger devices issue
6 participants