rpc: Change RPCExamples to bech32 #18208

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

yusufsahinhamza commented at 4:18 PM on February 25, 2020: contributor

This is a follow-up PR to #18197 that fixes RPCExamples.

Fixes #18185.
DrahtBot added the label Docs on Feb 25, 2020
DrahtBot added the label RPC/REST/ZMQ on Feb 25, 2020
DrahtBot added the label Wallet on Feb 25, 2020
DrahtBot commented at 10:26 PM on February 25, 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:
- #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.
leto commented at 2:40 PM on February 26, 2020: contributor

utACK: looks like an improvement to the tests + docs
Empact commented at 11:24 PM on February 27, 2020: member

Concept ACK

in doc/developer-notes.md:1092 in 1cb833e632 outdated

1088 | @@ -1089,11 +1089,11 @@ A few guidelines for introducing and reviewing new RPC interfaces:
1089 |      new RPC is replacing a deprecated RPC, to avoid both RPCs confusingly
1090 |      showing up in the command list.
1091 |  
1092 | -- Use *invalid* bech32 addresses (e.g. the constant `EXAMPLE_ADDRESS`) for
1093 | +- Use *invalid* addresses (e.g. in the constant array `EXAMPLE_ADDRESS`) for

theStack commented at 12:04 AM on March 1, 2020:

The "bech32" part is actually important here, as we want to get rid of legacy addresses in the long term. Without that the reader of the guideline may think that any address format is fine.

in doc/developer-notes.md:1096 in 1cb833e632 outdated

1093 | +- Use *invalid* addresses (e.g. in the constant array `EXAMPLE_ADDRESS`) for
1094 |    `RPCExamples` help documentation.
1095 |  
1096 |    - *Rationale*: Prevent accidental transactions by users and encourage the use
1097 | -    of bech32 addresses by default.
1098 | +    of example addresses by default.

theStack commented at 12:11 AM on March 1, 2020:

That doesn't make sense. "Encourage the use of..." adresses the user of the software here, not the developer. As above, "bech32" should stay here.

in src/rpc/misc.cpp:47 in 1cb833e632 outdated

  43 | @@ -44,8 +44,8 @@ static UniValue validateaddress(const JSONRPCRequest& request)
  44 |              "}\n"
  45 |                  },
  46 |                  RPCExamples{
  47 | -                    HelpExampleCli("validateaddress", EXAMPLE_ADDRESS) +
  48 | -                    HelpExampleRpc("validateaddress", EXAMPLE_ADDRESS)
  49 | +                    HelpExampleCli("validateaddress", EXAMPLE_ADDRESS[1]) +

theStack commented at 12:21 AM on March 1, 2020:

The quotation marks around the address are missing. Previously they were included in EXAMPLE_ADDRESS, now as the constant contains the bare address, so they have to be added here.

in src/wallet/rpcwallet.cpp:3777 in 1cb833e632 outdated

3773 | @@ -3774,8 +3774,8 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
3774 |              "}\n"
3775 |                  },
3776 |                  RPCExamples{
3777 | -                    HelpExampleCli("getaddressinfo", EXAMPLE_ADDRESS) +
3778 | -                    HelpExampleRpc("getaddressinfo", EXAMPLE_ADDRESS)
3779 | +                    HelpExampleCli("getaddressinfo", EXAMPLE_ADDRESS[1]) +

theStack commented at 12:25 AM on March 1, 2020:

The quotation marks around the address are missing (see comment to "validateaddress" above).

theStack changes_requested
theStack commented at 12:43 AM on March 1, 2020: member

I'm not sure if it's a good idea to mix legacy and bech32 addresses in the same examples address array. The confusing consequence is that the index of the address array determines the resulting address type, and if someone e.g. would add another legacy address in front, many help strings that accessed at index 1 to get bech32 examples address woud then show legacy addresses again. To prevent that, it would be even okay right now if the legacy addresses stay in hardcoded (and the guidelines remain untouched).
theStack commented at 7:35 AM on March 1, 2020: member

LGTM now, can you squash the commits, please? In the course of that, you could also change the commit title to be more descriptive (e.g. "change to bech32" than just "fix").
yusufsahinhamza force-pushed on Mar 1, 2020

in src/rpc/util.h:32 in c5e620d4af outdated

  28 | @@ -29,10 +29,10 @@
  29 |  extern const std::string UNIX_EPOCH_TIME;
  30 |  
  31 |  /**
  32 | - * Example bech32 address used in multiple RPCExamples. The address is intentionally
  33 | + * Example bech32 addresses for multiple use in RPCExamples. The address is intentionally

jonatack commented at 2:22 PM on March 1, 2020:

s/address is/addresses are/, remove "multiple" (it's understood with RPCExamples being plural), suggestion:

- * Example bech32 address used in multiple RPCExamples. The address is intentionally
+ * Example bech32 addresses for the RPCExamples help documentation. They are intentionally

jonatack commented at 3:06 PM on March 1, 2020: member

Concept ACK
Change example addresses to bech32 3e32499909
yusufsahinhamza force-pushed on Mar 1, 2020
MarcoFalke renamed this:
~~rpc: Fix RPCExamples~~
rpc: Change RPCExamples to bech32
on Mar 11, 2020
MarcoFalke commented at 2:53 PM on March 11, 2020: member

ACK 3e32499909ca8127baaa9b40ad113b25ee151bbd
jonatack commented at 3:12 PM on March 11, 2020: member

ACK 3e32499
MarcoFalke merged this on Mar 11, 2020
MarcoFalke closed this 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

Docs Wallet RPC/REST/ZMQ

Linked (view graph)

#18185 Change address format in RPCExamples from legacy to bech32 #18197 rpc: update some RPCExamples to bech32

rpc: Change RPCExamples to bech32 #18208

Conflicts