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.

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

cli, test, doc: bitcoin-cli -getinfo multiwallet balances follow-ups #19089

Closed

Conversation

jonatack
Copy link
Member

@jonatack jonatack commented May 28, 2020

These are follow-ups to #18594:

  • release note for -getinfo multiwallet balances

  • update the -getinfo help:

$ bitcoin-cli -h | grep -A3 getinfo
  -getinfo
       Get general information from the remote server, including the balances
       of each loaded wallet when in multiwallet mode. Note that
       -getinfo is the combined result of several RPCs (getnetworkinfo,
       getblockchaininfo, getwalletinfo, getbalances, and in multiwallet
       mode, listwallets), each with potentially different state.

doc/release-notes-18594.md Outdated Show resolved Hide resolved
@jonatack jonatack force-pushed the cli-getinfo-multiwallet-follow-ups branch from daab3ce to 8ab19a3 Compare May 28, 2020 12:40
src/bitcoin-cli.cpp Outdated Show resolved Hide resolved
@DrahtBot
Copy link
Contributor

DrahtBot commented May 28, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

@hebasto
Copy link
Member

hebasto commented Jun 5, 2020

Concept ACK.

@jonatack jonatack force-pushed the cli-getinfo-multiwallet-follow-ups branch from a53a8c9 to 8019cf2 Compare June 5, 2020 12:43
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Approach ACK 8019cf2.

@jonatack
Could you consider to replace s/ArgsManager::ALLOW_ANY/ArgsManager::ALLOW_BOOL/ for -getinfo command-line argument?

There is a general direction to move from the ArgsManager::ALLOW_ANY flag to more specific ones:

If you decide to implement this suggestion, the first commit and the last one could be combined.

@jonatack
Copy link
Member Author

jonatack commented Jun 5, 2020

Thanks for reviewing, @hebasto! From what I can tell, it would be best to not change this (yet):

  • ALLOW_BOOL appears to be unused in the codebase, apart from -rpcwhitelistdefault
  • the change would have no effect on -getinfo command parsing behavior and tests
  • in the event that we move to using more specific flags and that this one needs to be changed, many commands will need to be updated and ISTM it would be most coherent in terms of git history that they be changed together in the same commit or PR.

@hebasto
Copy link
Member

hebasto commented Jun 5, 2020

Thanks for reviewing, @hebasto! From what I can tell, it would be best to not change this (yet):

  • ALLOW_BOOL appears to be unused in the codebase, apart from -rpcwhitelistdefault

  • the change would have no effect on -getinfo command parsing behavior and tests

  • in the event that we move to using more specific flags and that this one needs to be changed, many commands will need to be updated and ISTM it would be most coherent in terms of git history that they be changed together in the same commit or PR.

FWIW, ArgsManager::ALLOW_BOOL is designed to use along with ArgsManager::GetBoolArg(). Additional error checking has not been implemented yet, unfortunately.

In any case, will continue my review :)

@jonatack jonatack force-pushed the cli-getinfo-multiwallet-follow-ups branch 2 times, most recently from 68e4a1e to 320b510 Compare June 6, 2020 09:33
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Thanks for a separate commit that introduces the assert_scale() finction.

test/functional/test_framework/util.py Outdated Show resolved Hide resolved
@jonatack jonatack force-pushed the cli-getinfo-multiwallet-follow-ups branch 2 times, most recently from 059f317 to 9785b10 Compare June 6, 2020 15:19
@jonatack jonatack force-pushed the cli-getinfo-multiwallet-follow-ups branch from 9785b10 to 0215c23 Compare June 6, 2020 16:38
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 9, 2020
per Luke Dashjr review feedback in PR 18594

Github-Pull: bitcoin#19089
Rebased-From: d83ae3e
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 9, 2020
These tests fail without the changes in the first commit of this PR.

Github-Pull: bitcoin#19089
Rebased-From: 1989439
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 9, 2020
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 9, 2020
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 9, 2020
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 13, 2020
per Luke Dashjr review feedback in PR 18594

Github-Pull: bitcoin#19089
Rebased-From: d83ae3e
@jonatack
Copy link
Member Author

Closing, will propose the changes one by one.

@jonatack jonatack closed this Jun 22, 2020
maflcko pushed a commit that referenced this pull request Jun 27, 2020
…et balances

6d35d0d doc: add release note for -getinfo displaying multiwallet balances (Jon Atack)

