Page MenuHomePhabricator

Partial Merge #14726: Use RPCHelpMan for all RPCs
ClosedPublic

Authored by nakihito on Mar 23 2020, 19:30.

Details

Reviewers
Fabien
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rSTAGING3a84ae8b0c04: Partial Merge #14726: Use RPCHelpMan for all RPCs
rABC3a84ae8b0c04: Partial Merge #14726: Use RPCHelpMan for all RPCs
Summary

fa5e0452e875a7ca6bf6fe61fdd652d341eece40 rpc: Documentation fixups (MarcoFalke)
fa91e8eda541acdb78ca481b74605639f319c108 Use RPCHelpMan for all RPCs (MarcoFalke)

Pull request description:

The resulting documentation should not change unless the type in the oneline-summary was previously incorrect. (E.g. string vs bool)

Tree-SHA512: 4ff355b6a53178f02781e97a7aca7ee1d0d97ff348b6bf5a01caa1c96904ee33c704465fae54c2cd7445097427fd04c71ad3779bb7a7ed886055ef36c1b5a1d0

Partial backport of Core PR14726
Excluded the linter commit.

Reviewer note: the following RPCs have their help changed in another
related PR:

testmempoolaccept
createrawtransaction
createmultisig

Depends on D5567

Test Plan
ninja
ninja check
./bitcoind
./bitcoin-cli help <RPCs>

Verify help is printed without error.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Owners added a reviewer: Restricted Owners Package.Mar 23 2020, 19:30

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those PRs have been inserted into the summary for reference.

Need to re-layout some strings.

src/rpc/server.cpp
341–345 ↗(On Diff #17124)

This function was added out-of-order in D5173.

Formatting and re-layout strings.

Missed an argument in unparkblock.

Fabien requested changes to this revision.Mar 24 2020, 09:17
Fabien added a subscriber: Fabien.

You may need to rebase on top of D5537 to take the new RPC into account.
Also you're missing the rpc/abc.cpp RPCs.

src/rpc/blockchain.cpp
288 ↗(On Diff #17131)

Should be STR_HEX ?

1232 ↗(On Diff #17131)

STR_HEX

1712 ↗(On Diff #17131)

Fix layout

src/rpc/mining.cpp
290 ↗(On Diff #17131)

STR_HEX

863 ↗(On Diff #17131)

Not that smart

src/rpc/net.cpp
660 ↗(On Diff #17131)

BOOL

src/rpc/rawtransaction.cpp
383 ↗(On Diff #17131)

STR_HEX

1683 ↗(On Diff #17131)

STR

1951 ↗(On Diff #17131)

Remove

src/rpc/util.h
57 ↗(On Diff #17131)

Move comment above (and remove the doxygen <, but the linter should tell you).

src/wallet/rpcdump.cpp
107 ↗(On Diff #17131)

Fix layout

303 ↗(On Diff #17131)

Fix layout

305 ↗(On Diff #17131)

Dito

416 ↗(On Diff #17131)

Fix layout

502 ↗(On Diff #17131)

Ditp

504 ↗(On Diff #17131)

Dito

556 ↗(On Diff #17131)

Dito

558 ↗(On Diff #17131)

Dito

651 ↗(On Diff #17131)

Dito

910 ↗(On Diff #17131)

Dito

913 ↗(On Diff #17131)

Dito

916 ↗(On Diff #17131)

Dito

1434 ↗(On Diff #17131)

If you keep the clang-format off, you need to keep this one as well.

1450 ↗(On Diff #17131)

Please check if this is still required (core removes it). The long line below should really be split.

1464 ↗(On Diff #17131)

Remove.
pubkeys and keys arrays are missing (also on core, idk why ?).

src/wallet/rpcwallet.cpp
176 ↗(On Diff #17131)

Remove

870 ↗(On Diff #17131)

BOOL

2890 ↗(On Diff #17131)

Should be AMOUNT ?

3106 ↗(On Diff #17131)

Fix layout

3179 ↗(On Diff #17131)

Missing blank

3381 ↗(On Diff #17131)

Missing "query_options"

3743 ↗(On Diff #17131)

Remove.
Missing changePosition

3756 ↗(On Diff #17131)

Remove

4663 ↗(On Diff #17131)

Not from this PR, but the \n is wrong here.

4801 ↗(On Diff #17131)

AMOUNT

4807 ↗(On Diff #17131)

Missing "options"

This revision now requires changes to proceed.Mar 24 2020, 09:17

Fixed critiques and made changes to rpc/abc.cpp. Also Made estimatefee RPC less smart.

Fabien requested changes to this revision.Mar 25 2020, 08:43
Fabien added inline comments.
src/rpc/abc.cpp
42 ↗(On Diff #17156)

Not sure if this is the right place to fix the RPC doc, but at least you can add the parameter to RPCHelpMan

src/wallet/rpcdump.cpp
1458 ↗(On Diff #17156)

You missed that comment:
Remove.
pubkeys and keys arrays are missing (also on core, idk why ?).

1481 ↗(On Diff #17156)

This is barely readable, probably worth leaving a clang-off/clang-on for this section.
I'll leave that up to you.

This revision now requires changes to proceed.Mar 25 2020, 08:43
src/rpc/abc.cpp
42 ↗(On Diff #17156)

Rebased off D5567.

src/wallet/rpcdump.cpp
1458 ↗(On Diff #17156)

Looks like they added it back in the next patch. I'll restore it here since it doesn't make any sense for it to be missing.

1481 ↗(On Diff #17156)

I think its fine for now. The next PR fixes the readability issue.

Fixed critiques and rebased off D5567.

This revision is now accepted and ready to land.Mar 25 2020, 20:09