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

transport: propagate hid devices descriptors #13220

Merged
merged 3 commits into from
Jul 12, 2024

Conversation

mroz22
Copy link
Contributor

@mroz22 mroz22 commented Jul 7, 2024

With the old bridge going out of service, we are no longer able to work with very old Trezor HID devices. We are only able to detect them but no communication is possible.

To test this, you need an ancient device.

Features (126 bytes) {
    bootloader_hash: 32 bytes 0xc4c32539b4a025a8e753a4c46264285911a45fcb14f4718179e711b1ce990524,
    device_id: 'F7F53817FDC08323ECA22200',
    imported: False,
    initialized: True,
    label: 'My Trezor',
    major_version: 1,
    minor_version: 2,
    passphrase_protection: False,
    patch_version: 0,
    pin_protection: False,
    revision: 20 bytes 0xdf524b9f35fd5cdba14eaa2bf2d948e3dc75254a,
    vendor: 'bitcointrezor.com',
}

This PR unifies how these devices are communicated for both bridge and webusb transports.

  • c8712c0 - transport now propagates descriptors even for hid devices to higher layers. connect then tries to work with these descriptors resulting into 'Unable to open device' error. which already results in creation of an 'Unreadable device'.
  • 719aa39 - a couple of fixes were needed to make it behave the same way for node-bridge

While testing this for possible regressions I noticed a nasty bug that we might need to address before releasing node-bridge publically

Screenshots of how it looks like in trezor suite now. cc @martykan since he is now addressing this #12487, cc @Hannsek since we might need some designs/copy. Those bullet points are not very accurate.
image

image

@Hannsek
Copy link
Contributor

Hannsek commented Jul 8, 2024

The bullet points are changed in this comment.

@@ -395,10 +393,28 @@ export class UsbApi extends AbstractApi {
return device.device;
}

private createDevices(nonHidDevices: USBDevice[], signal?: AbortSignal) {
private async createDevices(
nonHidDevices: USBDevice[],
Copy link
Contributor

@szymonlesisz szymonlesisz Jul 11, 2024

Choose a reason for hiding this comment

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

we can get rid of code duplicates from enumetate and listen functions by passing all the devices here and do filterDevices before const loadedDevices = await Promise.all(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, added as a new commit 5d893ee

@mroz22 mroz22 merged commit 91cf20d into develop Jul 12, 2024
44 of 53 checks passed
@mroz22 mroz22 deleted the transport-report-hid-devices branch July 12, 2024 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants