rpc: Auto-format RPCResult #17809

maflcko · 2019-12-27T21:43:38Z

This enforces most syntax rules of the RPCResult at compile time (or some at run time during unit and functional tests)

Apart from normalizing the syntax, by separating stylistic formatting from the structure, we could in theory directly generate the html for e.g. https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/importmulti/

maflcko · 2019-12-27T21:44:27Z

Currently based on #17804, will squash after that was merged

DrahtBot · 2019-12-27T22:28:16Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpc: generalize address in decoderawtransaction RPCResult #18164 (rpc: generalize address in decoderawtransaction RPCResult by theStack)
config, net, test: asmap feature refinements and functional tests #17812 (config, net, test: asmap feature refinements and functional tests by jonatack)
[BIP 174] PSBT version, proprietary, and xpub fields #17034 (Bip174 extensions by achow101)
Let validateaddress locate error in Bech32 address #16807 (Let validateaddress locate error in Bech32 address by meshcollider)
rpc: have raw transaction decoding infer output descriptors #16795 (rpc: have raw transaction decoding infer output descriptors by instagibbs)
rpc: Report reason for replaceable txpool transactions #16490 (rpc: Report reason for replaceable txpool transactions by MarcoFalke)
[BIP 174] Implement serialization support for GLOBAL_XPUB field. #16463 ([BIP 174] Implement serialization support for GLOBAL_XPUB field. by achow101)
BIP-322: Generic signed message format #16440 (BIP-322: Generic signed message format by kallewoof)
wallet: Show fee in results for signrawtransaction* for segwit inputs #12911 (wallet: Show fee in results for signrawtransaction* for segwit inputs by kallewoof)
RPC: Add ancestor{count,size,fees} to listunspent output #12677 (RPC: Add ancestor{count,size,fees} to listunspent output by luke-jr)

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.

promag · 2019-12-27T22:32:51Z

Concept ACK 👏

paymog

This is a heroic PR. I left some comments but I didn't finish going through all the code. I'll take a look again after the comments are responded to - I don't want to provide bad comments and I'll calibrate based on how these are received.

However, concept ACK. This is awesome.

src/wallet/rpcwallet.cpp

paymog · 2019-12-28T10:23:13Z

src/wallet/rpcwallet.cpp

It might be worthwhile to add a constructor where these two empty strings aren't needed. It's quite confusing as a reader to see these everywhere.

The first one is the name and it is only used when it is needed to name it (i.e. as a key in a dictionary). Not sure if it makes sense to omit it from the constructor when it is not needed. I'll wait for others to chime in.

The second string is the description, which should never be empty. Or at least it shouldn't be made easy to leave empty. I prefer if new code is explicit about not providing a description.

src/wallet/rpcwallet.cpp

src/rpc/blockchain.cpp

paymog · 2019-12-28T10:48:48Z

src/rpc/blockchain.cpp

NUM_AMOUNT?

Unfortunately not. This should be fixed in a follow-up pull request

jonasschnelli · 2020-01-05T21:07:32Z

Concept ACK. Great to see progress on the RPC output as well..

cvengler · 2020-01-24T19:20:52Z

Concept ACK

maflcko · 2020-01-24T19:24:49Z

Please review #18098 first

src/rpc/util.h

cvengler · 2020-01-25T10:57:01Z

I just ran #17804 and this PR on:

