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.

Sign up for GitHub

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

Jump to bottom

Add Ledger address verification #363

Merged
merged 15 commits into from
Mar 23, 2019
Merged

Add Ledger address verification #363

merged 15 commits into from
Mar 23, 2019

Conversation

SebastienGllmt
Copy link
Contributor

@SebastienGllmt SebastienGllmt commented Mar 21, 2019 •
edited by nicarq
Loading

  • expose showAddress in bridge (see here)
  • propogate derivation path in code
  • add icon in receive screen for address verification
  • create dialog for address verification
  • only show dialog when using Ledger
  • error message when device not connected
  • loader while loading and block button

Note: we should add this to Trezor too once we have the time

Here is what the UI looks like

Receive screen

image

Address details dialog (non-hardware wallet)

image

Address details dialog (hardware wallet)

image

Screen Shot 2019-03-23 at 1 32 53 AM

Screen Shot 2019-03-23 at 1 33 16 AM

@SebastienGllmt SebastienGllmt added fix for next version This issue will be taken care of asap small-scale change Changes that are not NFC but small enough in scope that they don't need to go through triage labels Mar 21, 2019
@SebastienGllmt SebastienGllmt added the WIP / DO NOT MERGE Shows that a PR shouldn't be merge label Mar 21, 2019
@nicarq nicarq removed the WIP / DO NOT MERGE Shows that a PR shouldn't be merge label Mar 23, 2019
@SebastienGllmt
Copy link
Contributor Author

Found a bug. If open the Address Details button, close it, then open it again the verify address button breaks until you refresh the page

@SebastienGllmt SebastienGllmt added the WIP / DO NOT MERGE Shows that a PR shouldn't be merge label Mar 23, 2019
@SebastienGllmt SebastienGllmt removed the WIP / DO NOT MERGE Shows that a PR shouldn't be merge label Mar 23, 2019
@SebastienGllmt SebastienGllmt merged commit c481aad into develop Mar 23, 2019
@SebastienGllmt SebastienGllmt deleted the feature/ledgerVerify branch March 23, 2019 10:46
@SebastienGllmt SebastienGllmt mentioned this pull request Mar 23, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicarq nicarq Awaiting requested review from nicarq

@shinsuny shinsuny Awaiting requested review from shinsuny

Assignees
No one assigned
Labels
fix for next version This issue will be taken care of asap small-scale change Changes that are not NFC but small enough in scope that they don't need to go through triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SebastienGllmt @nicarq