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

rpcclient: implement createwallet with functional options #1650

Merged
merged 1 commit into from
Oct 26, 2020

Conversation

onyb
Copy link
Collaborator

@onyb onyb commented Oct 9, 2020

Rebased on #1645, to avoid having to resolve merge conflicts later. This PR only implements createwallet as a first-step towards functional options.

Background

I find the current way of dealing with optional parameters in RPC commands very hard to maintain, and also clunky from a user's perspective. We implement one rpcclient method per optional parameter, with long-winding names, closely following the order of the parameters!

This has been previously discussed a bit in #1589.

Current style

We have multiple methods to choose from, depending upon the options you want to use. In the example below, it is impossible to specify just watchOnly without specifying count. 😞

ListTransactions(account string)
ListTransactionsCount(account string, count int)
ListTransactionsCountFrom(account string, count int, from int)
ListTransactionsCountFromWatchOnly(account string, count int, from int, watchOnly bool)

Proposed style

ListTransactions(
        "myaccount",
        WithListTransactionsWatchOnly(),  // btcjson.Bool(true) if specified, default (nil) otherwise
        WithListTransactionsCount(100),   // Use options in any order you like!
)

The above is known as functional options pattern, which is widely used in the Go community. Dave Cheney has written about it here. Here's another article I really liked.

The API is made possible using the following variadic signature:

func (c *Client) ListTransactions(account string, opts ...ListTransactionsOpt) FutureListTransactionsResult

@coveralls
Copy link

coveralls commented Oct 9, 2020

Pull Request Test Coverage Report for Build 298236815

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 10 of 48 (20.83%) changed or added relevant lines in 2 files are covered.
  • 83 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.1%) to 53.371%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rpcclient/wallet.go 0 38 0.0%
Files with Coverage Reduction New Missed Lines %
integration/rpctest/blockgen.go 2 85.61%
database/ffldb/blockio.go 4 92.62%
peer/peer.go 11 75.47%
btcjson/walletsvrcmds.go 16 96.09%
rpcclient/wallet.go 50 0.0%
Totals Coverage Status
Change from base Build 289529546: -0.1%
Covered Lines: 20787
Relevant Lines: 38948

💛 - Coveralls

@onyb onyb force-pushed the create-wallet-functional-opts branch from 135ac7f to 26f0d27 Compare October 22, 2020 11:04
@onyb onyb added the rpc label Oct 22, 2020
@onyb onyb added this to the 0.22.0 milestone Oct 22, 2020
@onyb onyb force-pushed the create-wallet-functional-opts branch from 26f0d27 to 30622e2 Compare October 26, 2020 13:36
@jcvernaleo
Copy link
Member

High level, I'm cool with this style. Going to do a real review now though.

Copy link
Member

@jcvernaleo jcvernaleo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I like the new style. Will give it a few days in case anyone else wants to look and then i'll merge.

@jakesylvestre
Copy link
Collaborator

I like this. Must cleaner than what we have now and less tedious to implement than the builder strategy

Copy link
Collaborator

@jakesylvestre jakesylvestre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! The only thing I might add is we should clarify some of our style guidelines here (https://github.com/btcsuite/btcd/blob/master/docs/code_contribution_guidelines.md) at some point

@onyb
Copy link
Collaborator Author

onyb commented Oct 26, 2020

@jakesyl Indeed, we should document this. I'll do a separate PR to update the guidelines.

If you can, please feel free to merge the PR.

@jakesylvestre jakesylvestre merged commit 535f255 into btcsuite:master Oct 26, 2020
@onyb onyb deleted the create-wallet-functional-opts branch October 26, 2020 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants