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

achow101 · 2023-05-30T23:46:08Z

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 · 2023-05-30T23:46:11Z

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.

jarolrod · 2023-06-02T14:02:51Z

Concept ACK

maflcko · 2023-06-15T08:11:14Z

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

See also #20017

achow101 · 2023-06-15T16:12:16Z

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.

luke-jr · 2023-06-24T01:31:36Z

#17356

maflcko · 2023-09-03T08:39:03Z

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 · 2023-09-04T09:37:18Z

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?

maflcko · 2023-10-06T06:48:36Z

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 · 2023-10-24T15:49:36Z

@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.

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

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.

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.

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

maflcko · 2024-01-18T21:10:18Z

Can be closed after #29277 ?

30a6c99 rpc: access some args by name (stickies-v) bbb3126 rpc: add named arg helper (stickies-v) 13525e0 rpc: add arg helper unit test (stickies-v) Pull request description: Adds string overloads for the `RPCHelpMan::Arg` and `RPCHelpMan::MaybeArg` helpers to be able to access RPC arguments by name instead of index number. Especially in RPCs with a large number of parameters, this can be quite helpful. Example usage: ```cpp const auto action{self.Arg<std::string>("action")}; ``` Most of the LoC is adding test coverage and documentation updates. No behaviour change. An alternative approach to #27788 with significantly less overhaul. ACKs for top commit: fjahr: Code review ACK 30a6c99 maflcko: ACK 30a6c99 🥑 ryanofsky: Code review ACK 30a6c99. Nice change! Implementation is surprisingly simple and additional unit test coverage is welcome, too. Tree-SHA512: 4904f5f914fe1d421d32f60edb7c5a028c8ea0f140a2f207a106b4752d441164e073066a6bf2e17693f859fe847815a96609d3cf521e0ac4178d8cd09362ea3d

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

achow101 force-pushed the rpc-params-by-name branch from 857991f to 35c5ffb Compare May 30, 2023 23:59

DrahtBot added the CI failed label May 30, 2023

achow101 mentioned this pull request May 31, 2023

RPC: Accept options as named-only parameters #26485

Merged

achow101 marked this pull request as draft May 31, 2023 01:03

achow101 force-pushed the rpc-params-by-name branch from 35c5ffb to 2d41b19 Compare May 31, 2023 02:47

DrahtBot mentioned this pull request May 31, 2023

Support JSON-RPC 2.0 when requested by client #27101

Open

DrahtBot removed the CI failed label May 31, 2023

This was referenced May 31, 2023

test: autogenerate bash completion #25243

Closed

Implement BIP 370 PSBTv2 #21283

Open

DrahtBot added the Needs rebase label Jun 1, 2023

achow101 marked this pull request as ready for review June 1, 2023 20:28

achow101 force-pushed the rpc-params-by-name branch from 2d41b19 to 176f61d Compare June 1, 2023 20:28

DrahtBot removed the Needs rebase label Jun 1, 2023

This was referenced Jun 5, 2023

Renamed UniValue::__pushKV to UniValue::pushKVEnd. #27822

Merged

rest: bugfix, fix crash error when calling /deploymentinfo #27853

Merged

DrahtBot added the Needs rebase label Jun 12, 2023

achow101 force-pushed the rpc-params-by-name branch from 176f61d to 3fe03c7 Compare June 12, 2023 23:40

DrahtBot removed the Needs rebase label Jun 13, 2023

DrahtBot mentioned this pull request Jun 16, 2023

Silent Payments: send and receive #27827

Closed

3 tasks

DrahtBot added the Needs rebase label Jun 21, 2023

achow101 force-pushed the rpc-params-by-name branch from 38662de to 86e640c Compare June 28, 2023 17:56

DrahtBot removed the Needs rebase label Jun 28, 2023

DrahtBot mentioned this pull request Jul 22, 2023

refactor: Use util::Result class for wallet loading #25722

Draft

DrahtBot mentioned this pull request Aug 2, 2023

Silent Payments: receiving #28202

Closed

DrahtBot mentioned this pull request Sep 13, 2023

wallet: Receive silent payment transactions #28453

Closed

This was referenced Oct 2, 2023

wallet, rpc: FundTransaction refactor #28560

Merged

wallet: No BDB creation, unless -deprecatedrpc=create_bdb #28597

Merged

DrahtBot added the Needs rebase label Oct 5, 2023

achow101 force-pushed the rpc-params-by-name branch from 86e640c to 41027ad Compare October 5, 2023 22:45

DrahtBot added CI failed and removed Needs rebase labels Oct 5, 2023

DrahtBot mentioned this pull request Oct 6, 2023

Fix typos #28605

Merged

DrahtBot mentioned this pull request Oct 24, 2023

Remove the legacy wallet and BDB dependency #28710

Draft

achow101 force-pushed the rpc-params-by-name branch from 41027ad to 73751f8 Compare October 24, 2023 16:34

DrahtBot removed the CI failed label Oct 25, 2023

DrahtBot added the Needs rebase label Nov 16, 2023

achow101 added 5 commits November 28, 2023 10:54

rpc: Move isObject check to within transformArguments

9334456

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

rpc: Introduce JSONRPCParameters class for JSONRPCRequest params

364b5b6

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.

rpc: Process received parameters into named and positional mappings

3ca9189

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.

rpc, tests: Tests for retrieving RPC parameters by name

5e9cc2e

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

e87477c

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

achow101 force-pushed the rpc-params-by-name branch from 73751f8 to e87477c Compare November 28, 2023 16:02

DrahtBot removed the Needs rebase label Nov 28, 2023

DrahtBot mentioned this pull request Dec 6, 2023

kernel: Streamline util library #29015

Open

DrahtBot mentioned this pull request Dec 21, 2023

wallet, rpc: add BIP44 account in createwallet #29129

Draft

DrahtBot added the CI failed label Jan 13, 2024

stickies-v mentioned this pull request Jan 18, 2024

RPC: access RPC arguments by name #29277

Merged

achow101 closed this Jan 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

achow101 commented May 30, 2023 •

edited

DrahtBot commented May 30, 2023 •

edited

jarolrod commented Jun 2, 2023

maflcko commented Jun 15, 2023

achow101 commented Jun 15, 2023

luke-jr commented Jun 24, 2023

maflcko commented Sep 3, 2023 •

edited

stickies-v commented Sep 4, 2023

maflcko commented Oct 6, 2023

achow101 commented Oct 24, 2023

maflcko commented Jan 18, 2024

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

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

Conversation

achow101 commented May 30, 2023 • edited

DrahtBot commented May 30, 2023 • edited

Code Coverage

Reviews

Conflicts

jarolrod commented Jun 2, 2023

maflcko commented Jun 15, 2023

achow101 commented Jun 15, 2023

luke-jr commented Jun 24, 2023

maflcko commented Sep 3, 2023 • edited

stickies-v commented Sep 4, 2023

maflcko commented Oct 6, 2023

achow101 commented Oct 24, 2023

maflcko commented Jan 18, 2024

achow101 commented May 30, 2023 •

edited

DrahtBot commented May 30, 2023 •

edited

maflcko commented Sep 3, 2023 •

edited