ListTransactions API is non-standard

[onyb]

The listtransactions RPC command accepts 4 optional arguments:

• label
• count
• skip
• include_watchonly

The way this is implemented in the rpcclient is via the following 4 methods (note: signature is out of date):

• ListTransactions(account string)
• ListTransactionsCount(account string, count int)
• ListTransactionsCountFrom(account string, count, from int)
• ListTransactionsCountFromWatchOnly(account string, count, from int, watchOnly bool)
  • Not implemented yet, but there is a pending PR: Extend the ListTransaction api to support queries to "watch only" wallets #1368

I don't know the rationale behind this design, but it encourages bespoke signatures of publicly exposed methods in the rpcclient, which is a maintenance hazard. Also, imagine if tomorrow there is a new option added to the RPC, we'll need to implement yet another method as per our current approach.

I think we should really follow the standard here and have just one method with the following signature:

ListTransactions(label *string, count *int, skip *int, includeWatchOnly *bool)

[Rjected]

I do agree that we should have one exported function that has all parameters, including optional parameters, as the corresponding Core RPC command. Small nit on your signature: label probably shouldn't be a pointer, because it is not optional.

I think it's okay for the methods ListTransactionsCounts, ListTransactionsCountFrom, ListTransactionsCountFromWatchOnly to exist though, and don't think that contradicts the need for a general method with all the optional parameters - the inclusion of ListTransactionsCountFromWatchOnly would mean that our rpcclient would support all of the same features as Core.
I think it would be valuable to add a method that accepts a includeWatchOnly bool as a parameter, and think that is preferable to what is currently in #1368, which has the same function signature as ListTransactionsCountFrom. From our implementation of commands such as listreceivedbyaddress, the optional bool is included as a parameter, so I'll submit a review on #1368 on this point. The current ListTransactions method seems to be correct because the only parameter required for the listtransactions RPC is the label parameter, called account here, so I think the method which includes all the parameters would just be this:

func (c *Client) ListTransactionsCountFromIncludeWatchOnly(account string, count, from int, includeWatchOnly bool) ([]btcjson.ListTransactionsResult, err error)

The rationale behind the design of the API we have in wallet.go is because there are many combinations of optional parameters, so I wouldn't call them bespoke for that reason. This pattern is replicated consistently throughout rpcclient, so if we want to discuss adding something like what you have:

func (c *Client) ListTransactions(label string, count *int, skip *int, includeWatchOnly *bool) ([]btcjson.ListTransactionsResult, err error)

Then we should also be discussing whether or not we should add something similar to the rest of the methods in rpcclient. Similarly, if we want to remove these methods in favor of using one method for every RPC command, we would need to rewrite the entire package, which would for sure be a breaking change. At the end of the day, we need to be consistent across the entire rpcclient package. Let me know what you think.

[onyb]

Small nit on your signature: label probably shouldn't be a pointer, because it is not optional.

This is what I see in the bitcoin core 0.19.0 docs:

1. label                (string, optional) If set, should be a valid label name to
                        return only incoming transactions

Regarding the rest of your argument, I think the original reason why multiple methods were introduced over one single generic one, was to avoid requiring the caller to use nils. I have don't much experience with Go, but maybe this is the idiomatic way of doing things.

Thanks for the detailed explanation. I tend to agree with your reasoning, and it's much clearer in my head now.

Let's wait for the author of #1368 to update his PR. Or maybe we can cherry-pick the commit in a new PR and sneak in the fix for optional label field.

[Rjected]

Yep, you're right :) must have passed over it in the docs. I don't think it would hurt to cherry-pick, address review comments and make a new PR though.