Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow BIP39 passphrase for Wallet #2721

Conversation

beemeeupnow
Copy link

@beemeeupnow beemeeupnow commented Feb 21, 2022

I found that using HDNode.fromMnemonic() with a password and using the HDNode returned from result.derivePath() for a new Wallet would not work.

The goal of this PR is to avoid breaking any current usage of the library while adding the necessary functionality to make use of the passphrase in Wallet.

I added the passphrase in Mnemonic because it is associated with/only used with the mnemonic phrase. [BIP39]

All tests passed:

  Test Suite: Test HD Node Derivation is Case Agnostic (test-hdnode.js)
    Total Tests: 1409/1409 passed (0:18)  

  Test Suite: Test HD Node Derivation from Seed (test-hdnode.js)
    [ Still running suite - test # 791 ]
    Total Tests: 1049/1049 passed (0:40)  

  Test Suite: Test HD Node Derivation from Mnemonic (test-hdnode.js)
    [ Still running suite - test # 718 ]
    Total Tests: 1049/1049 passed (0:43)  

  Test Suite: Test HD Mnemonic Phrases (test-hdnode.js)
    Total Tests: 1409/1409 passed (0:03)  

  Test Suite: HD Extended Keys (test-hdnode.js)
    Total Tests: 2/2 passed (0:00)  

  Test Suite: HD error cases (test-hdnode.js)
    Total Tests: 7/7 passed (0:00)  

...(output truncated manually)

  Test Suite: Test JSON Wallets (test-wallet.js)
    [ Still running suite - test # 12 ]
    Total Tests: 12/12 passed (0:32)  

  Test Suite: Test Transaction Signing and Parsing (test-wallet.js)
    Total Tests: 2134/2134 passed (0:10)  

  Test Suite: Test Signing Messages (test-wallet.js)
    Total Tests: 9/9 passed (0:00)  

  Test Suite: Serialize Transactions (test-wallet.js)
    Total Tests: 1/1 passed (0:00)  

  Test Suite: Wallet Errors (test-wallet.js)
    Total Tests: 3/3 passed (0:00)  

  Test Suite: Check Wordlists (test-wordlists.js)
    Total Tests: 18/18 passed (0:00)  

  Total Tests: 27470/27470 passed (13:22)  

# status:0

@ricmoo
Copy link
Member

ricmoo commented Feb 22, 2022

This changes method signatures, so cannot be introduced in a patch version bump. I am planning a minor version bump, but may defer this until the next major bump, where the HDNode class is getting its own sub-class of Wallet with the HD extended properties.

You can continue handling HDNodes though to achieve similar results, and an HDNode may be passed directly into the wallet as its private key.

Does that work for you?

@beemeeupnow
Copy link
Author

beemeeupnow commented Feb 22, 2022

If instead of const wallet = new Wallet(new_node) I use const wallet = new Wallet(new_node.privateKey) it works around the issue.

I'm not yet sure if there are any unintended side-effects, but this should help until it's fixed here.

Thank you!

@beemeeupnow
Copy link
Author

I see that HDNodeWallet.fromMnemonic() accepts type Mnemonic, which now allows using the password since it gets handled in computeSeed() so the functionality has been implemented.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants