Page MenuHomePhabricator

Partial Merge #14726: Use RPCHelpMan for all RPCs
ClosedPublic

Authored by nakihito on Mon, Mar 23, 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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; 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.Mon, Mar 23, 19:30
Herald added a reviewer: Restricted Project. · View Herald TranscriptMon, Mar 23, 19:30
teamcity edited the summary of this revision. (Show Details)Mon, Mar 23, 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.

nakihito planned changes to this revision.Mon, Mar 23, 19:55

Need to re-layout some strings.

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

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

nakihito updated this revision to Diff 17130.Tue, Mar 24, 00:21

Formatting and re-layout strings.

nakihito updated this revision to Diff 17131.Tue, Mar 24, 00:25

Missed an argument in unparkblock.

Fabien requested changes to this revision.Tue, Mar 24, 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.Tue, Mar 24, 09:17
nakihito updated this revision to Diff 17156.Tue, Mar 24, 19:45

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

Fabien requested changes to this revision.Wed, Mar 25, 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.Wed, Mar 25, 08:43
nakihito added inline comments.Wed, Mar 25, 19:40
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.

nakihito edited the summary of this revision. (Show Details)Wed, Mar 25, 19:41
nakihito updated this revision to Diff 17173.Wed, Mar 25, 19:42

Fixed critiques and rebased off D5567.

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