[DDW-926] Improve error handling for incorrect passphrase / incorrect hardware wallet device #2860

marcin-mazurek · 2022-02-04T13:39:23Z

This PR improves error handling in scenarios when user provides an incorrect passphrase (different than the one provided while pairing the wallet) or uses an incorrect wallet (in case the user has multiple wallets from the same manufacturer).

Screenshots

Before

After

Testing Checklist

Slack QA thread

Testing scenarios:

No passphrase provided while pairing, non-empty passphrase provided on making transaction

1. Restart Daedalus
2. Pair a Trezor wallet with no passphrase
3. Try to make a transaction (regular transfer or delegation), provide passphrase abc
4. You should see the new error message
5. Press back
6. Try to make a transaction with no passphrase
7. The transaction should succeed

Then try the same once again, without restarting Daedalus.

Non-empty passphrase provided while pairing, no passphrase provided on making transaction

1. Restart Daedalus
2. Pair a Trezor wallet with passphrase abc
3. Try to make a transaction (regular transfer or delegation), provide empty passphrase
4. You should see the new error message
5. Press back
6. Try to make a transaction with passphrase abc
7. The transaction should succeed

Then try the same once again, without restarting Daedalus.

Passphrase provided while pairing, different passphrase provided on making transaction

1. Restart Daedalus
2. Pair a Trezor wallet with passphrase xyz
3. Try to make a transaction (regular transfer or delegation), provide passphrase abc
4. You should see the new error message
5. Press back
6. Try to make a transaction with passphrase xyz
7. The transaction should succeed

Then try the same once again, without restarting Daedalus.

Ledger wallet paired using standard PIN, transaction made with hidden PIN

1. Pair a Ledger wallet with standard PIN
2. Set up a hidden passphrase wallet on Ledger Nano if you haven't done that yet (https://www.youtube.com/watch?v=RJzSQtGVaA0)
3. Try to make a transaction (regular transfer or delegation) using hidden PIN
4. You should see the new error message

Ledger wallet paired using hidden PIN, transaction made with standard PIN

1. Set up a hidden passphrase wallet on Ledger Nano if you haven't done that yet (https://www.youtube.com/watch?v=RJzSQtGVaA0)
2. Pair a Ledger wallet with hidden PIN
3. Try to make a transaction (regular transfer or delegation) using standard PIN
4. You should see the new error message

Ledger S paired but Ledger X used for transaction

1. Pair a Ledger S wallet
2. Unplug and plug Ledger X wallet instead
3. Try to make a transaction 
4. You should see the new error message
5. Connect the right device
6. Try to make the transaction
7. It should succeed

Ledger X paired but Ledger S used for transaction

1. Pair a Ledger X wallet
2. Unplug and plug Ledger S wallet instead
3. Try to make a transaction 
4. You should see the new error message
5. Connect the right device
6. Try to make the transaction
7. It should succeed

Pairing two Trezor wallets one by one and making transactions

1. Pair a Trezor wallet with passphrase abc
2. Make a transaction with that wallet
3. Pair a Trezor wallet with passphrase def
4. Make a transaction with that wallet
9. Make a transaction with previous wallet
10. You should not see the new error message at any point, all transactions should succeed

Hints: always test on clean state folder, also try to do some transactions after pairing, before making the transaction which should produce the new error.

Review Checklist

Basics

Code Quality

Important parts of the code are properly commented and documented
Code is properly typed with typescript types
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

Update Slack QA thread by marking it with a green checkmark

tomislavhoracek

@marcin-mazurek I left a few comments/explanations. Please take a look!

source/renderer/app/stores/HardwareWalletsStore.ts

…-error-handling

lucas-barros · 2022-02-08T20:21:41Z

source/renderer/app/components/hardware-wallet/HardwareWalletStatus.tsx

