[RPC][Wallet] Deprecate internal account system #1731
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
11 tasks
random-zebra
reviewed
Jul 5, 2020
furszy
reviewed
Jul 6, 2020
|
rebase needed. |
Fuzzbawls
force-pushed
the
2020_deprecate-accounts
branch
from
b2673dc
to
d6d05b5
Compare
|
rebased and addressed comments |
Fuzzbawls
force-pushed
the
2020_deprecate-accounts
branch
from
d6d05b5
to
5b760a8
Compare
backported from bitcoin#9614
This is more consistent with `JSONRPCReplyObj`.
Fuzzbawls
force-pushed
the
2020_deprecate-accounts
branch
from
d3b3725
to
0e94c29
Compare
random-zebra
reviewed
Jul 8, 2020
Fuzzbawls
force-pushed
the
2020_deprecate-accounts
branch
from
0e94c29
to
922cca7
Compare
furszy
reviewed
Jul 8, 2020
These initially mirror the account functions, with the following differences: * These functions aren't DEPRECATED in the help * Help mentions 'label' instead of accounts. In the language used, labels are associated with addresses, instead of addresses associated with labels. (unlike with accounts.) * Labels have no balance * No balances in listlabels * listlabels has no minconf or watchonly argument * Like in the GUI, labels can be set on any address, not just receiving addreses * Unlike accounts, labels can be deleted. Being unable to delete them is a common annoyance. Currently only by reassigning all addresses using `setlabel`, but an explicit call `deletelabel` which assigns all address to the default label may make sense.
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=accunts` startup option now in order to see account things.
New optional input argument "addressFilter" allows the results to be filtered so that only that address' transactions are shown
Fuzzbawls
force-pushed
the
2020_deprecate-accounts
branch
from
922cca7
to
7ebebe8
Compare
All functional tests that were using the, now deprecated, account API have been switched over to the new label API. Testing of the account API still exists in the `rpc_deprecated.py` script however, which has been added to the list of tests to run.
This was preventing the `help` command from listing or returning help information on newer commands.
Fuzzbawls
force-pushed
the
2020_deprecate-accounts
branch
from
7ebebe8
to
6238aa4
Compare
furszy
reviewed
Jul 9, 2020
|
|
||
| # Node1's "from0" account balance should be just the doublespend: | ||
| assert_equal(self.nodes[1].getbalance("from0"), (1240 * 5)) | ||
| assert_equal(self.nodes[1].getbalance(), 6250 + (1240 * 5)) |
There was a problem hiding this comment.
nit: use starting_balance instead of static 6250.
furszy
approved these changes
Jul 9, 2020
Remove internal accounting system
automation
moved this from In progress
to Reviewer approved
Jul 9, 2020
random-zebra
approved these changes
Jul 9, 2020
There was a problem hiding this comment.
Minor non-blocking thing in getaddressinfo. But it can be addressed in a subsequent PR.
Tested with and without -deprecatedrpc=accounts.
All good ACK 6238aa4
Comment on lines
+135
to
138
| if (request.fHelp || request.params.size() > 1) | ||
| throw std::runtime_error( | ||
| "getaddressinfo ( \"address\" )\n" | ||
| "\nReturn information about the given PIVX address.\n" |
There was a problem hiding this comment.
It was already like this, but shouldn't the address be a required field? i.e.
if (request.fHelp || request.params.size() != 1)
throw std::runtime_error(
"getaddressinfo \"address\"\n"
...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the combined upstream PRs necessary to finalize Stage 2 of removing the internal wallet 'account' system in favor of a new 'label' system as detailed in #1706.
With this, references to 'account' in RPC command output has either been changed to 'label' (where appropriate), marked as deprecated and slated for removal in v5.0, or hidden/disabled entirely unless using a specific startup argument (
-deprecatedrpc=accounts)The functional testing suite has also been updated to use the 'label' API, and only continue to use the old 'account' API to verify expected same results with newer counterpart RPC commands, if available.