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

Use a GRPC API as core API and use wrappers around for other API types #136

Closed
chimp1984 opened this issue Nov 10, 2019 · 15 comments
Closed
Assignees
Labels
a:proposal https://bisq.wiki/Proposals re:features was:approved

Comments

@chimp1984
Copy link

This is a Bisq Network proposal. Please familiarize yourself with the submission and review process.

Overview

I am aware that we had in the past the discussion if we should use GRPC and it was rejected because we preferred that non-technical users have it easier to write their own applications agains an HTTP API.

I would like to suggest though to rethink that again and suggest that we use GRPC as core API infrastructure and build any other APIs like a HTTP API around that. The benefit it that GRPC is much faster and easier to add and require only 2 dependencies (Googles grpc and netty.io) which are from huge projects and can be considered low risk dependencies (I assume netty could be replaces by other library as well if there is reason for it).

GRPC proof of concept

I got my old GRPC project updated to master and running again and could add 2 new endpoints (getTradeStatistics and getOffers) in roughly 20 min each. We can reuse our existing protobuf definitions for the domain objects and adding a simple endpoint is very simple.

There are still some smaller issues open to improve (see FIXME comments) but as a proof of concept I think it is good enough.

There is still the question open if we should have 2 desktop modules (one without API and one with API embedded) to don't add those dependencies to all Bisq users even if the API is not enabled, but as those dependencies are much lower risk I think it is at least worth to discuss if that is needed.

The GRPC API has no authentication atm and is designed to be accessed via localhost only but still we should do the same way how Bitcoin Core is dealing with it.

Any wrapper API exposing the API to public network need to implement a more secure authentication mechanism but I think the core API we deliver should not take that burden.

If we leave the wrapper APIs to other developers and not require it to become part of the official Bisq repo we do not carry those burdens and it help us to get out of those deadlock situations whoich we have at the moment that no maintainer has enough time and motivation to take the risk to review and audit that.

@blabno Do you think you could adjust your HTPP API to become a wrapper around such a GRPC API?

@chimp1984
Copy link
Author

Place offer is basically working in that prototype. I think it is much easier to stay in the protobuf environment to add endpoint and strongly recommend that we go that route.
The current GRPC would require maybe 2-3 days more to get it production ready.

@blabno
Copy link

blabno commented Nov 11, 2019

The benefit it that GRPC is much faster and easier to add and require only 2 dependencies

Does that include transitive dependencies? Because last time I've checked there were like 15 or more transitive dependencies, which was almost the same as in case of HTTP API.

Do you think you could adjust your HTTP API to become a wrapper around such a GRPC API?

In short yes. But the devil is in the details. The greatest challenge while working on HTTP API was untangling the core and desktop and making some of the core operations sort of stateless.

Apart from that, we've already had this discussion many times and it has been decided that we go with HTTP API. HTTP API has been ready for almost a year but delayed due to amount of code that needed to be reviewed. Finally we've got some initial version accepted into incubator.
The bottleneck are the maintainers. There is a bunch of PRs already prepared and waiting for being merged (and also blocking preparations of further PRs that depend on not merged PRs).

@cbeams
Copy link
Contributor

cbeams commented Nov 11, 2019

How to run the code for this proposal

Here's a quick primer on how to sync up to these changes and exercise the fledgling gRPC API for yourself (@chimp1984, I'm not sure if this was written down elsewhere; I couldn't find it if so).

At the command line

cd $YOUR_LOCAL_BISQ_CLONE
git remote add chimp1984  # this shortcut syntax assumes you have `hub` installed
git fetch chimp1984
git checkout grpc
./gradlew build
./bisq-desktop --appName=Bisq-grpc-demo --desktopWithGrpcApi=true
idea .  # open the project in IDEA

In IDEA

  • Open BisqGrpcClient and run its main method
  • Type getVersion into the console, notice that 1.2.3 is returned
  • Type getBalance into the console, notice that 0.00 is returned
  • See the switch statement in BisqGrpcClient for all supported commands

Proposal feedback

