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

Add support for BIP39 passphrase #1022

Closed

Conversation

beemeeupnow
Copy link
Contributor

@beemeeupnow beemeeupnow commented Feb 23, 2022

Allow advanced usage of BIP39 passphrase when importing an existing wallet from seed phrase

Relies on tahowallet/hd-keyring#8

@cairomassimo
Copy link
Contributor

I see the UI is still missing in this PR (I'm assuming that's why it's still in draft). @beemeeupnow you could add a quick-and-dirty UI for this, protected by a feature flag. This way we have a much higher of merging this (on one hand the code could be tested immediately, on the other hand we don't have to wait for a proper UI design). @mhluongo what do you think about it?

@beemeeupnow
Copy link
Contributor Author

Thank you for looking at this @cairomassimo

Yes, I need to add some simple interface next.

I was waiting on tahowallet/hd-keyring#8 to be reviewed and merged first to further simplify testing of this PR for others. (it can be merged before this one without breakage)

@beemeeupnow beemeeupnow force-pushed the bip39_passphrase_support branch 3 times, most recently from 1875885 to c35dd31 Compare March 4, 2022 06:24
@beemeeupnow
Copy link
Contributor Author

@cairomassimo I added a feature-flagged text box for the passphrase (env var HIDE_IMPORT_PASSPHRASE)

tahowallet/hd-keyring#8 needs to be merged and a new package release before I move this one from draft, as I'll need to bump the expected hd-keyring version

@beemeeupnow beemeeupnow force-pushed the bip39_passphrase_support branch 3 times, most recently from 735282f to bbd7ba5 Compare March 16, 2022 02:13
@beemeeupnow
Copy link
Contributor Author

As discussed in tallycash/hd-keyring#8, once #1165 is complete I will update this PR so that keyrings with passphrases are handled in such a way that we don't store the passphrase.

That PR will allow adding a special UI flow where we can ask for the passphrase when we are signing and enable me to better implement the feature.

@beemeeupnow
Copy link
Contributor Author

beemeeupnow commented Jun 30, 2022

looping back to this now that #1165 has been merged for a bit and hopefully gets fully implemented

My goal is to have it behave like an imported wallet in that you don't have to enter the mnemonic each time you use it but each time the key material is needed it will require entering the passphrase.

@Shadowfiend
Copy link
Contributor

If you start building on the work in #1697, the final implementation of RFB 2 should be substantially similar. The SignerKeyringFrame is going to be the place to hook in.

@Shadowfiend
Copy link
Contributor

Closing this for now as we've drifted from the feature flag approach (SHOW_ vs HIDE_), we have to hold for a while on keyring changes until we land an audit for account backup (#3252), onboarding has been refactored a bit, and finally we're looking at a different approach to handling signing.

If and when you want to pick this back up @beemeeupnow , feel free to reopen the PR or open a new one built against the latest state of main. I do still think this is worthy work!

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.

3 participants