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

[DDW-491] Verify HW wallet receiving address - Ledger #2282

Merged

Conversation

tomislavhoracek
Copy link
Contributor

@tomislavhoracek tomislavhoracek commented Dec 17, 2020

This PR introduces Hardware Wallet receiving address verification for Ledger devices.

Todos

Testing Checklist

In-App testing

  • Go to wallet receive screen for some Hardware Wallet (only Ledger is supported) and check address verification process. You can test the same steps / cases as is for signing transactions. Instead of "Sign transaction" you will see Address Verification message.
  • Address verification is enabled ONLY for Ledger and should not be visible for Trezor or Regular wallets.
  • Spending and Staking paths are added to the "Share wallet address" for ALL types of wallets under QR code.

NOTES:

  1. I added screenshots with titles / descriptions in the exact order on how the process is going so please check before you start with testing

  2. There are multiple cases presented:

  • On the first attempt you will be asked to export the address (with device) and after that address will also show on the device. When you confirm all the actions you will see that the first checkbox is checked in UI. After that, you will be asked to answer a question ( is address true or not). If you select "No, reverify", you will pass through the same process but with one additional option to select (inside answers section), "No, I am sure that address displayed in Daedalus is different from the address displayed on my Ledger device" one. If you select that option new section will appear. This section will ask you to confirm that address is not valid. If you check that the "Submit request" button becomes enabled, it is disabled otherwise.
  • If you select an option that address is valid you should see "Save QR" and "Save PDF" buttons ass well as the possibility to enter a PDF note.
  • Important thing is also that both checkboxes on the top "Daedalus verified the address" and "You have verified the address" are disabled and will be checked automatically as you are walking through the process. The first one is checked after you confirm everything in the device and the second one when you choose an option from "Is the address you have verified correctly".

Test Cases

Scenario 1 - Access to share wallet address dialog with device
disconnected

Given that I am on the receive screen from a Ledger X|S device
And the device is not connected
When I click on "Share" for an address on the list
Then I can see a dialog with the address
And it displays an input that asks to connect the device to validate the address

Scenario 2 - Access to share wallet address dialog with the device connected

Given that I am on the receive screen from a Ledger X|S device
And the device is connected and unlocked
And Cardano app is not running
When I click on "Share" for an address on the list
Then I can see a dialog with the address
And it displays an input that asks to run the Cardano app to validate the address

Scenario 3 - Access to share wallet address dialog with Cardano app running

Given that I am on the receive screen from a Ledger X|S device
And the device is connected, unlocked, and running the Cardano app
When I click on "Share" for an address on the list
Then I can see a dialog with the address
And it displays an input that asks to export the public key

Scenario 4 - Export public key

Given that I am on the "share wallet address" dialog from a Ledger X|S device
And the device is connected, unlocked, and running the Cardano app
And Daedalus asks to export the public key
When I export the public key on the device
Then I can see the input now asks to verify the address on the device

Scenario 5 - Verify the Address on the device

Given that I am on the "share wallet address" dialog from a Ledger X|S device
And the device is connected, unlocked, and running the Cardano app
And Daedalus asks to verify the address
When I verify that the address displayed on the device matches the address on Daedalus
Then I can see the input now asks to answer the question below
And the first check below the input is checked
And I can see the question "Is the address you verified correct?"
And I can see two options at the bottom of the dialog to answer it

Scenario 6 - Answer that the address is correct

Given that I am on the "share wallet address" dialog from a Ledger X|S device
And Daedalus asks to answer the question at the bottom
When I click on "Yes"
Then I can see the input is highlighted in green 
And the second check below the input is checked
And the other option at the bottom is hidden
And the buttons "Save as image" and "Save as PDF" are displayed
And the input to insert notes for the pdf is displayed

Scenario 7 - Answer that the address is incorrect

Given that I am on the "share wallet address" dialog from a Ledger X|S device
And the device is connected, unlocked, and running the Cardano app
And Daedalus asks to answer the question at the bottom
When I click on "No, reverify"
Then I can see the question and options are hidden
And Daedalus asks to verify the address on the device again

Scenario 8 - Recheck address

Given that I am on the "share wallet address" dialog from a Ledger X|S device
And I have checked that the address was incorrect before
And the device is connected, unlocked, and running the Cardano app
And Daedalus asks to verify the address
When I verify the address on the device
Then I can see the input now asks to answer the question below
And I can see the question "Is the address you verified correct?"
And I can see three options at the bottom of the dialog to answer it

Scenario 9 - Confirm the address is not correct

Given that I am on the "share wallet address" dialog from a Ledger X|S device
And I have checked that the address was incorrect before
And the device is connected, unlocked, and running the Cardano app
And Daedalus asks to verify the address
When I verify the address on the device
And I select the option "No, I am sure" at the bottom
Then I can see the input is highlighted in red 
And the other options are hidden
And a checkbox to confirm the address is not correct is displayed
And disabled "Submit request" button is displayed at the bottom