Pull request description:

  Release note for #18594. This is one of the commits from #19089, which had one concept ACK and approach ACK since late May. It seems better to submit the changes atomically.

Top commit has no ACKs.

Tree-SHA512: 38616d14b02c39f4ee4b93bf14f72043423cef177b595e85181bc9dc610fbe19d8271f2d2c9e5e17bb46423ffe27746e8e510b13a23ae6fd0e5bc4418a00dafa
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 9, 2020
per Luke Dashjr review feedback in PR 18594

Github-Pull: bitcoin#19089
Rebased-From: d83ae3e
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 9, 2020
These tests fail without the changes in the first commit of this PR.

Github-Pull: bitcoin#19089
Rebased-From: 1989439
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 9, 2020
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 9, 2020
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 9, 2020
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 27, 2021
…ltiwallet balances

6d35d0d doc: add release note for -getinfo displaying multiwallet balances (Jon Atack)

Pull request description:

  Release note for bitcoin#18594. This is one of the commits from bitcoin#19089, which had one concept ACK and approach ACK since late May. It seems better to submit the changes atomically.

Top commit has no ACKs.

Tree-SHA512: 38616d14b02c39f4ee4b93bf14f72043423cef177b595e85181bc9dc610fbe19d8271f2d2c9e5e17bb46423ffe27746e8e510b13a23ae6fd0e5bc4418a00dafa
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 28, 2021
…ltiwallet balances

6d35d0d doc: add release note for -getinfo displaying multiwallet balances (Jon Atack)

Pull request description:

  Release note for bitcoin#18594. This is one of the commits from bitcoin#19089, which had one concept ACK and approach ACK since late May. It seems better to submit the changes atomically.

Top commit has no ACKs.

Tree-SHA512: 38616d14b02c39f4ee4b93bf14f72043423cef177b595e85181bc9dc610fbe19d8271f2d2c9e5e17bb46423ffe27746e8e510b13a23ae6fd0e5bc4418a00dafa
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
…ltiwallet balances

6d35d0d doc: add release note for -getinfo displaying multiwallet balances (Jon Atack)

Pull request description:

  Release note for bitcoin#18594. This is one of the commits from bitcoin#19089, which had one concept ACK and approach ACK since late May. It seems better to submit the changes atomically.

Top commit has no ACKs.

Tree-SHA512: 38616d14b02c39f4ee4b93bf14f72043423cef177b595e85181bc9dc610fbe19d8271f2d2c9e5e17bb46423ffe27746e8e510b13a23ae6fd0e5bc4418a00dafa
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
…ltiwallet balances

6d35d0d doc: add release note for -getinfo displaying multiwallet balances (Jon Atack)

Pull request description:

  Release note for bitcoin#18594. This is one of the commits from bitcoin#19089, which had one concept ACK and approach ACK since late May. It seems better to submit the changes atomically.

Top commit has no ACKs.

Tree-SHA512: 38616d14b02c39f4ee4b93bf14f72043423cef177b595e85181bc9dc610fbe19d8271f2d2c9e5e17bb46423ffe27746e8e510b13a23ae6fd0e5bc4418a00dafa
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
…ltiwallet balances

6d35d0d doc: add release note for -getinfo displaying multiwallet balances (Jon Atack)

Pull request description:

  Release note for bitcoin#18594. This is one of the commits from bitcoin#19089, which had one concept ACK and approach ACK since late May. It seems better to submit the changes atomically.

Top commit has no ACKs.

Tree-SHA512: 38616d14b02c39f4ee4b93bf14f72043423cef177b595e85181bc9dc610fbe19d8271f2d2c9e5e17bb46423ffe27746e8e510b13a23ae6fd0e5bc4418a00dafa
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 15, 2021
…ltiwallet balances

6d35d0d doc: add release note for -getinfo displaying multiwallet balances (Jon Atack)

Pull request description:

  Release note for bitcoin#18594. This is one of the commits from bitcoin#19089, which had one concept ACK and approach ACK since late May. It seems better to submit the changes atomically.

Top commit has no ACKs.

Tree-SHA512: 38616d14b02c39f4ee4b93bf14f72043423cef177b595e85181bc9dc610fbe19d8271f2d2c9e5e17bb46423ffe27746e8e510b13a23ae6fd0e5bc4418a00dafa
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants