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

trezorlib API improvements #4490

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

matejcik
Copy link
Contributor

fixes #4464

the following have been implemented in trezorlib:

  • new exception UnexpectedMessageError to raise when Trezor sends a, well, unexpected message
  • client.call(msg, expect=SomeType) will raise UnexpectedMessageError whenever expect is not met, analogous to context.call() in core
  • all of trezorlib now uses this mechanism to enforce expected responses
  • this is type-safe and fully deprecates the @expect decorator, so that has been deprecated & all usages removed from trezorlib
  • functions (mostly) in trezorlib.device, which were returning the string from Success(message="...") have been changed to return str | None, and emit a deprecation warning when someone so much as peeks at the resulting str. We'll change them to -> None in v0.14 or maybe v0.15.
  • function trezorlib.device.recover was returning a Success instance!! 🙄 got the same treatment, still returns an object that kind of looks like a Success but emits deprecation warnings if you touch it.
  • trezorlib CLI calls using these functions were modified so that they don't print the return value anymore. That's the Unix convention: on success, don't print anything.
  • a couple protobuf fields coming from Trezor to device were changed to required
  • changed device.reset to a deprecated kind-of-like-an-alias to newly introduced device.setup.
  • there is no more separation between reset and reset_entropy_check: the entropy check is an integral part of device.setup, and by default the function will pick a random number of entropy check rounds
  • device.setup accepts a private argument _get_entropy() which makes it easier to supply external entropy when needed (such as in tests)
  • tests were updated to deal with these changes
  • added testcases that check expected_responses for the entropy check flow

@matejcik matejcik self-assigned this Jan 10, 2025
@matejcik matejcik requested a review from prusnak as a code owner January 10, 2025 13:46
@matejcik matejcik added the translations Put this label on a PR to run tests in all languages label Jan 10, 2025
Copy link

github-actions bot commented Jan 10, 2025

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3B1 Safe 3 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3T1 Safe 5 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@matejcik matejcik mentioned this pull request Jan 13, 2025
@matejcik matejcik force-pushed the matejcik/entropy-check-python branch from 6450448 to de9174a Compare January 13, 2025 11:16
@matejcik matejcik marked this pull request as draft January 13, 2025 12:13
it's a Python class that emits a DeprecationWarning if you try to use it
for almost anything
(two exceptions that can't be overriden from the wrapper:
 * isinstance(depr, SomeOtherClass)
 * depr is None)

we will return instances of this class to indicate that a return value
of something will be going away
this deprecates `device.reset()`, and moves the new arguments to
`device.setup()`.

Also it changes default backup type on core devices to SLIP39-single,
same as Suite, and randomizes the number of entropy check rounds, if not
provided from outside.
@matejcik matejcik force-pushed the matejcik/entropy-check-python branch from de9174a to f787013 Compare January 13, 2025 14:27
@matejcik matejcik marked this pull request as ready for review January 13, 2025 14:49
@matejcik matejcik requested a review from romanz January 13, 2025 14:49
Copy link
Contributor

@romanz romanz left a comment

Choose a reason for hiding this comment

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

Looks good, added some questions and suggestions:

python/src/trezorlib/client.py Show resolved Hide resolved
python/src/trezorlib/device.py Show resolved Hide resolved
python/src/trezorlib/btc.py Outdated Show resolved Hide resolved
python/src/trezorlib/btc.py Outdated Show resolved Hide resolved
python/src/trezorlib/device.py Show resolved Hide resolved
python/src/trezorlib/stellar.py Show resolved Hide resolved
python/src/trezorlib/tools.py Outdated Show resolved Hide resolved
python/src/trezorlib/tools.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
translations Put this label on a PR to run tests in all languages
Projects
Status: 🔎 Needs review
Development

Successfully merging this pull request may close these issues.

Entropy check: improve Python API and tests
2 participants