[RFC] Drop Keepkey

[achow101]

Should we drop support for the Keepkey?

Keepkey is a clone of the Trezor, so we currently just reuse the Trezor implementation. As such, trezorlib and the TrezorClient needs a couple of modifications to support the Keepkey. However these changes do make updating Trezor things harder to ensure that Keepkey still work. Furthermore, doing so also means that we lose Trezor functionality.

The specific incompatibility between the Trezor and Keepkey that causes the issues is that the Features message differs. Using a stripped down Features message to support will result in lost functionality on the Trezor. I've tried a couple of other things to try to swap out the the module implementing the message either don't work, or involve hijacking some very internal things in Python that we probably shouldn't be doing. In the latter case, while it may work for the CLI tool, it's possible that library users won't work.

Instead of doing some weird Python thing, we could instead copy the trezorlib implementation and just make it Keepkey specific (note that the actual keepkeylib is not usable for us as it is based on an older trezorlib that phones home and would require even more significant modification to fit our standards). So Keepkey would be implemented as a separate device instead of as a Trezor device. However this will mean that there's a lot of duplicated code and we still have to go through the work of patching trezorlib to work with the keepkey every time it is updated.

But there's also the question of whether it is even worthwhile to keep the Keepkey. It does not seem like the Keepkey is actually used. So in that case, we could probably just drop it entirely without any adverse effect on the users of HWI.

[prusnak]

There is a 6.6.0 release of python-keepkey here: https://github.com/keepkey/python-keepkey/releases/tag/v6.6.0 - it might include some improvements that address the main painpoints.

It would be ideal if python-trezor dependency could be used as is (modulo stuff you don't want in HWI - we could work together to make upstream even more modular), but because it's patched to support Keepkey is well, it creates extra friction while updating the codebase.

TL;DR: not suggesting to drop Keepkey, but saying HWI should use python-keepkey (or its modification), not patch python-trezor to work with Keepkey too.

[achow101]

@prusnak I still think it is preferable to make trezorlib be able to support the keepkey, either through direct modification, or more extensive upstream changes that makes it possible to have different messages for different devices (I am working on a modification that would allow this that could be upstreamed, it is not keepkey specific).

Even though keepkey has their own python library, I have found it difficult to modify it to meet the standards we use. Furthermore, given the similarities between trezor and keepkey, I think it's a waste of space to duplicate the Trezor implementation for the keepkey just to use the keepkey library. It also ends up being a bit of a waste of time when implementing new features as it has to be implemented twice, in basically identical ways (but slightly different because the libraries have slightly different APIs even though underneath they do the same thing).

The original move to combining Keepkey with Trezor was because stripping down and modifying python-keepkey was an incredibly non-trivial task and it had diverged enough from trezorlib that I couldn't just do approximately the same thing I had done there. It was also getting annoying to have the same thing implemented twice and having two files that were almost identical except for a few places.

[achow101]

With the new trezorlib, I've worked out a slightly more permanent solution. Given that the keepkey seems to still be maintained, support for it will not be dropped in the foreseeable future.