Skip to content

Hide accounts system behind deprecation switch #11497

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
Closed
achow101 wants to merge 1 commit into from
Closed

Hide accounts system behind deprecation switch #11497

achow101 wants to merge 1 commit into from

Conversation

@achow101
Copy link
Member

@achow101 achow101 commented Oct 14, 2017

Deprecate all accounts functionality and make it only accessible by using -deprecatedrpc=accounts.

Accounts specific RPCs, account arguments, and account related results all require the -deprecatedrpc=acocunts startup option now in order to see account things.

A couple of the tests use the accounts system for labeling things, so instead of changing those, I left them as is and set the tests to start the nodes with -deprecatedrpc=accounts. That switch can be removed as those RPCs are replaced with a labels system.

@fanquake fanquake added this to the 0.16.0 milestone Oct 14, 2017
@meshcollider
Copy link
Contributor

meshcollider commented Oct 14, 2017 •
edited
Loading

Concept ACK after label system has been merged

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Oct 14, 2017

I thought the plan was to have labels in place before removing (or, really, formally deprecating) accounts?

@achow101
Copy link
Member Author

achow101 commented Oct 14, 2017

I thought the plan was to have labels in place before removing (or, really, formally deprecating) accounts?

I have no idea.

Maybe it would be a good idea to have this and #7729 at the same time to allow for not using accounts and labels at the same time

@achow101 achow101 mentioned this pull request Oct 14, 2017
Closed
@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Oct 14, 2017

Yea, I mean this and labels in the same release sounds good, though I cant say I'm excited about removing accounts pre-labels.

@jnewbery
Copy link
Contributor

jnewbery commented Oct 16, 2017

Thanks for tackling this @achow101 . I'd echo @TheBlueMatt's feedback that labels needs to be in place before we remove the accounts system. The -deprecatedrpc framework was put in place with a specific purpose: to make sure that RPCs can be removed or changed in a release without suddenly breaking clients. The idea is that the -deprecatedrpc option is only in place for a single release before the RPC is removed entirely. Without a plan already in place to migrate wallets to the label system and fully remove the accounts system, this feels like an abuse of the -deprecatedrpc mechanism.

At a conceptual level, I'm not convinced that accounts system removal should depend on node version. It feels far more appropriate for it to be dependant on the wallet version than the node version.

meshcollider
meshcollider reviewed Oct 20, 2017
View reviewed changes
src/wallet/rpcwallet.cpp Outdated
Copy link
Contributor

@meshcollider meshcollider Oct 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Also, this should be re-purposed to label rather than removed, after #7729 merge, same as other RPCs with account arguments

Copy link
Member Author

@achow101 achow101 Nov 30, 2017

Choose a reason for hiding this comment

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

Agreed.

@meshcollider meshcollider mentioned this pull request Oct 20, 2017
Merged
@achow101
Hide accounts system behind deprecation switch
e572d3c 
Deprecate all accounts functionality and make it only accessible
by using -deprecatedrpc=accounts.
@achow101 achow101 force-pushed the deprecate-account-rpcs branch from 74085ad to e572d3c Compare November 30, 2017 16:42
@achow101
Copy link
Member Author

achow101 commented Nov 30, 2017

Rebased.

This should be done after #7729 is merged. I will update it accordingly then.

@laanwj laanwj modified the milestones: 0.16.0, 0.17.0 Jan 11, 2018
@ryanofsky
Copy link
Contributor

ryanofsky commented Apr 11, 2018

labels needs to be in place before we remove the accounts system

Are we ready for this PR now that #12892 is merged?

@ryanofsky
Copy link
Contributor

ryanofsky commented Apr 11, 2018

Might be helpful for updating this PR: release notes https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes-pr12892.md list the parts of the accounts api that are deprecated.

@Sjors
Copy link
Member

Sjors commented Apr 11, 2018

Concept ACK. Agree with @ryanofsky that all prerequisites seem to be in place now.

@jnewbery
Copy link
Contributor

jnewbery commented Apr 11, 2018

@achow101 - I've already started rebasing this on #12892. I'll open my own PR later today unless you really want to keep hold of this one.

@achow101
Copy link
Member Author

achow101 commented Apr 11, 2018

Closing in favor of whatever @jnewbery does because I'm too lazy to rebase this.

@achow101 achow101 closed this Apr 11, 2018
This was referenced Apr 11, 2018
Merged
Closed
@jnewbery jnewbery mentioned this pull request Oct 8, 2018
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@meshcollider meshcollider meshcollider left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

RPC/REST/ZMQ Wallet

Projects

None yet

Milestone

0.17.0

Development

Successfully merging this pull request may close these issues.

8 participants

@achow101 @meshcollider @TheBlueMatt @jnewbery @ryanofsky @Sjors @laanwj @fanquake