Improve porting documentation for legacy-only wallet RPCs #25363

1. 
   laanwj commented at 5:28 pm on June 13, 2022: member

   importaddress (and possibly other legacy-only wallet functions), when used on a descriptor wallet, return a fairly nondescript error:

   0$ ./bitcoin-cli -regtest importaddress test
   1error code: -4
   2error message:
   3This type of wallet does not support this command
   

   The RPC help of importaddress doesn’t mention nor further explain this incompatibility, nor what alternative to use instead (e.g. importdescriptors with addr(X) in this case).

   This potentially applies to the following RPCs:

   • importaddress (done in #25368)
   • importprivkey
   • importpubkey
   • importmulti
   • addmultisigaddress
   • dumpprivkey
   • getnewaddress
   • dumpwallet
   • importwallet
   • sethdseed
2. laanwj added the label Docs on Jun 13, 2022
3. laanwj added the label Wallet on Jun 13, 2022
4. laanwj added the label good first issue on Jun 14, 2022
5. 
   brokenprogrammer commented at 9:03 am on June 14, 2022: contributor

   Is the goal here to update the error messages to something more descriptive as well as the documentation for this RPC?

   Documentation update should include that the command is incompatible with descriptor wallets as well as mention the alternative to use?

   It looks like it is always the case that the specified wallet is a descriptor wallet if the JSONRPCError is thrown within EnsureLegacyScriptPubKeyMan and EnsureConstLegacyScriptPubKeyMan.

   I’ve never touched documentation for this project before, is it just to edit the object returned by importaddress function in backup.cpp that returns a RPCHelpMan object?

6. 
   laanwj commented at 9:38 am on June 14, 2022: member

   Is the goal here to update the error messages to something more descriptive as well as the documentation for this RPC?

   There are multiple possible angles here:

   • Update the error message. Note that error messages have limited space and are not supposed to be documentation, though, so this can at most be a hint.

   • Add RPC documentation. At least mention in the help that the functions don’t work with descriptor wallets. Adding instructions on how it can be done with descriptor wallets is even better, though also see next point.

   • Add a dedicated “porting guide” to port a code base from legacy wallets to descriptor wallets.

   • Make the functions backwards compatible. I don’t personally like this very much, as it’s just something that will be deprecated and removed again later in the interest of not cluttering the API.

   I’ve never touched documentation for this project before, is it just to edit the object returned by importaddress function in backup.cpp that returns a RPCHelpMan object?

   Yes.

7. laanwj renamed this:
   Confusion around importaddress for descriptor wallets
   Improve porting documentation for legacy-only wallet RPCs
   on Jun 15, 2022
8. achow101 referenced this in commit 51eebe082d on Jun 15, 2022
9. sidhujag referenced this in commit 44b71da40e on Jun 15, 2022
10. 
    Frank466-tech commented at 9:28 am on July 1, 2022: none

    Apologies for interrupting, but I feel like lost while reading your comments and stuffs […] I’m willing to find out “how exactly one is supposed to work on topics and for which purposes?” Please.

    Kind regards.

11. bitcoin deleted a comment on Jul 18, 2022
12. 
    Riahiamirreza commented at 7:19 pm on February 18, 2023: contributor

    How can I update error messages for one of the remaining rpc’s? Do they still need improvement? I’ve reviewed this PR and relatively know what should I do, but I’m not sure whether the changes are still needed or not. The issue was opened 8 months ago.
13. 
    furszy commented at 7:52 pm on February 18, 2023: member

    @Riahiamirreza seems that #25680 aims to fix this issue. Maybe you could go there and help reviewing. And to #27034 if you like the importaddress compatibility idea.
14. bitcoin deleted a comment on Mar 18, 2023
15. achow101 referenced this in commit 071308860a on May 1, 2023
16. sidhujag referenced this in commit 042478444a on May 1, 2023
17. 
    yusufsahinhamza commented at 10:30 am on May 2, 2023: contributor

    @laanwj Hello, as #25680 merged, do you think this issue may be closed?
18. 
    laanwj commented at 2:03 pm on May 2, 2023: member

    Yes, lgtm, thanks!
19. laanwj closed this on May 2, 2023

    ---

20. bitcoin locked this on May 1, 2024

Contributors
laanwj brokenprogrammer Frank466-tech Riahiamirreza furszy yusufsahinhamza

Labels
Docs Wallet good first issue