@@ -49,6 +49,12 @@ const messages = defineMessages({
    defaultMessage: '!!!Exporting the public key failed',
    description: '"Exporting public key failed" device state',
  },
+  unrecognized_wallet: {


Should this be camel case?

Here, I followed the existing pattern where we map the HwDeviceStatus enum values directly to translations. The enum uses snake_case therefore we have this mapping which doesn't follow JavaScript naming conventions. Would you like me to refactor this? I would have to introduce an additional map to map statuses to translation keys.

Ah ok, sounds good.

How about using lodash's camelCase for mapping enum values to translations? This way we could kill two birds with one stone. (btw it is a funny sentence)

This would work, however I'm afraid that it will make it harder to refactor and track which translations are actually in use. Currently you can just find translations by a string, with camelCase function it's not going to be that easy anymore.

alexander-rukin · 2022-02-09T08:36:01Z

can we center "x" icon?)

marcin-mazurek · 2022-02-10T11:57:18Z

@alexander-rukin I've uploaded the latest screenshots. The issue is no longer visible:

marcin-mazurek · 2022-02-10T11:57:36Z

@tomislavhoracek please re-review :)

tomislavhoracek

@marcin-mazurek AMAZING!!! 💯

alexander-rukin

👍

…-error-handling

tomislavhoracek · 2022-03-21T08:32:19Z

source/main/ipc/getHardwareWalletChannel.ts

-              path: devicePath,
-            },
-          });
+          deviceFeatures = await TrezorConnect.getFeatures();


https://docs.google.com/document/d/1kPTqIyKf6xmrpA30iD2KyqiqkE4kBkBvu370WfBW-yU/edit?disco=AAAAVvUK8dI

ManusMcCole · 2022-03-21T11:47:00Z

Hi @marcin-mazurek. I can confirm that the error handling is working for Trezor now 👍. However there is one scenario where the new correct error message is not shown

When a wallet is setup with a passphrase and when user is prompted for the passphrase and enters an empty(blank) passphrase the old message "Disconnect and reconnect your hardware wallet to restart the process" appears. Any other incorrect passphrase that isnt blank triggers the correct message Screenshot

…-error-handling

…r validation issue" This reverts commit 4952375.

marcin-mazurek · 2022-03-24T23:29:44Z

@gabriela-ponce You actually came across two issues.

First issue (DDW-970) is related to incorrect logic in address verification, which @szymonmaslowski is addressing in this PR: #2906.

Second issue is with Daedalus losing track of Ledger USB path - anytime you see the "Cannot open device with path..." error, that isn't an issue with logic for a particular action - it's an issue related to Ledger connectivity implementation and unfortunately may happen at various points while interacting with Ledger wallet. Currently @renanvalentin is heavily changing the way we handle Ledger USB paths in this PR: #2900. Hopefully when his piece of work is finalized and merged, we should no longer experience those path issues. For now, when you get an issue like this one: https://drive.google.com/file/d/1a48WtwdQaX71QDfuZ13wY73QB0jJ8ucy/view (or like the ones reported by @ManusMcCole here - #2860 (comment)) - the only workaround is to unplug the device and plug it again, then retry.

@ManusMcCole could you please re-test Trezor scenarios again? I've extended the error handling further to account for cases you've found, but this increases the risk and it would be good to do a full hardware wallet regression test before we ship this.

…rrent wallet ID does not match device wallet ID

ManusMcCole · 2022-04-06T15:25:27Z

Hi @marcin-mazurek. I have completed another round of testing.Tested on build 21375 on Mac OS. I found one scenario where a correct passphrase was not accepted

Pair a wallet with a passphrase. Initiate transaction. Insert Incorrect passphrase. Correct message "We do not recognise this wallet on your device" pops up.Press Back.Press Send again.Insert Correct passphrase this time and it still is accepted as false and the "We do not recognise this wallet on your device" pops up again

For a passphrase protected wallet for some reason getting the passphrase wrong one time causes even the correct passphrase to fail in subsequent attempts

Wallet with a blank passphrase accepts correct passphrase after an incorrect one has been entered previously

…-error-handling

…itch hidden wallets

…-error-handling

marcin-mazurek · 2022-04-21T12:42:48Z

@ManusMcCole this is ready for another round of review. I've added several more test cases to the PR description and executed them and they all seem to pass. Please re-check.

ManusMcCole · 2022-05-05T13:27:06Z

Hi Marcin. Here is the newest testing report for build 21659 Intel MacOS Testnet.Mostly all of the scenarios worked except when device is connected and unlocked after Send transaction has been initiated

There seems to be an issue for Ledger X for hidden pincode wallets when the device is only connected after Send is already selected for a transaction. Initiate a transaction then connect and unlock device. Even with a correct hidden pin entered a Transaction Confirmation failed error appears. Screenshot

For normal pincode wallets same scenario on Ledger X instead of the above error the device is not recognised after unlock(with correct or hidden pin entered) and continues to say "Connect the Ledger X device". Screenshot

