bitcoinj/bitcoinj

wallettool: Refactor, Improve UI and documentation

Opened this issue · 8 comments

msgilligan commented

This issue is a meta-issue to tie together various proposals to improve wallettool. In my opinion there are three categories of improvements that are needed and also related. So this issue is a place where we can agree on an overall strategy for addressing them and to track the various sub-issues. The three categories are:

  1. Refactoring
    a. WalletTool.java is 1358 lines of code that is now in its own subproject and can more clearly be broken into multiple files
    b. The call method is 200 lines long and shares setup/variables with subcommands in methods dispatched via a switchstatement
    c. The SENDcommand is inline with the switch statement.
    d. It's not easy to figure out which options apply to which commands.
    e. More modularity would help with unit/integration/functional testing.
    f. See PR #3109 for an outline of a proposed refactoring.
  2. Improve UI
    a. Help is difficult to read because of the large number of options and commands
    b. Formatting of subcommands input is not good in asciidoctor and HTML output of the build -- see #2284 (comment)
    c. --net option is mandatory, but redundant for operations where a --wallet is specified -- see Issue #645.
    d. It would be nice if wallettool would default to storing wallets in a "bitcoinj" AppDataDirectory.
    e. wallet toolcould offer a listwallets command that could list wallet files in the AppDataDirectory
    f. Other configuration options could be placed in a config file in the AppDataDirectory
    g. As an alternative or additional configuration options environment variables could be used to find the configured wallet, etc.
    h. A listunpsentcommand with abbreviated and specialized output would be nice. See #2470
    i. Add a --wallet-structure command line option, see PR #2437
    j. Use the picocli interactive option for reading the password option.
  3. Improve Documentation
    a. See 2a and 2b
    b. It would be nice to make the wallet tool available as Linux packages with man pages.
    c. It would be nice to publish the HTML and/or Asciidoctor documentation on GitHub pages.
    d. Use of the subcommand feature of picocli.
    e. The subcommand feature of picocli relies on annotations of classes and/or methods that for optimal use probably requires some refactoring.

I think it makes sense to make an overall plan as to how to proceed so we can address these issues and proposed enhancements. I also spent some time looking at picocli and its options for implementing and annotating subcommands and using subcommands looks like a big win.

In the comments to this issue we can discuss:

  • Which of the above issues we agree to implement.
  • What should the new UI be:
    ** Changes that arise out of restructuring improving the documentation
    ** Improvements to defaults (e.g. config file, AppDataDirectory, environment variables etc.)
  • What is the best approach to refactoring, for example:
    ** Use a method for each subcommand
    ** Use a nested class for each subcommand
    ** Put subcommands in separate files.
msgilligan commented

PR #2510 explores using the picocli sub-command feature which looks like a great solution for some of our needs. I'm thinking that we should probably migrate to the method per-subcommand approach first and once we get there consider using (either nested or separate-file) class-per-file, but perhaps it won't be necessary.

  • 👍1
adligo commented

Hi All,

I'm interested in contributing and this caught my eye. Do you all generally work as lone wolves or do you collaborate on branches etc?

Cheers,
Scott

msgilligan commented

Hi Scott,

Thanks for your interest! I was just thinking this would be a great project for a new contributor.

We collaborate very closely. This is a Bitcoin library and we try to thoroughly review every change. We also have an ongoing project to simplify and modernize the library and have some standards/guidelines/rules-of-thumb for this work.

It's also strongly encouraged (in this and any open source project) to communicate plans for an enhancement to others before working on them. (It sucks to reject a PR that has obviously been the result of significant effort that isn't aligned with the project vision. It sucks even more to be the person receiving the rejection.)

That is why I created this issue, to get @schildbach and I "on the same page" about how to approach this set of changes before I spend too much time coding it up.

We typically like to handle changes as a series of small, atomic changes that can be reviewed as individual PRs. For this particular issue I think we should try creating a feature branch and submitting PRs to that feature branch -- what do you think about that idea @schildbach? Maybe call it something like feature-wallettool-refactor?

@adligo have you used the wallettool? I'm developing a bitcoinj-based wallet app and I use the wallet tool for testing regularly. That "eating your own dog food" thing is what's driving many of the ideas I listed above. Do you agree with the ideas above? Do you have any objections, alternative suggestions, or additions in mind?

In terms of breaking this into a series of smaller steps, my current thought is that the overall approach would be something like:

  1. Refactor the current "actions" more cleanly into separate methods without changing the UI, behavior, or documentation.
    a. add/improve unit/integration tests
  2. With no or minimal changes in behavior adapt the code to the picocli "subcommand" approach (using methods not classes for subcommands)
  3. Start working on the first of the various "new features" or other enhancements posted above.

How does this all sound?

msgilligan commented

See #2510 (comment) for examples of what help subcommand output would look like. This is big win for UI/documentation IMO.

adligo commented
schildbach commented

I'd say before we add new subcommands, we should investigate into and perhaps migrate to the picocli subcommands feature. However some of the tasks on your list can be implemented independently I guess (like support for AppDataDirectory, or documentation improvements).

And yes, I agree to use the methods based subcommand approach and maybe move to classes later.

About a feature branch, I'm not sure what is it good for? I mean, each PR has its own branch anyway but in the end I'd prefer a linear sequence of commits on the master branch.

msgilligan commented

I'd say before we add new subcommands, we should investigate into and perhaps migrate to the picocli subcommands feature.

I agree. I've already investigated and I'm completely convinced that is the way to go (PR #2510 is the investigation)

some of the tasks on your list can be implemented independently I guess (like support for AppDataDirectory, or documentation improvements).

IMO we should implement subcommands before doing anything else because it will make the codebase easier to modify and make the documentation source annotations easier to read and to update.

About a feature branch, I'm not sure what is it good for? I mean, each PR has its own branch anyway but in the end I'd prefer a linear sequence of commits on the master branch.

The feature branch has several advantages:

  1. It allows us to review and approve commits one-at-a-time, without affecting the master branch until the end. (This is useful for wallettool because it doesn't have great unit test coverage so we can do the required manual testing at the end before merging to master) This also allows us to make mistakes or change our mind without affecting master.
  2. It makes it easier for multiple developers to collaborate and merge via PRs on the branch
  3. It makes it easier to rebase changes on the feature branch when independent changes are merged to master.
  4. When merging to master we have the choice of (a) squashing commits, (b) using a merge commit which preserves the history on the branch, or (c) rebasing to preserve the linear sequence of commits on the branch.
msgilligan commented

Sure sounds good to me, I have NOT used wallet tool

My recommendation for you to get started is to play with the "Forwarding" example and running wallettool on the Wallet you create while experimenting with that example. (Please note that I have some WIP to update the Forwarding example: PR #2506)

Are there any specific docs or books you would recommend before helping on this project? I have read programming bitcoin (the book in python) and some other books, although I've lost track.

I created Issue #2513 because I think some of your questions should be captured in the CONTRIBUTING document so let's move some of this discussion over there.

I'm particularly interested in having better redundancy in the
storage space of crypto wallets.

So am I. I think improving wallettool and getting it to work with hardware wallets might be a good start.