Skip to content

feat: create eth keystore with master seed#1247

Merged
2 commits merged into
masterfrom
eth-master-seed
Sep 26, 2019
Merged

feat: create eth keystore with master seed#1247
2 commits merged into
masterfrom
eth-master-seed

Conversation

@sangaman
Copy link
Copy Markdown
Collaborator

Closes #1242.

This commit uses the seed generated during the CreateNode call to create the raiden/ethereuem keystore along with the lnd wallets. This is the final step to having a single recovery seed and a single encryption password for all lnd and raiden wallets.

A new configuration option raiden.keystorepath specifies where xud should create the keystore directory.

@reliveyy You should pass in this new option to xud when creating the wallets in docker. We will still need to figure out the best approach to ensuring the seedutil binary is available through non-docker installations. For dev/testing purposes if you have Go installed you can just use npm run compile:seedutil.

@sangaman sangaman requested a review from a user September 24, 2019 19:22
@sangaman sangaman self-assigned this Sep 24, 2019
Copy link
Copy Markdown

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM in general :).

I'm also wondering if we should be using xud's master password that we use to encrypt the private keys on disk as the password for the seed (or let user specify an extra seed password). With the current implementation we can recover the wallet only with 24 words (no password required). This can be done in a separate PR if we choose to go that route.

Comment thread lib/cli/commands/create.ts Outdated
Comment thread lib/Config.ts
@kilrau
Copy link
Copy Markdown
Contributor

kilrau commented Sep 25, 2019

I'm also wondering if we should be using xud's master password that we use to encrypt the private keys on disk as the password for the seed (or let user specify an extra seed password). With the current implementation we can recover the wallet only with 24 words (no password required). This can be done in a separate PR if we choose to go that route.

We discussed this before and yes we want this at some point. It already has an issue: #1030 - but definitely not in this PR.

We decided to push this feature to post-1.0.0, since it's "advanced" functionality which needs proper UX (The "lost device" case is covered, but once you forget your password you are screwed and can't recover funds with your mnemonic)

@ghost
Copy link
Copy Markdown

ghost commented Sep 25, 2019

We discussed this before and yes we want this at some point. It already has an issue: #1030 - but definitely not in this PR.

Thanks for reminding me. I'll bring this up during our next call to discuss if we can implement this without a breaking change.

@kilrau kilrau added the P1 top priority label Sep 25, 2019
Comment thread lib/swaps/SwapClientManager.ts Outdated
await Promise.all(initWalletPromises);
if (!this.raidenClient.isDisabled()) {
const { keystorepath } = this.config.raiden;
const keystorePromise = seedutil(seedMnemonic, walletPassword, keystorepath).then(() => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In order to programmatically unlock raiden we'll need to store the password of the keystore file in plain text. If I'm reading the code correctly it's using the xud master password to protect the keystore. Can we temporarily set it to empty string until we have a way of unlocking raiden via the REST API?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good thinking, I will do this on a temporary basis until we get a much needed call to unlock the keystore.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done, I had to make a few changes to the seedutil tool to make the encryption password optional.

@sangaman sangaman changed the title feat: create eth keystore with master seed & pw feat: create eth keystore with master seed Sep 25, 2019
@sangaman sangaman requested a review from a user September 25, 2019 21:06
This modifies the seedutil tool so that it uses separate passwords for
the aezeed and for encrypting the private key in the keystore.
Previously, a single password was used for both, which is incompatible
with how xud uses them separately with a master password for decryption
and currently always using the default "aezeed" password for the aezeed
itself.

The encryption password, keystore path, and aezeed passwords are now
treated as optional flags.
This commit uses the seed generated during the `CreateNode` call to
create the raiden/ethereuem keystore along with the lnd wallets. This
is the next step towards having a single recovery seed and a single
encryption password for all lnd and raiden wallets. The keystore is
encrypted with an empty password instead of the master password until
raiden adds a way to decrypt the keystore without needing to store
the password in a file in plaintext.

A new configuration option `raiden.keystorepath` specifies where xud
should create the keystore directory.

Closes #1242.
Copy link
Copy Markdown

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ghost ghost merged commit bda4cc8 into master Sep 26, 2019
@ghost ghost deleted the eth-master-seed branch September 26, 2019 13:23
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 top priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Initialize ETH keystore file/wallet from master seed

2 participants