Skip to content

rpc, docs: Add note for commands that supports only legacy wallets #25680

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
Merged
merged 1 commit into from
May 1, 2023
Merged

rpc, docs: Add note for commands that supports only legacy wallets #25680

merged 1 commit into from
May 1, 2023

Conversation

yusufsahinhamza
Copy link
Contributor

@yusufsahinhamza yusufsahinhamza commented Jul 23, 2022 •
edited
Loading

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

jonatack
jonatack reviewed Jul 23, 2022
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This note you have copied over from #25368 does not seem relevant to these RPCs. In some cases, the error message may need to be updated as well.

@yusufsahinhamza
Copy link
Contributor Author

@jonatack

Looks like no need of this note for getnewaddress.

According to this release note:

bitcoin/doc/release-notes/release-notes-0.21.0.md

Lines 412 to 422 in 194f6dc

The following RPCs are disabled for Descriptor Wallets:
* `importprivkey`
* `importpubkey`
* `importaddress`
* `importwallet`
* `dumpprivkey`
* `dumpwallet`
* `importmulti`
* `addmultisigaddress`
* `sethdseed`

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:

bitcoin/doc/release-notes/release-notes-0.21.0.md

Lines 405 to 406 in 194f6dc

To import into a Descriptor Wallet, a new `importdescriptors` RPC has been added that uses a syntax
similar to that of `importmulti`.

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.

@luke-jr
Copy link
Member

luke-jr commented Jul 26, 2022

getnewaddress

?!?!?!

@yusufsahinhamza
Copy link
Contributor Author

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.

@yusufsahinhamza yusufsahinhamza force-pushed the improve-rpchelpman-notes branch from 6a0779c to 0885471 Compare July 26, 2022 08:10
achow101
achow101 reviewed Jul 28, 2022
View reviewed changes
@yusufsahinhamza yusufsahinhamza force-pushed the improve-rpchelpman-notes branch from 0885471 to c0d399a Compare July 29, 2022 22:14
@bitcoin bitcoin deleted a comment from tusher456 Aug 26, 2022
@yusufsahinhamza yusufsahinhamza force-pushed the improve-rpchelpman-notes branch 4 times, most recently from 1f27b23 to 775ba9c Compare August 26, 2022 23:05
@yusufsahinhamza
Copy link
Contributor Author

775ba9c Conflicts are resolved and rebased.

@DrahtBot DrahtBot mentioned this pull request Sep 20, 2022
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 22, 2022 •
edited
Loading

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK achow101
Concept ACK theStack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #27034 (rpc: make importaddress compatible with descriptors wallet by furszy)
  • #25907 (wallet: rpc to add automatically generated descriptors by achow101)

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.

@DrahtBot DrahtBot mentioned this pull request Oct 6, 2022
Closed
theStack
theStack reviewed Nov 27, 2022
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

For the importprivkey and importpubkey RPC notes, it could make sense to suggest using importdescriptor with combo(X) for descriptor wallets?

@yusufsahinhamza
Copy link
Contributor Author

For the importprivkey and importpubkey RPC notes, it could make sense to suggest using importdescriptor with combo(X) for descriptor wallets?

@theStack It seems to make sense for me. I can rebase and push changes if no other suggestion about that.

@yusufsahinhamza
rpc, docs: Add note for commands that supports only legacy wallets
9141e43 
Note is added for following rpc commands:

importprivkey, importpubkey, importwallet, dumpprivkey,
dumpwallet, importmulti, addmultisigaddress, sethdseed
@yusufsahinhamza yusufsahinhamza force-pushed the improve-rpchelpman-notes branch from 775ba9c to 9141e43 Compare November 30, 2022 01:21
@DrahtBot DrahtBot mentioned this pull request Feb 3, 2023
Closed
@furszy furszy mentioned this pull request Feb 18, 2023
Closed
@achow101 achow101 self-requested a review April 25, 2023 15:35
@achow101
Copy link
Member

achow101 commented May 1, 2023

ACK 9141e43

@DrahtBot DrahtBot removed the request for review from achow101 May 1, 2023 12:18
@achow101 achow101 merged commit 0713088 into bitcoin:master May 1, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 1, 2023
@achow101
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
@luke-jr
Copy link
Member

luke-jr commented Jul 25, 2023

@achow101 Why was this merged?

I'm not sure suggesting combo() is a good idea. Isn't combo only for having a migration path, not something we want people to actually use?

@achow101
Copy link
Member

While I don't think we should necessarily encourage people to use 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.

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Sep 16, 2023
@yusufsahinhamza @luke-jr
rpc, docs: Add note for commands that supports only legacy wallets
49a61ba 
Note is added for following rpc commands:

importprivkey, importpubkey, importwallet, dumpprivkey,
dumpwallet, importmulti, addmultisigaddress, sethdseed

Github-Pull: bitcoin#25680
Rebased-From: 9141e43
@bitcoin bitcoin locked and limited conversation to collaborators Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@achow101 achow101 achow101 left review comments

+3 more reviewers

@theStack theStack theStack left review comments

@jonatack jonatack jonatack left review comments

@Onyeali Onyeali Onyeali approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@yusufsahinhamza @luke-jr @DrahtBot @achow101 @theStack @jonatack @Onyeali