I5849 incremental account security #6874

danjm · 2019-07-18T09:40:03Z

Resolves #5849

This PR adds the following functionality:

user can choose to skip the seed phrase challenge during onboarding
if user skips seed phrase challenge, and later has a balance > 0, user is shown a reminder to back up their seed phrase
the reminder includes a button that takes them to the seed phrase backup page, after successfully completing the backup challenge, the reminder is no longer shown

Full demo visible here: https://streamable.com/5klzz

metamaskbot · 2019-07-23T17:46:43Z

Builds ready [b3a499e]: chrome, firefox, edge, opera

metamaskbot · 2019-07-23T18:32:02Z

Builds ready [e6bcbc9]: chrome, firefox, edge, opera

Gudahtt · 2019-07-26T04:03:54Z

ui/app/pages/first-time-flow/seed-phrase/seed-phrase.component.js


    if (!seedPhrase) {
-      history.push(DEFAULT_ROUTE)
+      console.log('!!!444!!!')


A couple more a few lines above as well

Gudahtt · 2019-07-26T04:08:44Z

ui/app/store/actions.js

+  hideSeedPhraseBackupAfterOnboarding,
+  HIDE_SEED_PHRASE_BACKUP_AFTER_ONBOARDING: 'HIDE_SEED_PHRASE_BACKUP_AFTER_ONBOARDING',
+
+  verifySeedPhrase,


Probably better to move this function somewhere else, since it's not really an action. It feels wrong to export it from here.

Then again, it'd be a lot of work to move it without creating an additional background connection. We'd probably want to create a module for all of the background interactions. Maybe that can be a future change.

whymarrh

A few comments scattered throughout, generally LGTM.

Comments specifically on the tests:

We might be able to reference files directly from the node_modules/ directory directly instead of adding them to the test/ directory. That is,

Current Path	node_modules
`test/e2e/send-eth-with-private-key-test/buffer.js`	`node_modules/buffer/index.js`
`test/e2e/send-eth-with-private-key-test/ethereumjs-tx.js`	`node_modules/ethereumjs-tx/es5/index.js`

I think that should work fine? We could also symlink them?

app/scripts/controllers/onboarding.js

whymarrh · 2019-07-30T13:34:05Z

app/scripts/controllers/onboarding.js

+ */
+
+/**
+ * Background controller responsible for maintaining


Suggested change

* Background controller responsible for maintaining

* Controller responsible for maintaining

I think the "Background" distinction here might not be needed

whymarrh · 2019-07-30T13:43:01Z

test/e2e/send-eth-with-private-key-test/index.html

@@ -0,0 +1,17 @@
+<html>


Suggested change

<html>

<html lang="en">

Also we should make sure we have a doctype: <!doctype html> as the line above this one

whymarrh · 2019-07-30T13:47:03Z

test/e2e/send-eth-with-private-key-test/send-eth-with-private-key.js

@@ -0,0 +1,28 @@
+/* eslint-disable */


Can we be more specific about what we're disabling? If all we need it to add globals, we can do that too:^[1]

Suggested change

/* eslint-disable */

/* global ethereumjs, Web3 */

whymarrh · 2019-07-30T13:47:35Z

test/e2e/send-eth-with-private-key-test/webjs.js

@@ -0,0 +1,2 @@
+/* eslint-disable */


Do we need this file?

whymarrh · 2019-07-30T13:49:09Z

...first-time-flow/create-password/import-with-seed-phrase/import-with-seed-phrase.container.js

@@ -0,0 +1,14 @@
+


Nit: extra newline

whymarrh · 2019-07-30T13:49:42Z

ui/app/pages/first-time-flow/seed-phrase/confirm-seed-phrase/confirm-seed-phrase.container.js

@@ -0,0 +1,24 @@
+


Nit: extra newline

Gudahtt · 2019-07-30T14:45:53Z

I think that should work fine? We could also symlink them?

Using the node_modules path directly works just fine, as long as they're both top-level dependencies (e.g. not transitive dependencies).

cjeria · 2019-07-30T22:04:17Z

"Remind me later" option

What's the logic behind the "remind me later" option introduced in this implementation?

How much time passes until MM asks me again?

Not against doing this, but in our discussions, I believe we decided that we were not going to give the option to dismiss this message at all to encourage better security behavior cc @bdresser @omnat

Component scaling for full-screen

Regarding scaling this component for full-screen mode, let's right align the buttons, reduce their width to ~130px. Specs in [figma] also see gif for example of how this could scale.(https://www.figma.com/file/coqVvQFBBWGtcechEkloSE/Incremental-Account-Security-Desktop?node-id=2%3A289)

In full-screen mode, let's float the notification bottom and right to the page (z-index above all other elements on the page).

Button states

Created a pen where you can pull state colors from https://codepen.io/cjeria/pen/mNmEYj

Behavior

As far as behavior goes, it works great!

Additional question:
In the original issue, we discussed the flow that begins when a new user visits a dapp they don;'t have mm installed. It's not implemented in this branch yet since i tested it and it doesn't work. I assume this comes next?
This is the flow and link to the figma file

metamaskbot · 2019-07-31T18:09:40Z

Builds ready [dc36e54]: chrome, firefox, edge, opera

metamaskbot · 2019-08-01T11:10:52Z

Builds ready [c7dad0d]: chrome, firefox, edge, opera

…ned tx with stand alone localhost provider

…e screen

…test/ethereumjs-tx.js

…ndex.html

metamaskbot · 2019-08-01T17:28:36Z

Builds ready [33683bb]: chrome, firefox, edge, opera

whymarrh

A few small comments

whymarrh · 2019-08-01T18:26:07Z

.gitattributes

@@ -4,3 +4,6 @@ CHANGELOG.md merge=union
 # we're using the dependencies we expect to be using
 package-lock.json linguist-generated=false
 yarn.lock linguist-generated=false
+
+/home/danjm/kyokan/metamask-extension/test/e2e/send-eth-with-private-key-test/ethereumjs-tx.js linguist-vendored


Suggested change

/home/danjm/kyokan/metamask-extension/test/e2e/send-eth-with-private-key-test/ethereumjs-tx.js linguist-vendored

test/e2e/send-eth-with-private-key-test/ethereumjs-tx.js linguist-vendored linguist-generated

whymarrh · 2019-08-01T18:26:38Z

.gitattributes

@@ -4,3 +4,6 @@ CHANGELOG.md merge=union
 # we're using the dependencies we expect to be using
 package-lock.json linguist-generated=false
 yarn.lock linguist-generated=false
+
+/home/danjm/kyokan/metamask-extension/test/e2e/send-eth-with-private-key-test/ethereumjs-tx.js linguist-vendored
+/home/danjm/kyokan/metamask-extension/test/e2e/send-eth-with-private-key-test/ethereumjs-tx.js linguist-generated=true


Suggested change

/home/danjm/kyokan/metamask-extension/test/e2e/send-eth-with-private-key-test/ethereumjs-tx.js linguist-generated=true

test/e2e/send-eth-with-private-key-test/index.html

whymarrh

Let's Get This Merged™

metamaskbot · 2019-08-02T03:52:57Z

Builds ready [ccd0bc5]: chrome, firefox, edge, opera

danjm requested review from Gudahtt and whymarrh as code owners July 18, 2019 09:40

danjm requested a review from cjeria July 23, 2019 18:01

danjm force-pushed the i5849-incremental-account-security branch from b3a499e to e6bcbc9 Compare July 23, 2019 18:13

Gudahtt reviewed Jul 26, 2019

View reviewed changes

whymarrh suggested changes Jul 30, 2019

View reviewed changes

cjeria mentioned this pull request Jul 30, 2019

Privacy mode as default #6904

Merged

3 tasks

danjm force-pushed the i5849-incremental-account-security branch from 5c06b18 to c7dad0d Compare August 1, 2019 10:58

danjm added 9 commits August 1, 2019 13:35

Implements ability to defer seed phrase backup to later

814c6b2

Adds incremental-security.spec.js, including test dapp that sends sig…

a550e45

…ned tx with stand alone localhost provider

Update metamask-responsive-ui for incremental account security changes

cf8dccb

Update backup-notification style and fix responsiveness of seed phras…

9643d52

…e screen

Remove uneeded files from send-eth-with-private-key-test/

2a04d40

Apply linguist flags in .gitattributes for send-eth-with-private-key-…

42e983e

…test/ethereumjs-tx.js

Improve docs in controllers/onboarding.js

de82d3a

Clean up metamask-extension/test/e2e/send-eth-with-private-key-test/i…

75633dc

…ndex.html

Remove unnecessary newlines in a couple first-time-flow/ files

f3a2d45

danjm force-pushed the i5849-incremental-account-security branch from c7dad0d to f3a2d45 Compare August 1, 2019 16:16

Fix import of backup-notification in home.component

33683bb

whymarrh suggested changes Aug 1, 2019

View reviewed changes

whymarrh added 2 commits August 1, 2019 16:12

Merge branch 'develop' into i5849-incremental-account-security

22c4ff2

Fix git attrs file

ccd0bc5

whymarrh approved these changes Aug 1, 2019

View reviewed changes

danjm merged commit 3eff478 into develop Aug 2, 2019

whymarrh deleted the i5849-incremental-account-security branch August 2, 2019 12:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

I5849 incremental account security #6874

I5849 incremental account security #6874

danjm commented Jul 18, 2019

metamaskbot commented Jul 23, 2019

metamaskbot commented Jul 23, 2019

Gudahtt Jul 26, 2019

Gudahtt Jul 26, 2019

Gudahtt Jul 26, 2019

whymarrh left a comment

whymarrh Jul 30, 2019

whymarrh Jul 30, 2019

whymarrh Jul 30, 2019

whymarrh Jul 30, 2019

whymarrh Jul 30, 2019

whymarrh Jul 30, 2019

whymarrh Jul 30, 2019

whymarrh Jul 30, 2019

Gudahtt commented Jul 30, 2019 •

edited

Loading

cjeria commented Jul 30, 2019

metamaskbot commented Jul 31, 2019

metamaskbot commented Aug 1, 2019

metamaskbot commented Aug 1, 2019

whymarrh left a comment

whymarrh Aug 1, 2019

whymarrh Aug 1, 2019

whymarrh left a comment

metamaskbot commented Aug 2, 2019

	* Background controller responsible for maintaining
	* Controller responsible for maintaining

	/home/danjm/kyokan/metamask-extension/test/e2e/send-eth-with-private-key-test/ethereumjs-tx.js linguist-vendored
	test/e2e/send-eth-with-private-key-test/ethereumjs-tx.js linguist-vendored linguist-generated

I5849 incremental account security #6874

I5849 incremental account security #6874

Conversation

danjm commented Jul 18, 2019

metamaskbot commented Jul 23, 2019

metamaskbot commented Jul 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whymarrh 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

Gudahtt commented Jul 30, 2019 • edited Loading

cjeria commented Jul 30, 2019

"Remind me later" option

Component scaling for full-screen

Button states

Behavior

metamaskbot commented Jul 31, 2019

metamaskbot commented Aug 1, 2019

metamaskbot commented Aug 1, 2019

whymarrh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whymarrh left a comment

Choose a reason for hiding this comment

metamaskbot commented Aug 2, 2019

Gudahtt commented Jul 30, 2019 •

edited

Loading