clang version 7.0.1-8 (tags/RELEASE_701/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

The base PR worked fine, however this PR gave me more than 20 errors.
I post the error log here:
https://pastebin.com/G5G6fNMJ

laanwj · 2020-01-31T15:06:57Z

Concept ACK

jonatack · 2020-02-07T09:07:17Z

Concept ACK

This is needed so that it can be used by RPCResult Also, * rename NAMED_ARG to NONE for generalization. * change RPCArg constructors to initialize the members by moving values

maflcko · 2020-02-25T15:46:25Z

Rebased due to trivial conflict in the includes. Should be easy to re-ACK

Sjors · 2020-02-25T19:29:29Z

Indeed, re-ACK fa6b061

ajtowns · 2020-03-02T09:59:49Z

src/wallet/rpcwallet.cpp

+                    {
+                        {RPCResult::Type::OBJ, "address", "json object with information about address",
+                        {
+                            {RPCResult::Type::STR, "purpose", "Purpose of address (\"send\" for sending address, \"receive\" for receiving address)"},


"(json object) json object with ...." seems a bit redundant. Maybe "(json object) keyed by address" and "(json object) information about the address" would be better?

Seems like a good follow-up issue: #18258

ajtowns · 2020-03-02T10:08:33Z

src/wallet/rpcwallet.cpp

+                            {RPCResult::Type::BOOL, "reused", "(only present if avoid_reuse is set) Whether this output is reused/dirty (sent to an address that was previously spent from)"},
+                            {RPCResult::Type::STR, "desc", "(only when solvable) A descriptor for spending this output"},
+                            {RPCResult::Type::BOOL, "safe", "Whether this output is considered safe to spend. Unconfirmed transactions"
            "                              from outside keys and unconfirmed replacement transactions are considered unsafe\n"


Not following why the leading spaces here are being kept?

To preserve the output of git blame. Our style-guideline is to keep the current style unless the line is changed for other reasons.

Hmm, perhaps I was unclear: I meant the leading spaces inside the quoted string

Even those are stylistic and should be trimmed at the beginning of a newline

Oh wait you are saying in the line above this line I missed a \n character? I think I did. Someone should fix that.

Fixed in #18346

ajtowns

ACK fa6b061 -- skimmed code changes and differences to rpc help output

fa6b061 rpc: Auto-format RPCResult (MarcoFalke) fa7d050 rpc: Move OuterType enum to header (MarcoFalke) Pull request description: This enforces most syntax rules of the RPCResult at compile time (or some at run time during unit and functional tests) Apart from normalizing the syntax, by separating stylistic formatting from the structure, we could in theory directly generate the html for e.g. https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/importmulti/ ACKs for top commit: Sjors: Indeed, re-ACK fa6b061 ajtowns: ACK fa6b061 -- skimmed code changes and differences to rpc help output Tree-SHA512: 5b510b3aa0b7c7b9189a48c77593159409069f939145b9a00c5478e894cf65f994d44d633eb7bb7dbea40ee820645a2930976c24772379d96929002b120efa28

Converted auxpow-specific RPC result help texts to the new "auto-format" system (bitcoin/bitcoin#17809).

Converted namecoin-specific RPC result help texts to the new "auto-format" system (bitcoin/bitcoin#17809).

Converted xaya-specific RPC result help texts to the new "auto-format" system (bitcoin/bitcoin#17809).

Summary: This is needed so that it can be used by RPCResult Also, * rename NAMED_ARG to NONE for generalization. * change RPCArg constructors to initialize the members by moving values Backport of Core [[bitcoin/bitcoin#17809 | PR17809]] [1/2] : bitcoin/bitcoin@fa7d050 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8123

Summary: Backport of Core [[bitcoin/bitcoin#17809 | PR17809]] part [2/2] : bitcoin/bitcoin@fa6b061 Depends on D8123 Test Plan: ninja all check-all Examine the differential in documentation to ensure it is correct: https://reviews.bitcoinabc.org/differential/diff/25141/ Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D8124

fa6b061 rpc: Auto-format RPCResult (MarcoFalke) fa7d050 rpc: Move OuterType enum to header (MarcoFalke) Pull request description: This enforces most syntax rules of the RPCResult at compile time (or some at run time during unit and functional tests) Apart from normalizing the syntax, by separating stylistic formatting from the structure, we could in theory directly generate the html for e.g. https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/importmulti/ ACKs for top commit: Sjors: Indeed, re-ACK fa6b061 ajtowns: ACK fa6b061 -- skimmed code changes and differences to rpc help output Tree-SHA512: 5b510b3aa0b7c7b9189a48c77593159409069f939145b9a00c5478e894cf65f994d44d633eb7bb7dbea40ee820645a2930976c24772379d96929002b120efa28

Summary: This is needed so that it can be used by RPCResult Also, * rename NAMED_ARG to NONE for generalization. * change RPCArg constructors to initialize the members by moving values Backport of Core [[bitcoin/bitcoin#17809 | PR17809]] [1/2] : bitcoin/bitcoin@fa7d050 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8123

fanquake added RPC/REST/ZMQ Docs labels Dec 27, 2019

maflcko mentioned this pull request Dec 27, 2019

doc: Misc RPC help fixes #17804

Merged

paymog reviewed Dec 28, 2019

View reviewed changes

maflcko force-pushed the 1906-rpcResult branch from fe3e2f1 to bf8feaa Compare January 2, 2020 16:05

DrahtBot added the Needs rebase label Jan 7, 2020

maflcko force-pushed the 1906-rpcResult branch from bf8feaa to 37fc4d6 Compare January 9, 2020 16:51

DrahtBot removed the Needs rebase label Jan 9, 2020

cvengler reviewed Jan 24, 2020

View reviewed changes

src/rpc/util.h Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Jan 29, 2020

maflcko force-pushed the 1906-rpcResult branch 3 times, most recently from a254bd0 to a6db5ef Compare February 9, 2020 14:49

DrahtBot removed the Needs rebase label Feb 9, 2020

This was referenced Feb 19, 2020

Let validateaddress locate error in Bech32 address #16807

Merged

External signer support - Wallet Box edition #16546

Merged

UI external signer support (e.g. hardware wallet) #16549

Closed

refactor: consolidate sendmany and sendtoaddress code #18202

Merged

DrahtBot added the Needs rebase label Feb 25, 2020

MarcoFalke added 2 commits February 25, 2020 22:33

rpc: Move OuterType enum to header

fa7d050

This is needed so that it can be used by RPCResult Also, * rename NAMED_ARG to NONE for generalization. * change RPCArg constructors to initialize the members by moving values

rpc: Auto-format RPCResult

fa6b061

maflcko force-pushed the 1906-rpcResult branch from a6db5ef to fa6b061 Compare February 25, 2020 15:37

DrahtBot removed the Needs rebase label Feb 25, 2020

ajtowns reviewed Mar 2, 2020

View reviewed changes

ajtowns approved these changes Mar 2, 2020

View reviewed changes

maflcko merged commit a71c347 into bitcoin:master Mar 4, 2020

maflcko deleted the 1906-rpcResult branch March 4, 2020 13:19

maflcko mentioned this pull request Mar 4, 2020

rpcdoc: Remove redundant type from description #18258

Closed

domob1812 added a commit to domob1812/namecoin-core that referenced this pull request Mar 13, 2020

Merge branch 'bitcoin' into auxpow

fea9d3c

Converted auxpow-specific RPC result help texts to the new "auto-format" system (bitcoin/bitcoin#17809).

domob1812 added a commit to domob1812/namecoin-core that referenced this pull request Mar 13, 2020

Merge branch 'auxpow'

c667dc1

Converted namecoin-specific RPC result help texts to the new "auto-format" system (bitcoin/bitcoin#17809).

domob1812 added a commit to xaya/xaya that referenced this pull request Mar 13, 2020

Merge branch 'namecoin'

080a363

Converted xaya-specific RPC result help texts to the new "auto-format" system (bitcoin/bitcoin#17809).

maflcko mentioned this pull request Mar 13, 2020

rpc: Document an RPCResult for all calls; Enforce at compile time #18346

Merged

domob1812 mentioned this pull request Apr 3, 2020

Replace concatenated headers with JSON array namecoin/electrum-nmc#216

Open

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

promag mentioned this pull request Jul 26, 2020

rpc: RPCResult Type of MempoolEntryDescription should be OBJ. #19585

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

rpc: Auto-format RPCResult #17809

rpc: Auto-format RPCResult #17809

Uh oh!

Conversation

maflcko commented Dec 27, 2019

Uh oh!

maflcko commented Dec 27, 2019

Uh oh!

DrahtBot commented Dec 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag commented Dec 27, 2019

Uh oh!

paymog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jan 5, 2020

Uh oh!

cvengler commented Jan 24, 2020

Uh oh!

maflcko commented Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cvengler commented Jan 25, 2020

Uh oh!

laanwj commented Jan 31, 2020

Uh oh!

jonatack commented Feb 7, 2020

Uh oh!

maflcko commented Feb 25, 2020

Uh oh!

Sjors commented Feb 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

DrahtBot commented Dec 27, 2019 •

edited

Loading

maflcko commented Jan 24, 2020 •

edited

Loading