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

[Fixes #9088] Store eip1581 path #9096

Merged
merged 1 commit into from Oct 4, 2019
Merged

[Fixes #9088] Store eip1581 path #9096

merged 1 commit into from Oct 4, 2019

Conversation

@cammellos
Copy link
Member

cammellos commented Oct 2, 2019

When creating the account we store as well the path specified in eip1581
https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1581.md ,
m / 43' / 60' / 1581'.

The reason for doing so is that eventually we might want to derive an
encryption key from it, which would require the user to re-enter their
seed phrase if we would not store this.

This commit changes also the behavior not to store the master key, and instead store the key at
m /44'/60' /0'/0 from which wallets can be derived.

I could not test on keycard as I do not own one.

status: ready

@cammellos cammellos requested review from gravityblast, yenda and flexsurfer Oct 2, 2019
@cammellos cammellos requested a review from status-im/status-core as a code owner Oct 2, 2019
@cammellos cammellos self-assigned this Oct 2, 2019
@auto-assign auto-assign bot removed the request for review from status-im/status-core Oct 2, 2019
@status-github-bot

This comment has been minimized.

Copy link

status-github-bot bot commented Oct 2, 2019

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-github-bot status-github-bot bot added this to REVIEW in Pipeline for QA Oct 2, 2019
@status-im-auto

This comment has been minimized.

Copy link
Member

status-im-auto commented Oct 2, 2019

Jenkins Builds

Click to see older builds (24)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 945ce4c #1 2019-10-02 13:18:21 ~9 min ios 📦ipa 📲
✔️ 945ce4c #1 2019-10-02 13:22:47 ~13 min macos 📦dmg
✔️ 945ce4c #1 2019-10-02 13:23:04 ~14 min linux 📦App
✔️ 945ce4c #1 2019-10-02 13:23:34 ~14 min windows 📦exe
✔️ 945ce4c #1 2019-10-02 13:23:37 ~14 min android-e2e 📦apk 📲
✔️ 945ce4c #1 2019-10-02 13:23:45 ~14 min android 📦apk 📲
✔️ c51b70a #3 2019-10-03 07:41:05 ~11 min ios 📦ipa 📲
✔️ c51b70a #3 2019-10-03 07:42:13 ~12 min android-e2e 📦apk 📲
✔️ c51b70a #3 2019-10-03 07:42:18 ~12 min android 📦apk 📲
✔️ c51b70a #3 2019-10-03 07:44:35 ~14 min linux 📦App
✔️ c51b70a #3 2019-10-03 07:44:46 ~15 min windows 📦exe
✔️ c51b70a #3 2019-10-03 07:49:38 ~19 min macos 📦dmg
✔️ c4ec970 #4 2019-10-03 09:51:28 ~9 min ios 📦ipa 📲
✔️ c4ec970 #4 2019-10-03 09:55:06 ~13 min linux 📦App
✔️ c4ec970 #4 2019-10-03 09:55:10 ~13 min macos 📦dmg
✔️ c4ec970 #4 2019-10-03 09:55:20 ~13 min windows 📦exe
✔️ c4ec970 #4 2019-10-03 09:56:12 ~14 min android-e2e 📦apk 📲
✔️ c4ec970 #4 2019-10-03 09:56:17 ~14 min android 📦apk 📲
✔️ a786f2a #5 2019-10-03 13:16:28 ~10 min ios 📦ipa 📲
✔️ a786f2a #5 2019-10-03 13:19:51 ~13 min linux 📦App
✔️ a786f2a #5 2019-10-03 13:19:57 ~13 min macos 📦dmg
✔️ a786f2a #5 2019-10-03 13:20:02 ~13 min windows 📦exe
✔️ a786f2a #5 2019-10-03 13:21:16 ~15 min android-e2e 📦apk 📲
✔️ a786f2a #5 2019-10-03 13:21:28 ~15 min android 📦apk 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ d179a17 #7 2019-10-04 11:03:01 ~9 min ios 📦ipa 📲
✔️ d179a17 #7 2019-10-04 11:07:23 ~13 min macos 📦dmg
✔️ 1e3ede7 #6 2019-10-04 11:05:45 ~14 min android 📦apk 📲
✔️ 1e3ede7 #6 2019-10-04 11:05:57 ~14 min linux 📦App
✔️ 1e3ede7 #6 2019-10-04 11:06:00 ~14 min windows 📦exe
✔️ 1e3ede7 #6 2019-10-04 11:06:03 ~14 min android-e2e 📦apk 📲
Copy link
Member

gravityblast left a comment

LGTM

@cammellos cammellos force-pushed the chore/store-eip1581 branch 2 times, most recently from 68354f0 to c51b70a Oct 3, 2019
@cammellos

This comment has been minimized.

Copy link
Member Author

cammellos commented Oct 3, 2019

@guylouis @dmitryn I am not able to test this change on keycard accounts as I don't own one.
The relevant changes is that the master key is not stored anymore on the filesystem for normal accounts (as I understand with a keycard this has never been the case, the master key is indeed stored, but only on the keycard).
For non-keycard accounts we store m /44'/60' /0'/0 and use that to create new wallet accounts (which the keycard is not storing), and if that has not been created, we use the address of the master key instead (which will fail for non keycard accounts, but should work for keycards, as my understanding is that we do store the master key there).

Is that correct or I misunderstand something?

@churik are you able to test creating/recovering an account and generating new wallets (in both cases) on this?

@cammellos cammellos requested a review from corpetty Oct 3, 2019
@cammellos cammellos force-pushed the chore/store-eip1581 branch from c51b70a to c4ec970 Oct 3, 2019
@cammellos cammellos moved this from REVIEW to E2E Tests in Pipeline for QA Oct 3, 2019
@statustestbot

This comment has been minimized.

Copy link

statustestbot commented Oct 3, 2019

96% of end-end tests have passed

Total executed tests: 47
Failed tests: 2
Passed tests: 45

Failed tests (2)

Click to expand
1. test_block_user_from_public_chat

Device 1: Type 'Before block from 1' to ChatMessageInput
Device 1: Tap on SendMessageButton

Device 2: 'ChatMessageInput' is not found on the screen

Device sessions

2. test_offline_messaging_1_1_chat

Device 2: Tap on AirplaneModeButton
Device 1: Tap on AirplaneModeButton

Device 1: 'ConnectionStatusText' is still visible on the screen after 30 seconds

Device sessions

Passed tests (45)

Click to expand
1. test_filters_from_daap
Device sessions

2. test_copy_and_paste_messages
Device sessions

3. test_send_transaction_from_daap
Device sessions

4. test_deploy_contract_from_daap
Device sessions

5. test_open_transaction_on_etherscan
Device sessions

6. test_public_chat_messaging
Device sessions

7. test_long_press_to_delete_1_1_chat
Device sessions

8. test_password_in_logcat_sign_in
Device sessions

9. test_text_message_1_1_chat
Device sessions

10. test_add_to_contacts
Device sessions

11. test_sign_typed_message
Device sessions

12. test_unread_messages_counter_1_1_chat
Device sessions

13. test_ens_in_public_chat
Device sessions

14. test_logcat_send_transaction_from_daap
Device sessions

15. test_send_message_in_group_chat
Device sessions

16. test_logcat_send_transaction_from_wallet
Device sessions

17. test_send_token_with_7_decimals
Device sessions

18. test_modify_transaction_fee_values
Device sessions

19. test_send_eth_from_wallet_to_address
Device sessions

20. test_add_account_to_multiaccount_instance
Device sessions

21. test_manage_assets
Device sessions

22. test_long_press_to_delete_public_chat
Device sessions

23. test_send_emoji
Device sessions

24. test_search_chat_on_home
Device sessions

25. test_logcat_recovering_account
Device sessions

26. test_can_add_existing_ens
Device sessions

27. test_messaging_in_different_networks
Device sessions

28. test_logcat_backup_recovery_phrase
Device sessions

29. test_logcat_sign_message_from_daap
Device sessions

30. test_switch_users_and_add_new_account
Device sessions

31. test_send_stt_from_wallet
Device sessions

32. test_login_with_new_account
Device sessions

33. test_start_chat_with_ens
Device sessions

34. test_add_contact_from_public_chat
Device sessions

35. test_send_two_transactions_one_after_another_in_dapp
Device sessions

36. test_password_in_logcat_creating_account
Device sessions

37. test_backup_recovery_phrase
Device sessions

38. test_offline_status
Device sessions

39. test_open_google_com_via_open_dapp
Device sessions

40. test_unread_messages_counter_public_chat
Device sessions

41. test_sign_message_from_daap
Device sessions

42. test_user_can_remove_profile_picture
Device sessions

43. test_share_contact_code_and_wallet_address
Device sessions

44. test_refresh_button_browsing_app_webview
Device sessions

45. test_backup_recovery_phrase_warning_from_wallet
Device sessions

@churik

This comment has been minimized.

Copy link
Member

churik commented Oct 3, 2019

OFC, on it!

@corpetty

This comment has been minimized.

Copy link
Contributor

corpetty commented Oct 3, 2019

It seems as though this was prematurely approved without testing out keycard. Am I misreading that? If that is the case, maybe a label like needs testing would be helpful as status: ready signals that nothing needs to be done even though it's intent was more along the lines of "the code is done."

@churik

This comment has been minimized.

Copy link
Member

churik commented Oct 3, 2019

Tested on Android 8:

  • creating ordinary account (passcode with special chars)
  • restoring ordinary account (passcode with special chars)
  • creating account with keycard
  • restoring account with keycard (create account, reinstall, restore)

All cases work fine for me.

@churik

This comment has been minimized.

Copy link
Member

churik commented Oct 3, 2019

@corpetty we have such label request-manual-qa

@cammellos

This comment has been minimized.

Copy link
Member Author

cammellos commented Oct 3, 2019

@corpetty I think there's some confusion on the workflow:

status: ready means that the PR is ready to be reviewed (as opposed to status: wip which means that we haven't finished writing the code yet), not that is ready to be merged or that it has been tested (we add a label/move in the to merge review).

In terms of testing, up until last week all PRs went through manual QA, and I believe we add a label when manual QA is required. This one does not really need manual QA, other than someone needs to validate that it works with keycard, as specified in the comment (any developer can do).

@churik thanks, I'll wait for either @yenda or @flexsurfer to approve then is ready to merge.

@yenda

This comment has been minimized.

Copy link
Member

yenda commented Oct 3, 2019

Does it means that a second account created with keycard will be different than one created with the same multiaccount without keycard?

@cammellos

This comment has been minimized.

Copy link
Member Author

cammellos commented Oct 3, 2019

@yenda No it should not, the path is identical, m /44'/60' /0'/0/x, the only difference is that in the case of keycard it will be generated from the master key, while if it's not a keycard account, it will be generated from the BIP44 Wallet Root key.

But best to check that is the case:

@churik would you mind doing a test please if you have some time?

  1. Import the same account once with keycard once without
  2. Create an extra wallet on both, they should have identical addresses (the initial one as well)

Thanks!

@cammellos

This comment has been minimized.

Copy link
Member Author

cammellos commented Oct 3, 2019

mmh turns out that they do seem to differ (I tried with a pre this commit and this commit, not on keycard), it might be that the path is relative to the key, let me check

@cammellos cammellos force-pushed the chore/store-eip1581 branch from c4ec970 to a786f2a Oct 3, 2019
@cammellos

This comment has been minimized.

Copy link
Member Author

cammellos commented Oct 3, 2019

@yenda turns out we need to use the relative path, so if deriving from m/a/b/c you derive m/0 for example, but if deriving from the master key, you'd derive m/a/b/c/0, which makes sense.

@churik could you have a look at the test with keycard when you have some time? It should show identical wallet addresses now.
Thanks!

@churik

This comment has been minimized.

Copy link
Member

churik commented Oct 4, 2019

@cammellos

Import the same account once with keycard once without

It shouldn't be possible according to #9062 (comment)

Create an extra wallet on both, they should have identical addresses (the initial one as well)

Due to #9019 it is not possible for now

So can't help here.

@guylouis

This comment has been minimized.

Copy link

guylouis commented Oct 4, 2019

@cammellos
just want to make sure: are we blocked here by something ?
can keycard team help in anyway ? cc @dmitryn

@cammellos

This comment has been minimized.

Copy link
Member Author

cammellos commented Oct 4, 2019

@guylouis No, I think it's fine, as one functionality is not supported (restore same account with keycard), and the other is not implemented yet (add multiple wallets with keycard).
Just waiting for a review from a clojure dev, otherwise is ready to merge. cc @flexsurfer @yenda

@flexsurfer

This comment has been minimized.

Copy link
Member

flexsurfer commented Oct 4, 2019

@cammellos could you summarize why so many changes? I thought we just want to store an encryption key in this PR?

(if error
(re-frame/dispatch [::generate-new-account-error])
(let [path (str constants/path-root "/" path-num)]
(fn [{:keys [derivation-info

This comment has been minimized.

Copy link
@flexsurfer

flexsurfer Oct 4, 2019

Member

just wondering why keys in a column and not in a row?

This comment has been minimized.

Copy link
@cammellos

cammellos Oct 4, 2019

Author Member

i like columns :) but i can put them on a row, np

@cammellos

This comment has been minimized.

Copy link
Member Author

cammellos commented Oct 4, 2019

@flexsurfer that was he initial idea, but turns out we were also storing the master key on disk, which we should not have ( after a discussion with @corpetty), so I have changed the behaviour to store the root wallet key instead. It is explained as well in the issue

@cammellos cammellos force-pushed the chore/store-eip1581 branch 3 times, most recently from d179a17 to ba34af0 Oct 4, 2019
master key

When creating the account we store as well the path specified in eip1581
https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1581.md ,
`m / 43' / 60' / 1581'`.

The reason for doing so is that eventually we might want to derive an
encryption key from it, which would require the user to re-enter their
seed phrase if we would not store this.

This commit changes the behavior not to store the master key, and
instead store `m /44'/60' /0'/0`, from which wallets are now derived.

Signed-off-by: Andrea Maria Piana <andrea.maria.piana@gmail.com>
@cammellos cammellos merged commit ba34af0 into develop Oct 4, 2019
12 checks passed
12 checks passed
packages-check-bot No changes to dependencies
Details
GPG All commits have a verified GPG signature
WIP Ready for review
Details
security/snyk - android/app/build.gradle (Status-im) No manifest changes detected
security/snyk - android/build.gradle (Status-im) No manifest changes detected
security/snyk - desktop/js_files/package.json (Status-im) No manifest changes detected
security/snyk - fastlane/Gemfile.lock (Status-im) No manifest changes detected
security/snyk - mobile/js_files/package.json (Status-im) No manifest changes detected
security/snyk - modules/react-native-desktop-linking/package.json (Status-im) No manifest changes detected
security/snyk - modules/react-native-desktop-notification/package.json (Status-im) No manifest changes detected
security/snyk - modules/react-native-status/package.json (Status-im) No manifest changes detected
security/snyk - test/appium/requirements.txt (Status-im) No manifest changes detected
Pipeline for QA automation moved this from E2E Tests to DONE Oct 4, 2019
@delete-merged-branch delete-merged-branch bot deleted the chore/store-eip1581 branch Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.