Introduce gRPC API proof of concept #3888
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit introduces a new `grpc` module including the following key
types:
- BisqGrpcServer: The API implementation itself, along with generated
gRPC Response/Reploy types defined in grpc/src/main/proto/grpc.proto.
- BisqGrpcServerMain: A 'headless' / daemon-like entry point for
running a Bisq node without the JavaFX desktop UI.
- BisqGrpcClient: A simple, repl-style client for the API that allows
the user to exercise the various endpoints as seen in the example
below.
In the `desktop` module, the BisqAppMain class has been modified to
start a BisqGrpcServer instance if the `--desktopWithGrpcApi` option has
been set to `true`.
In the `core` module, a new `CoreApi` class has been introduced
providing a kind of comprehensive facade for all Bisq functionality to
be exposed via the RPC API.
How to explore the proof of concept:
1. Run the main() method in BisqAppMain providing
`--desktopWithGrpcApi=true` as a program argument or alternatively, run
the main() method in BisqGrpcServerMain, where no special option is
required. In either case, you'll notice the following entry in the log
output:
INFO bisq.grpc.BisqGrpcServer: Server started, listening on 8888
2. Now run the main() method in BisqGrpcClient. Once it has started up
you are connected to the gRPC server started in step 1 above. To
exercise the API, type `getVersion` via stdin and hit return. You
should see the following response:
INFO bisq.grpc.BisqGrpcClient - 1.2.4
Likewise, you can type `getBalance` and you'll see the following
response:
INFO bisq.grpc.BisqGrpcClient - 0.00 BTC
and so forth for each of the implemented endpoints. For a list of
implemented endpoints, see BisqGrpcServer.start().
Note once again that the code here is merely a proof of concept and
should not be considered complete or production-ready in any way. In a
subsequent commit, the `--desktopWithGrpcApi` option will be disabled in
order to avoid any potential production use.
The content of this commit is the result of squashing a number of
commits originally authored by chimp1984 in the `chimp1984` fork's `grpc`
branch.
Co-authored-by: Chris Beams <chris@beams.io>
The previous commit introduces the BisqGrpcServer as a proof of concept, but it is not yet ready for production use. This commit removes the `--desktopWithGrpcApi` option that starts the gRPC server until such time that it is production-ready. This change also removes the `--desktopWithHttpApi` option for starting an HTTP API server. The option has been in place for some time, but it was 'false advertising' in the sense that nothing actually happened if the user specified it, because there is in fact no HTTP API implementation to be started. Note that when the gRPC API option is reintroduced, it will be renamed to `--rpcserver` or similar, following the convention in Bitcoin Core.
|
JFYI: I recently activated codacy's PR Quality Review for the Bisq repository. It found for your PR a couple of issues which you can view in the |
ripcurlx
previously requested changes
Jan 17, 2020
Will do, thanks. |
cbeams
dismissed
ripcurlx’s stale review
I fixed the code quality issues @ripcurlx mentioned in his NACK and the Codacy report is now clean.
ripcurlx
reviewed
Jan 20, 2020
| log.error(e.toString(), e); | ||
| } | ||
| break; | ||
| case "stopServer": |
There was a problem hiding this comment.
Somehow this command didn't work for me.
There was a problem hiding this comment.
I just checked it locally and it does work for me. That is, after I call the stopServer RPC from the client, no further commands can be issued from the client because indeed the server has been stopped.
ripcurlx
reviewed
Jan 20, 2020
| result = paymentAccounts.toString(); | ||
| break; | ||
| case "placeOffer": | ||
| // test input: placeOffer CNY BUY 750000000 true -0.2251 1000000 500000 0.12 5a972121-c30a-4b0e-b519-b17b63795d16 |
There was a problem hiding this comment.
NIT: 0.12 should be at least 0.15 otherwise the security deposit is too low for our current setup
There are two structural / organizational reasons for this move:
1. References from one package to another should always be upward or
lateral, never downward, as the latter causes package cycles (aka
'tangles') which damage the suppleness and understandability of a large
codebase. Prior to this change the high-level bisq.core.CoreModule
class imported many classes from child packages like
bisq.core.{btc,dao,user,util}, etc. By moving CoreModule down into the
'.app' package, it can reference all these other packages as siblings
instead of doing so as a parent.
2. the bisq.core.desktop and bisq.core.app packages are the only
locations that reference the CoreModule class. By moving the class
into bisq.core.app, greater cohesion is acheived, again making the
codebase that much easier to read and understand.
This change stubs out the `bisq-cli` utility with a placeholder main
method, such that the following now works:
$ gradle :cli:build
$ ./bisq-cli
Hello, World!
Such that :grpc (soon to be renamed to :daemon), :cli and :desktop can access these types.
This change replaces the previous "Hello, World!" :cli main stub with
the contents of the BisqGrpcClient as originally implemented in :grpc,
such that the following now works:
$ gradle build
$ ./bisq-daemon # in first terminal
$ ./bisq-cli # in second terminal
getVersion # user input to stdin
1.2.3 # rpc response
getBalance # user input to stdin
0.00 # rpc response
...
Or for a user experience closer to where things are headed:
$ echo getVersion | ./bisq-cli
1.2.3
Note that processing the above command is currently far too slow:
$ time echo getVersion | ./bisq-cli
1.2.3
real 0m1.634s # ouch
user 0m1.746s
sys 0m0.180s
Subsequent commits will work to bring this time down to a minimum.
Note: Includes the code quality changes originally made to
BisqGrpcClient in commit 7595387.
This change: - Removes several superfluous dependencies not required for our purposes with gRPC - Cleans up the way Gradle source sets are managed for generated gRPC sources and classes - Makes use of Gradle's new `implementation`, `compileOnly` and `runtimeOnly` dependency configurations where changes were otherwise being made. See https://stackoverflow.com/a/47365147 for details. Remaining uses of the now-deprecated `compile` and `runtime` configurations should be eliminated in a refactoring separate and apart from the present gRPC API work. - Upgrades several existing dependencies to align with newer versions of the same dependencies introduced transitively by grpc-* 1.25.0 libraries, including: - protoc from 3.9.1 => 3.10.0 - gson from 2.7 => 2.8.5 Note that a number of the grpc-* libraries depend on Guava v28, and our existing dependency on Guava v20 has *not* been upgraded to this newer version because it is incompatible with the way we have used Guava's Futures API. It appears that the grpc-* libraries function correctly against this older version of Guava, and more investigation would be required see whether upgrading our uses to the new Guava API is feasible / worth it. The way we are preventing this upgrade is with the use of `exclude(module: "guava")` directives on grpc-* dependencies.
The :grpc module will soon be renamed to :daemon. These two modules represent two separate and equal modes of running bisq, either as a desktop GUI or as a daemon. They are both applications, and one should not depend on the other as it would be illogical and confusing to model things that way. The reason for the current dependency from :desktop to :grpc is because :grpc is the home of BisqGrpcServer. This change moves this class up to :core, in a new bisq.core.grpc package, such that both the :desktop and :daemon applications can pull it in cleanly. The CoreApi 'facade' that BisqGrpcServer uses to abstract away bisq internals has been moved from bisq.core to bisq.core.grpc as well and for the same reasons detailed in 8b30c22. This change also renames the Java package for generated grpc types from bisq.grpc.protobuf to bisq.core.grpc (the same new package that BisqGrpcServer and CoreApi now live in). Again, this is for reasons of cohesion: BisqGrpcServer is the only user of these grpc-generated types, and they should logically live in the same package (even if they physically live in separate source dirs at the build level).
- Rename package bisq.grpc => bisq.daemon.app
- Rename BisqGrpcApp => BisqDaemon
- Rename BisqGrpcServerMain => BisqDaemonMain
The script `bisq-grpc` has been renamed to `bisq-daemon` accordingly
(and will later be customized to `bisqd`). To see everything in action,
issue the following commands:
$ gradle build
$ ./bisq-daemon --appName=Bisq-throwaway --daoActivated=false
$ echo getVersion | ./bisq-cli # in a second terminal
1.2.3
Problem: a stack trace was being thrown during daemon startup from BisqDaemonMain.onSetupComplete when it attempted to start a second BisqGrpcServer and failed to bind to the already-bound port. The first BisqGrpcServer is started in BisqDaemonMain.onApplicationStarted much earlier in the startup process. Solution: remove the second attempt to start the server by removing BisqDaemonMain's implementation of onSetupComplete, and in turn remove the now-obsolete bisqGrpcServer field as well. This change also eliminates the BisqGrpcServer.blockUntilShutdown method, which in turn called the underlying grpc server's awaitTermination method. As the comment there explained, this was thought to be necessary because grpc "does not use daemon threads by default", but this is actually incorrect. According to the grpc Javadoc at [1], "Grpc uses non-daemon Threads by default and thus a Server will continue to run even after the main thread has terminated." [1]: https://git.io/JePjn
And remove entry for the no longer used io.bisq.generated package.
Per review comment at bisq-network#3888 (comment)
sqrrm
reviewed
Jan 20, 2020
gradle/witness/gradle-witness.gradle
Outdated
| 'org.bitcoinj:orchid:f836325cfa0466a011cb755c9b0fee6368487a2352eb45f4306ad9e4c18de080', | ||
| 'org.bouncycastle:bcpg-jdk15on:de3355b821fc81dd32e1f3f560d5b3eca1c678fd2400011d0bfc69fb91bcde85', | ||
| 'org.bouncycastle:bcprov-jdk15on:963e1ee14f808ffb99897d848ddcdb28fa91ddda867eb18d303e82728f878349', | ||
| 'org.codehaus.mojo:animal-sniffer-annotations:92654f493ecfec52082e76354f0ebf87648dc3d5cec2e3c3cdb947c016747a53', |
There was a problem hiding this comment.
Is this one needed at all?
Reading at https://github.com/mojohaus/animal-sniffer it sounds like this functionality is already provided with javac
I've added a number of new commits to this PR above, in order to introduce commit
It appears not to be. I have a change building right now that will confirm that and will push asap. |
cbeams
added a commit
to cbeams/bisq
that referenced
this pull request
Jan 20, 2020
So just to be explicit, all of the dependencies in this PR at its current revision (1978868) are required by the grpc-* dependencies brought in for the PoC. Note that I did try to exclude some of those that look "optional" such as findbugs, perfmark and opencensus, but they caused build and/or runtime failures. Again for further details on these dependencies, see the comment on commit eaad2e0. |
|
utACK from my side for this. Unfortunately I’m afk right now, so I can’t run it locally for verification. |
ripcurlx
added
a:feature
is:priority
cbeams
removed this from Done
in Establish support team scheduling and escalation process
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the third in a series of PRs leading up to a larger refactoring effort. It should be merged immediately after PRs #3886 and #3887, in that order.
The proof of concept introduced here was authored by @chimp1984 and is not strictly related to the refactoring effort that will follow in the next and final PR in this series, but that refactoring effort began on top of these changes. To avoid rework in teasing these changes apart and submitting them fully separately, I'm choosing to introduce the gRPC PoC here such that the refactoring work can be submitted immediately afterward. Please note that commit 65175a7 completely disables the PoC functionality such that the API cannot be used in production. Once this series of PRs is merged, I plan to return to the gRPC API effort, where I already have a number of modifications to this proof of concept in flight. So this code won't just "sit there"; it will be reworked into production-readiness as one of my next priorities.
Please disregard the several additional unrelated related commits in this PR. They are part of PRs #3886 and #3887 and will disappear here when those PRs are merged.