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

trezor: support trezorlib==0.13.9, add compat for <=0.13.8. fixes #9116 #9118

Closed
wants to merge 1 commit into from

Conversation

accumulator
Copy link
Member

No description provided.

@ecdsa
Copy link
Member

ecdsa commented Jul 5, 2024

Do we we really want to support incompatible versions of trezorlib?
Testing the trezorlib version by try.. except ImportError is ugly. If we keep support for the old version, we should be checking trezorlib.__version__ instead.

I am not saying we should do that, though. I would rather drop support for the old version, unless we have a good reason to keep supporting it. Has this been thought through?

@accumulator
Copy link
Member Author

I prefer dropping support for the old version, however I haven't had time to test against 0.13.9 so this PR is just meant as a convenience to test against 0.13.9

@spesmilo spesmilo deleted a comment from Sentini14 Jul 5, 2024
@PiRK
Copy link

PiRK commented Jul 7, 2024

On my end the reasoning was that 0.13.8 is well tested and I would rather use it for another release or two while people running from source can start testing 0.13.9 in real life scenarios and confirm that there is no other hidden broken compatibility issue.

@dlitz
Copy link
Contributor

dlitz commented Jul 20, 2024

I just noticed this after submitting my PR, but if you want a simpler change that only supports 0.13.8 and 0.13.9, I made a patch that does that in #9141.

@SomberNight
Copy link
Member

I have merged the other PR (#9141).
That still has the "compat" part but in a cleaner way IMO.
Thanks for the comments and code itself in both PRs.

Do we we really want to support incompatible versions of trezorlib?
I would rather drop support for the old version, unless we have a good reason to keep supporting it.

In general we don't, I agree, but in this case it can be done relatively cleanly.

Do we we really want to support incompatible versions of trezorlib?
Has this been thought through?

One reason to support somewhat older python-trezor, would be that debian is backporting new versions of electrum much more frequently than new versions of python-trezor. Atm debian stable (backports) has electrum 4.5.5 but only python-trezor 0.12.4. i.e. they are incompatible (debian ticket). Anyway, clearly the gap is too large and this specific change does not help.

Still, in general I don't think it is worth a lot of code complexity or effort.

@accumulator accumulator deleted the trezor_0.13.9 branch August 16, 2024 09:29
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.

None yet

5 participants