fix: throw error when creating HD keyring with duplicate account #5675
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PatrykLucka
force-pushed
the
fix-duplicate-hd-keyring-account
branch
from
f289dfe to
3d7a30d
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
PatrykLucka
changed the title
ccharly
reviewed
Apr 18, 2025
Co-authored-by: Charly Chevalier <[email protected]>
ccharly
reviewed
Apr 18, 2025
| @@ -2587,7 +2587,8 @@ export class KeyringController extends BaseController< | |||
| const accounts = await this.#getAccountsFromKeyrings(); | |||
|
|
|||
| switch (type) { | |||
| case KeyringTypes.simple: { | |||
| case KeyringTypes.simple: | |||
| case KeyringTypes.hd: { | |||
There was a problem hiding this comment.
Just for the record:
- Given that we only check for duplicates when creating new keyrings, this shouldn't impact the "manual" HD keyring account creation.
- For multi-SRP, this will prevent duplicated addresses when importing a new SRP (and thus, creating a new HD keyring).
ccharly
changed the title
ccharly
approved these changes
Apr 18, 2025
mikesposito
reviewed
Apr 24, 2025
| @@ -2587,7 +2587,8 @@ export class KeyringController extends BaseController< | |||
| const accounts = await this.#getAccountsFromKeyrings(); | |||
|
|
|||
| switch (type) { | |||
| case KeyringTypes.simple: { | |||
| case KeyringTypes.simple: | |||
| case KeyringTypes.hd: { | |||
There was a problem hiding this comment.
It looks like we are leaving account additions unchecked, which may be also problematic because if a user:
- Imports an SRP
- Imports a private key, part of the above SRP
- Adds an account until reaching the index of the imported private key
User will endup in the same faulty situation
There was a problem hiding this comment.
The reason I'm flagging this is that it is the fastest way to brick the wallet: #5701
mikesposito
reviewed
Apr 24, 2025
There was a problem hiding this comment.
This can mitigate some issues for HD specifically. Is there a reason we are addressing those specifically instead of the account duplication for all types? I feel like we should find a solution that can work for all keyrings or we won't be able to really fix the underlying issues (i.e. hardware wallet accounts, snaps)
|
I opened an alternative PR that could avoid different cases of duplicates and fix users that are currently locked out: #5710 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
Currently, we are not checking whether the HD keyring account being added by importing SRP is a duplicate. This creates the possibility of adding the same account first to the simple keyring and then again with import HD keyring.
This PR adds a duplicate account check when adding an HD keyring, preventing this situation.
References
Changelog
Checklist