Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#9749] Support importing private key and seed #10100

Merged
merged 3 commits into from
Mar 3, 2020

Conversation

flexsurfer
Copy link
Member

@flexsurfer flexsurfer commented Feb 28, 2020

fixes #9749

QA: keycard is not supported #10101

@flexsurfer flexsurfer requested a review from a team as a code owner February 28, 2020 11:40
@flexsurfer flexsurfer self-assigned this Feb 28, 2020
@status-github-bot
Copy link

Pull Request Checklist

  • Docs: Updated the documentation, if affected
  • Docs: Added or updated inline comments explaining intention of the code
  • Tests: Ensured that all new UI elements have been assigned accessibility IDs
  • Tests: Signaled need for E2E tests with label, if applicable
  • Tests: Briefly described what was tested and what platforms were used
  • UI: In case of UI changes, ensured that UI matches Figma
  • UI: In case of UI changes, requested review from a Core UI designer
  • UI: In case of UI changes, included screenshots of implementation

@status-im-auto
Copy link
Member

status-im-auto commented Feb 28, 2020

Jenkins Builds

Click to see older builds (15)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ d64fc99 #1 2020-02-28 11:51:02 ~10 min ios 📦ipa 📲
✔️ d64fc99 #1 2020-02-28 11:54:14 ~13 min android-e2e 📦apk 📲
✔️ d64fc99 #1 2020-02-28 12:00:02 ~19 min android 📦apk 📲
✔️ 18d2221 #2 2020-02-28 12:55:40 ~10 min ios 📦ipa 📲
✔️ 18d2221 #2 2020-02-28 12:56:54 ~11 min android-e2e 📦apk 📲
✔️ 18d2221 #2 2020-02-28 13:06:44 ~21 min android 📦apk 📲
✔️ 34b4372 #3 2020-02-28 15:42:14 ~10 min ios 📦ipa 📲
✔️ 34b4372 #3 2020-02-28 15:45:52 ~13 min android-e2e 📦apk 📲
✔️ 34b4372 #3 2020-02-28 15:45:52 ~13 min android 📦apk 📲
✔️ 6575b94 #4 2020-03-02 11:08:14 ~11 min ios 📦ipa 📲
✔️ 6575b94 #4 2020-03-02 11:11:04 ~14 min android-e2e 📦apk 📲
✔️ 6575b94 #4 2020-03-02 11:11:04 ~14 min android 📦apk 📲
✔️ 134a675 #5 2020-03-02 11:22:49 ~10 min ios 📦ipa 📲
✔️ 134a675 #5 2020-03-02 11:25:34 ~12 min android 📦apk 📲
✔️ 134a675 #5 2020-03-02 11:25:34 ~13 min android-e2e 📦apk 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ f706a61 #7 2020-03-02 11:48:34 ~10 min ios 📦ipa 📲
✔️ f706a61 #7 2020-03-02 11:50:51 ~12 min android-e2e 📦apk 📲
✔️ f706a61 #7 2020-03-02 11:52:13 ~13 min android 📦apk 📲
✔️ 9515f03 #8 2020-03-02 16:20:04 ~11 min ios 📦ipa 📲
✔️ 9515f03 #8 2020-03-02 16:22:49 ~14 min android-e2e 📦apk 📲
✔️ 9515f03 #8 2020-03-02 16:22:49 ~14 min android 📦apk 📲

@churik churik self-assigned this Feb 28, 2020
@@ -24,7 +24,7 @@
(contains? multiaccounts key-uid))

(re-frame/reg-fx
::validate-mnemonic
:validate-mnemonic
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove the namespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

(fx/defn import-new-account-seed
[{:keys [db]} passphrase hashed-password]
{:db (assoc-in db [:add-account :step] :generating)
:validate-mnemonic [(security/safe-unmask-data passphrase)
Copy link
Contributor

Choose a reason for hiding this comment

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

keep the namespaced keywords and import via alias, if there is a circular dep it just means that the effect is not in the right place from a code organization pov

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@churik
Copy link
Member

churik commented Feb 28, 2020

ISSUE 1: field seed phrase is case-sensitive and contains autosuggestion

Steps:

  • open status
  • go to wallet > Add account > Enter seed phrase (try with capital letters)

Expected result:
Seed phrase input have the same validation rules as in flow for recovery account

Actual result:
Monosnap 2020-02-28 13-12-46

Also, keyboard has turned on autosuggestion when focusing in field (referred to the issue #9034)

@churik
Copy link
Member

churik commented Feb 28, 2020

ISSUE 2: Can add account with invalid password

Steps:

  • open status
  • go to wallet > Add account > Enter seed phrase
  • type invalid password
  • fill other fields

Expected result:
Validation error

Actual result:
photo_2020-02-28 13 39 27
but account is added

@flexsurfer flexsurfer force-pushed the feature/rework-new-accounts-in-wallet branch from d64fc99 to 18d2221 Compare February 28, 2020 12:45
@flexsurfer
Copy link
Member Author

issue1 : fixed

@flexsurfer
Copy link
Member Author

@churik can't reproduce issue 2, btw for importing seed and private key there is no wrong password, because you can use any password you want, yeah i know its confusing

@flexsurfer
Copy link
Member Author

summon @hesterbruikman , so when you import accounts with seed or private key, you can use any new password to import them , that might be confusing for sure

@hesterbruikman
Copy link
Contributor

summon @hesterbruikman , so when you import accounts with seed or private key, you can use any new password to import them , that might be confusing for sure

If I recall correctly this should be your existing password. Also as it says, Enter your password. cc @guylouis to verify

If it is indeed the existing password, validation is missing

@flexsurfer
Copy link
Member Author

@hesterbruikman we just store new account on the device we can use any password, it's not related anyhow to multiaccount, so its up to us how we want to implement it, if we want new password we should ask it twice , if multiaccount, we need to verify it somehow @gravityblast ?

@hesterbruikman
Copy link
Contributor

hesterbruikman commented Feb 28, 2020

I'm pretty sure the idea was to simply ask for the same password as to not let people juggle different passwords to sign within the same multiaccount in Status. That's why there's no confirm password and we only say Enter your password. Exception is the case where keys are encrypted with a password set up elsewhere.

@guylouis
Copy link
Contributor

100%

In my mind, from our past discussions, a new account imported from a private key should be protected the same way an account on the BIP44 tree is, and we should not create a new password to avoid any added complexity.

If the private key is encrypted in a json file (but I don't think this issue is about this case) of course, that's something else and at the time of importing, this encryption password is being asked.

@flexsurfer flexsurfer force-pushed the feature/rework-new-accounts-in-wallet branch from 18d2221 to 34b4372 Compare February 28, 2020 15:31
@flexsurfer
Copy link
Member Author

@churik issue 2 fixed, also now we check if password is the same as for multiaccount

@churik
Copy link
Member

churik commented Feb 28, 2020

ISSUE 3: Keychain suggests to save password

Steps:

  • open status
  • go to wallet > Add account > Enter seed phrase
  • fill all fields
  • proceed

Expected result:
Account is added, no suggestions about storing password

Actual result:
photo_2020-02-28 16 47 34

@churik
Copy link
Member

churik commented Feb 28, 2020

ISSUE 4: Can add 2 accounts with same seed

Steps:

  1. open status
  2. go to wallet > Add account > Enter seed phrase
  3. fill all fields
  4. proceed
  5. repeat steps 2-5 with same seed phrase

Expected result:
can't add same account twice

Actual result:
photo_2020-02-28 16 54 52
It will be deleted if you relogin.
Actually you can create new account with same seed phrase via recovery flow as well - not sure what is expected result here.

@flexsurfer
Copy link
Member Author

flexsurfer commented Feb 28, 2020

ISSUE 3 - hm is this solved somehow for other password fields ? i remember we had this but not sure how we solved it

@churik
Copy link
Member

churik commented Feb 28, 2020

@flexsurfer
#8837 - seems it's gone after redesign, but it appeared again here

@churik
Copy link
Member

churik commented Feb 28, 2020

ISSUE 7: With whitspaces between, after or before seed phrase restores different account

See #9670

Steps:

  1. open status
  2. go to wallet > Add account > Enter seed phrase
  3. on seed phrase put spaces in the end of phrase
  4. fill all fields
  5. proceed

Expected result:
account is restored according to trimmed seed phrase

Actual result:
with whitespaces in different places different accounts are recovered

@churik
Copy link
Member

churik commented Feb 28, 2020

@flexsurfer here fix for several cases will be required (and ideally 2 new cases).
So please wait for my commit.
Also would be awesome if you can add accessibility-ids to all fields in new screens - it makes e2e easier to support.
Thanks!

{:address address
:public-key publicKey
:type type
:path (if (string/starts-with? path "m/")
Copy link
Contributor

Choose a reason for hiding this comment

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

can we extract this path derivation part to a separate fn?

"/" (last (string/split path "/")))
path)}])))))))))))

(def pass-error "cannot retrieve a valid key for a given account: could not decrypt key with given password")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that returned form status-go side? We probably need to start using error codes or something, because comparing messages might not be the best idea and definitely isn't a very reliable way.

Comment on lines 98 to 101
(status/verify address hashed-password
#(re-frame/dispatch [:wallet.accounts/add-new-account-password-verifyied
%
hashed-password]))))
Copy link
Contributor

Choose a reason for hiding this comment

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

   (status/verify 
    address hashed-password
    #(re-frame/dispatch
      [:wallet.accounts/add-new-account-password-verifyied % hashed-password]))))

not a big deal, but that lonely percent sign looks weird to me

@flexsurfer
Copy link
Member Author

flexsurfer commented Mar 2, 2020

@churik thanks all issue fixed
ISSUE 3 - can't be fixed

@flexsurfer flexsurfer force-pushed the feature/rework-new-accounts-in-wallet branch from 6575b94 to 134a675 Compare March 2, 2020 11:12
@churik
Copy link
Member

churik commented Mar 2, 2020

@corpetty
is it OK to release this feature with #10100 (comment) ?
Seems there is no way to fix it shortly.

@flexsurfer flexsurfer force-pushed the feature/rework-new-accounts-in-wallet branch from 83e137d to f706a61 Compare March 2, 2020 11:38
@status-im-auto
Copy link
Member

100% of end-end tests have passed

Total executed tests: 4
Failed tests: 0
Passed tests: 4

Passed tests (4)

Click to expand
1. test_add_account_to_multiaccount_instance_seed_phrase
Device sessions

2. test_add_account_to_multiaccount_instance_private_key
Device sessions

3. test_add_account_to_multiaccount_instance_generate_new
Device sessions

4. test_mobile_data_usage_popup_stop_syncing
Device sessions

@churik
Copy link
Member

churik commented Mar 3, 2020

Tested:

  • fields validation on new screens
  • using new types of accounts in DApps + collectibles
  • changing settings on new accounts
  • using new accounts in commands
  • using ENS from main account
  • adding account via seed phrase in wallet and restoring it in user flow
  • changing networks
  • attempt to add the same account twice (seed phrase + public key)
  • search for sensitive info in logs

Apart from #9895 and #8837 don't see other issues.
Amazing work!

@flexsurfer flexsurfer force-pushed the feature/rework-new-accounts-in-wallet branch from 9515f03 to 7b7f567 Compare March 3, 2020 11:52
@flexsurfer flexsurfer merged commit 7b7f567 into develop Mar 3, 2020
@delete-merged-branch delete-merged-branch bot deleted the feature/rework-new-accounts-in-wallet branch March 3, 2020 11:52
@hesterbruikman
Copy link
Contributor

👋@flexsurfer what's the behavior for Keycard in this current implementation? @rasom mentioned we're missing designs for Add account > Generate keys in case of Keycard. Assuming the same is true here.

@flexsurfer
Copy link
Member Author

the behavior is that Keycard is not supported in this PR , here is the issue #10101

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support importing private key
7 participants