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.

Sign up for GitHub

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

Jump to bottom

[Bug]: Deleted accounts reappear #9560

Closed
plasmacorral opened this issue May 6, 2024 · 3 comments · Fixed by #9616
Closed

[Bug]: Deleted accounts reappear #9560

plasmacorral opened this issue May 6, 2024 · 3 comments · Fixed by #9616
Assignees
@dawnseeker8
Labels
area-hardware regression-prod-7.20.1 Regression bug that was found in production in release 7.20.1 release-7.24.0 Issue or pull request that will be included in release 7.24.0 Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking team-hardware-wallets type-bug Something isn't working

Comments

@plasmacorral
Copy link
Contributor

Describe the bug

Observed in while testing PR 9203 and confirmed to be present on production v7.20.1. After deleting a wallet with QR hardware present and creating a new one, prior QR accounts re-appear after setup.

Expected behavior

Accounts present prior to delete should not reappear after creating a new wallet.

Screenshots/Recordings

https://www.loom.com/share/c5c3d92db6da419eabf8aabbcb63afab?sid=d597eead-d642-4de1-bbbe-3476f1311728

Steps to reproduce

  1. Setup wallet with QR hardware accounts and using biometric auth
  2. Go to Settings>Security & Privacy
  3. Scroll all the way to the bottom and select Delete Wallet
  4. Tap I understand, continue
  5. input delete
  6. Tap Delete my wallet
  7. Tap Create new wallet
  8. Complete setup flow
  9. Kill the app from tray
  10. re-open and authenticate
  11. Tap accounts menu and note QR accounts have re-appeared

Error messages or log output

No response

Version

7.20.1

Build type

None

Device

Iphone Xs with iOS 17.4.1

Operating system

iOS

Additional context

Was testing side by side with Android devices, but this seems to be limited to just iOS.

Severity

No response
@plasmacorral plasmacorral added type-bug Something isn't working Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking area-hardware team-hardware-wallets labels May 6, 2024
@metamaskbot metamaskbot added the regression-prod-7.20.1 Regression bug that was found in production in release 7.20.1 label May 6, 2024
@angelcheung22
Copy link

@ccharly could you take a look at this one, as its related to the testing of #9202

@plasmacorral
Copy link
Contributor Author

@angelcheung22 while this was discovered in feature testing for PR 9303, once I confirmed this was present in our latest production version at the time 7.20.1 we can be confident that it was not introduced within that PR. My suspicion is that this may be related to some of the hardware changes within 7.19.0, but I have not had time to explore that theory. Could @vivek-consensys take a look?

cc @ccharly

@vivek-consensys
Copy link
Contributor

@angelcheung22 while this was discovered in feature testing for PR 9303, once I confirmed this was present in our latest production version at the time 7.20.1 we can be confident that it was not introduced within that PR. My suspicion is that this may be related to some of the hardware changes within 7.19.0, but I have not had time to explore that theory. Could @vivek-consensys take a look?

cc @ccharly

@plasmacorral I can confirm that I can reproduce this issue on the latest 7.22 RC3 build on TestFlight, the issue is only with the QR account as the Ledger account does not display when following the steps. @dawnseeker8 is looking into this issue.

@dawnseeker8 dawnseeker8 mentioned this issue May 13, 2024
Merged
7 tasks
dawnseeker8 added a commit that referenced this issue May 16, 2024
@dawnseeker8
fix: fix the issue 9560 which QR code accounts has been reappeared af…
afd4076 
…tter user `remove wallets` (#9616)

This PR will fix the issue #9560 which all QR code accounts have been
reappeared after user decide to `remove wallets` in metamask mobile.

The fix has been done in `Engine.ts` to make `qrKeyringBuilder` to reset
the qr keyring properties to default value once created a new
`QRHardwareKeyring`.

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

## **Related issues**

Fixes: #9560 

## **Manual testing steps**

1. Setup wallet with QR hardware accounts and using biometric auth
2. Go to Settings>Security & Privacy
3. Scroll all the way to the bottom and select `Delete Wallet`
4. Tap `I understand, continue`
5. input `delete`
6. Tap `Delete my wallet`
7. Tap `Create new wallet`
8. Complete setup flow
9. Tap accounts menu and all QR code accounts have been removed.
10. re-open and authenticate again.
11. Tap accounts menu in home page and confirm that those QR code
accounts haven't been reappeared again.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

https://www.loom.com/share/c5c3d92db6da419eabf8aabbcb63afab?sid=d597eead-d642-4de1-bbbe-3476f1311728

<!-- [screenshots/recordings] -->

### **After**
TODO 

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **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-7.24.0 Issue or pull request that will be included in release 7.24.0 label May 16, 2024
Andepande pushed a commit that referenced this issue May 21, 2024
@dawnseeker8 @Andepande
fix: fix the issue 9560 which QR code accounts has been reappeared af…
489abf3 
…tter user `remove wallets` (#9616)

This PR will fix the issue #9560 which all QR code accounts have been
reappeared after user decide to `remove wallets` in metamask mobile.

The fix has been done in `Engine.ts` to make `qrKeyringBuilder` to reset
the qr keyring properties to default value once created a new
`QRHardwareKeyring`.

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

## **Related issues**

Fixes: #9560 

## **Manual testing steps**

1. Setup wallet with QR hardware accounts and using biometric auth
2. Go to Settings>Security & Privacy
3. Scroll all the way to the bottom and select `Delete Wallet`
4. Tap `I understand, continue`
5. input `delete`
6. Tap `Delete my wallet`
7. Tap `Create new wallet`
8. Complete setup flow
9. Tap accounts menu and all QR code accounts have been removed.
10. re-open and authenticate again.
11. Tap accounts menu in home page and confirm that those QR code
accounts haven't been reappeared again.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

https://www.loom.com/share/c5c3d92db6da419eabf8aabbcb63afab?sid=d597eead-d642-4de1-bbbe-3476f1311728

<!-- [screenshots/recordings] -->

### **After**
TODO 

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dawnseeker8 dawnseeker8

Labels
area-hardware regression-prod-7.20.1 Regression bug that was found in production in release 7.20.1 release-7.24.0 Issue or pull request that will be included in release 7.24.0 Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking team-hardware-wallets type-bug Something isn't working
Projects
Bugs by severity
Archived in project
Milestone
No milestone
5 participants
@dawnseeker8 @plasmacorral @metamaskbot @angelcheung22 @vivek-consensys