Scenario 10 - Open form to submit a request

Given that I am on the "share wallet address" dialog from a Ledger X|S device
And I have confirmed that the address was incorrect
When I check the option "Yes, I am sure"
And I click the "Submit Request" button
Then the form to submit a request is open in the default browser

Scenario 10 - Reject exporting the Address on the device

Given that I am on the "share wallet address" dialog from a Ledger X|S device
And the device is connected, unlocked, and running the Cardano app
And Daedalus asks to verify the address
When I reject the export of the address on the device
Then I can see the input is highlighted in red
And it indicates that the verification failed

Testing Summary


Screenshots

ENGLISH

1. Start Veifying

1  Verify (start) - EN

2. Answer to the question

2  Answer to question

2.1. Answer - YES

2 1  YES - answer

2.2. Answer - NO, reverify ( State after reverifying)

2 2 NO - reverify

2.2.1 Answer NO after reverifying

2 2 1 - Wrong address confirmed by the user

2.2.2 Confirm wrong address

2 2 2 - Confirm manual check

3. Rejected Verification

3  Verification failed - cancel address export

4. Default Regular and Trezor wallets

4  Default Regular and Trezor wallets

JAPANESE

1. Start Veifying

1  Verify (start) - JP

2. Answer to the question

2  Answer to question - JP

2.1. Answer - YES

2 1  YES - answer - JP

2.2. Answer - NO, reverify ( State after reverifying)

2 2 NO - reverify - JP

2.2.1 Answer NO after reverifying

2 2 1 - Wrong address confirmed by the user - JP

2.2.2 Confirm wrong address

2 2 2 - Confirm manual check - JP

3. Rejected Verification

3  Verification failed - cancel address export - JP

4. Default Regular and Trezor wallets

4  Default Regular and Trezor wallets - JP


Review Checklist

Basics

  • PR has been assigned and has appropriate labels (feature/bug/chore, release-x.x.x)
  • PR is updated to the most recent version of the target branch (and there are no conflicts)
  • PR has a good description that summarizes all changes
  • PR has default-sized Daedalus window screenshots or animated GIFs of important UI changes:
    • In English
    • In Japanese
  • CHANGELOG entry has been added to the top of the appropriate section (Features, Fixes, Chores) and is linked to the correct PR on GitHub
  • Automated tests: All acceptance and unit tests are passing (yarn test)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in development build (yarn dev)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in production build (yarn package / CI builds)
  • There are no flow errors or warnings (yarn flow:test)
  • There are no lint errors or warnings (yarn lint)
  • There are no prettier errors or warnings (yarn prettier:check)
  • There are no missing translations (running yarn manage:translations produces no changes)
  • Text changes are proofread and approved (Jane Wild / Amy Reeve)
  • Japanese text changes are proofread and approved (Junko Oda)
  • UI changes look good in all themes (Alexander Rukin)
  • Storybook works and no stories are broken (yarn storybook)
  • In case of dependency changes yarn.lock file is updated

Code Quality

  • Important parts of the code are properly commented and documented
  • Code is properly typed with flow
  • React components are split-up enough to avoid unnecessary re-renderings
  • Any code that only works in main process is neatly separated from components

Testing

  • New feature/change is covered by acceptance tests
  • New feature/change is manually tested and approved by QA team
  • All existing acceptance tests are still up-to-date
  • New feature/change is covered by Daedalus Testing scenario
  • All existing Daedalus Testing scenarios are still up-to-date

After Review

  • Merge the PR
  • Delete the source branch
  • Move the ticket to done column on the YouTrack board
  • Update Slack QA thread by marking it with a green checkmark

@tomislavhoracek tomislavhoracek requested review from nikolaglumac and a team December 18, 2020 17:15
@nikolaglumac nikolaglumac removed the WIP label Dec 19, 2020
@nikolaglumac
Copy link
Contributor

@ManusMcCole can you please test this one? 🙏

@nikolaglumac nikolaglumac changed the title [DDW-491] Verify Hardware Wallet Receiving Address [DDW-491] Verify Hardware Wallet Receiving Address for Ledger devices Dec 21, 2020
@darko-mijic darko-mijic changed the title [DDW-491] Verify Hardware Wallet Receiving Address for Ledger devices [DDW-491] Verify HW wallet receiving address - Ledger Dec 21, 2020
@gabriela-ponce
Copy link

@tomislavhoracek The first issue still reproduces on 17817 (check screenshot) but I think this is related to DDW-670, so not sure if that would be fixed on another task.
I was able to reproduce the second issue in Windows just once but got no video. I'll check that a few more times just to be sure.

Regarding this, is there any confirmation to implement it on this PR?

