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

python: Allow overriding some Trezor specific things #1449

Closed
wants to merge 3 commits into from

Conversation

achow101
Copy link
Contributor

@achow101 achow101 commented Feb 4, 2021

When working with Trezor clones, it is useful to use trezorlib over their forks of the library. As such, we need to be able to specify alternative vendors, minimum firmware versions, and alternative message classes to use for those devices. This PR adds attributes to TrezorClient that allow a caller to override those things.

Specifically, msg_type_to_class_override is a map of wire types to message classes. When decoding a message, the wire type is looked up in this message first before falling back to the default Trezor messages. vendors is added for the vendors check. This defaults to the VENDORS tuple, but can be overridden with another tuple by the caller. Lastly minimum_versions replaces MINIMUM_FIRMWARE_VERSION in the is_outdated check. This defaults to MINIMUM_FIRMWARE_VERSION but can be overridden by the caller.

Copy link
Member

@prusnak prusnak left a comment

Choose a reason for hiding this comment

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

utACK

@matejcik
Copy link
Contributor

matejcik commented Feb 4, 2021

re supported vendors, minimum fw versions, also USB IDs in #1448, i would move in a slightly different direction: introduce something like a "DeviceModelDescriptor" that has a name, USB ID, vendor, and minimum FW version. This would be part of transport and the client could query things it cares about -- and enumerate() would get a list of these descriptors to know which things to recognize.

i'll try to implement this probably tomorrow; it is also practical for trezorlib internally, as it allows us to handle T1 vs TT split more nicely

re message overrides: wondering why you think this is a better solution than inserting the overrides into mapping directly? ISTM for HWI's usage that would be sufficient?

@achow101
Copy link
Contributor Author

achow101 commented Feb 4, 2021

i'll try to implement this probably tomorrow; it is also practical for trezorlib internally, as it allows us to handle T1 vs TT split more nicely

Sounds good.

re message overrides: wondering why you think this is a better solution than inserting the overrides into mapping directly? ISTM for HWI's usage that would be sufficient?

Since mapping is global, inserting into it directly causes the message to change globally. It's problematic for enumerate which needs to communicate with all devices that are plugged in. So if both a Trezor and Keepkey are plugged in, it can end up overwriting the mapping with the Keepkey message before a Trezor is being enumerated which then causes Trezor enumeration to fail. Having specific overrides within the TrezorClient itself lets us have the overrides be contained within the scope that actually needs them.

@tsusanka tsusanka added this to the 21.05 milestone Mar 4, 2021
@alex-jerechinsky alex-jerechinsky modified the milestones: 21.05, 21.06 May 11, 2021
@tsusanka tsusanka modified the milestones: 21.06, backlog May 20, 2021
@alex-jerechinsky alex-jerechinsky added this to To be reviewed in Firmware Pull Requests via automation Jul 21, 2021
@matejcik matejcik moved this from To be reviewed to In progress in Firmware Pull Requests Jul 21, 2021
@tsusanka tsusanka moved this from 📥 Inbox to 👨‍💻 Code in Firmware · Backlog 🗂 Oct 5, 2021
@tsusanka tsusanka removed this from the backlog milestone Oct 6, 2021
@alex-jerechinsky alex-jerechinsky removed this from 💻 Code in Firmware · Backlog 🗂 Oct 22, 2021
@alex-jerechinsky alex-jerechinsky added this to To be reviewed in Pull Requests via automation Oct 22, 2021
@alex-jerechinsky alex-jerechinsky added this to 💻 Code in Backlog 🗂 Oct 22, 2021
@alex-jerechinsky alex-jerechinsky moved this from To be reviewed to In progress in Pull Requests Oct 22, 2021
@alex-jerechinsky alex-jerechinsky removed this from In progress in Firmware Pull Requests Oct 22, 2021
matejcik added a commit that referenced this pull request Nov 26, 2021
This keeps information about vendors and USB IDs in one place, and
allows us to extend with model-specific information later.

By default, this should be backwards-compatible -- TrezorClient can
optionally accept model information, and if not, it will try to guess
based on Features.

It is possible to specify which models to look for in transport
enumeration. Bridge and UDP transports ignore the parameter, because
they can't know what model is on the other side.

supersedes #1448 and #1449
matejcik added a commit that referenced this pull request Nov 26, 2021
This keeps information about vendors and USB IDs in one place, and
allows us to extend with model-specific information later.

By default, this should be backwards-compatible -- TrezorClient can
optionally accept model information, and if not, it will try to guess
based on Features.

It is possible to specify which models to look for in transport
enumeration. Bridge and UDP transports ignore the parameter, because
they can't know what model is on the other side.

supersedes #1448 and #1449
@matejcik
Copy link
Contributor

@achow101 would you mind looking at the current state of hwi-compat branch https://github.com/trezor/trezor-firmware/compare/hwi-compat, in particular commits 9caa667 and cab4ca9, and see if this works for your usecase?

matejcik added a commit that referenced this pull request Nov 29, 2021
This keeps information about vendors and USB IDs in one place, and
allows us to extend with model-specific information later.

By default, this should be backwards-compatible -- TrezorClient can
optionally accept model information, and if not, it will try to guess
based on Features.

It is possible to specify which models to look for in transport
enumeration. Bridge and UDP transports ignore the parameter, because
they can't know what model is on the other side.

supersedes #1448 and #1449
@matejcik
Copy link
Contributor

matejcik commented Dec 2, 2021

closing in favor of #1967

@matejcik matejcik closed this Dec 2, 2021
Backlog 🗂 automation moved this from 💻 Code to ❌ Closed Dec 2, 2021
matejcik added a commit that referenced this pull request Dec 7, 2021
This keeps information about vendors and USB IDs in one place, and
allows us to extend with model-specific information later.

By default, this should be backwards-compatible -- TrezorClient can
optionally accept model information, and if not, it will try to guess
based on Features.

It is possible to specify which models to look for in transport
enumeration. Bridge and UDP transports ignore the parameter, because
they can't know what model is on the other side.

supersedes #1448 and #1449
matejcik added a commit that referenced this pull request Dec 7, 2021
This keeps information about vendors and USB IDs in one place, and
allows us to extend with model-specific information later.

By default, this should be backwards-compatible -- TrezorClient can
optionally accept model information, and if not, it will try to guess
based on Features.

It is possible to specify which models to look for in transport
enumeration. Bridge and UDP transports ignore the parameter, because
they can't know what model is on the other side.

supersedes #1448 and #1449
matejcik added a commit that referenced this pull request Dec 7, 2021
This keeps information about vendors and USB IDs in one place, and
allows us to extend with model-specific information later.

By default, this should be backwards-compatible -- TrezorClient can
optionally accept model information, and if not, it will try to guess
based on Features.

It is possible to specify which models to look for in transport
enumeration. Bridge and UDP transports ignore the parameter, because
they can't know what model is on the other side.

supersedes #1448 and #1449
@matejcik matejcik moved this from In progress to Merged in Pull Requests Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Backlog 🗂
❌ Closed
Development

Successfully merging this pull request may close these issues.

None yet

5 participants