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

Add `paytxfee` and `errors` JSON fields where appropriate #6257

Merged
merged 1 commit into from Jun 15, 2015

Conversation

Projects
None yet
3 participants
@scmorse
Contributor

scmorse commented Jun 8, 2015

Added fields that were missing from a few of the basic info rpc requests.

getwalletinfo

  • paytxfee

getnetworkinfo

  • warnings
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 9, 2015

Member

utACK for the paytxfee in getwalletinfo.

Tend to NACK the errors extending. The errors are mostly warnings and i think it won't make sense to add them to serval rpc commands.
I think a good example of how the errors field could get handled is the signrawtransaction rpc call.
The getinfo rpc call is not e good example IMO.

Member

jonasschnelli commented Jun 9, 2015

utACK for the paytxfee in getwalletinfo.

Tend to NACK the errors extending. The errors are mostly warnings and i think it won't make sense to add them to serval rpc commands.
I think a good example of how the errors field could get handled is the signrawtransaction rpc call.
The getinfo rpc call is not e good example IMO.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 9, 2015

Member

@jonasschnelli I agree that errors shouldn't be added to multiple RPC calls. It does belong on one other RPC call though as otherwise it would disappear when the deprecated getinfo is removed.
(same for version, and protocolversion)

Edit: protocolversion is already on getnetworkinfo, I forgot.

Member

laanwj commented Jun 9, 2015

@jonasschnelli I agree that errors shouldn't be added to multiple RPC calls. It does belong on one other RPC call though as otherwise it would disappear when the deprecated getinfo is removed.
(same for version, and protocolversion)

Edit: protocolversion is already on getnetworkinfo, I forgot.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 9, 2015

Member

It just came to my mind that there is also the possibility of adding general information "one level above JSON" and that would mean add a custom HTTP response header (X-Bitcoin-Version, etc.). But not sure if this is a good idea.

Member

jonasschnelli commented Jun 9, 2015

It just came to my mind that there is also the possibility of adding general information "one level above JSON" and that would mean add a custom HTTP response header (X-Bitcoin-Version, etc.). But not sure if this is a good idea.

@laanwj laanwj added the RPC/REST/ZMQ label Jun 9, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 9, 2015

Member

@jonasschnelli Sounds like a good idea, but it may be harder to get at in some client implementations. It doesn't exclude including the version on some RPC call.

Member

laanwj commented Jun 9, 2015

@jonasschnelli Sounds like a good idea, but it may be harder to get at in some client implementations. It doesn't exclude including the version on some RPC call.

@scmorse

This comment has been minimized.

Show comment
Hide comment
@scmorse

scmorse Jun 9, 2015

Contributor

@jonasschnelli which one RPC call do you think the errors flag from getinfo should be included in, then? Maybe errors isn't the best name, could be alert or alerts?

Contributor

scmorse commented Jun 9, 2015

@jonasschnelli which one RPC call do you think the errors flag from getinfo should be included in, then? Maybe errors isn't the best name, could be alert or alerts?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 9, 2015

Member

@scmorse: at the moment, the general error states (actually mostly warnings) can be loaded over the getinfo() call (which is okay IMO). It should be named "warnings", but changing this would break the API (which should be avoided).

My recommendation would be to just add the paytxfee field as you did and remove all the "error" changes. Or do you see a benefit in having the error object in getblockchaininfo and getnetworkinfo?

Member

jonasschnelli commented Jun 9, 2015

@scmorse: at the moment, the general error states (actually mostly warnings) can be loaded over the getinfo() call (which is okay IMO). It should be named "warnings", but changing this would break the API (which should be avoided).

My recommendation would be to just add the paytxfee field as you did and remove all the "error" changes. Or do you see a benefit in having the error object in getblockchaininfo and getnetworkinfo?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 10, 2015

Member

Let's put it in getnetworkinfo. Nearly all the alerts come from the network or at least signal network conditions.

Member

laanwj commented Jun 10, 2015

Let's put it in getnetworkinfo. Nearly all the alerts come from the network or at least signal network conditions.

@scmorse

This comment has been minimized.

Show comment
Hide comment
@scmorse

scmorse Jun 11, 2015

Contributor

@laanwj @jonasschnelli Updated getnetworkinfo to have a "warnings" field and getwalletinfo to include the "paytxfee".

Contributor

scmorse commented Jun 11, 2015

@laanwj @jonasschnelli Updated getnetworkinfo to have a "warnings" field and getwalletinfo to include the "paytxfee".

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 11, 2015

Member

utACK.

Something for another PR would be to rename the "statusbar" warnings category to just "status" or something to avoid that one could think this is for UI only. But really really a minor low prio thing.

Member

jonasschnelli commented Jun 11, 2015

utACK.

Something for another PR would be to rename the "statusbar" warnings category to just "status" or something to avoid that one could think this is for UI only. But really really a minor low prio thing.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 12, 2015

Member

utACK.

I'd say don't bother renaming the category in this pull. I agree it's named wrongly and it's weird to pass in a string there anyway (should be an enum). Also the other category "rpc" is never used except to detect "safe mode" in OnRPCPreCommand by checking if the string is empty, which is also quite strange. It's very weird code so renaming the category wouldn't do it justice :)

Member

laanwj commented Jun 12, 2015

utACK.

I'd say don't bother renaming the category in this pull. I agree it's named wrongly and it's weird to pass in a string there anyway (should be an enum). Also the other category "rpc" is never used except to detect "safe mode" in OnRPCPreCommand by checking if the string is empty, which is also quite strange. It's very weird code so renaming the category wouldn't do it justice :)

@laanwj laanwj merged commit ef2a3de into bitcoin:master Jun 15, 2015

1 check passed

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

laanwj added a commit that referenced this pull request Jun 15, 2015

Merge pull request #6257
ef2a3de Add paytxfee to getwalletinfo, warnings to getnetworkinfo (Stephen)

@str4d str4d referenced this pull request Feb 14, 2017

Merged

Bitcoin 0.12 RPC PRs 1 #2100

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment