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

(Feature) HD wallets' support #183

Merged
merged 22 commits into from
Nov 21, 2018
Merged

(Feature) HD wallets' support #183

merged 22 commits into from
Nov 21, 2018

Conversation

vbaranov
Copy link
Collaborator

@vbaranov vbaranov commented Nov 8, 2018

Menu item:
screen shot 2018-11-08 at 14 13 09

Choose HD wallet page:
screen shot 2018-11-08 at 14 13 42

Trezor:
screen shot 2018-11-08 at 14 17 19

Ledger:
screen shot 2018-11-08 at 14 18 28

Guide to configure Trezor simulator: https://github.com/poanetwork/nifty-wallet/blob/master/docs/trezor-emulator.md

@vbaranov vbaranov added this to the NW release 4.9.0 milestone Nov 8, 2018
@dennis00010011b
Copy link

dennis00010011b commented Nov 9, 2018

@vbaranov
Improper error message if 2nd attempt to connect to Trezor

screen shot 2018-11-08 at 5 09 33 pm

Steps (don't plug-in HD wallet)

  1. Go to 'Connect to hardware wallet'
  2. Select 'Trezor', click 'Connect'
  3. Switch account or refresh the page, main screen will opens
  4. Go to 'Connect to hardware wallet'
  5. Select 'Trezor', click 'Connect'

@dennis00010011b
Copy link

@vbaranov
Misspelling in notification. Letter 'd' in word 'device' should be capital
screen shot 2018-11-12 at 5 45 37 pm

@dennis00010011b
Copy link

dennis00010011b commented Nov 13, 2018

@vbaranov
See #183 (comment)
App is frozen if attempting to delete Trezor's account after choice to forget device

Steps for reproduce:

  1. Connect TrezorOne device to PC
  2. Open NW, import any account from device
  3. Go to Accounts-> Connect hw and click 'Forget this device'
  4. Open Accounts and select close icon for deleting Trezor's account

Actual result:

Expected result:

  • user should be able to delete hw account at any time
    OR
  • hw accounts should be removed if user selects to forget device

screen shot 2018-11-12 at 7 52 14 pm

@vbaranov
Copy link
Collaborator Author

@dennis00010011b

Improper error message if 2nd attempt to connect to Trezor

Fixed here a33ba78

App is frozen if attempting to delete Trezor's account after choice to forget device

Good catch! Also, fixed here a33ba78

Misspelling in notification. Letter 'd' in word 'device' should be capital

Fixed here bad4943

@dennis00010011b
Copy link

dennis00010011b commented Nov 13, 2018

Improper error message if 2nd attempt to connect to Trezor

Fixed here a33ba78

App is frozen if attempting to delete Trezor's account after choice to forget device

Good catch! Also, fixed here a33ba78

Misspelling in notification. Letter 'd' in word 'device' should be capital

Fixed here bad4943

Tested it with Trezor One device. These issues were not found

@dennis00010011b
Copy link

@vbaranov
Uncaught error in console when try to export private key for Trezor's account
screen shot 2018-11-13 at 12 14 09 pm

@dennis00010011b
Copy link

dennis00010011b commented Nov 13, 2018

@vbaranov
Ledger: notification's text is incorrect when a transaction rejected by device
screen shot 2018-11-13 at 2 23 23 pm

@dennis00010011b
Copy link

dennis00010011b commented Nov 14, 2018

@vbaranov
Error messages should be more friendly and understandable for non-proficient user. Something like that:

  1. Please plug-in Ledger, open Eth app and try again

screen shot 2018-11-13 at 4 22 07 pm

  1. Can not recognize device

screen shot 2018-11-13 at 4 30 08 pm

  1. Ledger connection issues, follow the instruction https://support.ledgerwallet.com/hc/en-us/articles/115005165269-Fix-connection-issues

screen shot 2018-11-12 at 9 45 16 pm

@dennis00010011b
Copy link

@vbaranov
Ledger: list of accounts is lost (reconnection is needed ) when user exports the private key for any account
https://www.useloom.com/share/b83694df7c544a59b2849b33d9ac4899
Steps:

  1. Connect Ledger, unlock any account
  2. Export PR for any account
  3. Go to Accounts-> Connect HW

Expected result: list of accounts should be displayed
Actual result: reconnection is needed

Same things happens when delete account
Device isn't really disconnected, just account list isn't displayed

@dennis00010011b
Copy link

@vbaranov
Empty screen while waiting for confirmation from device. Could be better to display text 'Please confirm on device ...' and countdown timer for 30 sec (because NW automatically fails of transaction if there is no confirmation from device in during 30 sec)
screen shot 2018-11-13 at 6 17 24 pm

@vbaranov
Copy link
Collaborator Author

@dennis00010011b

Ledger: notification's text is incorrect when a transaction rejected by device

This has been fixed.

Please, create separate issues for other findings. I think we could solve them in a separate PRs. They require some sort of refactoring. As about friendly errors, of course, we can parse errors from HD wallet and display friendly errors, but it will be hard to manage because errors definitions from HD device are out from our control, and if errors will be changed on HD wallet side, every time we will need to change parsing on NW side.

)
}

capitalizeDevice = (device) => {

Choose a reason for hiding this comment

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

This function seems to do the same as capitalizeFirstLetter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. Duplicate has been removed 256e585

@vbaranov vbaranov merged commit b2066c4 into develop Nov 21, 2018
@ghost ghost removed the awaiting for review label Nov 21, 2018
@vbaranov vbaranov deleted the hw-support branch November 21, 2018 12:03
@vbaranov vbaranov mentioned this pull request Nov 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants