Wallet Decrypt - Mnemonic #180

skubakdj · 2017-09-08T05:32:27Z

What This Does

Broadly speaking, two things are accomplished:

Adds mnemonic wallet decrypt as described in Wallet Class Structure #103 & referenced by Wallet Decrypt #80
Adds mnemonic support to the DeterministicWalletsModal & accompanying sagas

What Still Needs to Happen

A few things to get done before this is merged:

~~Need to write unit tests for decryptMnemonicToPrivKey~~
~~Need to do more manual testing~~
Need to test against Trezor to make sure I didn't break anything (may need some help for this one)

skubakdj · 2017-09-08T05:34:31Z

common/libs/validators.js

-  return dPath.split("'/").length === 4;
+  //TODO: use a regex to detect proper paths
+  const len = dPath.split("'/").length;
+  return len === 3 || len === 4;


Ledger uses m/44'/60'/0' for some reason:

https://ethereum.stackexchange.com/questions/19055/what-is-the-difference-between-m-44-60-0-0-and-m-44-60-0/19061

Oh wow, BIP44 is way more readable than BIP32. Would have saved me a lot of headaches: https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki

Would you mind changing the comment to reflect that BIP 44 is really where we're determining that 4 is the amount, but that ledger is the exception for 3?

yep, will do 👍

skubakdj · 2017-09-08T05:37:20Z

common/sagas/deterministicWallets.js

-      })
-    );
+
+  try {


try/catch introduced because derived addresses wouldn't populate on network/api failure

dternyak · 2017-09-08T17:00:31Z

common/components/WalletDecrypt/Mnemonic.jsx

+                : 'is-invalid'}`}
+              value={phrase}
+              onChange={this.onMnemonicChange}
+              placeholder={translateRaw('x_Mnemonic')}


You can save the translateRaw import by passing true as the argument for the textOnly parameter:

translate('x_Mnemonic', true)

Makes me question if translateRaw should even be exported.

I kinda like it as a separate function, makes the flow typing a little easier (Not two return types) and is very easy to understand over mystery args.

wbobeirne

Unfortunately I'm out of town sans Trezor, but I'll run this ASAP on Tuesday to make sure it still works.

wbobeirne · 2017-09-09T15:56:37Z

common/components/WalletDecrypt/Mnemonic.jsx

+                : 'is-invalid'}`}
+              value={phrase}
+              onChange={this.onMnemonicChange}
+              placeholder={translateRaw('x_Mnemonic')}


I kinda like it as a separate function, makes the flow typing a little easier (Not two return types) and is very easy to understand over mystery args.

wbobeirne · 2017-09-09T15:58:15Z

common/components/WalletDecrypt/Mnemonic.jsx

