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

Trezor Safe 3 implementation #12687

Merged
merged 13 commits into from Apr 2, 2024
Merged

Conversation

Whem
Copy link
Collaborator

@Whem Whem commented Mar 18, 2024

Fixes: #12143

Introduction

The HWI already supports the Trezor Safe 3 hardware wallet, but until now, Wasabi wallet did not. Therefore, we decided to implement this in the current codebase.

Solution

We acquired a Trezor Safe 3 hardware wallet and initialized it with the appropriate parameters. After this, I implemented the new Enums, HardwareWalletModel and WalletType, in the code.

I started the Wasabi wallet and was able to add it as a wallet.

Subtasks

  • Writing the Trezor Safe 3 Test
  • Writing the Trezor Safe 3 Mock Test

Tests on Main net

  • Detect Device after Connect To Hardware Wallet
  • Receive
  • Send

Tested operating systems

  • Windows
  • MacOs
  • Ubuntu 22.4
  • Debian

@molnard
Copy link
Collaborator

molnard commented Mar 20, 2024

Add reviewers if you need review.

LGTM please fix the conflict and we can merge.

@Whem Whem requested a review from molnard March 22, 2024 06:56
Copy link
Collaborator

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

@Whem
Copy link
Collaborator Author

Whem commented Mar 22, 2024

don't forget to also add it to the supported hww doc https://github.com/zkSNACKs/WalletWasabi/blob/master/WalletWasabi.Documentation/WasabiCompatibility.md#officially-supported-hardware-wallets

yes, you are absolutely right, I will correct it

@Whem Whem requested a review from MarnixCroes March 22, 2024 07:53
WalletWasabi.Tests/AcceptanceTests/HwiKatas.cs Outdated Show resolved Hide resolved
WalletWasabi.Tests/UnitTests/Hwi/MockedDeviceTests.cs Outdated Show resolved Hide resolved
WalletWasabi.Tests/UnitTests/Hwi/MockedDeviceTests.cs Outdated Show resolved Hide resolved
@@ -62,10 +62,12 @@ public bool IsOfflinePsbtWorkflowCompatible()
HardwareWalletModels.Coldcard => true,
HardwareWalletModels.Ledger_Nano_S or HardwareWalletModels.Ledger_Nano_X or HardwareWalletModels.Ledger_Nano_S_Plus => false,
HardwareWalletModels.Trezor_1 => false,
HardwareWalletModels.Trezor_T => true,
HardwareWalletModels.Trezor_T => false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this value wrong and it got corrected?

Do we still need IsOfflinePsbtWorkflowCompatible?

Copy link
Collaborator Author

@Whem Whem Mar 25, 2024

Choose a reason for hiding this comment

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

The first time when I read the Trezor T specs, I thought it could support native PSBT workflow

Yes, I need it in upcoming features

WalletWasabi/Hwi/Models/HwiEnumerateEntry.cs Outdated Show resolved Hide resolved
yahiheb
yahiheb previously approved these changes Mar 23, 2024
Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

Untested, LGTM.

@yahiheb
Copy link
Collaborator

yahiheb commented Mar 24, 2024

Should we also add Trezor Safe 3s regex into HwiValidationHelper.ValidatePathString?

@Whem
Copy link
Collaborator Author

Whem commented Mar 25, 2024

Should we also add Trezor Safe 3s regex into HwiValidationHelper.ValidatePathString?

Yes, but it would be in another PR

@yahiheb
Copy link
Collaborator

yahiheb commented Mar 25, 2024

Should we also add Trezor Safe 3s regex into HwiValidationHelper.ValidatePathString?

Yes, but it would be in another PR

Why not in this PR, it is a small change.
I already opened a PR #12713 to add those for other HW models.

@Whem
Copy link
Collaborator Author

Whem commented Mar 26, 2024

Should we also add Trezor Safe 3s regex into HwiValidationHelper.ValidatePathString?

Yes, but it would be in another PR

Why not in this PR, it is a small change. I already opened a PR #12713 to add those for other HW models.

Okay, I put in there the validation, but what you made PR, please remove it, I need to recheck it with real data that comes from the device. I can only do this if I go through the validation from each device.

Copy link
Collaborator

@molnard molnard left a comment

Choose a reason for hiding this comment

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

Tested what I could:

  • Mock test.
  • Code boulds.
  • Code LGTM and consistent.

@molnard molnard merged commit e487765 into zkSNACKs:master Apr 2, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trezor Safe3 not recognized
4 participants