I think the proposal makes sense. Having a 'core' RPC API on top of which any further API (RESTful HTTP, JSON-RPC, etc) can be layered seems workable, and is actually what I originally suggested way back in this March 2018 slide deck.

I don't have much to say about the implementation as I haven't spent enough time looking at and thinking about it. From a UX perspective, though, I'd like to see the following:

  1. a --daemon option to the main (bisq-desktop) binary that allows it to run headlessly with the core (gRPC) API enabled. So passing --daemon would imply passing --desktopWithGrpcApi=true and would suppress the UI. This is consistent with running the Bitcoin Core desktop client with --daemon enabled in order to run it in the background without launching the UI at all.

  2. Ideally, we would ship a bisqd shell script that implies --daemon as well (just as Bitcoin Core ships with a bitcoind binary), but it's not strictly necessary for getting started.

  3. The --desktopWithGrpcAPI option should be renamed to --server or --rpcserver. This is consistent with the way you can run the Bitcoin Core desktop client with --server in order to enable its JSON-RPC server while also launching the UI.

With the --daemon and --server options in place, any external gRPC client can be written to exercise the API and as suggested in bisq-network/events#32 (comment), I'd like to see us ship a bisq-cli utility for convenient access to the API and to serve as a reference client how to program against it. It could start out by talking gRPC directly, and if we grow a JSON-RPC API down the road, it could perhaps be ported to that, as I think it's what many people writing, say, a trading bot would want to use and have a reference for.

There is still the question open if we should have 2 desktop modules (one without API and one with API embedded) to don't add those dependencies to all Bisq users even if the API is not enabled.

Agreed, NACK on having 2 desktop clients. The proposed core API would be just that: quite core to the project. It would be terrible UX to have to ship and explain two clients, and I see no significant threat in adding these minimal dependencies into the mix.

Thanks for putting this together, @chimp1984!

@chimp1984
Copy link
Author

chimp1984 commented Nov 11, 2019

@cbeams
Thanks for looking into it! I did not had time to write a Readme - great that you added those instructions how to get it running quickly.

For the headless/deamon version you use bisq.core.app.BisqHeadlessAppMain and pass the same data directory (e.g. using --appName=bisq-API-demo) as you use in Desktop so you can setup all in a normal desktop app (like BTC funding, payment accounts) and then close it and start the headless app which will execute the API calls (or use the Desktop app with the API enabled option flag). Using the same app for UI and headless is probably not possible due the JavaFX app architecture. But the application code is isolated out from both and the UI app and headless app are just 2 different wrappers to start up Bisq.

Yes, I agree that current option keys are way too verbose ;-).
We also should add at least a new option for the port.

I added just recently the placeOffer endpoint which allows you to create an offer via the API.
Here is an example call:
placeOffer CNY BUY 750000000 true -0.2251 100000000 50000000 0.12 ea6b8f0d-217e-4190-8417-0f02cf90ab1e
The last param is the payment account ID which needs to be adjusted to your custom accounts. You get all payment accounts with the getPaymentAccounts endpoint.
Currency need to be adjusted as well.

Here are the all parameters for placeOffer:

String currencyCode,
String direction,
long price,
boolean useMarketBasedPrice,
double marketPriceMargin,
long amount,
long minAmount,
double buyerSecurityDeposit,
String paymentAccountId

The current state was just a proof of concept to let me see how much effort it is to get the main use cases implemented and it was surprisingly fast to get that working. The refactoring of code from the Desktop module to a new Core service class was also not too hard (about 3 hours work).

Whats mainly missing is an authentication mechanism, some more sophisticated input parser (atm it uses a scanner and splits parameters) and input validation. For the placeOffer use case it also misses atm the fee selection (BSQ or BTC) and it would require more checks (e.g. check balance,...). And of course some endpoint documentations.

I will not have time to continue to work on that so anyone who is interested is welcome to take over...

@chimp1984
Copy link
Author

chimp1984 commented Nov 12, 2019

@blabno

Does that include transitive dependencies?

Sure all big libraries will have more transitive dependencies but I think a library provided from Google is less risky than other less prominent libraries.

untangling the core and desktop and making some of the core operations sort of stateless