+    this.setState({ phrase: e.target.value });
+  };
+
+  onChooseAddress = (e: SyntheticInputEvent) => {


Very minor, but this feels like a misnomer. This isn't when you choose the address, this is more like opening the modal, or validating the phrase / pass.

Good call, I'll switch it to onDWModalOpen, unless you can suggest something else.

wbobeirne · 2017-09-09T16:00:51Z

common/config/dpaths.json

+    {
+      "label": "Expanse",
+      "value": "m/44'/40'/0'/0"
+    }


Seeing multiple now, might be worth making this a JS file so that we can assign these repeated ones to variables and reuse them.

Converted dpaths.json to dpaths.js.

wbobeirne · 2017-09-09T16:06:10Z

common/libs/validators.js

-  return dPath.split("'/").length === 4;
+  //TODO: use a regex to detect proper paths
+  const len = dPath.split("'/").length;
+  return len === 3 || len === 4;


Oh wow, BIP44 is way more readable than BIP32. Would have saved me a lot of headaches: https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki

Would you mind changing the comment to reflect that BIP 44 is really where we're determining that 4 is the amount, but that ledger is the exception for 3?

wbobeirne · 2017-09-09T16:09:21Z

common/components/WalletDecrypt/DeterministicWalletsModal.jsx

@@ -40,13 +40,15 @@ type Props = {
  dPaths: { label: string, value: string }[],
  publicKey: string,
  chainCode: string,
+  seed: string,


This is a required prop, but the DecryptWallet/Trezor.jsx wasn't altered to send it (And probably shouldn't, I think this should be optional.) Any idea why flow didn't catch that?

Agree, I'll make it optional. Not sure why flow didn't catch that, it definitely should have...

wbobeirne · 2017-09-09T16:11:34Z

common/sagas/deterministicWallets.js

+    }
+  } catch (err) {
+    console.log(err);
+    //TODO: maybe communicate API call error to user?


Definitely, I think you can just get away with a yield put(showNotification(...

wbobeirne · 2017-09-09T16:13:05Z

common/sagas/deterministicWallets.js

+    }
+  } catch (err) {
+    console.log(err);
+    //TODO: maybe communicate API call error to user?


Same as above, showNotification

wbobeirne · 2017-09-12T17:19:09Z

Tested the branch on my Trezor, everything seems to work fine. Still not sure how it's not complaining more about the lack of seed arg, but looks like no regressions here.

dternyak · 2017-09-12T17:32:25Z

@wbobeirne Interesting. I'm currently unable to send via Trezor on the latest develop. You're able to send via Trezor on this branch? Notice: The signed tx generates, but when attempting to broadcast it the node always responds with

Error: Insufficient funds. The account you tried to send transaction from does not have enough funds. Required XXX000000000000 and got: 0.

There is likely something wrong with the generation of the signedTx.

I have more info listed here: MyEtherWallet/MyEtherWallet#182 (comment)

dternyak · 2017-09-12T17:33:46Z

I'm going to attempt to send via Trezor on this branch -- will update with my results.

I get a similar failure when testing on this branch.

@wbobeirne Can you confirm whether or not you attempted to actually broadcast the signedTx, and not just generate it?

wbobeirne · 2017-09-12T18:39:31Z

Sorry, allow me to reiterate, I only tested unlocking. This branch is a bit behind develop, and I didn't test sending until it was up to date.

wbobeirne · 2017-09-12T19:50:40Z

common/components/WalletDecrypt/index.jsx

@@ -165,7 +171,7 @@ export class WalletDecrypt extends Component {

  onUnlock = (payload: any) => {
    this.props.dispatch(
-      WALLETS[this.state.selectedWalletKey].unlock(this.state.value || payload)
+      WALLETS[this.state.selectedWalletKey].unlock(payload || this.state.value)


Payload is a SyntheticEvent in some cases. We'll either need to check if payload isn't one, or change how onUnlock is called so that it's never sent an event.

I believe I have fixed this in one of my recent PRs https://github.com/MyEtherWallet/MyEtherWallet/pull/183/files#diff-c2a1968700ed6452c7c0257d4dde4cd4R168

Sweet, in that case we just need to bring develop back into this bad boy.

skubakdj · 2017-09-14T18:13:16Z

@dternyak, @wbobeirne I think I've addressed all the comments with these latest commits. Also added some tests, rebased this branch with develop, and did some manual testing. Any additional feedback on the changes?

Also, still haven't been able to test using Trezor, but I can get to it tonight if need be.

dternyak · 2017-09-14T18:23:46Z

@wboberine is working on a fix to Trezor, and is planning on commiting that directly to this branch. After that we should be good to merge! :)

…

On Thu, Sep 14, 2017 at 11:13 AM skubakdj ***@***.***> wrote: @dternyak <https://github.com/dternyak>, @wbobeirne <https://github.com/wbobeirne> I think I've addressed all the comments with these latest commits. Also added some tests, rebased this branch with develop, and did some manual testing. Any additional feedback on the changes? Also, still haven't been able to test using Trezor, but I can get to it tonight if need be. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <MyEtherWallet/MyEtherWallet#180 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHf02TBDYLVBm_EEf_g_q521wzHQ9Hsuks5siWy-gaJpZM4PQtnT> .

* Match v3 more closely. * Require wallet index on deterministic wallets, update trezor to send index. * remove redundent stripAndLower function and rename existing stripHex to stripHexPrefixAndLower

skubakdj commented Sep 8, 2017

View reviewed changes

skubakdj requested review from wbobeirne and dternyak September 8, 2017 05:42

dternyak added the wip label Sep 8, 2017

dternyak reviewed Sep 8, 2017

View reviewed changes

wbobeirne suggested changes Sep 9, 2017

View reviewed changes

wbobeirne suggested changes Sep 12, 2017

View reviewed changes

skubakdj added 14 commits September 14, 2017 13:26

add 'bip39' package

49b1c79

add mnemonic decrypt, wallet wrapper

c32a463

add mnemonic path config

ad51a6c

add mnemonic support to deterministic components

67733d5

add mnemonic support

bdbbbfc

accomodate for ledger ETH path

430174c

remove comments

c7546aa

update comments regarding path length

9688133

rename modal open handler

82c8c49

make several props optional

543de64

add basic tests for mnemonic decrypt

3d25f2e

make flow happy, add user error notifications

0bb9312

convert dpaths to js file, update references

96bceba

add ledger path to test

d518ba6

skubakdj force-pushed the wallet-decrypt-mnemonic branch from 79241a1 to d518ba6 Compare September 14, 2017 17:35

skubakdj added status: ready for review Open PR's that are ready for technical review. Replaced with "changes needed" or "ready to merge". and removed wip labels Sep 14, 2017

Trezor DPath Fix (#196)

98156e4

* Match v3 more closely. * Require wallet index on deterministic wallets, update trezor to send index. * remove redundent stripAndLower function and rename existing stripHex to stripHexPrefixAndLower

wbobeirne approved these changes Sep 15, 2017

View reviewed changes

dternyak merged commit c88e96d into develop Sep 15, 2017

dternyak deleted the wallet-decrypt-mnemonic branch September 15, 2017 19:29

dternyak mentioned this pull request Sep 18, 2017

Mnemonic Generation Support #109

Closed

snyk-bot mentioned this pull request Mar 17, 2020

[Snyk] Security upgrade qrcode from 1.2.0 to 1.3.4 protokiller/MyCrypto#9

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wallet Decrypt - Mnemonic #180

Wallet Decrypt - Mnemonic #180

skubakdj commented Sep 8, 2017 •

edited

Loading

skubakdj Sep 8, 2017

wbobeirne Sep 9, 2017

skubakdj Sep 11, 2017

skubakdj Sep 8, 2017

dternyak Sep 8, 2017

dternyak Sep 8, 2017

wbobeirne Sep 9, 2017

wbobeirne left a comment

wbobeirne Sep 9, 2017

wbobeirne Sep 9, 2017

skubakdj Sep 11, 2017

wbobeirne Sep 9, 2017

skubakdj Sep 14, 2017

wbobeirne Sep 9, 2017

wbobeirne Sep 9, 2017

skubakdj Sep 11, 2017

wbobeirne Sep 9, 2017

wbobeirne Sep 9, 2017

wbobeirne commented Sep 12, 2017

dternyak commented Sep 12, 2017 •

edited

Loading

dternyak commented Sep 12, 2017 •

edited

Loading

wbobeirne commented Sep 12, 2017

wbobeirne Sep 12, 2017

dternyak Sep 12, 2017

wbobeirne Sep 12, 2017

skubakdj commented Sep 14, 2017

dternyak commented Sep 14, 2017 via email

-                    })
-                  );
+                try {

Wallet Decrypt - Mnemonic #180

Wallet Decrypt - Mnemonic #180

Conversation

skubakdj commented Sep 8, 2017 • edited Loading

What This Does

What Still Needs to Happen

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbobeirne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbobeirne commented Sep 12, 2017

dternyak commented Sep 12, 2017 • edited Loading

dternyak commented Sep 12, 2017 • edited Loading

wbobeirne commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skubakdj commented Sep 14, 2017

dternyak commented Sep 14, 2017 via email

skubakdj commented Sep 8, 2017 •

edited

Loading

dternyak commented Sep 12, 2017 •

edited

Loading

dternyak commented Sep 12, 2017 •

edited

Loading