Refactor rpc server implementation #4199
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
* Renamed CliCommand to RpcCommand, differentiating it from cmd string token(s) * Added new CliConfig, based on :common Config * Injected Config into CoreApi to make bisq.properties and cmd line args available to server * Added cleartext username:password BisqCallCredentials to :cli, and an AuthenticationInterceptor to :core (server) * Moved :daemon resources folder to expected location under src/main * Duplicated CompositeOptionSet, ConfigException and BisqException in :cli because they are not visible from :common. (TODO refactor?) CompositeOptionSet will be used to let command line parameters override params set in config file. * Removed outdated references to :cli in a couple of comments in :common Config * gRPC parameters & command names are lowercase to mimic bitcoind/rpc TBD * Decide what rpc auth param names to use, to differentiate them from bitcoind rpc user/passord param names
There is no need or benefit to injecting Config into CoreApi for the purpose of accessing it indirectly through BisqGrpcServer.
Final fields should be favored wherever possible to advertise and enforce immutability. Also as a matter of style, favor declaring fields one per line instead of multiple on the same line. This style guideline is not consistent throughout the codebase, but is favored because it makes diffs / patches cleaner and is the more widely accepted style througout most modern Java codebases.
There is actually no use case for both username and password forming the authentication token. This change simplifies the arrangement such that a single token is passed. --rpcUser and --rpcPassword are no longer used and are replaced by --apiToken on both cli and daemon sides.
When e.g. a given option is not recognized, print a clean error message and exit 1 as opposed to printing a stack trace.
Fail fast if the specified command is unknown
To increase simplicity and make the implementation more intention-revealing.
And remove unused @slf4j lombok annotation
Strip double quotes from :cli --password arg before it is passed to server. Otherwise, a correct "password" will fail authentication.
The client was displaying Error: UNAUTHENTICATED: incorrect 'password' rpc header value from the StatusRuntimeException message. Strip "UNAUTHENTICATED: " from the exception message string before printing it.
Versus needing to manage calling custom shutdown method in all the right places.
No StatusRuntimeException can be thrown from this code, so it is not necessary to include in the try block.
Co-authored-by: ghubstan <[email protected]>
- Ensure script runs from root directory regardless of where it is invoked from - Touch up documentation and whitespace
Don't instruct the user to create a fresh data directory every time, as this takes quite a bit longer to initialize the wallet than running against the same data directory repeatedly. Just be clear that the requirement is an unencrypted wallet with 0 BTC balance.
Stop attempting to align Option and Method description columns with one another. It's a moving target as we add options and method names, and this help output format will probably change in the future anyway.
This is a partial reversion of the earlier commit. Marking the password option as required at the parser level made it impossible to run ./bisq-cli without options or arguments and get the help text. This is a useful thing to do, and not worth creating a bad user experience to get the free required option error handling and error messaging.
There is no actual requirement for this method, so removing it. It also helps improve the implementation by removing the need for the static 'instance' field.
So they can easily access the enclosing BisqGrpcServer#coreApi field.
|
@ghubstan, please review the commits unique to this PR, thanks. |
|
uACK I'll test this branch with a bats script after I port the expect script. |
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 PR implements a number of refactorings and a couple minor substantive changes that I've been meaning to get to since @chimp1984's original PoC in #3888.
The substantive changes include:
stopServermethod (until a clear use case for it emerges)--apiPortconfiguration option to avoid hardcoding the 9998 portOtherwise these are pure refactorings that make the implementation of GrpcServer more idiomatic and hopefully easier to contribute to going forward, e.g. in the PR that will come together as part of #4198.
Note that this PR builds atop the commits in the as-yet unmerged PR #4189. The commits specific to this PR begin with 01580ae ("Remove StopServer rpc method"). In any case, PR #4189 should be merged before this one.