Refactor option handling

[cbeams]

This is the fourth and final PR in a series. It should ideally be merged immediately after PRs #3886, #3887 and #3888, in that order.

Introduction

As detailed in commit 614035f, this change replaces BisqEnvironment with a new Config class that unifies, simplifies and otherwise improves Bisq's option handling in a number of ways. Please read that commit message first to get proper context and rationale.

Guidance for reviewers

Many of the commits in this PR represent the step-by-step, option-by-option migration from BisqEnvironment to Config. I recommend carefully reviewing at least several of these commits to understand what this migration means and how things work in the new infrastructure, but carefully reviewing every one of them is probably overkill. Please note that every commit in this PR should compile and run without errors.

How these changes were tested

The majority of commits (and certainly the final one) were subjected to a complete Gradle build cycle including all tests, followed by bringing up a localnet environment using the Makefile introduced in PR #3718, e.g.:

$ ./gradlew build && make clean-localnet deploy

Note that the Bisq instances launched by make deploy make use of many Bisq command line options. So ensuring that these nodes came up error-free was an important part of making sure that the migration was working at every step.

Testing was also performed on an ad-hoc basis against a non-localnet Bisq instance, ensuring that passing certain options works as expected, e.g.

$ bitcoind # start my local Bitcoin node
...
$ ./gradlew build && ./bisq-desktop --appDataDir=$(mktemp -d) --ignoreLocalBtcNode

The option being tested above is --ignoreLocalBtcNode, and passing a newly created temp directory to --appDataDir was a technique I commonly used to avoid any possible interference with my real production Bisq installation. In this case, I would verify that --ignoreLocalBtcNode worked as expected by ensuring the local Bitcoin node popup did not appear in the Bisq client and that the default approach of connecting to Bisq's Bitcoin node federation took place as expected. I did not test each and every option in this way; after making a number of such migrations, I got quite confident in the process. If a migration deviated from the typical process in some way, then I usually manually tested that change.

Refactorings not strictly related to option handling

• Introduction of ConfigFileEditor to encapsulate the process of making automated updates to the bisq.properties config file. See commit 614035f.

• Introduction of LocalBitcoinNode to encapsulate everything to do with detecting whether a local Bitcoin node is running. Of particular note here is the elimination of a dedicated thread for doing the actual detection. See commit 30bef16 for details.

Functional / user-facing / non-refactoring changes

• From commit 614035f:

  Note that while this change and those that follow it are principally a
  refactoring effort, certain functional changes have been introduced. For
  example, Bisq now supports a --configFile argument at the command line
  that functions very similarly to Bitcoin Core's -conf option.

• Commit 799eb12 renames the numConnectionForBtc option to numConnectionsForBtc.

Other notes

Please disregard the unrelated Kotlin, Git and gRPC proof of concept commits in this PR. They belong respectively to PRs #3886, #3887 and #3888 and will disappear here as each gets merged.

[cbeams]

My section headings for the description did not come through in the original PR submission. I've re-added them above, and they should improve readability.

[ripcurlx]

NACK - I activated yesterday codacy's PR Quality Review for the Bisq repository. It found for your PR a couple of issues which you can view in the Details link next to the PR check. Please review and push fixes for this issues to trigger a re-review by codacy. Thanks!

[cbeams]

I've just pushed fixes for the code quality issues, but a number of false positives will remain unfixed. Please see my commit comment for details. @ripcurlx, this may be an example of a PMD rule we want to eliminate. Would like to know why it's a false positive, though. Those imports are in fact used.

[cbeams]

Also note that the BisqGrpcClient issues in the Codacy report are fixed in my latest commit against PR #3888, so they don't really apply here anymore.

[ripcurlx]

I've just pushed fixes for the code quality issues, but a number of false positives will remain unfixed. Please see my commit comment for details. @ripcurlx, this may be an example of a PMD rule we want to eliminate. Would like to know why it's a false positive, though. Those imports are in fact used.

I just removed the unused on demand import pattern that seems to have caused the issue in the ConfigTests and I ignored the issues in BisqGrpcClient based on #3889 (comment).

[ripcurlx]

So only a merge conflict resolution with WalletsSetup.java is missing to make this PR mergeable again.

[cbeams]

So only a merge conflict resolution with WalletsSetup.java is missing to make this PR mergeable again.

Right, I've fixed that locally, and just talked with @ripcurlx about how we want to actually go ahead with the merge. No need to go into detail about it here, but anyone else who would like to review this PR, please do, thanks.

[ripcurlx]

^^ Travis build (https://travis-ci.org/bisq-network/bisq/builds/635462277) I just triggered it manually and everything seems to compile fine. Not sure why it is not updated by GitHub.

[ripcurlx]

@cbeams As we decided to be as restrictive as possible with new dependencies: Could you please point out why they are necessary (especially for the non-google libs)? Although they are required for the GRPC API I think it is good practice to have some documentation (even if it is just a comment in the PR) every time we add new dependencies to the project.

[cbeams]

Good catch. Some of these and a number of other dependencies are removed in a commit that currently exists only in my fork's grpc-api branch. These changes were made as part of further refactorings of @chimp1984's original PoC, and it was an oversight that I didn't include them here. It will require some rework of that commit to do so, but is probably worth it to make sure we don't ship a release that includes these deps, even if they will be removed soon thereafter. I'll push that additional commit as soon as possible. Though note that I'll probably push it against the grpc-poc branch (PR #3888), as it's really part of that change as opposed to this one.

[ripcurlx]

Thanks! Really enjoyed the command line access already 👍

[sqrrm]

@cbeams agreed, makes most sense to push to #3888

If there are still new deps please leave a comment in that PR as to why they're reasonable to introduce. I think it will be worthwhile doing that going forward so we have some motivation to refer to.

[ripcurlx]

NIT: 0.12 should be at least 0.15 otherwise the security deposit is too low for our current setup

[cbeams]

Please add this comment to #3888.

[ripcurlx]

Somehow this command didn't work for me.

[cbeams]

Please add this comment to #3888.

[ripcurlx]

NACK - Because of #3889 (review)
ACK - For functionality and the remaining parts of the code 👍

[cbeams]

@ripcurlx re: #3889 (review), this is also regarding #3888. To be clear, can you re-iterate that your ACK is for the changes specific to this PR, i.e. all things Config and related to option handling?

[ripcurlx]

@ripcurlx re: #3889 (review), this is also regarding #3888. To be clear, can you re-iterate that your ACK is for the changes specific to this PR, i.e. all things Config and related to option handling?

Actually for both - So I read through all the changes regarding Config changes and it worked for my local setup. Regarding the remarks in #3888: I didn't run the code before, just reviewed the changes, so I checked this time the GRPC prototype in my local setup as well.

[cbeams]

@ripcurlx, @sqrrm, please note that this PR needs to be reworked against the latest changes in #3888. I'm doing that now, will notify when complete.

[cbeams]

I've just rebased this PR branch against current master which includes the merge for #3888.

@ripcurlx, your NACK has been addressed in #3888, which as mentioned above is now merged. Please re-review and ACK if you see fit. No substantive changes have been made; only those that were necessary to eliminate conflicts resulting from the rebase.

@sqrrm you should be good to go to review this now.

[sqrrm]

NACK

See comments on exception usage.

Comments on braces are not blocking but it would be good to address the multiline blocks.

Apart from those comments it looks ready to merge.

[sqrrm]

The exception issue is gone so no longer blocking, would still be good to address the braces.

[cbeams]

@sqrrm wrote:

[it] would still be good to address the braces.

First some data:

Of 171 for loops in the codebase, 12 (7%) are written without braces:

~/dev/bisq[master]
$ git grep 'for (.*) {$' | wc -l
     171

~/dev/bisq[master]
$ git grep 'for (.*)$' | wc -l
      15

Note that 3 of the 15 above are actually aberrations in which the brace is on a newline (@wiz, I'm looking at you), so the total number is actually 12.

Of 3346 if statements in the codebase, 1796 (53%) are brace-free.

~/dev/bisq[master]
$ git grep 'if (.*) {$' | wc -l
    3346

~/dev/bisq[master]
$ git grep 'if (.*)$' | wc -l
    1796

As far as I am aware, we have not written down any rules on this. There is nothing in the bisq-network/style repository's issues, for example.

In the absence of any explicit rule, I would generally agree that one should fall back to the default rule to "follow convention" and thus the for loops should have braces added, and the if statements may be left as-is given that more than half of all such statements in the codebase already take this approach.

With that said, I'd like to argue for embracing braceless for loops under certain conditions on the grounds of elegance, aesthetics, readability and concision. Before we get there, though, consider the strong case for using braceless if statements in guard logic that you want to be as concise and readable as possible, e.g.:

public void myMethod(String arg) {
    if (arg == null)
        throw new IllegalArgumentException(...);

    // normal logic follows
}

To add braces around the guard logic is simply to add line noise. One wants the eye to flow over the guard logic, understanding it intuitively as guard logic, and to then get to the meat of the actual method. Adding braces makes it feel "heavier", subtly making the reader wonder whether there might be something there that requires more thought than: "this is just a null check".

I would argue the same for the for loops in question in this PR. While they do not represent guard logic, they have a clear and focused purpose, and the ability to write the loop without braces expresses the elemental nature of the logic within; that there is no fat, nothing extra, no side effects, just pure iteration, filtering and returning. For example here is such a for loop from CompositeOptionSet in this PR:

public boolean has(OptionSpec<?> option) {
    for (OptionSet optionSet : optionSets)
        if (optionSet.has(option))
            return true;

    return false;
}

I argue that nothing beneficial comes from adding braces here, and that doing so is to follow a convention blindly, purely for the sake of consistency, and at the cost of concision, readability and clarity.

Now let's consider a counterexample. The following is also taken from this PR in ConfigFileEditor#clearOption, lines 36-50:

for (String line : lines) {
    if (ConfigFileOption.isOption(line)) {
        ConfigFileOption existingOption = ConfigFileOption.parse(line);
        if (existingOption.name.equals(name)) {
            fileAlreadyContainsTargetOption = true;
            if (!existingOption.arg.equals(arg)) {
                ConfigFileOption newOption = new ConfigFileOption(name, arg);
                writer.println(newOption);
                log.warn("Overwrote existing config file option '{}' as '{}'", existingOption, newOption);
                continue;
            }
        }
    }
    writer.println(line);
}

// additional logic follows

It so happens that the way this is written, it would cause logic and/or compiler errors if any of these braces were left out, but consider if it had been written slightly differently and the author chose to eliminate braces wherever possible (note that this code is wrong and not functionally equivalent to the above; the changes I'm making are just for the purposes of illustration):

for (String line : lines)
    if (ConfigFileOption.isOption(line)) {
        ConfigFileOption existingOption = ConfigFileOption.parse(line);
        if (existingOption.name.equals(name)) {
            fileAlreadyContainsTargetOption = true;
            if (!existingOption.arg.equals(arg)) {
                ConfigFileOption newOption = new ConfigFileOption(name, arg);
                writer.println(newOption);
                log.warn("Overwrote existing config file option '{}' as '{}'", existingOption, newOption);
            }
            else
                writer.println(line);                
        }
    }

// additional logic follows

In the latter example above, the author has omitted braces from the for loop and from the else statement, while all other required braces remain in place. This is the kind of madness we are trying to avoid when we make rules about using braces. In a complex block of logic like the above, selectively omitting braces makes the whole construction much harder to read and reason about. Frankly, I wouldn't want anyone who thinks this is a good idea to be committing to the Bisq codebase. Nor do I want to prevent any such possibility of this by decreeing an (IMO) dumbed-down style rule like "always use braces". The real rule here is something more like "use braceless loops and conditionals judiciously", where your judgement demonstrates you know the difference between the good and bad examples above. If you don't it's a red flag about your coding skill and no overly strict set of style rules is going to save you.

So: lest this become a bikeshed session, I propose we leave the braceless for loops as-in in this PR on the merit of the arguments above, and that if strong opinions otherwise present themselves, that we take it to Keybase, a bisq-network/style discussion or similar. In any case, I am happy to write a style rule (shorter than the text above) titled something like "Use braceless loops and conditionals judiciously" in order to make our guidelines about this clear. If the conversation continues and we end up at a rough consensus that says we should in fact always use braces in for loops, I'll come back and revise this code accordingly. I'd appreciate not holding up the merge on it in the meantime, though.

[sqrrm]

I agree that the guard ifs are better without braces, but I don't agree on the for loops.

Since there is some contention here, and I was following a discussion I had with Manfred a while back on using braces I'll ack this one. I also don't think it's important enough to even discuss too much. Let's move on and make sure we avoid cases like your horrible example.

[sqrrm]

utACK

[wiz]

Note that 3 of the 15 above are actually aberrations in which the brace is on a newline (@wiz, I'm looking at you), so the total number is actually 12.

Guilty as charged. Would like me to do a PR to fix that?