search

summa-tx / coins

Rust implementations of BIP32/39 and Ledger device comms
Other
92 stars 31 forks source link

feat: add Termux support to `coins-ledger` #95

Closed xJonathanLEI closed 2 years ago

xJonathanLEI commented 2 years ago

Resolves #94.

Had to make some debatable decisions on this PR.

  1. Error Handling

    I'm adding a new variant InvalidTermuxUsbFd to NativeTransportError because I want to provide a helpful error message. If I simply make use of existing variants, users might just think the platform isn't supported, whereas in fact they're just not invoking the program correctly (must run as sub-process of termux-usb, which opens the file descriptor through Java API).

    However, adding a new enum variant is technically a source-breaking change (for those exhaustively listing all NativeTransportError variants in a match expression, though I doubt if anyone would do that). We might need to bump the version straight to 0.7.0 instead of 0.6.1. I don't know if that's desired.

    An alternative would be to gate the variant behind a new feature termux, but I think that's anti-pattern.

  2. Handling of non-Termux Android

    Users might run a shell in Android directly, without the Termux layer, if the device in question is rooted. I don't have such a device and thus cannot test how to make that work. As a result, I just copy-pasta-ed existing code to that else branch.

Please kindly let me know if you want me to change anything.

xJonathanLEI commented 2 years ago

@prestwich Sorry to push but any update on this? Been a while lolll

prestwich commented 2 years ago

We might need to bump the version straight to 0.7.0 instead of 0.6.1. I don't know if that's desired.

given that we're pre-1.0 I feel no qualms about breaking changes in minor versions