decred / dcrwallet

A secure Decred wallet daemon written in Go (golang).
https://decred.org
ISC License
218 stars 154 forks source link

Add Grpc functions to legacy rpc #93

Closed SG-O closed 7 years ago

SG-O commented 8 years ago

Pleas add the new functions from the new Grpc interface to the older legacy one as Grpc is a nightmare to work with. First of all the automatically generated API has almost 40000 LOC in Java (You could implement a new wallet in less). Then it needs almost 30 imports. And finally the Api is so bad to work with, that I stopped working on the wallet creation in my GUI.

jrick commented 8 years ago

First off I'd like to say that while I have not tried to use gRPC from a Java application before, Google (who develops gRPC) is a huge Java shop, and I would not expect the experience to be as bad as you are describing. This is also not intended to come off as harsh, but I want to explain why I think gRPC was a good choice for the new RPC server.

First of all the automatically generated API has almost 40000 LOC in Java

I have no idea why the generated files are so large. For comparison, the Go file is under 2K LOC. The C# files (1, 2) are together under 9K, and that's including a whole bunch of (IMHO) unnecessary methods for type equality and hashing functions to use the types as hash table keys.

A rough equivalent of these autogenerated files for the legacy JSON-RPC server would be the dcrjson and dcrrpcclient packages. Three things to note:

  1. These packages were hand written and are a PITA to maintain.
  2. These packages are specific to the Go language. Anything of similar quality for another language would have to also be hand written.
  3. These packages don't cover any parts of the server implementation, just the client side. You'd be on your own with yet another hand written server implementation if you decided to write one.

Autogenerating the RPC bindings for any language you want to use might have some downsides but it's not without a great share of upsides as well.

Then it needs almost 30 imports

This sound about right. gRPC uses HTTP/2 so it is unable to use the HTTP client and server from the Java standard library. Add on top of that Protocol Buffers for the message codec and TLS for transport security and you could be looking at quite a few dependencies. The gRPC Java implementation is also a pure Java implementation so you're not depending on wrapped libraries from gRPC Core.

And finally the Api is so bad to work with

Could you elaborate on this point? The new API was created largely to solve problems of the legacy JSON-RPC API. It is still very new and we're open to suggestions and changes, but saying "it's so bad to work with" won't help us improve it.

One thing to realize about the new API is that because it is so new, I wanted to minimize the API surface as much as possible instead of kitchen sinking every possible wallet feature. At the moment, the API only tries to deal with the wallet's core functionality (saving and using private keys, keeping track of unspent outputs, and saving transaction history). Anything else that is unrelated to the wallet is avoided.

A good example contrasting the new RPC API with the legacy JSON-RPC API one would be the absence of something like the legacy server's createrawtransaction RPC. Creating (unsigned) transactions is something that a client should be able to do, and asking the wallet to do it over RPC is just unnecessary overhead. It's only when the transaction needs to be signed that the wallet needs to become involved and an RPC is invoked.

SG-O commented 8 years ago

Sorry for my brief comment (I was really frustrated at that moment.)

First of all I found out that GRPC for Java is really buggy not really improved and lacks a lot of features other Languages have.

The auto generated API has a little over 39000 loc when optimized for speed and a little less when optimized for code size. In comparison the core Json RPC client I wrote for my wallet has only about 50 loc, with all wrappers it comes up to about 650 loc. I really would love to see this working, but at the moment GRPC for Java is more like a preview than a usable tool. (I'm not talking about Java for Android, as it seems to work on that platform quite well.)

I could live with the amount of imports, but thy reduce the build time significantly, so I would prefer as few as possible.

When I complained that the API was bad, I was not talking about yours. Your implementation looks great to me and I think that I completely understand it. My complaint was that the API for GRPC on Java is a pain to work with: First of all there is the generating of the classes from the .proto file. As GRPC is that new and changing very quickly, most guides are wrong or incomplete. It took me 2 hours to generate the API file and and 3 more to generate the class that handles the services. After that it took me another 2 hours to find out that at this moment, to run a HTTP/2 server on java you have to patch every JRE by hand on any computer the software should run on. At this point I gave up trying to use the official GRPC implementation. After all of that I wrote my own serializer for the command I wanted to execute. I then tried to use the RPC client implementation I wrote and that works perfectly fine, only to find out, that the HTTP/2 server is not backwards compatible with HTTP/1.1.

For now I will not attempt another shot at the new RPC implementation, which is a real problem for me as I still have no way to create a wallet from my GUI. The workaround I am now working on is creating an empty wallet db and then starting dcrwallet.

raedah commented 8 years ago

I am not seeing anything actionable here. Possibly this is an upstream issue that the gRPC java libraries need to be improved? I suggest this issue be closed.

jrick commented 7 years ago

After over a year of using gRPC I do have to say i am much happier working with it than JSON-RPC. This is where API improvements and additions are landing first, primarily to support the GUI wallets. The JSON-RPC interface at this point is really only useful for porting legacy software from other cryptocurrencies or using it with dcrctl on the command line. Some new features may still be added to the JSON-RPC server but our focus is on gRPC. If you want a particular feature added to the JSON-RPC server please open a separate issue for it.