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

[Bug]: Forget this device removes account from keyring, but not the UI #21742

Closed
plasmacorral opened this issue Nov 8, 2023 · 0 comments · Fixed by #21755
Closed

[Bug]: Forget this device removes account from keyring, but not the UI #21742

plasmacorral opened this issue Nov 8, 2023 · 0 comments · Fixed by #21755
Assignees
Labels
area-hardware hardware-ledger hardware-qr release-11.6.0 Issue or pull request that will be included in release 11.6.0 Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. team-accounts type-bug

Comments

@plasmacorral
Copy link
Contributor

plasmacorral commented Nov 8, 2023

Describe the bug

When I have a hardware wallet present, and I go thru the add new hardware flow to select Forget device the account remains in the drop-down list.

In the branch I was testing at the time, I found that the accounts cannot be subsequently removed as they no longer exist in the keyring after the device was forgotten. This has been observed on both Keystone and Ledger within the changes from PR 21437, and likely impacts both Trezor and Lattice. Was able to confirm that this behavior is present in production 11.4.0 and 11.4.1.

Note that starting with step 11 below, the inability to remove the account is limited to the branch under test. In 11.4.0 I can still remove an address from the list after I try to forget the device (but should not actually have to). However, that is not the case in the changes that were being tested and I am unable to remove the addresses.

Expected behavior

When the user selects Forget this device any associated accounts should be removed from the keyring AND no longer displayed in the accounts list.

User should be able to remove any accounts that are not derived from the SRP.

Screenshots/Recordings

11.4.0: ABLE to remove individual accounts after Forget this device fails: https://recordit.co/BV2omOAjFk

PR 21437 commit 141b1ef: UNABLE to remove individual accounts after Forget this device fails: https://recordit.co/qhg25TCZy9

Steps to reproduce

  1. Have hardware addresses already added in a configured wallet
  2. Tap accounts drop down
  3. tap Add account of hardware wallet
  4. tap Add hardware wallet
  5. Select the hardware wallet you are testing with
  6. If Ledger- you may need to tap Connect on HID device connection prompt
  7. Tap Continue
  8. Once account list is displayed tap Forget this device
  9. In the branch under test the address list page will re-appear with a notification that hardware is connected. If so, go ahead and tap Forget this device a second time.
  10. Tap the accounts drop down and note that hardware address that should be forgotten is displayed in the list
  11. Hit the kebab next to a given hardware account
  12. tap remove account
  13. Tap Remove
  14. Note that dialog remains open and account is not removed

Error messages or log output

KeyringController - No keyring found. Error info: There are keyrings, but none match the address

Version

11.4.0

Build type

None

Browser

Chrome

Operating system

MacOS

Hardware wallet

Ledger, Keystone

Additional context

Can remove individual addresses as long as the user does not first try to forget the device.

Severity

Would be incredibly frustrating to land in a spot where accounts cannot be removed that do not exist in the keyring.

@plasmacorral plasmacorral added type-bug Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. team-accounts labels Nov 8, 2023
@metamaskbot metamaskbot added the regression-prod-11.4.0 Regression bug that was found in production in release 11.4.0 label Nov 8, 2023
@plasmacorral plasmacorral added area-hardware hardware-ledger hardware-qr and removed regression-prod-11.4.0 Regression bug that was found in production in release 11.4.0 labels Nov 8, 2023
plasmacorral pushed a commit that referenced this issue Nov 9, 2023
…the device (#21755)

## **Description**

This pr fixes the issue where the identities of a hardware wallet were
not removed when removing the device.

## **Related issues**

Fixes: #21742

## **Manual testing steps**

1. Connect a ledger
2. Add a ledger account
3. Go to connect a ledger again 
4. Click on forget device
5. Check to see the ledger account was removed from the UI

## **Screenshots/Recordings**

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [x] In case it's not yet "ready for review", I've set it to "draft".
- [x] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **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.
@metamaskbot metamaskbot added the release-11.6.0 Issue or pull request that will be included in release 11.6.0 label Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-hardware hardware-ledger hardware-qr release-11.6.0 Issue or pull request that will be included in release 11.6.0 Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. team-accounts type-bug
Projects
None yet
3 participants