Restore wallet #612

valentinewallace · 2018-09-01T01:06:37Z

Closes #423.

The storybook is not happy if you enter values and click Next due to the store values being set as if the user were initializing a new wallet, not restoring an old one. Not sure if this is OK or worth fixing?

tanx

Took a first rough pass at the code. Perhaps let's discuss this via video before the app weekly?

tanx · 2018-09-04T10:36:21Z

src/action/wallet.js

   * @return {Promise<undefined>}
   */
-  async initWallet({ walletPassword, seedMnemonic }) {
+  async initWallet({ walletPassword, seedMnemonic, recoveryWindow }) {


Set default value initWallet({ walletPassword, seedMnemonic, recoveryWindow = 0 }) so that we don't have to set it above.

tanx · 2018-09-04T10:59:55Z

src/action/wallet.js

+        type: 'error',
+        msg: 'Initializing wallet failed',
+      });
+    }


Why do we need to navigate to SelectSeed in this case?

tanx · 2018-09-04T11:01:48Z

src/action/wallet.js

+   * @return {undefined}
+   */
+  initRestoreWallet() {
+    this._store.wallet.seedVerify = Array(24).fill('');


Seems like restoring the seed is semantically different. Perhaps we should use a separate param than store.wallet.seedVerify which is used in seed verification?

tanx · 2018-09-04T12:04:16Z

src/action/wallet.js

    await this.initWallet({
      walletPassword: password,
      seedMnemonic: this._store.seedMnemonic.toJSON(),
+      recoveryWindow: 0,


Not needed if default is set.

tanx · 2018-09-04T12:15:08Z

src/view/setting-unit.js

 const SettingUnitView = ({ store, nav, setting }) => {
  return (
-    <Background color={color.blackDark} style={styles.wrapper}>
+    <Background color={color.blackDark}>


Good catch!

tanx · 2018-09-04T12:21:10Z

src/action/wallet.js

+      });
+    } else {
+      await this.unlockWallet({ walletPassword: password });
+    }


Not sure I understand. Why are we using the password check screen for restoration?

tanx · 2018-09-04T12:23:16Z

src/computed/seed.js

+      } else {
+        const { seedMnemonic: words } = store;
+        return words.length ? getSeedIndexes(1, words.length, 3) : [];
+      }


Why mix seed verification logic and seed restore logic? Seems like this code would be much clean if the two were separated.

tanx · 2018-09-04T12:23:46Z

src/computed/seed.js

+        : 0;
+      const c0 = formatOrdinal(seedVerifyIndexes[startingIndex]);
+      const c1 = formatOrdinal(seedVerifyIndexes[startingIndex + 1]);
+      const c2 = formatOrdinal(seedVerifyIndexes[startingIndex + 2]);


Same point as above.

tanx · 2018-09-04T12:27:05Z

src/view/restore-wallet.js

-          />
-        ))}
+        {store.seedVerifyIndexes
+          .slice(store.wallet.restoreIndex, store.wallet.restoreIndex + 3)


Might be cleaner to pull out the slice(...) logic into a computed attribute?

tanx · 2018-09-04T12:28:49Z

src/view/select-seed.js

+  nav: PropTypes.object.isRequired,
+};
+
+export default observer(SelectSeedView);


I cleaned this view up a bit.

tanx

Looks much better. Second pass 🙂

tanx · 2018-09-05T13:52:30Z

src/action/wallet.js

+      this._notification.display({
+        type: 'error',
+        msg: `Initializing wallet failed: ${JSON.stringify(err)}`,
+      });


There's no need to set type: error if err is passed. Also, the JSON.stringify(err) is already done by the logger. So we can change this back to the way it was.

When I leave it the way it was, the error appears like this on the screen:

Which allows the user to click "Show error logs," bringing them to the CLI, and from there they can navigate to the home screen even though their wallet hasn't been initialized.

I do think it's a good idea to give some level of detail in the notification's error message.. Let me know if you like the new solution, I just show the detalis field of the error.

src/computed/seed.js

src/view/restore-password.js

tanx · 2018-09-07T13:50:16Z

src/action/wallet.js

+      this._notification.display({
+        type: 'error',
+        msg: `Initializing wallet failed: ${err.details}`,
+      });


The above does the same in one line. Why the change?

Sorry, did you see this comment? #612 (comment) This is the reason I changed it, you may not be getting the same issue?

ghost · 2018-09-11T22:18:54Z

Tested this manually - in general it seems to work as advertised. Nice work :).

Couple of remarks...

Firstly, when I made a mistake typing in the seed the first reaction was to click the back button and toggle through the steps to see if there was a typo. Clicking the back button takes us back to the SelectSeed route which resets all the fields. It would have been a better user experience to go back to the previous RestoreSeed step. This is probably out of scope of this PR and something we can improve upon later.

