rpc: update some RPCExamples to bech32 #18197

pull yusufsahinhamza wants to merge 1 commits into bitcoin:master from yusufsahinhamza:rpc-examples changing 1 files +12 −12

yusufsahinhamza commented at 1:07 PM on February 22, 2020: contributor

Update some RPC examples from legacy to bech32.

Fixes #18185.
fanquake added the label RPC/REST/ZMQ on Feb 22, 2020
DrahtBot commented at 2:44 PM on February 22, 2020: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #16378 ([WIP] The ultimate send RPC by Sjors)
- #14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)
- #11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)
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.
fanquake commented at 1:00 AM on February 23, 2020: member

cc @theStack

in src/wallet/rpcwallet.cpp:1226 in af23472cb3 outdated

1222 | @@ -1223,7 +1223,7 @@ static UniValue listreceivedbyaddress(const JSONRPCRequest& request)
1223 |                      HelpExampleCli("listreceivedbyaddress", "")
1224 |              + HelpExampleCli("listreceivedbyaddress", "6 true")
1225 |              + HelpExampleRpc("listreceivedbyaddress", "6, true, true")
1226 | -            + HelpExampleRpc("listreceivedbyaddress", "6, true, true, \"1M72Sfpbz1BPpXFHz9m3CdqATR44Jvaydd\"")
1227 | +            + HelpExampleRpc("listreceivedbyaddress", "6, true, true, " + EXAMPLE_ADDRESS + "")

Sjors commented at 11:10 AM on February 23, 2020:

The extra + "" isn't necessary.

Sjors commented at 1:33 PM on February 23, 2020:

Thanks for dropping that. Can you squash these two commits?

yusufsahinhamza commented at 2:00 PM on February 23, 2020:

@Sjors Done!

Sjors approved
Sjors commented at 11:14 AM on February 23, 2020: member

tACK af23472cb39829f88868577f41510d0dc25f246a

I would prefer to get #17809 merged first though.
rpc: update some RPCExamples to bech32 de83a30e7b
yusufsahinhamza force-pushed on Feb 23, 2020
Sjors commented at 10:38 AM on February 24, 2020: member

re-ACK de83a30e7b7244e7cd7f9df7ad829aa96e479f3f
theStack commented at 12:47 PM on February 25, 2020: member
Code-Review ACK https://github.com/bitcoin/bitcoin/commit/de83a30e7b7244e7cd7f9df7ad829aa96e479f3f, changes LGTM

Note though that this PR alone does not completely fix #18185 yet (which is fine as the PR title only says "some RPCExamples" and not all), as there are still some other RPCExamples containing legacy addresses:
- sendmany
- addmultisigaddress
- listunspent
Since those examples contain more than one address, it will probably make sense to add another constant for a second bech32 (invalid) address. I think it's okay to do all that in an extra follow-up PR.
yusufsahinhamza commented at 1:46 PM on February 25, 2020: contributor

@theStack Yes, actually i wasn't sure about adding another constant for a different address and EXAMPLE_ADDRESS isn't looks suitable for some examples because there is extra \ (back slash) before the " (quote) at the end of these addresses, that's why i left some of these examples:

And i think we can put these different example addresses to in a const array and use them like EXAMPLE_ADDRESS[0] and EXAMPLE_ADDRESS[1].
theStack commented at 2:10 PM on February 25, 2020: member

@yusufsahinhamza: Oh, thanks for pointing the backslash problem out -- considering that, it was not very wise from my side to include the quote characters in the EXAMPLE_ADDRESS string in https://github.com/bitcoin/bitcoin/commit/7f1475c7119e8c72bce39a63386a6ca859066b80 :thinking: I'd suggest then to remove the quotes from the constant and set the surrounding characters directly in the RPCExamples (not here, but in the course of a follow-up PR).

And i think we can put these different example addresses to in a const array and use them like EXAMPLE_ADDRESS[0] and EXAMPLE_ADDRESS[1].

Agreed.
yusufsahinhamza closed this on Feb 25, 2020
theStack commented at 4:42 PM on February 25, 2020: member

@yusufsahinhamza: It was not my intention that this PR gets closed, I think it would have been fine to merge it, since it already had two ACKs. Just because it doesn't fully fixes #18185 doesn't mean it's bad. With "follow-up PR" I meant a PR that is based on one that was already merged :-)
yusufsahinhamza commented at 4:52 PM on February 25, 2020: contributor

@theStack Oh okay, i thought we will continue in the new PR, no problem.
yusufsahinhamza reopened this on Feb 25, 2020
MarcoFalke commented at 5:40 PM on February 25, 2020: member

Given that you have already written the new pull and that all the lines touched here will be touched again in the other pull, it seems easier and less work for reviewers to just focus on that one. I'd suggest closing this one again.
yusufsahinhamza closed this on Feb 25, 2020
MarcoFalke referenced this in commit 0eebe45cf7 on Mar 11, 2020
sidhujag referenced this in commit db2052837f on Mar 11, 2020
sidhujag referenced this in commit b232f23b00 on Nov 10, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#18185 Change address format in RPCExamples from legacy to bech32 #18208 rpc: Change RPCExamples to bech32

rpc: update some RPCExamples to bech32 #18197

Conflicts