Skip to content
This repository was archived by the owner on Oct 7, 2024. It is now read-only.

Fix: saving serialized keyring for which corresponding keyring class is not present - #169

Merged
jpuri merged 14 commits into
mainfrom
restore_keyring_change
Nov 30, 2022
Merged

Fix: saving serialized keyring for which corresponding keyring class is not present#169
jpuri merged 14 commits into
mainfrom
restore_keyring_change

Conversation

@jpuri

@jpurijpuri commented Nov 25, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes: MetaMask/metamask-extension#16674

For MV3 currently we are not supporting hardware wallets, this can result in error being thrown if Keyring classes of hardware wallets is not passed to Keyring Controller.

PR fixes the error by adding null check. Keyring for which corresponding keyring class is not present is saved in state and then re-serialized.

@jpurijpuri mentioned this pull request Nov 25, 2022
Closed
@jpuri
jpuri marked this pull request as ready for review November 28, 2022 10:10
@jpuri
jpuri requested a review from a team as a code ownerNovember 28, 2022 10:11
@jpuri
jpuri requested review from Gudahtt and segunNovember 28, 2022 10:22
@segunsegun mentioned this pull request Nov 29, 2022
Closed
8 tasks
@jpurijpuri changed the title Fix: adding null check to avoid error if KeyringClass for a keyring type is not presentFix: saving serialized keyring for which corresponding keyring class is not presentNov 29, 2022
mcmire
mcmire reviewed Nov 29, 2022
View reviewed changes

@mcmiremcmire left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jpuri it's possible I'm missing some context but I had a question about how the new property being added here would be used.

Comment threadindex.js Outdated
Comment threadindex.js Outdated
Comment threadindex.js
Comment threadtest/index.js Outdated
});

describe('persistAllKeyrings', function () {
it('should persist keyrings in unsupported_keyrings array', async function () {

@mcmiremcmireNov 29, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we putting all of the keyrings in unsupported_keyrings when persistAllKeyrings is called? It makes sense that unsupported_keyrings would hold all keyrings that we can't find a Keyring class for, but that doesn't seem to be what this is doing. So I'm a bit confused as to how unsupported_keyrings would end up getting used.

@GudahttGudahttNov 30, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

persistAllKeyrings is reading from unsupported_keyrings, not writing to it. We're persisting the unsupported keyrings so that they remain in the users vault, in case they need to manually extract them, or in case we add back support for that keyring.

I can see how this description could be read either way. It doesn't mean the keyrings are being persisted to that array, just that that array is included in the set of data being persisted.

Comment threadtest/index.js Outdated
digiwand
digiwand reviewed Nov 29, 2022
View reviewed changes
Comment threadindex.js
jpuriand others added 5 commits November 30, 2022 14:57
@jpuri@mcmire
Update index.js
3003ebc
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
@jpuri@mcmire
Update test/index.js
0bc0bc5
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
@jpuri
fix
14416d9
@jpuri
fix
e0b7b80
@jpuri
fix
4927bbc
@jpuri
jpuri requested review from digiwand and mcmireNovember 30, 2022 09:45
Gudahtt
Gudahtt previously approved these changes Nov 30, 2022
View reviewed changes

@GudahttGudahtt left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I agree with @mcmire that we should have a test for restoreKeyring, and I've left a few more suggestions on improving the tests. But I didn't want to block on those; the tests here already aren't in great shape. The basic functionality is at least covered.

Comment threadindex.js Outdated
Comment threadtest/index.js Outdated
describe('persistAllKeyrings', function () {
it('should persist keyrings in unsupportedKeyrings array', async function () {
const unsupportedKeyring = 'DUMMY_KEYRING';
keyringController.unsupportedKeyrings = [unsupportedKeyring];

@GudahttGudahttNov 30, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It would be better for this test to set the keyrings in the same way the extension would. We wouldn't ever use the keyring controller this way, setting the unsupportedKeyrings property directly.

Unfortunately the way these tests are setup makes that difficult. The extension passes in existing vaults via the constructor, but these tests all construct the controller the same way in beforeEach. This is why we've been recommending that people avoid using beforeEach, and use setup functions instead. Then we can create new setup functions tailored for specific tests. Improving this test in the way I'm suggesting here might require a similar refactor of the other tests first.

Comment threadtest/index.js
segun
segun previously approved these changes Nov 30, 2022
View reviewed changes
@jpuri@Gudahtt
Update test/index.js
9b33800
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
@jpuri
jpuri dismissed stale reviews from segun and Gudahtt via 9b33800November 30, 2022 14:51
digiwand
digiwand reviewed Nov 30, 2022
View reviewed changes
Comment threadtest/index.js Outdated
jpuriand others added 3 commits November 30, 2022 20:31
@jpuri
fixes
f1e7a62
@jpuri@digiwand
Update test/index.js
9d0316d
Co-authored-by: Ariella Vu <20778143+digiwand@users.noreply.github.com>
@jpuri
merge
4769865
@jpuri
jpuri merged commit 46d8e17 into mainNov 30, 2022
@jpuri
jpuri deleted the restore_keyring_change branch November 30, 2022 15:13
digiwand
digiwand reviewed Nov 30, 2022
View reviewed changes
Comment threadindex.js
const Keyring = this.getKeyringClassForType(type);
if (!Keyring) {
this._unsupportedKeyrings.push(serialized);
return undefined;

@digiwanddigiwandNov 30, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @jpuri, late question: when we support hard wallets for MV3, will this logic added in this PR still be needed?

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mcmiremcmireAwaiting requested review from mcmire
@segunsegunAwaiting requested review from segun
2 more reviewers
@digiwanddigiwanddigiwand left review comments
@GudahttGudahttGudahtt approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: login fails when moving from mv2 to mv3

5 participants

@jpuri@mcmire@segun@Gudahtt@digiwand