[bisq-network/bisq] Http api (#2253)

[Chris Beams]

cbeams requested changes on this pull request.

NACK, particularly with regard to the dependency concerns below.

My understanding is that this PR supersedes #2240. Not sure why that one hasn't been closed in favor of this one.

I have limited time and so have not done anything approaching a complete review of this work, but here's what I see from the few minutes I've given to it:

1. There are a large number of dependencies introduced here. Has every effort been made to reduce these? Why for example is the Kotlin standard library included here? Is it actually required at compile or runtime, or is this (large) dependency actually optional but being being dragged in by default nevertheless? Manfred and I agreed some time ago to take a stricter stance with the introduction of new dependencies. There are already so many, and each is a potential attack vector. This is not to say the solution should be fundamentally reworked so as to reduce these dependencies, but that the dependencies themselves should be closely scrutinized so as to eliminate all those that are not absolutely necessary to the functioning of the 'minimal http api'

2. I would reconsider the `bisq.httpapi` packaging, and rename it to `bisq.http`. "httpapi" is awkward, IMO, and 'http' is clear enough, i.e. this package and its subpackages are about an HTTP interface to a running Bisq node. I don't think we need to explicitly qualify with "API" everywhere. Likewise, I would name the gRPC package simply `bisq.grpc`, not `bisq.grpcapi`.

3. Import organization needs some work. Add entries to the `IMPORT_LAYOUT` section of `.idea/codeStyles/Project.xml` to appropriately order new packages. Keep in mind that the ordering here is not alphabetical, but rather based on each package's relationship to other packages in the overall stack, i.e. how "high or low level the dependency is. This is not an exact science, but something that you eyeball and get approximately correct. The idea is to be able to read the import statements in any given class definition in a logical and useful way. They should read from top to bottom with high-level packages, e.g. `bisq.*` being at the top, and low-level packages, e.g. `java.*` being at the bottom, with significantly different packages being separated by blank lines where appropriate.

4. I don't see any documentation here or links to or mentions of plans for documentation elsewhere. Minimally, I would expect a `README.md` in the new `http-api` subproject directory that details what, at a minimum, is necessary to get up and running with the API (command line args, etc), and a simple-as-possible 'hello world' example of getting results from it, ideally using `curl` at the command line (possibly in conjunction with `jq`), such that anyone can just try the example out without having to bootstrap a particular programming environment to do so. Ideally, a larger worked example would be documented as well, e.g. something approaching the "simplest possible trading bot", but I agree it would be reasonable to do that after the inclusion of this most basic cut at the API.

Again, the above are my observations after merely looking through the PR textually. I have not spun up the API, attempted to use it, reviewed the structures and data that are produced by it, etc.

As a final and perhaps rather frank note, I am just not excited about adding a bespoke HTTP+JSON API  to Bisq. As has been discussed elsewhere over the last year or so, a (g)RPC API would be preferable for the creation of trading bots and most other applications that interact programmatically with a Bisq node, and with the GA release of gRPC-Web late last year ([announcement](https://grpc.io/blog/grpc-web-ga), docs [here](https://grpc.io/docs/quickstart/web.html) and [here](https://grpc.io/docs/tutorials/basic/web.html), [GitHub project](https://github.com/grpc/grpc-web)), I see no reason not to consolidate up front on gRPC as it appears to be able to handle _all_ API use cases now. In my opinion, an HTTP API such as the one presented here will quickly become legacy following the introduction of a proper gRPC API, and will just mean that much more maintenance burden. I know I would want to deprecate and remove it as soon as we have a working gRPC API. I've mentioned this before, but we can learn from the Lightning Labs team on this front: they view the `lnd` HTTP API as a legacy burden and direct users by default to the (newer) gRPC API by default. I understand a lot of work (and re-work) has gone into this HTTP API over the last two years, but this kind of "sunk cost" reasoning is not sufficient IMO to justify its inclusion if we have every expectation that it's going to create even more costs and burden down the road, i.e. if we can predict that we'll want to get rid of it as soon as a gRPC API exists. I won't go so far here as "vetoing" the inclusion of this API, as it could be of value to users in the interim, so long as we're agreed that we may deprecate and remove it in the relatively near future. I wouldn't want to create or imply any strong backward compatibility guarantees around this work if we can already see its successor on the horizon.



-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/2253#pullrequestreview-192081471
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20190114/d76e577e/attachment-0001.html>