Secondly, after entering the correct seed and password it redirects to SeedSuccess view which felt strange because I already had my wallet funded. Maybe we should redirect to Home instead after a successful restore?

Additionally, after restoring my console filled up with tons of warnings about a possible memory leak and the application felt janky for a second or two. In my test environment I had 400 blocks mined (simnet).

restoreIndex keeps track of how many seed words the user has entered so we know which ones are remaining for the user to enter. We want to reset these values on the start of the restore process, as well as ensure they are up to date as the user goes from one page of restore entries to the next.

This view is shown on a fresh start of the app to allow the user to either restore an existing wallet or initialize a new one.

…es and inits. Correctly initialize the next restore page before navigating to it, display the correct restore seed verify indices and enter the user input into wallet.restoreSeed.

This view was previously named RestoreWallet.

valentinewallace · 2018-09-12T19:21:00Z

@ERKarl Thanks so much for the feedback!

I agree that clicking the back button should go to the previous page and not erase the entries, so that's what it does now. :)

Not sure about going to home instead of seed success, thought it would be good to tell the user that it succeeded and give them an opportunity to deposit coin? I left that part out, but @tanx let me know what you think.

I'm also unable to reproduce the memory leaks. Good thing users don't have devtools open, haha.

ghost · 2018-09-13T08:24:18Z

@valentinewallace awesome :). I tested out your added changes and the back functionality is much better now.

Regarding the memory leak. I was able to consistently reproduce the errors on 2 machines following these steps:

rm -Rf data
npm run electron-dev
generate new seed -> add coins (copy address)
close electron-dev
btcd --txindex --simnet --rpcuser=kek --rpcpass=kek --datadir=data/btcd/data --logdir=data/btcd/logs --miningaddr=<YOUR_ADD_COIN_ADDRESS>
btcctl --simnet --rpcuser=kek --rpcpass=kek generate 400
close btcd
rm -Rf data/lnd
npm run electron-dev
restore from seed -> enter password
wait several seconds
error - the console is filled with (node) warning: possible EventEmitter memory leak detected. 11 listeners added. Use emitter.setMaxListeners() to increase limit. warnings (see screenshot above)

tanx

LGTM 👍

valentinewallace requested a review from tanx September 1, 2018 01:06

tanx suggested changes Sep 4, 2018

View reviewed changes

valentinewallace force-pushed the restore-wallet branch 3 times, most recently from 76947bf to 65fd00c Compare September 5, 2018 01:55

tanx suggested changes Sep 5, 2018

View reviewed changes

valentinewallace added 5 commits September 5, 2018 13:43

Remove unused styles from settings unit/fiat.

89564f0

Add recovery window const to config and initWallet.

ad92127

Add restoring and restoreSeed fields to store + setters.

f01ded5

Add wallet.restoreWallet, giving lnd the seed and password to restore.

90f9d73

Add computed fields for restore: seed indices, verify and copy.

bc93d7e

valentinewallace force-pushed the restore-wallet branch from 65fd00c to 7baa15f Compare September 5, 2018 20:43

tanx suggested changes Sep 7, 2018

View reviewed changes

valentinewallace added 6 commits September 12, 2018 12:08

Add "SelectSeed" view.

1fd414c

This view is shown on a fresh start of the app to allow the user to either restore an existing wallet or initialize a new one.

Navigate to SelectSeed instead of Seed on fresh app start.

aca414d

Modify RestoreWallet (now RestoreSeed) to utilize new seedVerifyIndic…

2ae3fa1

…es and inits. Correctly initialize the next restore page before navigating to it, display the correct restore seed verify indices and enter the user input into wallet.restoreSeed.

Add RestorePassword view for password entry during restore.

0c8b010

Hook up RestoreSeed view.

d778154

This view was previously named RestoreWallet.

valentinewallace force-pushed the restore-wallet branch from 7baa15f to d778154 Compare September 12, 2018 19:10

tanx approved these changes Sep 13, 2018

View reviewed changes

tanx merged commit 2794795 into master Sep 13, 2018

tanx deleted the restore-wallet branch September 13, 2018 14:43

Restore wallet #612

Restore wallet #612

Uh oh!

Conversation

valentinewallace commented Sep 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tanx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 11, 2018

Uh oh!

valentinewallace commented Sep 12, 2018

Uh oh!

ghost commented Sep 13, 2018

Uh oh!

tanx left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

valentinewallace commented Sep 1, 2018 •

edited

Loading

valentinewallace Sep 5, 2018 •

edited

Loading