rpc: Be able to access RPC parameters by name

achow101 commented at 11:46 pm on May 30, 2023: member

Although we accept named RPC parameters, we still access the parameters by position within the RPC implementations. This PR makes it possible to access these parameters directly by name. This is achieved by holding the parameters in a separate JSONRPCParameters class which contains mappings from both name to value, and position to value. Two operator[] methods are implemented which can accept both strings and ints. This allows for backwards compatibility, while also allowing for future implementation to access the parameters by name.

The processing of named arguments ends up being entirely overhauled in order to be able to store the mappings to the parameters.

Based on #26485 to avoid conflicting with it.

DrahtBot commented at 11:46 pm on May 30, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	jarolrod, stickies-v

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29129 (wallet, rpc: add BIP44 account in createwallet by brunoerg)
#28560 (wallet, rpc: FundTransaction refactor by josibake)
#28201 (Silent Payments: sending by josibake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label RPC/REST/ZMQ on May 30, 2023

achow101 force-pushed on May 30, 2023

DrahtBot added the label CI failed on May 30, 2023

achow101 marked this as a draft on May 31, 2023

achow101 force-pushed on May 31, 2023

DrahtBot removed the label CI failed on May 31, 2023

DrahtBot added the label Needs rebase on Jun 1, 2023

achow101 marked this as ready for review on Jun 1, 2023

achow101 force-pushed on Jun 1, 2023

DrahtBot removed the label Needs rebase on Jun 1, 2023

jarolrod commented at 2:02 pm on June 2, 2023: member

Concept ACK

DrahtBot added the label Needs rebase on Jun 12, 2023

achow101 force-pushed on Jun 12, 2023

DrahtBot removed the label Needs rebase on Jun 13, 2023

maflcko commented at 8:11 am on June 15, 2023: member

Might be nice to at least update one method to use the new code to see that it compiles and passes the existing tests?

achow101 commented at 4:12 pm on June 15, 2023: member

Might be nice to at least update one method to use the new code to see that it compiles and passes the existing tests?

Added a commit to update the RPCs that have >= 6 args to use these named params.

DrahtBot added the label Needs rebase on Jun 21, 2023

luke-jr commented at 1:31 am on June 24, 2023: member

#17356

achow101 force-pushed on Jun 28, 2023

DrahtBot removed the label Needs rebase on Jun 28, 2023

stickies-v commented at 5:47 pm on September 1, 2023: contributor

Concept ACK, I think this makes for a much more clear and intuitive developer interface.

Approach-wise, I think since #28230 is merged this can be simplified a lot. I’ve worked it out in a fair amount of detail on https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:named-args-on-arg-helper , including some unit tests. Needs polishing up and adding some Arg-helper implementations, but what do you think about this approach instead? The main risk I see, is that helper functions that currently expect a UniValue parameter (e.g. SetFeeEstimateMode) will need to be refactored to take the underlying type instead. More work, but I think that’s a safer approach regardless?

maflcko commented at 8:39 am on September 3, 2023: member

The main risk I see, is that helper functions that currently expect a UniValue parameter (e.g. SetFeeEstimateMode) will need to be refactored to take the underlying type instead.

I don’t think there is a need to? Arg<>() should be trivial to extend with a Arg<UniValue>() implementation, no?

stickies-v commented at 9:37 am on September 4, 2023: contributor

It’s trivial, but do we want to? It doesn’t really add significant benefit (besides accessing by name), and importantly it’d muddy the interface. For example, with MaybeArg, would we return a UniValue* or just UniValue? The first would be confusing to use imo, the last one would be changing the interface. Also, I think pushing the conversion of UniValue to the underlying type as closely to the edge as possible is a worthwhile effort so building an interface to do the opposite seems counterproductive.

In cases where eliminating UniValue usage is not really feasible (yet), I think just sticking to request.params for now makes more sense, even if it means no named parameters for those (hopefully few) occasions?

DrahtBot added the label Needs rebase on Oct 5, 2023

achow101 force-pushed on Oct 5, 2023

DrahtBot removed the label Needs rebase on Oct 5, 2023

DrahtBot added the label CI failed on Oct 6, 2023

maflcko commented at 6:48 am on October 6, 2023: member

In cases where eliminating UniValue usage is not really feasible (yet), I think just sticking to request.params for now makes more sense, even if it means no named parameters for those (hopefully few) occasions?

sgtm

achow101 commented at 3:49 pm on October 24, 2023: member

@stickies-v feel free to open an alternative approach. I’m not particularly attached to any approach, I just want to be able to access params by name.

achow101 force-pushed on Oct 24, 2023

DrahtBot removed the label CI failed on Oct 25, 2023

DrahtBot added the label Needs rebase on Nov 16, 2023

rpc: Move isObject check to within transformArguments

Instead of conditionally calling transformNamedArguments (renamed to
transformArguments), unconditionally call it and perform the isObject
check within that function.

9334456224

rpc: Introduce JSONRPCParameters class for JSONRPCRequest params

In order to support positional and named parameter lookup, introduce a
JSONRPCParameters class that contains the parameters. For now, this is
simply a passthrough for the UniValue parameters.

364b5b602c

rpc: Process received parameters into named and positional mappings

JSONRPCParameters contains a map of named parameter name to the
parameter, and a vector of the positional parameters. These allow for
lookup by both parameter position and parameter name.

3ca91891b7

rpc, tests: Tests for retrieving RPC parameters by name 5e9cc2ebe2

rpc: use param names instead of positions for RPCs with 6 or more args

scanblocks, createwallet, listsinceblock, sendmany, and sendtoaddress
all have 6 or more parameters. To make these easier to follow, use param
names rather than positions.

e87477ca31

achow101 force-pushed on Nov 28, 2023

DrahtBot removed the label Needs rebase on Nov 28, 2023

DrahtBot added the label CI failed on Jan 13, 2024

maflcko commented at 9:10 pm on January 18, 2024: member

Can be closed after #29277 ?

achow101 closed this on Jan 18, 2024

rpc: Be able to access RPC parameters by name #27788

Code Coverage

Reviews

Conflicts