-
Notifications
You must be signed in to change notification settings - Fork 38.8k
Use RPCHelpMan to generate RPC doc strings #14530
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
How does this compare/clash with #14502? |
fae42b5 to
faf5b3c
Compare
faf5b3c to
faed9d9
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
7017d03 to
034921c
Compare
034921c to
a97c67d
Compare
e00e2b7 to
4116397
Compare
|
Is the plan to write down all objects in the first argument line like this? For example, fundrawtransaction has a large object; is the plan to use that instead of "options" at the first line? If not, I would suggest to use an option to RPCArg, for example "m_writeFirstAsObject"; it could still then be printed as an object in the longer argument writedown. (The idea is great though.) |
|
Also the PR changes the RPC from
to
Is that intentional? I think it decreases readability... (On some places in RPC, bitcoin core uses notation I think this should be kept - so there should be more subtypes than |
|
There will also be support for "option object" - that is, something can be two different things - for example as in ...that all leads me to think this shouldn't use "univalue" types but its own types |
d0e3b06 to
e8573f6
Compare
513dc18 to
e03410e
Compare
fa73e0f to
fa75f1f
Compare
|
Rebased |
Just a preparatory commit to add the header to the includes and run clang-format to sort the include lists. Splitting this up into a separate commit makes future scripted-diffs easier.
fa75f1f to
fac524e
Compare
|
Note that the generated doc can be easily compared against the previous one by running this weird hack: Patchdiff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index f619857da1..188b2795c2 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -1782,7 +1782,6 @@ static const CRPCCommand commands[] =
{ "rawtransactions", "decodescript", &decodescript, {"hexstring"} },
{ "rawtransactions", "sendrawtransaction", &sendrawtransaction, {"hexstring","allowhighfees"} },
{ "rawtransactions", "combinerawtransaction", &combinerawtransaction, {"txs"} },
- { "hidden", "signrawtransaction", &signrawtransaction, {"hexstring","prevtxs","privkeys","sighashtype"} },
{ "rawtransactions", "signrawtransactionwithkey", &signrawtransactionwithkey, {"hexstring","privkeys","prevtxs","sighashtype"} },
{ "rawtransactions", "testmempoolaccept", &testmempoolaccept, {"rawtxs","allowhighfees"} },
{ "rawtransactions", "decodepsbt", &decodepsbt, {"psbt"} },
diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index dfd98bfc5f..4c08b6881b 100644
--- a/src/rpc/server.cpp
+++ b/src/rpc/server.cpp
@@ -165,7 +165,7 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
{
const CRPCCommand *pcmd = command.second;
std::string strMethod = pcmd->name;
- if ((strCommand != "" || pcmd->category == "hidden") && strMethod != strCommand)
+ if ((strCommand != "") && strMethod != strCommand)
continue;
jreq.strMethod = strMethod;
try
diff --git a/test/functional/rpc_help.py b/test/functional/rpc_help.py
index 78d6e78aed..c6e01d9cfc 100755
--- a/test/functional/rpc_help.py
+++ b/test/functional/rpc_help.py
@@ -33,7 +33,7 @@ class HelpRpcTest(BitcoinTestFramework):
# command titles
titles = [line[3:-3] for line in node.help().splitlines() if line.startswith('==')]
- components = ['Blockchain', 'Control', 'Generating', 'Mining', 'Network', 'Rawtransactions', 'Util']
+ components = ['Blockchain', 'Control', 'Generating', 'Hidden', 'Mining', 'Network', 'Rawtransactions', 'Util']
if self.is_wallet_compiled():
components.append('Wallet')
@@ -46,6 +46,7 @@ class HelpRpcTest(BitcoinTestFramework):
def dump_help(self):
dump_dir = os.path.join(self.options.tmpdir, 'rpc_help_dump')
os.mkdir(dump_dir)
+ dump_dir = '/tmp/temp_git/' ##HACK
calls = [line.split(' ', 1)[0] for line in self.nodes[0].help().splitlines() if line and not line.startswith('==')]
for call in calls:
with open(os.path.join(dump_dir, call), 'w', encoding='utf-8') as f:Which then gives the resulting diff for me: Doc diffdiff --git a/createpsbt b/createpsbt
index 4b75e7f..6e2ec5c 100644
--- a/createpsbt
+++ b/createpsbt
@@ -1,4 +1,4 @@
-createpsbt [{"txid":"id","vout":n},...] [{"address":amount},{"data":"hex"},...] ( locktime ) ( replaceable )
+createpsbt [{"txid":"hex","vout":n,"sequence":n},...] [{"address":amount},{"data":"hex"},...] ( locktime replacable )
Creates a transaction in the Partially Signed Transaction format.
Implements the Creator role.
diff --git a/gettxoutproof b/gettxoutproof
index 5ed7748..45ba17a 100644
--- a/gettxoutproof
+++ b/gettxoutproof
@@ -1,4 +1,4 @@
-gettxoutproof ["txid",...] ( blockhash )
+gettxoutproof ["txid",...] ( "blockhash" )
Returns a hex-encoded proof that "txid" was included in a block.
diff --git a/listunspent b/listunspent
index 39772c9..bc3baf8 100644
--- a/listunspent
+++ b/listunspent
@@ -1,4 +1,4 @@
-listunspent ( minconf maxconf ["addresses",...] [include_unsafe] [query_options])
+listunspent ( minconf maxconf ["address",...] include_unsafe {"minimumAmount":amount,"maximumAmount":amount,"maximumCount":n,"minimumSumAmount":amount} )
Returns array of unspent transaction outputs
with between minconf and maxconf (inclusive) confirmations.
diff --git a/lockunspent b/lockunspent
index e21df7a..9b26c69 100644
--- a/lockunspent
+++ b/lockunspent
@@ -1,4 +1,4 @@
-lockunspent unlock ([{"txid":"txid","vout":n},...])
+lockunspent unlock ( [{"txid":"hex","vout":n},...] )
Updates list of temporarily unspendable outputs.
Temporarily lock (unlock=false) or unlock (unlock=true) specified transaction outputs.
diff --git a/signrawtransactionwithkey b/signrawtransactionwithkey
index 583b3d9..6113c66 100644
--- a/signrawtransactionwithkey
+++ b/signrawtransactionwithkey
@@ -1,4 +1,4 @@
-signrawtransactionwithkey "hexstring" ["privatekey1",...] ( [{"txid":"id","vout":n,"scriptPubKey":"hex","redeemScript":"hex"},...] sighashtype )
+signrawtransactionwithkey "hexstring" ["privatekey",...] ( [{"txid":"hex","vout":n,"scriptPubKey":"hex","redeemScript":"hex","amount":amount},...] "sighashtype" )
Sign inputs for raw transaction (serialized, hex-encoded).
The second argument is an array of base58-encoded private
diff --git a/signrawtransactionwithwallet b/signrawtransactionwithwallet
index 5a70fa4..ad9ce09 100644
--- a/signrawtransactionwithwallet
+++ b/signrawtransactionwithwallet
@@ -1,4 +1,4 @@
-signrawtransactionwithwallet "hexstring" ( [{"txid":"id","vout":n,"scriptPubKey":"hex","redeemScript":"hex"},...] sighashtype )
+signrawtransactionwithwallet "hexstring" ( [{"txid":"hex","vout":n,"scriptPubKey":"hex","redeemScript":"hex","amount":amount},...] "sighashtype" )
Sign inputs for raw transaction (serialized, hex-encoded).
The second optional argument (may be null) is an array of previous transaction outputs that
diff --git a/walletcreatefundedpsbt b/walletcreatefundedpsbt
index 81ba09b..25e77bd 100644
--- a/walletcreatefundedpsbt
+++ b/walletcreatefundedpsbt
@@ -1,4 +1,4 @@
-walletcreatefundedpsbt [{"txid":"id","vout":n},...] [{"address":amount},{"data":"hex"},...] ( locktime ) ( replaceable ) ( options bip32derivs )
+walletcreatefundedpsbt [{"txid":"hex","vout":n,"sequence":n},...] [{"address":amount},{"data":"hex"},...] ( locktime {"changeAddress":"hex","changePosition":n,"change_type":"str","includeWatching":bool,"lockUnspents":bool,"feeRate":n,"subtractFeeFromOutputs":[int,...],"replaceable":bool,"conf_target":n,"estimate_mode":"str"} bip32derivs )
Creates and funds a transaction in the Partially Signed Transaction format. Inputs will be added if supplied inputs are not enough
Implements the Creator and Updater roles. |
|
Generated help looks fine to me; this also keeps the documentation with the RPC calls they document instead of moving it somewhere else, which is good. |
promag
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK.
src/rpc/rawtransaction.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove RPCArg throughout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@promag Done and thx! That makes it look less verbose and probably easier to read the documentation straight from the cpp file.
faa9a9d to
fa483e1
Compare
ryanofsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK fa483e1. Changes all look good but I'm a surprised to see how incomplete this is. The other PR #14502 which was closed was much more ambitious and completely overhauled everything, while this is only printing the initial lines of a few RPC methods.
I will create pull requests for the next steps after this one is merged.
Curious how many PRs were you thinking of, and how would they be divided up.
This is mostly refactoring, so I wouldn't want to create a pull request that takes more than, say, 30 minutes to review. Logically, I'd like to split them into auto-generating the summary line (1), description (2) and extended documentation about input and output variables (3). This is the first part of (1). In the next step I will add a linter to make sure all the new documentation is autogenerated and maybe add a scripted diff to convert all remaining ones. Then create one or two pull requests for everything else. So 2-3 in total. |
|
I implemented (2) in the branch I linked above; but it still needs some work and cleanup |
fa483e1 rpc: Add RPCHelpMan for machine-generated help (MarcoFalke) fa0d36f rpc: Include rpc/util.h where needed for RPCHelpMan (MarcoFalke) Pull request description: This introduces a manager for the RPC help generation and demonstrates its use of it in some RPCs. It is the first non-exhaustive step toward #14378 and I will create pull requests for the next steps after this one is merged. Tree-SHA512: 86f68322443ff01cd964aaf0ebe186be63fbebe4c47676cf7a622cc2b5305fd176bd57badfd1bbf788a036812253eb0dead74ecc3b30664c3e0d9392b2248054
fa483e1 rpc: Add RPCHelpMan for machine-generated help (MarcoFalke) fa0d36f rpc: Include rpc/util.h where needed for RPCHelpMan (MarcoFalke) Pull request description: This introduces a manager for the RPC help generation and demonstrates its use of it in some RPCs. It is the first non-exhaustive step toward bitcoin#14378 and I will create pull requests for the next steps after this one is merged. Tree-SHA512: 86f68322443ff01cd964aaf0ebe186be63fbebe4c47676cf7a622cc2b5305fd176bd57badfd1bbf788a036812253eb0dead74ecc3b30664c3e0d9392b2248054
fa483e1 rpc: Add RPCHelpMan for machine-generated help (MarcoFalke) fa0d36f rpc: Include rpc/util.h where needed for RPCHelpMan (MarcoFalke) Pull request description: This introduces a manager for the RPC help generation and demonstrates its use of it in some RPCs. It is the first non-exhaustive step toward bitcoin#14378 and I will create pull requests for the next steps after this one is merged. Tree-SHA512: 86f68322443ff01cd964aaf0ebe186be63fbebe4c47676cf7a622cc2b5305fd176bd57badfd1bbf788a036812253eb0dead74ecc3b30664c3e0d9392b2248054
This introduces a manager for the RPC help generation and demonstrates its use of it in some RPCs.
It is the first non-exhaustive step toward #14378 and I will create pull requests for the next steps after this one is merged.