Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate HTTP API to bisq-network #22

Closed
ManfredKarrer opened this issue May 30, 2018 · 9 comments
Closed

Integrate HTTP API to bisq-network #22

ManfredKarrer opened this issue May 30, 2018 · 9 comments

Comments

@ManfredKarrer
Copy link
Member

ManfredKarrer commented May 30, 2018

To integrate the Bisq HTTP-API work from @mrosseel and @blabno I suggest following proposal.

Adjust the BisqAppMain and BisqExecutable so that it can configure the correct startup for 3 different modes:

  • Desktop only (default)
  • Desktop with HTTP-API enabled (set by an option key)
  • HTTP-API only in headless mode (set by an option key)

The HTTP-API comes with it's own option keys which will be supported as well. The details how that will be done is not decided yet.

A big part of startup code is currently in MainViewModel and that has to be refactored to the core project.

The HTTP-API specific code would be added in a new project (bisq-http-api). There it might have initially still duplicated code which got extracted from ViewModel classes. That should be cleaned up over time by refactored code where code which is used now both from the UI and the API will be refactored to classes inside core, so we get rid of those code duplication.

An open question is we should already prepare also support for a grpc API model. I suggest to not do that now but once it is implemented.

One concern of @blabno is that we get too many different combinations of possible combinations how to run the app. I don't see that as an issue at least at the current state of basically possible 3 options. The grpc API would add another option but I doubt that all possible combinations makes sense. So from a first thought on it I assume there will be added 2 new options: grpc-only and grpc+Desktop. I don't see a use case to run both grpc + HTTP-API together. It might be well the case that the HTTP-API will wrap the grpc inside.

@cbeams, @mrosseel, @blabno: What is your opinion about that proposal?

@blabno
Copy link

blabno commented May 30, 2018

One other thing to consider are integration tests.
Right now they require special docker image with bisq-api in it running in headless mode.
Because we duplicate startup code (and api module depends on desktop) then it has all it needs to start.

Once we remove main class from api those tests might get problematic.

@ManfredKarrer
Copy link
Member Author

My plan is to have the API-only mode pure headless. So that should be ok for the integration tests, right?

@blabno
Copy link

blabno commented May 30, 2018

But as far as I understood you want to have Main class in bisq-desktop module and the dependency will be bisq-desktop -> bisq-http-api, not vice versa.
So it seems that without bisq-desktop we cannot start the api.

@ManfredKarrer
Copy link
Member Author

ManfredKarrer commented May 30, 2018

I added a demo project [1] for a gRPC example with 2 simple requests and a dummy placeholder for a Bisq daemon (access to core).

UPDATE:
The BisqMain class will create the BisqDaemon which is used in Desktop and the API. The BisqDaemaon is doing all the startup code and provides a fully featured headless Bisq version.
Any API implementation (HTTP or gRPC) would get the BisqDaemaon and can use that to make requests.
If the HTTP API would use the gRPC or directly the BisqDaemon is open to the devs implementing the API. In case they would use the gRPC API they would instantiate the gRPC and pass the BisqDaemaon over. They would then request the gRPC instead directly the BisqDaemon.

[1] https://github.com/ManfredKarrer/bisq-grpc

@ManfredKarrer
Copy link
Member Author

ManfredKarrer commented May 30, 2018

I started to add 2 projects to Bisq [1,2] and added basic setup for a BisqDaemon [3] and a new DesktopMain [4] class which supports 2 new option keys: desktopWithHttpApi and desktopWithGrpcApi.

I think it is cleaner to use a separate main class [5] for the HTTP-API-only (headless) case, so current plan is to not support the API-only version in the same main class which supports the combination Desktop + API.
@blabno: If there are strong reasons why such would be required, please let me know.

It is all WIP, but wanted to share early the direction....

[1] https://github.com/ManfredKarrer/bisq-grpc/tree/api-integration
[2] https://github.com/ManfredKarrer/bisq-http-api/tree/api-integration
[3] https://github.com/ManfredKarrer/bisq-core/tree/api-integration
[4] https://github.com/ManfredKarrer/bisq-desktop/tree/api-integration
[5] https://github.com/ManfredKarrer/bisq-http-api/blob/api-integration/src/main/java/bisq/httpapi/BisqHttpApiMain.java

@cbeams
Copy link
Member

cbeams commented Jun 1, 2018

I have yet to fully catch up on this proposal. Over the last weeks, I've written up and had a number of conversations with @blabno and @mrosseel about doing a proper proposal for getting the HTTP API integrated, and I want to reference some of those discussions quickly here:

I have other priorities I'm focused on right now, and @ManfredKarrer I'm glad to see that you're diving in here to get stuff done, but I want to state frankly that I'm worried about this work. We've had a hard time aligning on what should be done and how it should be done, and I don't want to see a lot of further implementation work on the integration itself until we can get high-level agreement about what users are going to experience and what the minimum set of requirements for getting the HTTP API out of incubation and into production are. That's why I've been pushing for a proposal, but the proposal as written above jumps pretty quickly into low-level implementation details. What I think everyone needs to get crystal clear on is:

  1. What are users going to experience when this integration is complete? Literally, what will users have to do to enable and interact with the Bisq HTTP API? How will they learn about this new functionality? Where and how will it be documented?
  2. Where will the code for the HTTP API live, both from a repository perspective and from a packaging perspective?
  3. What are the set of criteria for getting the HTTP API out of incubation and into production? I'm talking here about code review, automated testing, style issues, and so on. I've written about some of this in the links above, but it was all very quickly written, is not necessarily complete, and certainly has not been presented a way worthy of being called a proposal.
  4. Precisely how and where will the integration take place, i.e. at the level of code? Which classes will change, which will need to be refactored, etc.

I'm concerned that the proposal as written above focuses mostly on (2) and (4) above, and doesn't address (1) and (3). And really, I don't think that (4) needs to be spelled out in too much detail in a proposal. Those are code changes, and that's what pull requests are for. What's important is that we get aligned on the high-level stuff, the UX stuff, the the security and code review stuff, and the overall quality control stuff here. Actually doing the code changes to integrate should be relatively trivial after that, so please let's back up and deal with first things first. Somebody needs to take the time to write up a proper proposal that deals with all this stuff, so that we don't keep going around in circles about it. As I wrote in https://docs.bisq.network/proposals.html:

In general, good proposals take time to research and write. Every minute you spend clearly and logically articulating your proposal is a minute that you save other contributors in understanding it.

This is why I asked @blabno and @mrosseel to take the time to write this proposal early in the process. In the meantime, there has been a lot of work done based on my initial comments about what should probably go into that proposal, e.g. truncating the bisq-api repository history, repackaging stuff to network.bisq.*, writing documentation in the form of step-by-step getting started guides, etc. It's all great that this work has been done, but it's all been done based on one-off conversations, most of which have been in Slack, where stuff is ephemeral, not discoverable, and eventually disappears fully. No one is clear about the exact criteria for the HTTP API coming out of incubation and into production, and it results in a lot of wasted time and energy. It's just no way to manage the integration of a large, brand-new and user-facing software component into the Bisq stack.

In our most recent conversation about this, @blabno made it clear that he would in fact write such a proposal, but it's not clear to me, @ManfredKarrer, whether your proposal here was something you basically did on your own, without awareness of that conversation that @blabno, or if this proposal was in fact intended to be the same, all-inclusive proposal that I've been pushing for all along.

@ManfredKarrer, you wrote above:

One concern of @blabno is that we get too many different combinations of possible combinations how to run the app.

I am not concerned about there being too many modes / combinations.

I believe that for the first step toward integration, there should be an option in bisq-desktop to enable the HTTP API service, and that's it. No headless mode/option yet. Take things one step at a time. We know it will require further refactoring to get to a place where bisq-core can be run standalone with the HTTP API enabled and no ties whatsoever to bisq-desktop. Until that is possible, bisq-desktop should not try to "fake" any headless mode. When it is possible, people will literally be able to run bisq-core from the command line like a proper unix daemon, and they'll also be able to run bisq-desktop in 'headless server mode', but doing so will just be a very lightweight and convenient shortcut to running bisq-core headlessly in the way I just described above.

@ManfredKarrer, are you familiar with the getting started guide that @blabno has been writing at bisq-network/bisq-docs#54? Here is the latest deployment preview of it: https://deploy-preview-54--bisq-docs.netlify.com/http-api-monitor-offers.html.

Notice how the doc currently tells the user to build mrosseel/bisq-api from source and to start everything up via mvn exec:java. That's because the doc is designed to take the reader step by step from zero to writing their own dead-simple but fully working Bisq offer monitoring bot. And currently, without the HTTP API being integrated into bisq-core or bisq-desktop in any way, this workaround with mvn exec:java is how things have to be done. One question that we should be trying to answer from a high level in this proposal is how should it be done? When this integration is complete, what steps should the user have to take in that guide to enable the HTTP API? I believe the answer is that they should have to download and install the latest Bisq Desktop client, and instead of double-clicking the Bisq app icon, they should have to start it from the command line with, e.g. a --http-server option. That is already pretty onerous, though, so it would be good in addition for the user to be able to specify this same option in the ~/.config/bisq.properties file (which we now support in bisq-pricenode, and as an aside, I think we should probably rename to bisq.conf because .properties is a Java convention, and .conf is a widely-accepted unix convention, e.g. bitcoin.conf). That way, the user can create the config file, add a line that reads http-server=true and then double-click the Bisq app icon as usual (this is the exact same user experience as Bitcoin Core and its config file).

For reasons I mentioned above, there doesn't need to be any other option for running headlessly, etc at this point. The question is what is the simplest possible initial integration that we can do that makes it possible for the interact programmatically with their Bisq node via the new HTTP API.

We also need to align, of course, on the exact implementation details of the integration: whether bisq-http-api is a separate repository, or whether it gets integrated into bisq-core; what the package naming should look like; whether we should consider anything at all with gRPC yet; and so forth. All of these details should be fleshed out in this proposal, and IMO preferably before we do much more coding / implementation work on anything. We can save so much time and effort by writing this stuff down and aligning on it first.

@ManfredKarrer
Copy link
Member Author

@cbeams:
Thanks for your feedback!

Regarding proposal:
No I was not aware of the conversations you mentioned and kept myself intentionally rather out of the API topic.

I started on that work and proposal as I felt that @blabno was stuck in getting the current code base connected with the recent code base and how to integrate the startup process in a way that the headless API version, the Desktop+ API and the Desktop-only version can work.

I agree to all your points and my intention was not to write that kind of proposal but only to deliver a refactoring of the startup process (MainViewModel mostly) so that they are not blocked anymore to work on top of the current Bisq master.

When starting coding I got a bit further and added support for a simple dummy gRPC server/client and a BisqDaemon (headless full features Bisq - not implemented much there though). My plan was actually to make a small simple user case with gRPC like getting the real Bisq balance via gRPC calls and if that model works well the same should be possible for the HTTP-API stuff.
I was not planning to integrate the current HTTP-API as that comes with quite a bit of complexity and probably needs some changes. I just wanted to understand the requirements they have so I can deliver a structure which works for them (thats why I copied over their code in my http-api project to have the code at hand and see better the requirements, but that will get removed later and I leave it to @blabno to integrate the current API code to that project).

Regarding how to start the HTTP-API:
Yes my plan was to pass a option key which enables the HTTP-API. The HTTP-API will come with its own set of new options (e.g. for security) but that I leave to @blabno to integrate.
From a build/deployment point of view I was considering to have the combined (Desktop+API) modes without any change to Desktop from a build perspective (beside adding the dependencies), so the Bisq app will be API-enabled if the option is set. For the headless version I would add a new application (is setup in my projects in gradle) inside the new API projects.

Regarding headless versions:
The effort is not terrible high. It is mainly the MainViewModel (where I did a test refectoring a while agoe in 2 hours to get it headless, though that was very quick and dirty). I estimate 1-2 days to do it properly. And it is work need to be done anyway some day as the MainViewModel is terribly overloaded/polluted.