My Ledger S device is currently not operational so this issue may also appear there too I was not currently able to test

…-error-handling

marcin-mazurek · 2022-05-11T09:30:53Z

Hey @ManusMcCole, please have a look at the latest build which includes #2900 which brought significant improvements for Ledger. In the latest build I'm not able to reproduce scenarios you've posted.

ManusMcCole

Great job @marcin-mazurek. Looks good 👍

marcin-mazurek · 2022-05-12T15:12:22Z

Can't believe it 😱

danielmain

Great job @marcin-mazurek !!! you can now rest in peace 😂

Marcin Mazurek added 3 commits February 4, 2022 12:37

[DDW-926] Potential fix for incorrect phrase error handling

a49b2b3

[DDW-926] Potential fix for incorrect phrase error handling

45cc72f

[DDW-926] Potential fix for incorrect phrase error handling

d2e782f

marcin-mazurek added bug WIP ⏳release-vNext labels Feb 4, 2022

tomislavhoracek requested changes Feb 7, 2022

View reviewed changes

source/renderer/app/stores/HardwareWalletsStore.ts Show resolved Hide resolved

source/renderer/app/stores/HardwareWalletsStore.ts Show resolved Hide resolved

Marcin Mazurek added 3 commits February 8, 2022 20:51

[DDW-926] Improve translations

d9b2cdb

[DDW-926] Handle one more case when incorrect passphrase can be provided

d194e23

[DDW-926] Revert unrelated change

eb8fea0

marcin-mazurek removed the WIP label Feb 8, 2022

marcin-mazurek self-assigned this Feb 8, 2022

marcin-mazurek requested review from a team and alexander-rukin February 8, 2022 20:03

Marcin Mazurek added 2 commits February 8, 2022 21:04

[DDW-926] Fixes

5f48a4f

Merge remote-tracking branch 'origin/develop' into fix/ddw-926-trezor…

d9880ad

…-error-handling

marcin-mazurek changed the title ~~[DDW-926] Improve Trezor error handling for incorrect passphrase~~ [DDW-926] Improve error handling for incorrect passphrase / incorrect hardware wallet device Feb 8, 2022

[DDW-926] Fill in changelog

25938be

marcin-mazurek added the run Chromatic label Feb 8, 2022

lucas-barros reviewed Feb 8, 2022

View reviewed changes

miorsufianiohk self-requested a review February 9, 2022 10:04

Merge branch 'develop' into fix/ddw-926-trezor-error-handling

d6883af

tomislavhoracek self-requested a review February 10, 2022 13:04

tomislavhoracek approved these changes Feb 10, 2022

View reviewed changes

alexander-rukin approved these changes Feb 10, 2022

View reviewed changes

Merge remote-tracking branch 'origin/develop' into fix/ddw-926-trezor…

ea549b7

…-error-handling

tomislavhoracek reviewed Mar 21, 2022

View reviewed changes

Marcin Mazurek added 4 commits March 22, 2022 11:00

Merge remote-tracking branch 'origin/develop' into fix/ddw-926-trezor…

de38049

…-error-handling

Revert "[DDW-926] Remove passing null path param to fix missing Trezo…

889080d

…r validation issue" This reverts commit 4952375.

[DDW-926] Ensure null is not passed as device path

a662ac0

[DDW-926] Add logging for changing paths to debug Ledger issues

5e4659d

marcin-mazurek added the WIP label Mar 24, 2022

[DDW-926] Improve translations

2f86d73

[DDW-926] Apply UNRECOGNIZED_WALLET status for all cases where the cu…

68fa148

…rrent wallet ID does not match device wallet ID

marcin-mazurek force-pushed the fix/ddw-926-trezor-error-handling branch from 03c32cd to 68fa148 Compare March 24, 2022 23:31

Marcin Mazurek added 3 commits April 14, 2022 14:02

Merge remote-tracking branch 'origin/develop' into fix/ddw-926-trezor…

b65c17b

…-error-handling

[DDW-926] Reinitialize Trezor Connect session in order to let user sw…

07a541a

…itch hidden wallets

Merge remote-tracking branch 'origin/develop' into fix/ddw-926-trezor…

3bafb81

…-error-handling

[DDW-926] Fix translations

e17610d

marcin-mazurek removed the WIP label Apr 29, 2022

Trigger build

4182c72

Merge remote-tracking branch 'origin/develop' into fix/ddw-926-trezor…

e466617

…-error-handling

ManusMcCole approved these changes May 12, 2022

View reviewed changes

danielmain approved these changes May 12, 2022

View reviewed changes

danielmain merged commit 65d0800 into develop May 12, 2022

danielmain deleted the fix/ddw-926-trezor-error-handling branch May 12, 2022 17:37

michalrus added a commit that referenced this pull request Jun 13, 2022

Move PR #2860 to a proper location in CHANGELOG.md

2d7a62d

danielmain added release-4.11.0 and removed ⏳release-vNext labels Jul 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DDW-926] Improve error handling for incorrect passphrase / incorrect hardware wallet device #2860

[DDW-926] Improve error handling for incorrect passphrase / incorrect hardware wallet device #2860

marcin-mazurek commented Feb 4, 2022 •

edited

tomislavhoracek left a comment

lucas-barros Feb 8, 2022

marcin-mazurek Feb 10, 2022

lucas-barros Feb 10, 2022

szymonmaslowski Feb 18, 2022

marcin-mazurek Feb 28, 2022

alexander-rukin commented Feb 9, 2022 •

edited

marcin-mazurek commented Feb 10, 2022

marcin-mazurek commented Feb 10, 2022

tomislavhoracek left a comment

alexander-rukin left a comment

tomislavhoracek Mar 21, 2022

ManusMcCole commented Mar 21, 2022 •

edited

marcin-mazurek commented Mar 24, 2022 •

edited

ManusMcCole commented Apr 6, 2022

marcin-mazurek commented Apr 21, 2022

ManusMcCole commented May 5, 2022

marcin-mazurek commented May 11, 2022

ManusMcCole left a comment

marcin-mazurek commented May 12, 2022 •

edited

danielmain left a comment

[DDW-926] Improve error handling for incorrect passphrase / incorrect hardware wallet device #2860

[DDW-926] Improve error handling for incorrect passphrase / incorrect hardware wallet device #2860

Conversation

marcin-mazurek commented Feb 4, 2022 • edited

Screenshots

Before

After

Testing Checklist

Testing scenarios:

No passphrase provided while pairing, non-empty passphrase provided on making transaction

Non-empty passphrase provided while pairing, no passphrase provided on making transaction

Passphrase provided while pairing, different passphrase provided on making transaction

Ledger wallet paired using standard PIN, transaction made with hidden PIN

Ledger wallet paired using hidden PIN, transaction made with standard PIN

Ledger S paired but Ledger X used for transaction

Ledger X paired but Ledger S used for transaction

Pairing two Trezor wallets one by one and making transactions

Review Checklist

Basics

Code Quality

Testing

After Review

tomislavhoracek left a comment

Choose a reason for hiding this comment

lucas-barros Feb 8, 2022

Choose a reason for hiding this comment

marcin-mazurek Feb 10, 2022

Choose a reason for hiding this comment

lucas-barros Feb 10, 2022

Choose a reason for hiding this comment

szymonmaslowski Feb 18, 2022

Choose a reason for hiding this comment

marcin-mazurek Feb 28, 2022

Choose a reason for hiding this comment

alexander-rukin commented Feb 9, 2022 • edited

marcin-mazurek commented Feb 10, 2022

marcin-mazurek commented Feb 10, 2022

tomislavhoracek left a comment

Choose a reason for hiding this comment

alexander-rukin left a comment

Choose a reason for hiding this comment

tomislavhoracek Mar 21, 2022

Choose a reason for hiding this comment

ManusMcCole commented Mar 21, 2022 • edited

marcin-mazurek commented Mar 24, 2022 • edited

ManusMcCole commented Apr 6, 2022

marcin-mazurek commented Apr 21, 2022

ManusMcCole commented May 5, 2022

marcin-mazurek commented May 11, 2022

ManusMcCole left a comment

Choose a reason for hiding this comment

marcin-mazurek commented May 12, 2022 • edited

danielmain left a comment

Choose a reason for hiding this comment

marcin-mazurek commented Feb 4, 2022 •

edited

alexander-rukin commented Feb 9, 2022 •

edited

ManusMcCole commented Mar 21, 2022 •

edited

marcin-mazurek commented Mar 24, 2022 •

edited

marcin-mazurek commented May 12, 2022 •

edited