- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 129
 
Patch KeyringController to accept seedphrases passed as buffers/arrays of numbers #138
Conversation
…edphrase as either type string or array of type number, and passes to addNewKeyring as a buffer.
2935bd7    to
    8c52e14      
    Compare
  
    | @@ -1,5 +1,6 @@ | |||
| const { EventEmitter } = require('events'); | |||
| const bip39 = require('bip39'); | |||
| const { Buffer } = require('buffer'); | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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 ?
| 
           @GuillaumeRx added a test here: 8151216  | 
    
d7375d4    to
    8151216      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!

Patch KeyringController such that
createNewVaultAndRestoreacceptsseedphrasearg as either type string or array of type number, and passes to addNewKeyring as a buffer.Updates package to use
@metamaskversion ofbip39that also acceptsseedphrases passed as buffers tovalidateMnemonic.metamask-extensionalready implements this patch here & here (to be removed when this is cut into a release and pulled into the extension)