Jade wallet implement #12041

Whem · 2023-12-06T13:05:29Z

Fixes: zkSNACKs/WalletWasabi#11908

Introduction

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

Solution

We acquired a Blockstream Jade 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 Jade Test
Writing the Jade Mock Test

In the future, if the hardwarewallet fimware is changes, we can check the the path format to what comes from hwi.

…ertation

I needed to spoil hwi commands data and changed the long lined "if" statement to switch

nopara73 · 2023-12-06T15:46:12Z

Don't forget the docs:

kristapsk · 2023-12-06T16:00:49Z

I have Jade somewhere at home, will try to find it and test.

yahiheb · 2023-12-06T17:19:10Z

Don't forget the docs:

https://github.com/zkSNACKs/WalletWasabi/tree/master/WalletWasabi.Documentation

https://github.com/zkSNACKs/WasabiDoc/

1- Is done already.
2- The following FAQ covers that already: https://docs.wasabiwallet.io/FAQ/FAQ-UseWasabi.html#what-hardware-wallets-does-wasabi-support

yahiheb

cACK, code LGTM.
I run TrezorTKataAsync and tested using Trezor T.

You probably should add this to WalletDirTests.GetFriendlyNameTest:
Assert.Equal("Jade", HardwareWalletModels.Jade.FriendlyName());

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

Did you send a transaction using Jade?

I have Jade somewhere at home, will try to find it and test.

@kristapsk Can you also run this test JadeKataAsync?

kristapsk · 2023-12-07T04:37:06Z

I have Jade somewhere at home, will try to find it and test.

@kristapsk Can you also run this test JadeKataAsync?

Yes, hope so (need to find device), ETA - before end of this week.

nopara73 · 2023-12-07T14:09:34Z

fixes #11908 ?

Whem · 2023-12-07T15:51:39Z

fixes #11908 ?

Yes, i tried to added to main net and testnet too with successfully.

yahiheb

LGTM

turbolay

Easy PR, if it's tested and it works then merging is pretty trivial.
It's a shame tho that the implementation is not more generic

kiminuo

I'm not that much familiar with HWI stuff. So just a few comments.

WalletWasabi/Hwi/Parsers/HwiParser.cs

WalletWasabi.Tests/UnitTests/Hwi/HwiProcessBridgeMock.cs

WalletWasabi.Tests/UnitTests/Hwi/MockedDeviceTests.cs

kiminuo · 2023-12-09T20:14:45Z

WalletWasabi.Tests/AcceptanceTests/HwiKatas.cs

+		HwiEnumerateEntry entry = Assert.Single(enumerate);
+		Assert.NotNull(entry.Path);
+		Assert.Equal(HardwareWalletModels.Jade, entry.Model);
+		Assert.True(HwiParser.ValidatePathString(entry.Model, entry.Path));


I'm a bit confused: So there is this new ValidatePathString method but it's used only for Jade but not for others. Why is it not used for other HW models if the method supports other models?

This PR focus on Jade wallet, after if this PR is merged I will start to extend with new tests the current HWI Wallet tests and then i will put there the new parse method to the right places.

Make sure to use the "new" method everywhere in the following mini PR.

Co-authored-by: Kimi <58662979+kiminuo@users.noreply.github.com>

yahiheb

LGTM

molnard · 2023-12-11T16:03:17Z

WalletWasabi/Hwi/Parsers/HwiParser.cs

+	/// <param name="path">The wallet path which come from HWI enumerate command.</param>
+	/// <param name="model">The hardware wallet model.</param>
+	/// <returns><c>true</c> if the path matches the model's regex, <c>false</c> otherwise.</returns>
+	public static bool ValidatePathString(HardwareWalletModels model, string path)


You only use this in the tests. You should move this logic out of the production code.

WalletWasabi.Tests/AcceptanceTests/HwiKatas.cs

molnard · 2023-12-11T17:05:14Z

Manual testing

Connect and Add Wallet
Display address
Receive
Send

molnard

PSBT workflow can be enabled in the UI however the device is not supporting is. The settings should be disabled. Can you add it in this PR if the code is simple?

soosr · 2023-12-12T08:50:58Z

PSBT workflow can be enabled in the UI however the device is not supporting is. The settings should be disabled. Can you add it in this PR if the code is simple?

It wouldn't be simple as in that case for HW wallets the whole settings would be empty and the wallet settings should be also hidden.
Since as of now it is visible for several other HW wallets that do not support PSBT workflow, I would propose to merge this PR without that change.

And in a separate PR, it could be fixed for all HW wallets. Another plus point would be to fix it after the Wallet Rename feature is merged, as after that the wallet settings would never be empty even for HW wallets, so the fix would be simpler.

WDYT? @molnard

molnard · 2023-12-12T12:06:44Z

Since as of now it is visible for several other HW wallets that do not support PSBT workflow, I would propose to merge this PR without that change.

I agree.

molnard · 2023-12-12T12:07:07Z

And in a separate PR, it could be fixed for all HW wallets. Another plus point would be to fix it after the Wallet Rename feature is merged, as after that the wallet settings would never be empty even for HW wallets, so the fix would be simpler.

ACK.

molnard

tACK. 🥇 Well done!

MaxHillebrand · 2023-12-12T12:30:22Z

woop woop on getting your first PR merged @Whem, you started with a banger! 🎉🖤

Whem added 8 commits December 6, 2023 12:55

