Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve porting documentation for legacy-only wallet RPCs #25363

Closed
laanwj opened this issue Jun 13, 2022 · 7 comments
Closed

Improve porting documentation for legacy-only wallet RPCs #25363

laanwj opened this issue Jun 13, 2022 · 7 comments
Labels
Docs good first issue Wallet

Comments

@laanwj
Copy link
Member

laanwj commented Jun 13, 2022 •
edited

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

$ ./bitcoin-cli -regtest importaddress test
error code: -4
error message:
This 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:
@brokenprogrammer
Copy link
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?

@laanwj
Copy link
Member Author

laanwj commented Jun 14, 2022

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.

@brokenprogrammer brokenprogrammer mentioned this issue Jun 14, 2022
Merged
@laanwj laanwj changed the title Confusion around importaddress for descriptor wallets Improve porting documentation for legacy-only wallet RPCs Jun 15, 2022
achow101 added a commit that referenced this issue Jun 15, 2022
@achow101
Merge #25368: doc: Update importaddress mention incompatibility with …
51eebe0 
…descriptor wallet

e3609cd doc: Update importaddress mention incompatibility with descriptor wallet (BrokenProgrammer)

Pull request description:

  This is related to #25363 and offers a small update to the error messages from `EnsureLegacyScriptPubKeyMan` and `EnsureConstLegacyScriptPubKeyMan` to mention that they only are compatible with legacy wallets.

  The RPC documentation for `importaddress` is also updated to mention this as well as guide the user to the alternative `importdescriptors` for cases when using descriptor wallets.

  I'm thinking that we can introduce a "porting guide" document mentioned in #25363 in a separate PR since I would have to make myself more familiar with the subject before being able to tackle that.

ACKs for top commit:
  laanwj:
    Code review ACK e3609cd
  achow101:
    ACK e3609cd

Tree-SHA512: c7a924a7283fe59dc4e04c8c8fa034c15601f0b25eff09d975e98e2e8db5268ff470336b2d978d6916af9f782f9257b840d64bd15485b1742b4a8b8bfd0bb50f
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Jun 15, 2022
@achow101
Merge bitcoin#25368: doc: Update importaddress mention incompatibilit…
44b71da 
…y with descriptor wallet

e3609cd doc: Update importaddress mention incompatibility with descriptor wallet (BrokenProgrammer)

Pull request description:

  This is related to bitcoin#25363 and offers a small update to the error messages from `EnsureLegacyScriptPubKeyMan` and `EnsureConstLegacyScriptPubKeyMan` to mention that they only are compatible with legacy wallets.

  The RPC documentation for `importaddress` is also updated to mention this as well as guide the user to the alternative `importdescriptors` for cases when using descriptor wallets.

  I'm thinking that we can introduce a "porting guide" document mentioned in bitcoin#25363 in a separate PR since I would have to make myself more familiar with the subject before being able to tackle that.

ACKs for top commit:
  laanwj:
    Code review ACK e3609cd
  achow101:
    ACK e3609cd

Tree-SHA512: c7a924a7283fe59dc4e04c8c8fa034c15601f0b25eff09d975e98e2e8db5268ff470336b2d978d6916af9f782f9257b840d64bd15485b1742b4a8b8bfd0bb50f
@Frank466-tech
Copy link

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.

@bitcoin bitcoin deleted a comment Jul 18, 2022
@yusufsahinhamza yusufsahinhamza mentioned this issue Jul 23, 2022
Merged
@Riahiamirreza
Copy link
Contributor

Riahiamirreza commented Feb 18, 2023 •
edited

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.

@furszy
Copy link
Member

furszy commented Feb 18, 2023

@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.

@bitcoin bitcoin deleted a comment from Almas456 Mar 18, 2023
achow101 added a commit that referenced this issue May 1, 2023
@achow101
Merge #25680: rpc, docs: Add note for commands that supports only leg…
0713088 
…acy wallets

9141e43 rpc, docs: Add note for commands that supports only legacy wallets (Yusuf Sahin HAMZA)

Pull request description:

  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:

  - `importprivkey`
  - `importpubkey`
  - `importwallet`
  - `dumpprivkey`
  - `dumpwallet`
  - `importmulti`
  - `addmultisigaddress`
  - `sethdseed`

ACKs for top commit:
  achow101:
    ACK 9141e43

Tree-SHA512: f3dc05d26577fd8dbe2bd69cb5c14ffccebacd6010402af44427b3d01be8484895dfcf33d55dfa766eadb7f9f3bae5cc4c2add3ac816a2ac60e8beb5a97527f3
sidhujag pushed a commit to syscoin/syscoin that referenced this issue May 1, 2023
@achow101 @sidhujag
Merge bitcoin#25680: rpc, docs: Add note for commands that supports o…
0424784 
…nly legacy wallets

9141e43 rpc, docs: Add note for commands that supports only legacy wallets (Yusuf Sahin HAMZA)

Pull request description:

  Refs bitcoin#25363, apparently issue is not updated since over a month, so i decided to put the same `importaddress` note in bitcoin#25368 to other rpc commands that needs this note.

  Note is added for following commands:

  - `importprivkey`
  - `importpubkey`
  - `importwallet`
  - `dumpprivkey`
  - `dumpwallet`
  - `importmulti`
  - `addmultisigaddress`
  - `sethdseed`

ACKs for top commit:
  achow101:
    ACK 9141e43

Tree-SHA512: f3dc05d26577fd8dbe2bd69cb5c14ffccebacd6010402af44427b3d01be8484895dfcf33d55dfa766eadb7f9f3bae5cc4c2add3ac816a2ac60e8beb5a97527f3
@yusufsahinhamza
Copy link
Contributor

@laanwj Hello, as #25680 merged, do you think this issue may be closed?

@laanwj
Copy link
Member Author

laanwj commented May 2, 2023

Yes, lgtm, thanks!

@laanwj laanwj closed this as completed May 2, 2023
@bitcoin bitcoin locked and limited conversation to collaborators May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Docs good first issue Wallet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
12 participants
@laanwj @furszy @brokenprogrammer @yusufsahinhamza @Riahiamirreza @Frank466-tech and others