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: display multiwallet total balance in -getinfo #19092

Closed

Conversation

jonatack
Copy link
Member

@jonatack jonatack commented May 28, 2020

Per review suggestion in #18594 (comment), this PR adds a total_balance field for -getinfo in multiwallet mode.

  "balances": {
    "": 0.00001000,
    "Encrypted": 0.00003500,
    "day-to-day": 0.00000120,
    "side project": 0.00000094
  },
  "total_balance": 0.00003714
}

Updated help:

$ bitcoin-cli -h | grep -A3 getinfo
  -getinfo
       Get general information from the remote server, including the total
       balance and 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.

Credit to João Barbosa for the suggestion.

@DrahtBot DrahtBot added the Tests label May 28, 2020
@promag
Copy link
Contributor

promag commented May 28, 2020

Concept ACK.

Could update doc?

@jonatack
Copy link
Member Author

jonatack commented May 28, 2020

Thanks @promag. Added a release note update.

@promag
Copy link
Contributor

promag commented May 28, 2020

This is --help output

  -getinfo
       Get general information from the remote server. Note that unlike
       server-side RPC calls, the results of -getinfo is the result of
       multiple non-atomic requests. Some entries in the result may
       represent results from different states (e.g. wallet balance may
       be as of a different block from the chain state reported)

nit, I wonder if this should document the result.

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

@jonatack
Copy link
Member Author

jonatack commented May 30, 2020

I wonder it this should document the result.

Made an attempt at improving it.

@jonatack jonatack force-pushed the cli-getinfo-multiwallet-total-balance branch from 57f771b to aba6f91 Compare May 30, 2020 11:41
@vahidjalaliii
Copy link

#19089

@luke-jr
Copy link
Member

luke-jr commented Jun 3, 2020

"balance" seems to perhaps oversimplify things...

Maybe "total_balance"?

@Wiphawee112
Copy link

ฉันจะทราบได้อย่างไร่่่่ะว่ายอดเงินอยุ่ทึ่ใหน ช่วยแนะนำด้วยค่ะ

@jonatack
Copy link
Member Author

jonatack commented Jun 4, 2020

ฉันจะทราบได้อย่างไร่่่่ะว่ายอดเงินอยุ่ทึ่ใหน ช่วยแนะนำด้วยค่ะ

"I can know exactly what the balance is. Please help."

I'll take that as a Concept ACK :)

@jonatack
Copy link
Member Author

jonatack commented Jun 4, 2020

"balance" seems to perhaps oversimplify things...

Maybe "total_balance"?

That was my initial thought as well. @promag WDYT?

@promag
Copy link
Contributor

promag commented Jun 4, 2020

Also looks fine to me. I just mentioned balance to keep it backward compatible, but I guess that's not a concern in -cli.

@jonatack jonatack force-pushed the cli-getinfo-multiwallet-total-balance branch from aba6f91 to c37fab2 Compare June 4, 2020 09:23
@jonatack
Copy link
Member Author

jonatack commented Jun 4, 2020

Thanks @luke-jr and @promag, updated to total_balance displayed after the balances and improved the test coverage.

@jonatack jonatack force-pushed the cli-getinfo-multiwallet-total-balance branch 4 times, most recently from fbd14f5 to d1ae2e5 Compare June 6, 2020 16:45
Comment on lines +457 to +529
static CAmount AmountFromValue(const UniValue& value)
{
CAmount amount{0};
if (!ParseFixedPoint(value.getValStr(), 8, &amount))
throw std::runtime_error("Invalid amount");
if (!MoneyRange(amount))
throw std::runtime_error("Amount out of range");
return amount;
}

static UniValue ValueFromAmount(const CAmount& amount)
{
bool sign{amount < 0};
int64_t n_abs{sign ? -amount : amount};
int64_t quotient{n_abs / COIN};
int64_t remainder{n_abs % COIN};
return UniValue(UniValue::VNUM, strprintf("%s%d.%08d", sign ? "-" : "", quotient, remainder));
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we get these into the libbitcoin_util.a module? I think this is the third copy...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, third one. Agreed, a good follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not in this PR or in a base PR especially considering there aren't ACKs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that refactoring would best be done in a separate PR.

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 9, 2020
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 13, 2020
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 13, 2020
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 13, 2020
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 14, 2020
@jonatack jonatack force-pushed the cli-getinfo-multiwallet-total-balance branch from d1ae2e5 to 3bed024 Compare June 17, 2020 07:17
@jonatack jonatack force-pushed the cli-getinfo-multiwallet-total-balance branch from 3bed024 to 08ac1ab Compare June 21, 2020 16:50
@jonatack
Copy link
Member Author

Closing, there are many open PRs and this has no ACKs. At least this PR was pulled into other Bitcoin implementations, so it will be useful for those users.

@jonatack jonatack closed this Jun 22, 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
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 Oct 10, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 10, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 10, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 10, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 19, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 19, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 19, 2021
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 19, 2021
@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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants