[DDW-970] Prevent creation of new wallet during address verification #2906

szymonmaslowski · 2022-03-03T21:15:21Z

This PR

prevents creating a new wallet when paired incorrect one during address verification
refactors part of HW logic

Unfortunately, this PR requires full regression testing of hardware wallet cases

Diagram representing part of the logic covered by the refactoring:

Todos

Wait for PR 2860 to be merged and use HWDeviceStatus introduced there instead of creating new one here.

Screenshots

Testing Checklist

Slack QA thread
Full regression test of all hardware wallet cases. Regression document

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

…ncorrect passphrase during pairing

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

alexander-rukin · 2022-03-09T08:10:15Z

in the bottom there is too much space (because there is no buttons, but paddings are there 😓)

20px here

and 12px here

p.s. at least after I removed this in inspector it looked better

tomislavhoracek

@szymonmaslowski 💯 Nice improvements and code / flow is much more cleaner now.
Please check my comments...

source/renderer/app/stores/HardwareWalletsStore.ts

tomislavhoracek · 2022-03-09T08:17:32Z

source/renderer/app/stores/HardwareWalletsStore.ts

+      const recognizedSoftwareWallet = await this._recognizeSoftwareWalletByExtendedPublicKey(
+        { extendedPublicKey }
+      );
+
+      if (recognizedSoftwareWallet) {
+        await this._handleIdentifiedSoftwareWallet({
+          recognizedSoftwareWallet,
+          extendedPublicKey,
+          forcedPath: devicePath,
+          walletId,
+          address,
+        });


@szymonmaslowski following to the above comment. Now you have 2 functions that are connected to identifying / recognizing. It is better to say "ok, now get me an extended public key and then recognize"

Sorry I didn't get your comment. By two functions do you mean the _handleIdentifiedSoftwareWallet and _identifyAndHandleSoftwareWallet? The _handleIdentifiedSoftwareWallet method is responsible only for the case when the wallet was found. On the other hand the _identifyAndHandleSoftwareWallet method encapsulates all steps including the extended public key request, finding the device and handling it accordingly, and it is there for a reduction of code duplication, but I believe with further refactoring it may be broken to few less generic code blocks and distributed to the proper places in the logic (for example part regarding the transaction to the transaction logic etc.).

tomislavhoracek · 2022-03-09T08:19:44Z

source/renderer/app/stores/HardwareWalletsStore.ts

+  };
+
+  @action
+  _proceedWithTransactionAfterConnectingDevice = ({


@szymonmaslowski maybe we should call this _reinitiate... or _recover...? What do you think?

I would say this is rather a continuation of the transaction process as there is the _signTransactionTrezor method called at the end of this method. Am I right?

source/renderer/app/stores/HardwareWalletsStore.ts

…review

szymonmaslowski · 2022-03-11T12:07:00Z

@alexander-rukin Could you check again? Styling issues shouldn't be there anymore. I fixed it in a little different way than your suggestion, but I think I achieved the same result.

alexander-rukin · 2022-03-11T12:34:59Z

@szymonmaslowski pls make sure that your fix didn't affect scrollbar position

as now it ends too early (not with the end of content)

…ace of previous INCORRECT_PASSPHRASE_PROVIDED status

gabriela-ponce · 2022-04-27T18:30:33Z

@szymonmaslowski I found a specific scenario where the message is different from the one on the screenshots:

Pair the Trezor and do not unplug it. Then, try to validate the address and insert an incorrect recovery phrase. You'll see no new device is paired, but the error will be "Address verification failed" instead of "We do not recognize this wallet...". Check this video
Could this be improved and show the second message instead?

gabriela-ponce

Good job, lgtm!
Notes:

The issue on my previous comment is going to be addressed on this card: DDW-1090.
Please do not merge until we have at least one additional approval from the rest of @input-output-hk/daedalus-qa

miorsufianiohk

LGTM. Great job @szymonmaslowski 👍 . Tested on 21656

dmitrii-gaico · 2022-05-18T13:00:43Z

@danielmain Additional QA review is required after resolving the conflicts. So please do not merge before further notice.

ManusMcCole

Looks good @szymonmaslowski 👍

miorsufianiohk

LGTM. Tested on 21788. Great work @szymonmaslowski 👍

danielmain

Amazing job @szymonmaslowski

[DDW-970] Prevent creating new trezor software wallet when provided i…

a5d885c

…ncorrect passphrase during pairing

szymonmaslowski force-pushed the feature/ddw-970 branch from 584f81e to a5d885c Compare March 8, 2022 12:52

szymonmaslowski self-assigned this Mar 8, 2022

szymonmaslowski requested review from a team and alexander-rukin March 8, 2022 13:30

szymonmaslowski added ⏳release-vNext bug WIP and removed WIP labels Mar 8, 2022

szymonmaslowski marked this pull request as ready for review March 8, 2022 13:32

lucas-barros reviewed Mar 8, 2022

View reviewed changes

source/renderer/app/components/hardware-wallet/HardwareWalletStatus.tsx Outdated Show resolved Hide resolved

szymonmaslowski added 2 commits March 8, 2022 14:44

[DDW-970] Remove redundant console.logs

396022a

[DDW-970] Add changelog entry

11bf1c3

lucas-barros approved these changes Mar 8, 2022

View reviewed changes

Merge branch 'develop' into feature/ddw-970

0507f32

shawnbusuttil approved these changes Mar 8, 2022

View reviewed changes

source/renderer/app/components/hardware-wallet/HardwareWalletStatus.tsx Outdated Show resolved Hide resolved

tomislavhoracek requested changes Mar 9, 2022

View reviewed changes

szymonmaslowski added WIP DO NOT MERGE labels Mar 10, 2022

szymonmaslowski added 2 commits March 11, 2022 11:21

[DDW-970] Small styling and code quality fixes

ea732c3

[DDW-970] Change few names in the HardwareWalletStore after the code …

de43ddf

…review

szymonmaslowski requested a review from tomislavhoracek March 11, 2022 12:04

szymonmaslowski added 3 commits March 11, 2022 13:07

Merge branch 'develop' into feature/ddw-970

e26a627

[DDW-970] Minor code quality improvements after self review

ca54de3

[DDW-970] Remove unnecessary ts ignore comments

f2ca378

szymonmaslowski removed the WIP label Mar 11, 2022

szymonmaslowski added 2 commits April 25, 2022 12:58

[DDW-970] Use a UNRECOGNIZED_WALLET status and new translations in pl…

0b4a9b0

…ace of previous INCORRECT_PASSPHRASE_PROVIDED status

Merge remote-tracking branch 'origin/develop' into feature/ddw-970

ea8ad7e

szymonmaslowski force-pushed the feature/ddw-970 branch from 23dff70 to ea8ad7e Compare April 25, 2022 10:59

[DDW-970] Fix prettier issues

2961975

szymonmaslowski removed the DO NOT MERGE label Apr 26, 2022

szymonmaslowski requested a review from marcin-mazurek April 26, 2022 17:55

[DDW-970] Fix JP translation for the unrecognized_wallet status

30b9207

miorsufianiohk self-requested a review April 28, 2022 08:48

szymonmaslowski added 3 commits April 28, 2022 13:34

Merge remote-tracking branch 'origin/develop' into feature/ddw-970

67ec172

[DDW-970] Fix HardwareWalletStore implementaion after meging master

7a1fe76

[DDW-970] Remove unnecessary calls of the resetWalletPairing function

42977de

gabriela-ponce approved these changes May 2, 2022

View reviewed changes

miorsufianiohk approved these changes May 3, 2022

View reviewed changes

Merge remote-tracking branch 'origin/develop' into feature/ddw-970

9a3e897

gabriela-ponce requested review from gabriela-ponce and miorsufianiohk May 18, 2022 13:04

Merge branch 'develop' into feature/ddw-970

8fb0968

ManusMcCole self-requested a review May 19, 2022 09:42

ManusMcCole approved these changes May 19, 2022

View reviewed changes

gabriela-ponce approved these changes May 19, 2022

View reviewed changes

miorsufianiohk approved these changes May 19, 2022

View reviewed changes

danielmain approved these changes May 20, 2022

View reviewed changes

szymonmaslowski added 2 commits May 20, 2022 09:58

Merge branch 'develop' into feature/ddw-970

87825dd

[DDW-970] Move changelog entry to the appropiate section

9b4de1e

danielmain merged commit 5dfe3f6 into develop May 20, 2022

iohk-bors bot deleted the feature/ddw-970 branch May 20, 2022 08:21

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-970] Prevent creation of new wallet during address verification #2906

[DDW-970] Prevent creation of new wallet during address verification #2906

szymonmaslowski commented Mar 3, 2022 •

edited by gabriela-ponce

alexander-rukin commented Mar 9, 2022 •

edited

tomislavhoracek left a comment

tomislavhoracek Mar 9, 2022

szymonmaslowski Mar 11, 2022

tomislavhoracek Mar 9, 2022

szymonmaslowski Mar 11, 2022

szymonmaslowski commented Mar 11, 2022

alexander-rukin commented Mar 11, 2022

gabriela-ponce commented Apr 27, 2022

gabriela-ponce left a comment

miorsufianiohk left a comment

dmitrii-gaico commented May 18, 2022

ManusMcCole left a comment

miorsufianiohk left a comment

danielmain left a comment

[DDW-970] Prevent creation of new wallet during address verification #2906

[DDW-970] Prevent creation of new wallet during address verification #2906

Conversation

szymonmaslowski commented Mar 3, 2022 • edited by gabriela-ponce

Todos

Screenshots

Testing Checklist

Review Checklist

Basics

Code Quality

Testing

After Review

alexander-rukin commented Mar 9, 2022 • edited

tomislavhoracek left a comment

Choose a reason for hiding this comment

tomislavhoracek Mar 9, 2022

Choose a reason for hiding this comment

szymonmaslowski Mar 11, 2022

Choose a reason for hiding this comment

tomislavhoracek Mar 9, 2022

Choose a reason for hiding this comment

szymonmaslowski Mar 11, 2022

Choose a reason for hiding this comment

szymonmaslowski commented Mar 11, 2022

alexander-rukin commented Mar 11, 2022

gabriela-ponce commented Apr 27, 2022

gabriela-ponce left a comment

Choose a reason for hiding this comment

miorsufianiohk left a comment

Choose a reason for hiding this comment

dmitrii-gaico commented May 18, 2022

ManusMcCole left a comment

Choose a reason for hiding this comment

miorsufianiohk left a comment

Choose a reason for hiding this comment

danielmain left a comment

Choose a reason for hiding this comment

szymonmaslowski commented Mar 3, 2022 •

edited by gabriela-ponce

alexander-rukin commented Mar 9, 2022 •

edited