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

rpc: Add balances RPC #15930

Merged
merged 4 commits into from May 6, 2019

Conversation

Projects
None yet
10 participants
@MarcoFalke
Copy link
Member

commented May 1, 2019

This exposes the CWallet::GetBalance() struct over RPC.

In the future, incorrectly named rpcs such as getunconfirmedbalance or rpcs redundant to this such as getbalance could be removed.

@MarcoFalke MarcoFalke added the Wallet label May 1, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Concept ACK

or rpcs redundant to this such as getbalance could be removed.

Dunno, I think having a dedicated call to get the balance is useful. If it's not broken (like mis-named methods) I'd prefer keeping it.

@instagibbs

This comment has been minimized.

Copy link
Member

commented May 1, 2019

concept ACK

@jnewbery
Copy link
Member

left a comment

Looks good. I think this needs release notes to describe the new return object and document that getunconfirmedbalance is deprecated.

I think having a dedicated call to get the balance is useful.

How about we have a getbalances RPC that just returns the balances object added in this PR.

Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

How about we have a getbalances RPC that just returns the balances object added in this PR.

Then I'd rather not add the same struct to getwalletinfo. Imo a single place where to get this is enough.

@jnewbery

This comment has been minimized.

Copy link
Member

commented May 1, 2019

I don't have an issue with the same data appearing in multiple RPC methods, but if you think that's an issue, then I'd have a slight preference for a getbalances RPC method to be separate from getwalletinfo. I think either is an improvement though.

I also think all wallet get- and list- methods should return a best block hash, so it's clear at what point the returned values are valid.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Nice!
Concept ACK.

I agree with @jnewbery about returning the new object through getbalance. Why?
getwalletinfo, for historical and logical reasons, has the is_min is_spendable balance value in it and I think removing it makes little sense. We already have the same data type in getbalance and to me it would just be logical to switch from a single balance value to the new obect (in both calls).

I agree about depracating / removing getunconfirmedbalance

@jonatack
Copy link
Contributor

left a comment

Concept ACK

Initial manual testing, to be completed later:

test/functional/wallet_balance.py passes locally -> EDIT: all unit + extended functional tests pass locally

getwalletinfo

  "balances": {
    "mine": {
      "trusted": 0.00000000,
      "untrusted_pending": 0.00000000,
      "immature": 0.00000000
    }
  },

help getwalletinfo

getwalletinfo
Returns an object containing various wallet state info.

Result:
{
  "walletname": xxxxx,               (string) the wallet name
  "walletversion": xxxxx,            (numeric) the wallet version
  "balances": {                      (object) the wallet balances in BTC
    "mine": {                        (object) balances from outputs that the wallet can sign
      "trusted": xxx                 (numeric) trusted balance (outputs created by the wallet or confirmed outputs)
      "untrusted_pending": xxx       (numeric) untrusted pending balance (outputs created by others that are in the mempool)
      "immature": xxx                (numeric) balance from immature coinbase outputs
    },
    "watchonly": {                   (object) watchonly balances (not present if wallet does not watch anything)
      "trusted": xxx                 (numeric) trusted balance (outputs created by the wallet or confirmed outputs)
      "untrusted_pending": xxx       (numeric) untrusted pending balance (outputs created by others that are in the mempool)
      "immature": xxx                (numeric) balance from immature coinbase outputs
    },
  },
  "balance": xxxxxxx,                (numeric) Identical to balances.mine.trusted
  "unconfirmed_balance": xxx,        (numeric) Identical to balances.mine.untrusted_pending
  "immature_balance": xxxxxx,        (numeric) Identical to balances.mine.immature
  "txcount": xxxxxxx,                (numeric) the total number of transactions in the wallet
  "keypoololdest": xxxxxx,           (numeric) the timestamp (seconds since Unix epoch) of the oldest pre-generated key in the key pool
  "keypoolsize": xxxx,               (numeric) how many new keys are pre-generated (only counts external keys)
  "keypoolsize_hd_internal": xxxx,   (numeric) how many new keys are pre-generated for internal use (used for change outputs, only appears if the wallet is using this feature, otherwise external keys are used)
  "unlocked_until": ttt,             (numeric) the timestamp in seconds since epoch (midnight Jan 1 1970 GMT) that the wallet is unlocked for transfers, or 0 if the wallet is locked
  "paytxfee": x.xxxx,                (numeric) the transaction fee configuration, set in BTC/kB
  "hdseedid": "<hash160>"            (string, optional) the Hash160 of the HD seed (only present when HD is enabled)
  "private_keys_enabled": true|false (boolean) false if privatekeys are disabled for this wallet (enforced watch-only wallet)
}

Examples:
> bitcoin-cli getwalletinfo 
> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "getwalletinfo", "params": [] }' -H 'content-type: text/plain;' http://127.0.0.1:8332/

getunconfirmedbalance

0.00000000

help getunconfirmedbalance

getunconfirmedbalance
DEPRECATED
Identical to getwalletinfo().balances.mine.untrusted_pending

