mikeshultz / ledger-eth-lib

Library to interface with Ethereum app on Ledger hardware wallets
https://ledgereth.readthedocs.io/
MIT License
19 stars 9 forks source link

Exception handling improvements #13

Closed antazoey closed 2 years ago

antazoey commented 2 years ago

The error experience when not properly connected to a device could be improved.

Another case would be to explore errors for not having the app installed and making APDU specific errors come out okay.

mikeshultz commented 2 years ago

I spent some time on this trying to translate exceptions:

https://github.com/mikeshultz/ledger-eth-lib/blob/fc2962cdce6a8cf3bd9eae180591fe163d4977bf/ledgereth/comms.py#L85-L100

But there are still a bunch of gaps, like when trying to init the dongle. For example:

ledgerblue.commException.CommException: Exception : No dongle found

It's still somewhat clear, but maybe all exceptions should be from this library and not its dependencies. Do you have any specific gripes you've run into?

antazoey commented 2 years ago

I have noticed that if I don't have the Ethereum app open, it will also complain about not finding the dongle. Here is how we are handling this in ape-ledger. It is not perfect but gives a few scenarios:

https://github.com/ApeWorX/ape-ledger/blob/main/ape_ledger/client.py#L173-L179

Here is a response verification we do: https://github.com/ApeWorX/ape-ledger/blob/main/ape_ledger/client.py#L173-L179

Also here is our comms error mapping: https://github.com/ApeWorX/ape-ledger/blob/main/ape_ledger/client.py#L68-L82

Timeout is also nice to have: https://github.com/ApeWorX/ape-ledger/blob/main/ape_ledger/exceptions.py#L23

Also, when trying to sign an unknown message type: https://github.com/ApeWorX/ape-ledger/blob/main/ape_ledger/accounts.py#L86

mikeshultz commented 2 years ago

Used a bunch of your error defs and pulled a few from the app-ethereum source. Added some better exceptions and handling in PR #23. Since you raised this issue, I would appreciate your thoughts on the changes at your leisure.