I did that for placeOffer now in about 3 hours. It was not a complex and challenging task. I don't see why that could not have been done by any other developer. It is trivial refactoring be moving methods from Desktop to a Core class.

Finally we've got some initial version accepted into incubator.

We have to re-discuss the incubator idea. I think it is with our current dev setup not feasible. It would require a Bisq maintainer to do the merge and reviews otherwise it is the same as it is on the developers repo but gives the appearance to be official Bisq as living inside the Bisq repo and can cause risks that users trust that it was reviewed and audited by Bisq maintainer but it was not. So I think the current setup is completely broken.
EDIT: Seems you do not merge yourself, which is good. I just saw in the Monero incubator project that the devs are the maintainers and that is a broken concept.

The bottleneck are the maintainers.

Yes, lack of developers is a big problem. I think we should keep the core Bisq app as minimal as possible to get out of that bottleneck problem and make it easy for others to use the Bisq core project for sub-projects. Then all those problems are easier to handle. If you are the project owner of the HTTP API and use the official GRPC API as basic and wrap around whatever you want you don't need permission and you don't feel blocked by busy maintainers. For users of your HTTP API it is clear that this is a side project and not Bisq core and that needs to be clearly communicated (e.g. by an altered logo, name etc). Also for Bisq maintainers it is easier when they release a new version that they do not need to test and maintain additional code base. It is like anyone forks Bisq, they are responsible to stay up to date with the root project.
Compensation can still be done if the BSQ stakeholder find such a side project valuable. We also fund Tor relay operators so this aspect should not have too much influence in that discussion IMO.

@cbeams
Copy link
Contributor

cbeams commented Nov 15, 2019

I will not have time to continue to work on [gRPC support] so anyone who is interested is welcome to take over...

I'll pick up on this, as I have some bandwidth for it now. I'll take the changes that @chimp1984 has made in his grpc branch and will iterate on it in my own fork. With what follows below in mind, I think we can close this proposal and move forward.

I plan to deliver this work in a series of small-as-possible PRs that get merged to master and released with each upcoming minor version. Each PR will include complete, usable functionality, e.g.:

  • support for running Bisq as a daemon with the gRPC service enabled using --daemon
  • support for running Bisq as a desktop ui with the gRPC service enabled using --rpcserver
  • support for a few simple commands such as getversion and getbalance
  • support for conveniently exercising these commands using a new bisq-cli client utility

All of the above might constitute the first PR. Then, with this foundation in place, we can iterate on more involved commands to get to a level of coverage where a real trading bot can be developed. I intend to design and stub out such a bot and gradually replace the stubs with real RPC calls as they are implemented until the bot actually works end-to-end. Once that's done, the template should be set and it should be straightforward for anyone to follow convention to contribute new commands on an as-needed basis.

If we can manage to release incrementally as described above, it'll be good for getting users engaged. With each Bisq release, they'll have new API goodies to play with, and will be able to explore them easily with the bisq-cli client. It should be fun and exciting for (dev) users to watch it evolve, and will likely solicit good feedback from folks with real-world needs for a Bisq API. We can make a little noise about it with each release, e.g. via Twitter (/cc @bisq-network/twitter-admins). I imagine if we do so, certain podcasts and news outlets will pick up on it and help get the word out.

Whats mainly missing is an authentication mechanism

I haven't looked deeply enough into this to get into specifics here, but generally the goal will be to implement a basic username/password auth scheme modeled after Bitcoin Core. We can also look at implementing IP restrictions and hands-free cookie auth just as Core does too. From the little bit of looking I've done so far, gRPC does not provide out of the box basic auth, but it has the hooks necessary to implement it on our own. It's also worth noting that LND's gRPC API authentication is managed using TLS and macaroons. I have no experience with the latter, but have heard good things. Macaroons may be overkill for our needs, but if anyone wants to dig deeper (or already knows them well) it would be worth discussing whether they might be a good option for Bisq. You can read up on the original Google Research paper here, there's a Java implementation here and an online macaroons playground here.

As for communication, I propose we rename the existing Keybase #api channel to #rest-api and create a new channel named #grpc-api where we can focus on the work described above. (I've just created the latter now, feel free to join).

Feedback welcome. At a minimum, please react with a 👍 (or otherwise) on this comment if you've taken the time to read this far. Let's get this proposal closed and I'll get to work!

@chimp1984
Copy link
Author

I think that proposal has been accepted and @cbeams is working on the implementation. Should we close it?

@julianknutsen
Copy link

It sure looks approved from the positive feedback. It would be great to have some way to track progress and refer back to the roadmap and discussion that was created. I don't think we have anything like that in Github right now so just searching for the "closed" issue might be sufficient.

Maybe @cbeams has an idea on how to capture the roadmap/progress and if that should be this proposal or something else.

@chimp1984
Copy link
Author

Yes would be good to have closed proposals still highly visible. We use tags to make search easier but not the best solution. Maybe something like a project board for work in progress or staged work/proposals waiting for devs to work on it might be good?

@cbeams
Copy link
Contributor

cbeams commented Dec 12, 2019

@julianknutsen wrote:

Maybe @cbeams has an idea on how to capture the roadmap/progress and if that should be this proposal or something else.

As I mentioned in today's #standup, yes I want to write this up in a fresh doc, better define what MVP looks like, make explicit how we plan to get the word out and to get early feedback from users, get a list of milestones everyone can see getting checked off, etc. As I've been busy with a lot of preliminary refactoring / restructuring work, I'm now planning to do this after the first of the year.

@cbeams
Copy link
Contributor

cbeams commented Dec 12, 2019

@chimp1984 wrote:

Yes would be good to have closed proposals still highly visible. We use tags to make search easier but not the best solution. Maybe something like a project board for work in progress or staged work/proposals waiting for devs to work on it might be good?

I suggest that we:

  • tag approved proposals as was:approved
  • use a GitHub project board as you describe above to transition approved proposals into, e.g., On Hold and Work in Progress columns, and
  • only close an approved proposal when the work it describes has actually been delivered OR when consensus has been reached that the proposal should be dropped, at which point we add the was:dropped label and close the issue with an explanatory comment.

Note that the proposed On Hold column above is a little more general version of "waiting for devs to work on it" without being overly specific that this is the only reason that an approved proposal could be put on hold. It could be blocked on other work getting delivered first (like the Android app can't be completed without the rpc api) Maybe we want that level specificity though. Waiting for Contributors? If we want the generality, maybe Blocked is better than On Hold?

@julianknutsen
Copy link

I like the terms Blocked vs Backlog to indicate the various was:approved states. It makes it easier to understand if it just a resource/priority problem or if there are prerequisites required for the work.

The project board seems like the right way to do this. Maybe we can try it with the gRPC project and see if there is a good workflow that can be duplicated for the other dev projects that are larger than a single bugfix.

@cbeams
Copy link
Contributor

cbeams commented Dec 12, 2019

Just to be clear, you mean 'let's try using a project board for the bisq-network/proposals repo, and use the (to be written) fresh grpc proposal as a guinea pig on that new board?

I thought for a moment perhaps you were suggesting using per-proposal boards to track all related sub tasks as issues.

@julianknutsen
Copy link

Just to be clear, you mean 'let's try using a project board for the bisq-network/proposals repo, and use the (to be written) fresh grpc proposal as a guinea pig on that new board?

Yes.

I thought for a moment perhaps you were suggesting using per-proposal boards to track all related sub tasks as issues.

I don't think the projects need this level of granularity especially going from step 0 to step 1 with actual project tracking. Individual developers may choose to use per-task tracking if it makes their workflow easier, but that doesn't have to be part of GitHub or even publicly available.

@cbeams cbeams self-assigned this Jan 27, 2020
@cbeams cbeams added a:proposal https://bisq.wiki/Proposals re:features labels Mar 13, 2020
@cbeams
Copy link
Contributor

cbeams commented May 18, 2020

Closing as approved and being implemented in bisq-network/projects#11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:proposal https://bisq.wiki/Proposals re:features was:approved
Projects
None yet
Development

No branches or pull requests

4 participants