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.

Sign up for GitHub

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

Jump to bottom

HWW Validate String Path Platform specific fixes #12781

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

HWW Validate String Path Platform specific fixes #12781

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f7b7b72
fix every platform
Whem Apr 8, 2024
6779d6f
fix windows part
Whem Apr 8, 2024
a858c73
separate to platform validate string
Whem Apr 9, 2024
fe5e12a
fixes mocktests
Whem Apr 9, 2024
3eda40a
rename to speakable method name of runtime extension
Whem Apr 9, 2024
671aae2
delete runtimeextension and use osplatform as raw
Whem Apr 9, 2024
a25f279
Update WalletWasabi.Tests/Helpers/HwiValidationHelper.cs
Whem Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 43 additions & 10 deletions WalletWasabi.Tests/Helpers/HwiValidationHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System.Runtime.InteropServices;
using System.Text.RegularExpressions;
using WalletWasabi.Hwi.Models;

Expand All @@ -10,20 +11,52 @@
/// </summary>
/// <param name="path">The wallet path which come from HWI enumerate command.</param>
/// <param name="model">The hardware wallet model.</param>
/// <param name="osPlatform">Mock test specific platform</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)
public static bool ValidatePathString(HardwareWalletModels model, string path, OSPlatform? osPlatform = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this new parameter anymore, it can be reverted. Same for the mocked tests.

Also, you can remove these extra checks || (osPlatform != null && osPlatform == OSPlatform.Windows).
No need for them.

Copy link
Contributor Author

@Whem Whem Apr 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Mock tests are only compatible with Windows operating system. You can check predefined paths. In that case, osplatform parameter is needed as the mocktest may fail.

https://github.com/zkSNACKs/WalletWasabi/blob/0516c1831ae3ac780df277d43360a2d778c94df8/WalletWasabi.Tests/UnitTests/Hwi/MockedDeviceTests.cs#L657

It's important to note that at this time we do not have the capability to separate mock tests for multiple platforms.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We do not write code only for the tests, please remove this parameter,
If the test is working only on Windows, then disable it on other platforms:

if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
	return;
}

Here you are forcing the tests to act like they were running on Windows which is an extremely bad practice.

Copy link
Contributor Author

@Whem Whem Apr 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The HwiKatas test is compatible with all platforms. However, the predefined data used in the mock test is specifically designed for Windows.

Initially, I thought that passing Windows as a parameter in the mocktest would display the expected results since it's only a mock test and not real data.

However, if required, I could expand the mock bridge test to a platform-specific approach.

{
string pattern = model switch
string? pattern = null;

if (OSPlatform.Windows.Running() || (osPlatform != null && osPlatform == OSPlatform.Windows))
Whem marked this conversation as resolved.
Show resolved Hide resolved
{
pattern = model switch
{
HardwareWalletModels.Trezor_T => "^webusb:",
HardwareWalletModels.Trezor_1 => @"(?i)^\\\\\?\\HID#VID_534C&PID_0001",
HardwareWalletModels.Coldcard => @"(?i)^\\\\\?\\HID#VID_D13E&PID_CC10",
HardwareWalletModels.Ledger_Nano_S or HardwareWalletModels.Ledger_Nano_X => @"(?i)^\\\\\?\\HID#VID_2C97&PID_0001",
HardwareWalletModels.Jade => @"^COM\d+",
HardwareWalletModels.BitBox02_BTCOnly => @"(?i)^\\\\\?\\HID#VID_03EB&PID_2403",
_ => "",
};
}
else if (OSPlatform.Linux.Running() || (osPlatform != null && osPlatform == OSPlatform.Linux))
{
pattern = model switch
{
HardwareWalletModels.Trezor_T => "^webusb:",
HardwareWalletModels.Trezor_1 => @"\d+(-\d+\.\d+)+:\d+\.\d+",
HardwareWalletModels.Coldcard => @"\d+(-\d+\.\d+)+:\d+\.\d+",
HardwareWalletModels.Ledger_Nano_S or HardwareWalletModels.Ledger_Nano_X => @"\d+(-\d+\.\d+)+:\d+\.\d+",
HardwareWalletModels.Jade => @"/dev/ttyACM\d+",
HardwareWalletModels.BitBox02_BTCOnly => @"\d+(-\d+\.\d+)+:\d+\.\d+",
_ => "",
};
}
else if (OSPlatform.OSX.Running() || (osPlatform != null && osPlatform == OSPlatform.OSX))

Check warning on line 46 in WalletWasabi.Tests/Helpers/HwiValidationHelper.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (master)

❌ New issue: Complex Conditional 

ValidatePathString has 3 complex conditionals with 6 branches, threshold = 2. A complex conditional is an expression inside a branch (e.g. if, for, while) which consists of multiple, logical operators such as AND/OR. The more logical operators in an expression, the more severe the code smell.
{
HardwareWalletModels.Trezor_T => "^webusb:",
HardwareWalletModels.Trezor_1 => @"^hid:\\\\.*?vid_534c&pid_0001&mi_00",
HardwareWalletModels.Coldcard => @"^hid:\\\\.*?vid_d13e&pid_cc10&mi_00",
HardwareWalletModels.Ledger_Nano_S or HardwareWalletModels.Ledger_Nano_X => @"^hid:\\\\.*?vid_2c97&pid_0001&mi_00",
HardwareWalletModels.Jade => @"^COM\d+",
HardwareWalletModels.BitBox02_BTCOnly => @"^\\\\\?\\hid#vid_03eb&pid_2403",
pattern = model switch
{
HardwareWalletModels.Trezor_T => "^webusb:",
HardwareWalletModels.Trezor_1 => @"DevSrvsID:\d+",
HardwareWalletModels.Coldcard => @"DevSrvsID:\d+",
HardwareWalletModels.Ledger_Nano_S or HardwareWalletModels.Ledger_Nano_X => @"DevSrvsID:\d+",
HardwareWalletModels.Jade => @"/dev/cu\.usbserial-[A-Za-z0-9]+",
HardwareWalletModels.BitBox02_BTCOnly => @"DevSrvsID:\d+",
_ => "",
};
}

Check notice on line 59 in WalletWasabi.Tests/Helpers/HwiValidationHelper.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (master)

ℹ Getting worse: Complex Method 

ValidatePathString increases in cyclomatic complexity from 9 to 34, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.

Check warning on line 59 in WalletWasabi.Tests/Helpers/HwiValidationHelper.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Cloud Delta Analysis (master)

❌ New issue: Bumpy Road Ahead 

ValidatePathString has 3 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function. The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. The deeper the nesting and the more bumps, the lower the code health.
_ => "",
};
return Regex.IsMatch(path, pattern);
}
}
16 changes: 16 additions & 0 deletions WalletWasabi.Tests/Helpers/RuntimeExtension.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Runtime.InteropServices;
using System.Text;
using System.Threading.Tasks;

namespace WalletWasabi.Tests.Helpers;

public static class RuntimeExtension
{
public static bool Running(this OSPlatform val)
{
return RuntimeInformation.IsOSPlatform(val);
}
}
5 changes: 3 additions & 2 deletions WalletWasabi.Tests/UnitTests/Hwi/MockedDeviceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using NBitcoin;
using System.Collections.Generic;
using System.Linq;
using System.Runtime.InteropServices;
using System.Threading;
using System.Threading.Tasks;
using WalletWasabi.Blockchain.Keys;
Expand Down Expand Up @@ -562,7 +563,7 @@ public async Task JadeMockTestsAsync(Network network)
IEnumerable<HwiEnumerateEntry> enumerate = await client.EnumerateAsync(cts.Token);
HwiEnumerateEntry entry = Assert.Single(enumerate);
Assert.Equal(HardwareWalletModels.Jade, entry.Model);
Assert.True(HwiValidationHelper.ValidatePathString(entry.Model, "COM3"));
Assert.True(HwiValidationHelper.ValidatePathString(entry.Model, "COM3", OSPlatform.Windows));
Copy link
Contributor

@molnard molnard Apr 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with @turbolay comment. Possible implementation idea:
remove this entirely or add a method called GetPlatformSpecificDevicePath which will return an example of what would a device give back on a specific platform.

Assert.Equal("COM3", entry.Path);
Assert.False(entry.NeedsPassphraseSent);
Assert.False(entry.NeedsPinSent);
Expand Down Expand Up @@ -654,7 +655,7 @@ public async Task BitBox02BtcOnlyMockTestsAsync(Network network)
Assert.Single(enumerate);
HwiEnumerateEntry entry = enumerate.Single();
Assert.Equal(HardwareWalletModels.BitBox02_BTCOnly, entry.Model);
Assert.True(HwiValidationHelper.ValidatePathString(entry.Model, @"\\?\hid#vid_03eb&pid_2403#6&229ae20&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}"));
Assert.True(HwiValidationHelper.ValidatePathString(entry.Model, @"\\?\hid#vid_03eb&pid_2403#6&229ae20&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}", OSPlatform.Windows));
Assert.Equal(@"\\?\hid#vid_03eb&pid_2403#6&229ae20&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030}", entry.Path);
Assert.False(entry.NeedsPassphraseSent);
Assert.False(entry.NeedsPinSent);
Expand Down
Loading