Support BIP39 recovery and use BIP39 for new accounts (fix #207 #199) by axelchalon · Pull Request #284 · openethereum/fether

axelchalon · 2018-12-12T18:19:54Z

amaury1093 · 2018-12-13T15:21:00Z

Reviewing now. Just one small issue, when you yarn electron:

Creating an optimized production build...
Failed to compile.

Failed to minify the code from this file: 

 	/Users/amaurymartiny/Workspaces/fether/node_modules/base-x/index.js:15 

Read more here: http://bit.ly/2tRViJ9

info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

    at Promise.all.then.arr (/Users/amaurymartiny/Workspaces/fether/node_modules/lerna/node_modules/execa/index.js:236:11)
    at process.internalTickCallback (internal/process/next_tick.js:77:7)
  code: 1,

It might be solved by #212, not sure if you want to find another package for this, or to wait for #212 first.

amaury1093

Very nice! Just small comments, and need to solve the uglify problem.

I tested, I can import from bip39 and from parity phrase. Other stuff gives me an error.

amaury1093 · 2018-12-14T11:43:46Z

packages/fether-react/src/stores/createAccountStore.js

+    if (
+      words.length < 12 ||
+      words.length > 24 ||
+      !words.every(word => PARITY_WORDLIST.has(word))


O(n2). Not a big deal, just putting it here.

amaury1093 · 2018-12-14T11:44:38Z

packages/fether-react/src/stores/createAccountStore.spec.js

    createAccountStore.name = 'account name';
-    createAccountStore.clear();
-    expect(createAccountStore.phrase).toBe(null);
+    await createAccountStore.clear();


Is await needed here?

ah, you're right, createAccountStore.clear doesn't return a promise. I'll make it async for consistency

amaury1093 · 2018-12-14T11:45:15Z

packages/fether-react/src/BackupAccount/BackupAccount.js

 import React, { Component } from 'react';
 import { AccountHeader, Card, Form as FetherForm } from 'fether-ui';
-import { inject, observer } from 'mobx-react';
+import backupAccount from '../utils/backupAccount';


Relative import in below block

Tbaut

This is awesome!! I could test (with a yarn start) and didn't manage to make it crash :)

axelchalon · 2018-12-14T16:37:55Z

Updated to CRA2; everything seems to be working fine, although the Electron window opened by yarn start seems to need a refresh to display Fether, not sure why that is.
edit: need to figure out how to add the decorators plugin to the test environment

amaury1093

Nice! I'll have a look asap

amaury1093 · 2018-12-14T16:45:09Z

packages/fether-react/config-overrides.js

  // use the MobX rewire to use @decorators
-  config = injectBabelPlugin('transform-decorators-legacy', config);
-
+  config = injectBabelPlugin(


https://github.com/timarney/react-app-rewired I was a bit worried about this, as they state they don't support CRA2. But hey, it seems to work.

amaury1093 · 2018-12-14T16:59:16Z

@axelchalon If tests are hard to convert, might be worth giving https://github.com/sharegate/craco a try

amaury1093

🎉 yarn start, electron and package all work for me!

axelchalon · 2018-12-14T17:04:28Z

ah nice, I think it's best to switch to this then.

Currently react-app-rewired build works fine but react-app-rewired test says "Support for the experimental syntax 'decorators-legacy' isn't currently enabled"

amaury1093 · 2018-12-14T17:13:08Z

package.json

  },
  "scripts": {
-    "build": "lerna run build",
+    "build": "cross-env SKIP_PREFLIGHT_CHECK=true lerna run build",


why is this needed?

CRA2 requires a version of eslint (uses a version of eslint internally) that is more recent than the version installed by our dependencies (semistandard); this environment variable suppresses the warning

axelchalon · 2018-12-14T17:57:57Z

ready!

I logged the issue of craco test not respecting the --react-scripts path (a flag used to make craco work with monorepos) here

Support BIP39 recovery and use BIP39 for new accounts (fix #207 #199)

94f40c0

axelchalon requested a review from amaury1093 December 12, 2018 18:19

Don't keep the Parity wordlist in memory the whole time

2e5dbf3

axelchalon added the A0-pleasereview label Dec 13, 2018

axelchalon force-pushed the ac-bip39 branch from daf5e6e to f0c240a Compare December 13, 2018 14:57

Update tests for createAccountStore

f0c240a

amaury1093 approved these changes Dec 14, 2018

View reviewed changes

amaury1093 added A5-grumble and removed A0-pleasereview labels Dec 14, 2018

Tbaut approved these changes Dec 14, 2018

View reviewed changes

Grumbles

ed6d3fd

axelchalon added A0-pleasereview A3-inprogress and removed A5-grumble A0-pleasereview A3-inprogress labels Dec 14, 2018

axelchalon force-pushed the ac-bip39 branch from fd34e9f to 8b7d9ed Compare December 14, 2018 16:37

Update fether-react to create-react-app 2 (closes #212)

8b7d9ed

amaury1093 reviewed Dec 14, 2018

View reviewed changes

amaury1093 approved these changes Dec 14, 2018

View reviewed changes

amaury1093 reviewed Dec 14, 2018

View reviewed changes

axelchalon added 3 commits December 14, 2018 18:28

CRA overrides: switch from react-app-rewired to craco

a4802f7

Merge branch 'master' into ac-bip39

2baa1f2

Fix craco test not honoring --react-scripts with an ugly hack

4966b84

package.json: move SKIP_PREFLIGHT_CHECK from workspace root to packages

d628be9

amaury1093 approved these changes Dec 17, 2018

View reviewed changes

amaury1093 added A9-buythatmanabeer A8-looksgood and removed A0-pleasereview labels Dec 17, 2018

amaury1093 merged commit 782a0f0 into master Dec 17, 2018

amaury1093 deleted the ac-bip39 branch December 17, 2018 11:12

Tbaut mentioned this pull request Dec 17, 2018

Fether does not validate recovery phrase #199

Closed

amaury1093 mentioned this pull request Jan 17, 2019

Update to Electron 4 #309

Closed

axelchalon mentioned this pull request Feb 5, 2019

Fix tests #404

Closed

Conversation

axelchalon commented Dec 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amaury1093 commented Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amaury1093 left a comment • 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!

amaury1093 Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

amaury1093 Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

axelchalon Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

amaury1093 Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

Tbaut left a comment

Choose a reason for hiding this comment

Uh oh!

axelchalon commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amaury1093 left a comment

Choose a reason for hiding this comment

Uh oh!

amaury1093 Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

amaury1093 commented Dec 14, 2018

Uh oh!

amaury1093 left a comment

Choose a reason for hiding this comment

Uh oh!

axelchalon commented Dec 14, 2018

Uh oh!

amaury1093 Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

axelchalon Dec 14, 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!

axelchalon commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

axelchalon commented Dec 12, 2018 •

edited

Loading

amaury1093 commented Dec 13, 2018 •

edited

Loading

amaury1093 left a comment •

edited

Loading

axelchalon commented Dec 14, 2018 •

edited

Loading

axelchalon Dec 14, 2018 •

edited

Loading

axelchalon commented Dec 14, 2018 •

edited

Loading