Skip to content

rpc, wallet: use the same next_index key in listdescriptors and importdescriptors #26194

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

rpc, wallet: use the same next_index key in listdescriptors and importdescriptors #26194

achow101 merged 1 commit into from
Mar 8, 2023

Conversation

@w0xlt
Copy link
Contributor

@w0xlt w0xlt commented Sep 28, 2022

Currently listdescriptors RPC uses next key to represent WalletDescriptor::next_index while importdescriptors uses next_index. This creates two different descriptor formats.

This PR changes listdescriptors to use the same key as importdescriptors.

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 28, 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 aureleoules, achow101

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

Conflicts

No conflicts as of last run.

@furszy
Copy link
Member

furszy commented Sep 28, 2022

As it's an API change, probably should add it to the release-notes.

luke-jr
luke-jr suggested changes Oct 1, 2022
View reviewed changes
src/wallet/rpc/backup.cpp Outdated
Copy link
Member

@luke-jr luke-jr Oct 1, 2022

Choose a reason for hiding this comment

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

There should be some compatibility at least for a release or two.

Copy link
Contributor Author

@w0xlt w0xlt Dec 6, 2022

Choose a reason for hiding this comment

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

Done in b082f28.

@w0xlt w0xlt force-pushed the next_index_listdescriptors branch from 80517c6 to b9ea106 Compare November 1, 2022 22:36
@w0xlt
Copy link
Contributor Author

w0xlt commented Nov 1, 2022

b9ea106 adds the next_index instead of replacing next as suggested in #26194 (comment) for compatibility reasons and adds the release notes as suggested in #26194 (comment).

aureleoules
aureleoules approved these changes Nov 2, 2022
View reviewed changes
Copy link
Contributor

@aureleoules aureleoules left a comment

Choose a reason for hiding this comment

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

ACK b9ea106f1da4e018ec4876d8e5b1209401ace382

maflcko
maflcko reviewed Nov 14, 2022
View reviewed changes
src/wallet/rpc/backup.cpp Outdated
Copy link
Member

@maflcko maflcko Nov 14, 2022

Choose a reason for hiding this comment

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

If this is kept for compatibility, it might be good to mention it, and that the value is equal to the other. Otherwise users might be confused to see the same field twice and wonder what the difference is?

Copy link
Contributor Author

@w0xlt w0xlt Dec 6, 2022

Choose a reason for hiding this comment

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

Done in b082f28.

@w0xlt w0xlt force-pushed the next_index_listdescriptors branch from b9ea106 to b082f28 Compare December 6, 2022 14:38
aureleoules
aureleoules approved these changes Jan 2, 2023
View reviewed changes
Copy link
Contributor

@aureleoules aureleoules left a comment

Choose a reason for hiding this comment

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

reACK b082f28

@aureleoules aureleoules mentioned this pull request Jan 18, 2023
Closed
@fanquake fanquake requested a review from achow101 February 8, 2023 11:15
@achow101
Copy link
Member

achow101 commented Mar 8, 2023

ACK b082f28

@DrahtBot DrahtBot removed the request for review from achow101 March 8, 2023 17:08
@achow101 achow101 merged commit 1ff135c into bitcoin:master Mar 8, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 8, 2023
@achow101
Merge bitcoin#26194: rpc, wallet: use the same next_index key in `l…
3e32619 
…istdescriptors` and `importdescriptors`

b082f28 rpc, wallet: use the same `next_index` in listdescriptors and importdescriptors (w0xlt)

Pull request description:

  Currently `listdescriptors` RPC uses `next` key to represent `WalletDescriptor::next_index` while `importdescriptors` uses `next_index`. This creates two different descriptor formats.

  This  PR changes `listdescriptors` to use the same key as `importdescriptors`.

ACKs for top commit:
  achow101:
    ACK b082f28
  aureleoules:
    reACK b082f28

Tree-SHA512: c29ec59051878e614d749ed6dc85e5c14ad00db0e8fcbce3f5066d1aae85ef07ca70f02920299e48d191b7387024fe224b0054c4191a5951cb805106f7b8e37b
@bitcoin bitcoin locked and limited conversation to collaborators Mar 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@maflcko maflcko maflcko left review comments

@luke-jr luke-jr luke-jr requested changes

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

@w0xlt @DrahtBot @furszy @achow101 @luke-jr @maflcko @aureleoules