Http api

[ManfredKarrer]

No description provided.

[cbeams]

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, docs here and here, GitHub project), 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.

[blabno]

dependencies

I'll try to remove each transitive dependency and run tests to see if they are really required.
Regarding kotlin, it is not part of this request. It was pulled in before.

bisq.httpapi vs bisq.http

The latter doesn't tell you if this is a set of tools to make http requests or something else.
Perhaps bisq.api.http would be more clear.

readme

As to missing readme, this is not a complete solution. Manfred wanted to go step by step in as small PRs as possible.

[blabno]

@ManfredKarrer why did you remove integration tests from travis? They are the best testing effort we've got.

[blabno]

ACK api works in both headless and desktop mode, integration tests are passing.

[blabno]

@ManfredKarrer I've tried trimming the dependencies for the api. Most were required.
Here are the changes you can pull into your branch: https://github.com/blabno/exchange/tree/manfred-http-api

[mrosseel]

hi @cbeams, thx for the response, my 2cents (as @blabno has been doing most of the work lately) on the http/grpc discussion - most of the work in getting the http api merged is actually reworking bisq itself. Once the bisq code is 'api ready', integrating grpc would be relatively straightforward. Anyone can whip up the grpc code, but getting the bisq code to the point that it's api-suitable without breaking things is long and hard work that manfred and blabno are doing right now, adding grpc into the mix would just be a distraction in my opinion. Even after gRPC is available an http api will probably be useful for a subset of users. If the grpc-web thing works well then the current http api could indeed be replaced by it, why not.

[ManfredKarrer]

Thanks @cbeams for the review!

I will discuss with @blabno about grpc.

As you might be surprised why we created a new branch next to master for the api (as well for the new trade protocol) here is my reasoning behind that:
Keeping those projects outside the scope of Bisq (as it was before) turned out to does not work well. I did not actively follow those projects and the merge policy by the code owner was not following our rules. With the branch (where we apply same strict rules as for master - e.g. need to be production quality) we force ourself (myself mainly) to review and test and get more progress in smaller iterations. I also wanted to avoid that those big projects will get added in one big chunk adding a lot of risk and the difficulty to estimate the BSQ value for it. To break it down in to many PRs over months with requests each month we can stream it better.
For the api there is the problem and risk that people are using it already and it can potentially cause problem for the network and other users if there are bugs. So to get that work reviewed and tested (by me) became more a priority and doing it with a separate branch is the best I could come up. Using master would add much more test efforts as with each release we would need to have much more test scenarios covered. As the branch is "experimental" and not intended to be used by normal user we can relax a bit the requirements for testing compared to the master branch and releases.

Re Kotlin:
I dont' see the dependency defined anywhere. I think it is a transitive dependency from netlayer.
@freimair might have a better explaination...

Re bisq.httpapi:
Not 100% sure about http as we have some http package in the p2p module.
So I would prefer api as main package and http as subpackage inside. Thought that might
conflict later if we add grpc, thought to have a grpc as mainpackage would not conflict with expections like with http so to omit there the api might be ok.
Still not optimal as if there is a api module its not clear that the grpc is also an api.
I will discuss with Bernard about grpc and lets postpone that naming issue for a bit later.

Re codeStyles:
Fixed by @blabno and pushed in new commit

Re docs:
Yes a README.md must be added. As well instructions how to run the API.
I for instance could not get it working (although I had it working earlier).
@blabno can you add those? The doc only need to hanlde the implemented use case (version)
at that moment. Lets add the relevant docs with each feature getting added.

[ManfredKarrer]

@blabno re travis, lets discuss in a call. I have a few questions reagarding that...

[ManfredKarrer]

@blabno
As discussed can you make a PR to my branch with those changes?

• get issue at headless mode with wallet not initialized solved
• add Readme and docs
• lets rename module to http and base package to bisq.http.api

Let me know once you had time to investigate in grpc. I would independently just go on with that branch and if we decide to move directly to grpc we can delete that branch.

[blabno]

@ManfredKarrer PR ready here: ManfredKarrer#1

Headless issue is more complex and I lack knowledge about the startup process. I think that the problem is located somewhere around https://github.com/bisq-network/bisq/blob/master/core/src/main/java/bisq/core/app/BisqSetup.java#L475

[ManfredKarrer]

@blabno @cbeams
From my side the PR is now ready for merge. Do you want to have a final check?

@cbeams I discussed with @blabno to investigate effort for moving directly to grpc. It will probably take a bit until he can report something. In the meantime I would prefer to go on with the currnet http api version.

[blabno]

@ManfredKarrer Just add following tweaks to readme: ManfredKarrer#1

[ManfredKarrer]

@blabno Ah good. I will merge then.