getunconfirmedbalance true

error code: -1
error message:
getunconfirmedbalance
DEPRECATED
Identical to getwalletinfo().balances.mine.untrusted_pending
@promag
Copy link
Member

left a comment

Concept ACK either way (getbalances or here). I tend to prefer getbalances though.

Show resolved Hide resolved test/functional/wallet_balance.py
@luke-jr

This comment has been minimized.

Copy link
Member

commented May 2, 2019

This seems redundant with JSON-RPC batching?

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

@luke-jr It would be horribly redundant to provide a separate rpc for each balance type, when it comes at no cost to return them all.

@luke-jr

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Perhaps just a getbalances as @jnewbery suggested then?

@promag

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Having a separate RPC makes getwalletinfo constant (after removing deprecated fields).

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

Ok, going to rework to make this a fresh getbalances call (not to be confused with getbalance)

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-rpcWalletBalances branch from fac5d96 to 7529929 May 2, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

commented May 2, 2019

This seems redundant with JSON-RPC batching?

Note that batched RPCs are not atomic (and nor could they ever be, since the RPC server can't take the main lock).

MarcoFalke added some commits May 2, 2019

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-rpcWalletBalances branch from 7529929 to fafb024 May 2, 2019

@promag
Copy link
Member

left a comment

I also think all wallet get- and list- methods should return a best block hash, so it's clear at what point the returned values are valid.

Agree with @jnewbery, it can be useful.

Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
Show resolved Hide resolved src/wallet/rpcwallet.cpp
Show resolved Hide resolved src/wallet/rpcwallet.cpp
@jnewbery

This comment has been minimized.

Copy link
Member

commented May 2, 2019

utACK eeee149

@luke-jr

This comment has been minimized.

Copy link
Member

commented May 2, 2019

PR title/description needs update

Show resolved Hide resolved doc/release-notes.md Outdated

@MarcoFalke MarcoFalke changed the title rpc: Add balances to getwalletinfo rpc: Add balances RPC May 2, 2019

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-rpcWalletBalances branch from eeee149 to facfb41 May 3, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

commented May 3, 2019

utACK facfb41

Only change is adding "DEPRECATED" to the balance descriptions in getwalletinfo and replacing the despicable * with a glorious - in the release notes.

@laanwj

This comment has been minimized.

Copy link
Member

commented May 6, 2019

lightly tested ACK facfb41


- `getbalances` returns an object with all balances (`mine`,
`untrusted_pending` and `immature`). Please refer to the RPC help of
`getbalances` for details. The new RPC is intended to replace

This comment has been minimized.

Copy link
@laanwj

laanwj May 6, 2019

Member

Good call on referring to the RPC help for details. This is what the release notes should do instead of substitute for documentation 👍

@laanwj laanwj merged commit facfb41 into bitcoin:master May 6, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request May 6, 2019

Merge #15930: rpc: Add balances RPC
facfb41 rpc: Deprecate getunconfirmedbalance and getwalletinfo balances (MarcoFalke)
999931c rpc: Add getbalances RPC (MarcoFalke)
fad13e9 rpcwallet: Make helper methods const on CWallet (MarcoFalke)
fad40ec wallet: Use IsValidNumArgs in getwalletinfo rpc (MarcoFalke)

Pull request description:

  This exposes the `CWallet::GetBalance()` struct over RPC.

  In the future, incorrectly named rpcs such as `getunconfirmedbalance` or rpcs redundant to this such as `getbalance` could be removed.

ACKs for commit facfb4:
  jnewbery:
    utACK facfb41

Tree-SHA512: 1f54fedce55df9a8ea82d2b6265354b39a956072621876ebaee2355aac0e23c7b64340c3279502415598c095858529e18b50789be956250aafda1cd3a8d948a5
@jonatack

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

Post-merge tested ACK a3d2d6b

@MarcoFalke MarcoFalke deleted the MarcoFalke:1904-rpcWalletBalances branch May 6, 2019

@promag

This comment has been minimized.

Copy link
Member

commented May 6, 2019

utACK facfb41.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 6, 2019

Merge bitcoin#15930: rpc: Add balances RPC
facfb41 rpc: Deprecate getunconfirmedbalance and getwalletinfo balances (MarcoFalke)
999931c rpc: Add getbalances RPC (MarcoFalke)
fad13e9 rpcwallet: Make helper methods const on CWallet (MarcoFalke)
fad40ec wallet: Use IsValidNumArgs in getwalletinfo rpc (MarcoFalke)

Pull request description:

  This exposes the `CWallet::GetBalance()` struct over RPC.

  In the future, incorrectly named rpcs such as `getunconfirmedbalance` or rpcs redundant to this such as `getbalance` could be removed.

ACKs for commit facfb4:
  jnewbery:
    utACK facfb41

Tree-SHA512: 1f54fedce55df9a8ea82d2b6265354b39a956072621876ebaee2355aac0e23c7b64340c3279502415598c095858529e18b50789be956250aafda1cd3a8d948a5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.