Refs #25363, apparently issue is not updated since over a month, so i decided to put the same importaddress note in #25368 to other rpc commands that needs this note.
Note is added for following commands:
importprivkeyimportpubkeyimportwalletdumpprivkeydumpwalletimportmultiaddmultisigaddresssethdseed
Looks like no need of this note for getnewaddress.
According to this release note:
We can simply add “Note: This command is only compatible with legacy wallets.” note to these RPCs, but for example where there is alternative command to use for descriptor wallets; e.g. release note says:
In this case for importmulti we can add additional note “Use “importdescriptors” with addr(X)” for descriptor wallets." after the first note to point out what to use as an alternative.
getnewaddress
?!?!?!
getnewaddress
?!?!?!
Didn’t do any changes as i’m waiting for an answer about my last message, or let’s say suggestion to what to do.
1262@@ -1258,7 +1263,8 @@ RPCHelpMan importmulti()
1263 "may report that the imported keys, addresses or scripts exist but related transactions are still missing.\n"
1264 "The rescan parameter can be set to false if the key was never used to create transactions. If it is set to false,\n"
1265 "but the key was used to create transactions, rescanwallet needs to be called with the appropriate block range.\n"
1266- "Note: Use \"getwalletinfo\" to query the scanning progress.\n",
1267+ "Note: Use \"getwalletinfo\" to query the scanning progress.\n"
1268+ "Note: This command is only compatible with legacy wallets. Use \"importdescriptors\" with \"addr(X)\" for descriptor wallets.\n",
addr() here as importmulti allows the import of a ton of different things, including descriptors. Mentioning a specific descriptor type, which only applies to one of the things that importmulti can deal with, doesn’t seem to make sense to me.
@achow101 I agree, I’d rather like it as “Use “importdescriptors” for descriptor wallets.”, mentioning a specific parameter doesn’t seem necessary. People would get help by using only “importdescriptors” anyway.
This line can also be changed: https://github.com/bitcoin/bitcoin/blob/1abbae65eb9c99df5d8941008068d83ad99bf117/src/wallet/rpc/backup.cpp#L213
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, 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.
Concept ACK
For the importprivkey and importpubkey RPC notes, it could make sense to suggest using importdescriptor with combo(X) for descriptor wallets?
For the
importprivkeyandimportpubkeyRPC notes, it could make sense to suggest usingimportdescriptorwithcombo(X)for descriptor wallets? @theStack It seems to make sense for me. I can rebase and push changes if no other suggestion about that.
Note is added for following rpc commands:
importprivkey, importpubkey, importwallet, dumpprivkey,
dumpwallet, importmulti, addmultisigaddress, sethdseed
combo(), I don’t think it’s bad to mention it either. in the case of importpubkey and importprivkey, combo() with importdescriptors does replicate their behavior so it isn’t incorrect. I don’t really care either way whether it’s mentioned or not.