Also, I have a small suggestion and I think this was mentioned before for other dialogs: Can we avoid the dialog closing when clicking outside of it? (Check video) I know it's implemented this way in production, but I think it's a little inconvenient in this case. As the process of verification takes so many steps if you accidentally click outside of the dialog you have to start over.

@tomislavhoracek
Copy link
Contributor Author

@gabriela-ponce Fix is merged into 4.1.0-FC1 release branch. It is still not in develop branch and also not merged in this PR. I will let you know when you can check fix on this PR.

@tomislavhoracek
Copy link
Contributor Author

@gabriela-ponce you can test now. Fix is merged!

@miorsufianiohk
Copy link

@tomislavhoracek Looks good to me when tested with Nano X on build 17841. However, I am not sure what has been decided on @gabriela-ponce's suggestion about this "Can we avoid the dialog closing when clicking outside of it? "?

@gabriela-ponce
Copy link

@tomislavhoracek The first issue (HW not detected) no longer reproduces with the mentioned steps, but I'm still running some tests just to be sure.

But I have found the reproduction steps for the second issue (dialog closes after exporting the key):

On Windows and Linux:
1 - Open the "Share wallet" dialog
2 - Connect HW and unlock
3 - Run Cardano app on HW
4 - Export public key

You'll see that the dialog closes. Check this video.
You can switch steps 1 and 2 and it would reproduce (connect and unlock first, open dialog next). The key to reproduce it is to run the Cardano app after opening the dialog.

After that, if you open the dialog again (no changes on the HW), Daedalus would ask to export the key, but nothing would be prompted on the device. It works after a second retry without changes on the HW (Cardano app still running).

@tomislavhoracek
Copy link
Contributor Author

tomislavhoracek commented May 5, 2021

@gabriela-ponce Can you write what was the step before the issue, in what state the device was, did you restarted Daedalus, reconnected the device...
Please provide full reproduction steps by following the extended HW testing docs. Also provide new / clean Daedalus logs

@miorsufianiohk you tested only on Mac or...?

@gabriela-ponce
Copy link

gabriela-ponce commented May 5, 2021

@tomislavhoracek I'll try to clarify the steps for this issue:

Platform: Windows 10 and Ubuntu 20.04
Hardware wallet: Ledger Nano S
Firmware version: 1.6.1
Cardano app version: 2.2.1

Given that I have connected and unlocked a Ledger Nano S device
When I open the "Share" dialog on the "Receive" screen for this wallet
And I run the Cardano app
And I export the public key
Then I can see that the dialog closes on Daedalus
And the Ledger device is waiting for commands
  • No other transactions were made before reproducing the issue, Daedalus was just initialized.
  • The device can be connected at any stage. So you can connect it before starting Daedalus or after.
  • The only prerequisite to reproduce the issue is to open the "share" dialog before opening the Cardano app
  • After you export the public key on the device, the dialog closes and no instructions are visible on the device

You can check the logs here.

@tomislavhoracek
Copy link
Contributor Author

tomislavhoracek commented May 5, 2021

@gabriela-ponce thanks.
And step BEFORE you saw an issue was "Wallet Restore" or...? and then you restarted Daedalus or not...? what was the previous step?

P.S. in the previous message you wrote that Win and Linux are affected and in the last message Platform: macOS Big Sur 11.2.3. Did you found the issue on all platforms or it is a typo?

@gabriela-ponce
Copy link

@tomislavhoracek You can pair the device, close the Cardano app, and follow the steps to reproduce the issue. But also you can pair the device, unplug it, restart Daedalus, plug the device and follow the steps to reproduce the issue, I found no difference for these. Let me know if that's clear or if we can have a meeting to discuss it 👍
Sorry, I pasted the details from another issue. I updated the comment, but just to confirm, it happens in Windows and Linux.

@tomislavhoracek
Copy link
Contributor Author

@gabriela-ponce thanks, it is much more clearer now <3

@tomislavhoracek
Copy link
Contributor Author

@gabriela-ponce Issue should be fixed now. Can you please test again?
P.S. Closing dialog on overlay click is prevented as you suggested!
cc @miorsufianiohk

Copy link

@gabriela-ponce gabriela-ponce left a comment

Choose a reason for hiding this comment

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

Looks great, nice job 👍

Copy link
Contributor

@nikolaglumac nikolaglumac left a comment

Choose a reason for hiding this comment

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

Flawless work @tomislavhoracek 🌟

@nikolaglumac nikolaglumac merged commit 71356e4 into develop May 7, 2021
@iohk-bors iohk-bors bot deleted the feature/ddw-491-verify-hardware-wallet-receiving-address branch May 7, 2021 08:35
@nikolaglumac nikolaglumac added release-4.1.0 Daedalus Mainnet and removed ⏳release-vNext labels Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature release-4.1.0 Daedalus Mainnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants