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

Patch KeyringController to accept seedphrases passed as buffers/arrays of numbers #138

Merged
merged 2 commits into from
May 10, 2022

Conversation

adonesky1
Copy link
Contributor

@adonesky1 adonesky1 commented May 10, 2022

Patch KeyringController such that createNewVaultAndRestore accepts seedphrase arg as either type string or array of type number, and passes to addNewKeyring as a buffer.

Updates package to use @metamask version of bip39 that also accepts seedphrases passed as buffers to validateMnemonic.

metamask-extension already implements this patch here & here (to be removed when this is cut into a release and pulled into the extension)

@adonesky1 adonesky1 requested a review from a team as a code owner May 10, 2022 16:44
@adonesky1 adonesky1 requested review from Gudahtt and GuillaumeRx May 10, 2022 16:44
…edphrase as either type string or array of type number, and passes to addNewKeyring as a buffer.
@adonesky1 adonesky1 force-pushed the patch-keyring-controller branch from 2935bd7 to 8c52e14 Compare May 10, 2022 17:00
@@ -1,5 +1,6 @@
const { EventEmitter } = require('events');
const bip39 = require('bip39');
const { Buffer } = require('buffer');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly a bit confused why this is necessary... but it seems to be for lint? It's just a node method? cc @Gudahtt

Copy link
Member

Choose a reason for hiding this comment

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

What did the lint error say?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

103:11  error  'Buffer' is not defined  no-undef
104:11  error  'Buffer' is not defined  no-undef

Screen Shot 2022-05-10 at 1 17 31 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense to me as buffer is a package in the Node standard library.

Copy link
Member

Choose a reason for hiding this comment

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

In a Node.js context, Buffer is a global. ESLint is complaining here because we've declared the environment as being commonjs in the ESLint config, so it is not aware that the Node.js globals exist.

The globals are set correctly in our Node.js ESLint configuration, but that isn't being used by this library for some reason. So that would be another way to solve this. Not a blocker though, this seems OK for now.

Copy link

@GuillaumeRx GuillaumeRx left a comment

Choose a reason for hiding this comment

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

Can you add a test to be sure that it supports the array buffer type seedPhrase ?

@adonesky1
Copy link
Contributor Author

adonesky1 commented May 10, 2022

@GuillaumeRx added a test here: 8151216

@adonesky1 adonesky1 force-pushed the patch-keyring-controller branch from d7375d4 to 8151216 Compare May 10, 2022 17:36
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@adonesky1 adonesky1 merged commit e24c709 into main May 10, 2022
@adonesky1 adonesky1 deleted the patch-keyring-controller branch May 10, 2022 18:28
@adonesky1 adonesky1 mentioned this pull request May 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants