search

LedgerHQ / ledger-dotnet-api

.NET API for Ledger
MIT License
48 stars 30 forks source link

Ethereum app not working with api #9

Open mtleliever opened 6 years ago

mtleliever commented 6 years ago

Hi there.

I've noticed that the api runs into errors when trying to run the sample code with the Ethereum app opened instead of Bitcoin.

First off, I noticed that LedgerClient.GetHIDLedgers() returns a 0 length IEnumerable when the "Browser Support" setting is enabled on the Ledger Ethereum App. When Browser Support is disabled, I do end up getting a proper result with LedgerClient.GetHIDLedgers().First().

However, any call I make after that gets me a LedgerWalletException: INS not supported. Even a simple call where I can't really make any mistakes, such as ledger.GetFirmwareVersion() runs into the same exception.

When running this same code on the Bitcoin app for example, I get the proper firmware version and don't run into any exceptions.

This was tested using the latest firmware version of the Ledger.

NicolasDorier commented 6 years ago

I don't know about how the Ethereum app is working, but it has probably different API than Bitcoin.

mtleliever commented 6 years ago

I decided to look more deeply into this to determine more accurately what was going on, and discovered a few things in the process.

It is not only the Ethereum app which runs into the LedgerWalletException: INS not supported, but it is several apps. Ethereum, Ripple, Stellar, Nano, and Ark all run into the same error when performing any call, such as ledger.GetFirmwareVersion() for example. The one thing these apps all have in common is they are the only apps (that I found) on the Ledger which have Settings in the app. The Bitcoin app, Dogecoin, Dash, and many others, have no Settings in their app, and they all function fine with the ledger.GetFirmwareVersion() call. The contents of Settings can vary. Some will have the "Browser Support" option, while others have different ones, but they all run into the error no matter what if they have Settings.

I decided to sniff around in the code a bit to see where things actually started changing. In HIDTransportBase, in the Read method, on line 182, when the data is read from the HID, is where the data starts to differentiate. The packet byte[] with the Bitcoin app is the same as every other app without Settings, while the packet byte[] with the Ethereum app is the same as every other app with Settings. So then finally, when the command is unwrapped on line 185, the result is incorrect and completely different, which is what ends up causing the error I think.

Do you have any plans on looking into this and potentially providing a fix for it? If not, do you have any idea how I might go about approaching a fix for this?

NicolasDorier commented 6 years ago

I will not look into this myself, but I would be happy to merge any PR you would make as long as it does not break Bitcoin related currencies.

MelbourneDeveloper commented 6 years ago

@ThatSlyGuy I am starting to wade through the code here: https://github.com/LedgerHQ/blue-app-eth . But, it's not exactly pretty. You are right about these issues. It seems that there is no standard across the Ledger 3rd party apps. It appears that each app will have its own logic and messaging system.