fix: keyrings restore failure #5535
Merged
Conversation
This file contains 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
Gudahtt
reviewed
Mar 24, 2025
| return keyring; | ||
| } catch (_) { | ||
| } catch (error) { | ||
| console.error(error); |
There was a problem hiding this comment.
Ideally we'd capture this as a Sentry error as well, but that would be more work. I'm not even sure the best way to handle a case like this, where we want to continue the operation despite the error, but report the error later.
This is a good start, at least this will ensure we see it as a breadcrumb.
mikesposito
force-pushed
the
mikesposito/keyring-controller/restore-keyring
branch
from
39153e6 to
14479a4
Compare
mikesposito
changed the title
|
Looks great! CI is failing, I haven't looked at why yet. I'll take another look tomorrow morning |
|
There is no error being thrown when the controller is unlocked after being instantiated with too many metadata objects; Adding this check and eventually throwing the error should fix the new test case. Though, the error will also prevent the user from logging in |
mikesposito
commented
Mar 25, 2025
Comment on lines
+2669
to
+2670
| this.#password = currentPassword; | ||
| await this.#restoreSerializedKeyrings(currentSerializedKeyrings); |
There was a problem hiding this comment.
we must restore metadata first, or #restoreSerializedKeyrings will throw an error because it could find new metadata added by the failed operation
mikesposito
commented
Mar 25, 2025
mikesposito
commented
Mar 25, 2025
There was a problem hiding this comment.
Most of the adjusted tests needed these as initial parameters in order to correctly test a controller with no initial HD keyring and arbitrary initial keyrings:
{ skipVaultCreation: true, state: { vault: 'my vault' } }
Gudahtt
reviewed
Mar 25, 2025
| @@ -2155,6 +2155,10 @@ export class KeyringController extends BaseController< | |||
| for (const serializedKeyring of serializedKeyrings) { | |||
| await this.#restoreKeyring(serializedKeyring); | |||
| } | |||
|
|
|||
| if (this.#keyrings.length !== this.#keyringsMetadata.length) { | |||
There was a problem hiding this comment.
Interesting. Previously this was not checked until vault update, correct? So a mismatch here previously would allow the user to log in, but any actions triggering vault failure would fail.
With this check added here, we detect the corruption sooner. Maybe less risk of other operations failing in a confusing (potentially harmful) way, but has the downside of locking the user out completely if the failure occurs.
Probably this is a good call. We'll soon have the vault recovery flow as backup in case this does happen to a user somehow.
There was a problem hiding this comment.
Exactly! I found this issue when adding a test case for too many objects in metadata.
We'll soon have the vault recovery flow as backup in case this does happen to a user somehow
That would be awesome. My doubt was indeed because of locking out users, but I thought that letting them continue the execution would pose a greater risk
There was a problem hiding this comment.
Though, I can think of an additional problem that this will bring:
- I have a vault with an HD keyring and a keyring of type X
- Keyring X becomes unsupported (e.g. we drop support for it and remove it from the client keyring builders)
- My keyring X will end up in
#unsupportedKeyrings - I won't be able to unlock because I'll still have its metadata in
#keyringsMetadata, which is now out of sync with the number of supported keyrings
There was a problem hiding this comment.
Good point. We can make an issue for this, and treat it as a blocker for ever dropping support for a keyring (which maybe we'll never intentionally want to do anyway)
mikesposito
commented
Mar 25, 2025
Comment on lines
-263
to
-264
| describe('when the keyringMetadata length is different from the number of keyrings', () => { | ||
| it('should throw an error if the keyring metadata length mismatch', async () => { |
There was a problem hiding this comment.
This has been removed from addNewAccount test cases, because it should fail way earlier (i.e. when the controller is unlocked, as opposed to when adding an account)
|
@metamaskbot publish-preview |
Gudahtt
reviewed
Mar 25, 2025
| skipVaultCreation: true, | ||
| state: { | ||
| vault: JSON.stringify({ data: '0x123', salt: 'my salt' }), | ||
| // @ts-expect-error we want to force the controller to have an |
There was a problem hiding this comment.
Hmm. Strange that the constructor is typed to only allow a subset of state, seems like a mistake in the type definition. This shouldn't be an error.
Ah well, we can fix later.
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
montelaidev
reviewed
Mar 25, 2025
| // If metadata is missing, assume the data is from an installation before | ||
| // we had keyring metadata. | ||
| if (this.#keyringsMetadata.length < this.#keyrings.length) { |
There was a problem hiding this comment.
If the two arrays are equal, we shouldn't add a new keyring metadata.
There was a problem hiding this comment.
since we moved down the #keyrings.push instruction, we are assuming that we are about to add an element to the keyrings and hence the <=
montelaidev
approved these changes
Mar 25, 2025
github-merge-queue bot
pushed a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Mar 25, 2025
<!-- 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? --> `@metamask/keyring-controller` is being updated to 19.2.2 including the following changes: ```markdown ## [19.2.2] ### Fixed - Fixed duplication of unsupported keyrings ([#5535](MetaMask/core#5535)) - Enforce keyrings metadata alignment when unlocking existing vault ([#5535](MetaMask/core#5535)) - Fixed frozen object mutation attempt when updating metadata ([#5535](MetaMask/core#5535)) ## [19.2.1] ### Changed - Bump `@metamask/keyring-api"` from `^17.0.0` to `^17.2.0` ([#5366](MetaMask/core#5366)) - Bump `@metamask/keyring-internal-api` from `^4.0.1` to `^4.0.3` ([#5356](MetaMask/core#5356)), ([#5366](MetaMask/core#5366)) ### Fixed - Ensure authorization contract address is provided ([#5353](MetaMask/core#5353)) ``` ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] 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** - [ ] 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.
mikesposito
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Mar 25, 2025
<!-- 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? --> Bumping `@metamask/keyring-controller` to 19.2.2 ```markdown ## [19.2.2] ### Fixed - Fixed duplication of unsupported keyrings ([#5535](MetaMask/core#5535)) - Enforce keyrings metadata alignment when unlocking existing vault ([#5535](MetaMask/core#5535)) - Fixed frozen object mutation attempt when updating metadata ([#5535](MetaMask/core#5535)) ## [19.2.1] ### Changed - Bump `@metamask/keyring-api"` from `^17.0.0` to `^17.2.0` ([#5366](MetaMask/core#5366)) - Bump `@metamask/keyring-internal-api` from `^4.0.1` to `^4.0.3` ([#5356](MetaMask/core#5356)), ([#5366](MetaMask/core#5366)) ### Fixed - Ensure authorization contract address is provided ([#5353](MetaMask/core#5353)) ``` [](https://codespaces.new/MetaMask/metamask-extension/pull/31293?quickstart=1) ## **Related issues** Related: #31200 ## **Manual testing steps** 1. Start with an installation from before we introduced keyringsMetadata, proceed past onboarding so that you have a vault setup (e.g. v12.12.0) 2. Upgrade to this branch 3. Start the extension (without logging in) 4. Leave the window open a while to give it time to initialize all controllers 5. Reload the extension before unlocking 6. Try to unlock the wallet ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **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. --------- Co-authored-by: MetaMask Bot <[email protected]>
github-merge-queue bot
pushed a commit
to MetaMask/metamask-extension
that referenced
this pull request
Mar 25, 2025
<!-- 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? --> Bumping `@metamask/keyring-controller` to ^21.0.1 ```markdown ## [21.0.1] ### Fixed - Fixed duplication of unsupported keyrings ([#5535](MetaMask/core#5535)) - Enforce keyrings metadata alignment when unlocking existing vault ([#5535](MetaMask/core#5535)) - Fixed frozen object mutation attempt when updating metadata ([#5535](MetaMask/core#5535)) ``` [](https://codespaces.new/MetaMask/metamask-extension/pull/31293?quickstart=1) ## **Related issues** Fixes: #31200 ## **Manual testing steps** 1. Start with an installation from before we introduced keyringsMetadata, proceed past onboarding so that you have a vault setup (e.g. v12.12.0) 2. Upgrade to this branch 3. Start the extension (without logging in) 4. Leave the window open a while to give it time to initialize all controllers 5. Reload the extension before unlocking 6. Try to unlock the wallet ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **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.
runway-github bot
added a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Mar 25, 2025
<!-- 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? --> `@metamask/keyring-controller` is being updated to 19.2.2 including the following changes: ```markdown ## [19.2.2] ### Fixed - Fixed duplication of unsupported keyrings ([#5535](MetaMask/core#5535)) - Enforce keyrings metadata alignment when unlocking existing vault ([#5535](MetaMask/core#5535)) - Fixed frozen object mutation attempt when updating metadata ([#5535](MetaMask/core#5535)) ## [19.2.1] ### Changed - Bump `@metamask/keyring-api"` from `^17.0.0` to `^17.2.0` ([#5366](MetaMask/core#5366)) - Bump `@metamask/keyring-internal-api` from `^4.0.1` to `^4.0.3` ([#5356](MetaMask/core#5356)), ([#5366](MetaMask/core#5366)) ### Fixed - Ensure authorization contract address is provided ([#5353](MetaMask/core#5353)) ``` ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] 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** - [ ] 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.
runway-github bot
added a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Mar 25, 2025
<!-- 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? --> `@metamask/keyring-controller` is being updated to 19.2.2 including the following changes: ```markdown ## [19.2.2] ### Fixed - Fixed duplication of unsupported keyrings ([#5535](MetaMask/core#5535)) - Enforce keyrings metadata alignment when unlocking existing vault ([#5535](MetaMask/core#5535)) - Fixed frozen object mutation attempt when updating metadata ([#5535](MetaMask/core#5535)) ## [19.2.1] ### Changed - Bump `@metamask/keyring-api"` from `^17.0.0` to `^17.2.0` ([#5366](MetaMask/core#5366)) - Bump `@metamask/keyring-internal-api` from `^4.0.1` to `^4.0.3` ([#5356](MetaMask/core#5356)), ([#5366](MetaMask/core#5366)) ### Fixed - Ensure authorization contract address is provided ([#5353](MetaMask/core#5353)) ``` ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] 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** - [ ] 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.
runway-github bot
pushed a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Mar 25, 2025
…o `^19.2.2` (#14229) <!-- 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? --> `@metamask/keyring-controller` is being updated to 19.2.2 including the following changes: ```markdown ## [19.2.2] ### Fixed - Fixed duplication of unsupported keyrings ([#5535](MetaMask/core#5535)) - Enforce keyrings metadata alignment when unlocking existing vault ([#5535](MetaMask/core#5535)) - Fixed frozen object mutation attempt when updating metadata ([#5535](MetaMask/core#5535)) ## [19.2.1] ### Changed - Bump `@metamask/keyring-api"` from `^17.0.0` to `^17.2.0` ([#5366](MetaMask/core#5366)) - Bump `@metamask/keyring-internal-api` from `^4.0.1` to `^4.0.3` ([#5356](MetaMask/core#5356)), ([#5366](MetaMask/core#5366)) ### Fixed - Ensure authorization contract address is provided ([#5353](MetaMask/core#5353)) ``` ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] 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** - [ ] 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.
MarioAslau
pushed a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Mar 25, 2025
…o `^19.2.2` (#14236) - fix: bump `@metamask/keyring-controller` to `^19.2.2` (#14229) <!-- 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? --> `@metamask/keyring-controller` is being updated to 19.2.2 including the following changes: ```markdown ## [19.2.2] ### Fixed - Fixed duplication of unsupported keyrings ([#5535](MetaMask/core#5535)) - Enforce keyrings metadata alignment when unlocking existing vault ([#5535](MetaMask/core#5535)) - Fixed frozen object mutation attempt when updating metadata ([#5535](MetaMask/core#5535)) ## [19.2.1] ### Changed - Bump `@metamask/keyring-api"` from `^17.0.0` to `^17.2.0` ([#5366](MetaMask/core#5366)) - Bump `@metamask/keyring-internal-api` from `^4.0.1` to `^4.0.3` ([#5356](MetaMask/core#5356)), ([#5366](MetaMask/core#5366)) ### Fixed - Ensure authorization contract address is provided ([#5353](MetaMask/core#5353)) ``` ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] 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** - [ ] 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. [99faccf](99faccf) Co-authored-by: Michele Esposito <[email protected]>
dbrans
pushed a commit
to MetaMask/metamask-extension
that referenced
this pull request
Apr 3, 2025
](https://codespaces.new/MetaMask/metamask-extension/pull/31293?quickstart=1){kind=link}
<!-- 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. --> <!-- 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? --> Bumping `@metamask/keyring-controller` to ^21.0.1 ```markdown - Fixed duplication of unsupported keyrings ([#5535](MetaMask/core#5535)) - Enforce keyrings metadata alignment when unlocking existing vault ([#5535](MetaMask/core#5535)) - Fixed frozen object mutation attempt when updating metadata ([#5535](MetaMask/core#5535)) ``` [](https://codespaces.new/MetaMask/metamask-extension/pull/31293?quickstart=1) Fixes: #31200 1. Start with an installation from before we introduced keyringsMetadata, proceed past onboarding so that you have a vault setup (e.g. v12.12.0) 2. Upgrade to this branch 3. Start the extension (without logging in) 4. Leave the window open a while to give it time to initialize all controllers 5. Reload the extension before unlocking 6. Try to unlock the wallet <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> <!-- [screenshots/recordings] --> <!-- [screenshots/recordings] --> - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [ ] 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.
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
KeyringController uses the
#unsupportedKeyringsarray to momentarily store keyrings that failed during restore/deserialization (the process of extracting a serialized keyring from the encrypted vault and deserializing it into a Keyring instance).However, if the failure happens due to this line, the affected keyring can end up being both in the standard
#keyringsarray and the#unsupportedKeyringsone. This leads to subsequent duplication of keyrings when updating the vault, since the keyring would be in both the arrays.The above-linked line fails because of this constructor line, as we are assigning a frozen object to
#keyringsMetadataReferences
Changelog
@metamask/keyring-controller#keyringsMetadatawith state property when unlockingChecklist