Add new enums to WalletType and HardwarWalletModel

e5d4212

add a hwi path validator

25cdb5d

In the future, if the hardwarewallet fimware is changes, we can check the the path format to what comes from hwi.

add the new wallet type to HwiEnumaretEntry's hardwaremodel type conv…

f040ff3

…ertation

Add Jade test for Hwi katas and use the new path validator too.

7a6fcf3

added Jade static data to HwiProcessBridgeMock

7449059

I needed to spoil hwi commands data and changed the long lined "if" statement to switch

Added Jade to Mock tests

4127de8

Added Jade to Compatibility list

d6a93c2

fixing comment format

6c9a19c

pull-request-size bot added the size/L label Dec 6, 2023

yahiheb previously approved these changes Dec 6, 2023

View reviewed changes

added Jade to FriendlyName test

d1e01e2

Whem dismissed yahiheb’s stale review via d1e01e2 December 7, 2023 12:14

Whem requested a review from yahiheb December 7, 2023 12:15

yahiheb previously approved these changes Dec 7, 2023

View reviewed changes

yahiheb requested a review from kiminuo December 7, 2023 17:12

turbolay approved these changes Dec 7, 2023

View reviewed changes

kiminuo reviewed Dec 9, 2023

View reviewed changes

Whem dismissed yahiheb’s stale review via 754b685 December 11, 2023 09:39

Whem and others added 5 commits December 11, 2023 10:39

Update WalletWasabi/Hwi/Parsers/HwiParser.cs

754b685

Co-authored-by: Kimi <58662979+kiminuo@users.noreply.github.com>

Update WalletWasabi/Hwi/Parsers/HwiParser.cs

6f64a82

Co-authored-by: Kimi <58662979+kiminuo@users.noreply.github.com>

Update WalletWasabi.Tests/UnitTests/Hwi/MockedDeviceTests.cs

a6d1f43

Co-authored-by: Kimi <58662979+kiminuo@users.noreply.github.com>

Merge Method to ValidatePathString

bbbc82e

fix values for camel-case and use network value directly

342891e

yahiheb previously approved these changes Dec 11, 2023

View reviewed changes

yahiheb requested a review from molnard December 11, 2023 13:47

molnard requested changes Dec 11, 2023

View reviewed changes

Whem added 2 commits December 12, 2023 12:36

sync the description to the user eyes

61cb137

move from production code the PathValidator to Tests folder

ab36838

Whem dismissed yahiheb’s stale review via ab36838 December 12, 2023 11:42

molnard approved these changes Dec 12, 2023

View reviewed changes

molnard merged commit 47ea380 into WalletWasabi:master Dec 12, 2023
6 of 7 checks passed

Whem mentioned this pull request Feb 2, 2024

[UI] Add new wallet icons for new HWs #12343

Closed

yahiheb mentioned this pull request Mar 24, 2024

[Trivial] Use ValidatePathString for other HW models #12713

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jade wallet implement #12041

Jade wallet implement #12041

Whem commented Dec 6, 2023

nopara73 commented Dec 6, 2023

kristapsk commented Dec 6, 2023

yahiheb commented Dec 6, 2023

yahiheb left a comment

kristapsk commented Dec 7, 2023 •

edited

nopara73 commented Dec 7, 2023

Whem commented Dec 7, 2023

yahiheb left a comment

turbolay left a comment

kiminuo left a comment

kiminuo Dec 9, 2023

Whem Dec 11, 2023

molnard Dec 12, 2023 •

edited

yahiheb left a comment

molnard Dec 11, 2023

Whem Dec 12, 2023

molnard commented Dec 11, 2023 •

edited

molnard left a comment

soosr commented Dec 12, 2023

molnard commented Dec 12, 2023

molnard commented Dec 12, 2023

molnard left a comment

MaxHillebrand commented Dec 12, 2023 •

edited

Jade wallet implement #12041

Jade wallet implement #12041

Conversation

Whem commented Dec 6, 2023

Introduction

Solution

Subtasks

nopara73 commented Dec 6, 2023

kristapsk commented Dec 6, 2023

yahiheb commented Dec 6, 2023

yahiheb left a comment

Choose a reason for hiding this comment

kristapsk commented Dec 7, 2023 • edited

nopara73 commented Dec 7, 2023

Whem commented Dec 7, 2023

yahiheb left a comment

Choose a reason for hiding this comment

turbolay left a comment

Choose a reason for hiding this comment

kiminuo left a comment

Choose a reason for hiding this comment

kiminuo Dec 9, 2023

Choose a reason for hiding this comment

Whem Dec 11, 2023

Choose a reason for hiding this comment

molnard Dec 12, 2023 • edited

Choose a reason for hiding this comment

yahiheb left a comment

Choose a reason for hiding this comment

molnard Dec 11, 2023

Choose a reason for hiding this comment

Whem Dec 12, 2023

Choose a reason for hiding this comment

molnard commented Dec 11, 2023 • edited

molnard left a comment

Choose a reason for hiding this comment

soosr commented Dec 12, 2023

molnard commented Dec 12, 2023

molnard commented Dec 12, 2023

molnard left a comment

Choose a reason for hiding this comment

MaxHillebrand commented Dec 12, 2023 • edited

kristapsk commented Dec 7, 2023 •

edited

molnard Dec 12, 2023 •

edited

molnard commented Dec 11, 2023 •

edited

MaxHillebrand commented Dec 12, 2023 •

edited