The other areas where they have atm duplicated code from the UI ViewModels to avoid the dependency to JavaFX I would not touch but it would be for me one "exit criteria of incubation" to get rid of that duplicated code and have properly refactored all those areas to core classes. That should be mostly rather trivial technical refactoring, expect the create and take offer parts which might be a bit more complex. But I think all in all it is no terrible much effort and I estimate that can be done in about 1 week.

Regarding the option key:
Yes all passed options to the app should work as options set in bisq.properties as well. I assume that is the case how they are implemented atm, but I have not tested it. Though the current code expects the bisq.properties file in the root directory of the data directory not in the .config subfolder.
And yes I agree it should be same like in Bitcoin Core. But as we already support the bisq.properties and maybe some users are using that, we have to be careful how to change that. The base currency (BTC, LTC,...) for instance was written in that property file. Supporting both is confusing as well... Maybe we keep that discussion to an own proposal/issue?

Regarding integration:
I would have seen it as a separate project as well as a gRPC project (which might get added later once someone works on its implementation, but the basics I wanted to provide as well). It's helpful for me to prove that the design works well with different APIs and it is not intended to get in Bisq master before it's implemented (and we have a dev who takes it as its project).

Regarding design:
My approach was to have BisqDaemon which lives in core. BisqDaemon does all the bootstrapping and delivers a fully initialized Bisq app.
You could also wrap that in a main class and start that as headless application, but it does not have any outside interface. One of the APIs (HTTP, gRPC) would be next to the Desktop an interface and we pass the BisqDaemon to those so they get a fully functional Bisq app.

The HTTP-API could either use BisqDaemon directly or startup gRPC and use that to access Bisq. I assume using BisqDaemon directly is a preferred approach but both are possible. But the setup for the gRPC is not really much and would be a valid alternative. But the main work will be to define all protobuffer data and as long that is not implemented in gRPC it would be not feasible.

The Desktop creates the BisqDaemon and in case the HTTP-API is enabled via options it passes it to the HTTP-API as well as to the JavaFX Application so both operate on the same process and instance of BisqDaemon.

If BisqDaemon also provides a high level API for all the typically required access methods (I would orient on the current HTTP-API feature set like in BisqProxy) is an open question. I think I prefer to use the existing APIs in the domains (e.g. OpenOfferManager,...) as otherwise there is a huge class which flattens all domains into one place. It would be a Facade to make access to the more complex and distributed APIs in the specific domains easier but I am not sure if that is needed and if it would just introduce more maintenance costs, etc.... But I have not a 100% clear/strong opinion regarding that so far.

Regarding:
bisq-network/bisq#1535 (review)

  • I use "bisq-http-api" as project name
  • I use gradle in my repo (I started to convert the dependencies of the API project to gradle, tough that will require more work missing the excluded, some issue with some test framework libs,... - but basically I could compile all after removing problematic classes and not including the tests from the API project)
  • Regarding API docs: I agree

Conclusion:
I am fine with any plan to go on, either to wait until a proper high level proposal is there or to continue in a WIP style to enable faster and easier integration.

My plan was not to integrate that in Bisq as long the code base is not fully reviewed, accepted and meet our acceptance requirements.

My suggested solution would be that I continue to deliver a base structure where they can start to integrate it with the current Bisq master in my repository. They can fork from my repo and do the required work to make the HTPTP-API work in that design. That way we can bring the API work in sync with master and get rid of that block regarding the startup process and the hacks how to start Bisq with the API in combined and headless mode.
I fear the longer it takes that they continue with the current out of sync project the harder it will become later to merge, specially once the DAO gets into master.

@ManfredKarrer
Copy link
Member Author

ManfredKarrer commented Jun 7, 2018

I close that proposal as it is implemented in the api-integration branch of my repo (https://github.com/ManfredKarrer).
There are 2 new projects bisq-grpc and bisq-http-api.
With that code base API can run in combination with the Desktop app as well as headless standalone version.

@nikicat
Copy link

nikicat commented May 14, 2023

Hello, does Bisq have an HTTP API?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

4 participants