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: Accept strings in AmountFromValue #6380

Merged
merged 3 commits into from Jul 27, 2015

Conversation

@laanwj
Copy link
Member

laanwj commented Jul 6, 2015

Accept strings containing decimal values, in addition to bare values.

note: untested

@laanwj laanwj added the RPC/REST/ZMQ label Jul 6, 2015
@jgarzik
Copy link
Contributor

jgarzik commented Jul 6, 2015

Context? Zero explanation of "why"

@laanwj
Copy link
Member Author

laanwj commented Jul 6, 2015

Useful from JSON-RPC implementations where it's not possible to have direct control over the text of numbers (e.g. where numbers are always doubles), and it's still desired to send an exact value.

@laanwj
Copy link
Member Author

laanwj commented Jul 6, 2015

See also #3759 for previous discussion - I hadn't thought any more context was necessary here.

@jgarzik
Copy link
Contributor

jgarzik commented Jul 6, 2015

@laanwj Someone looking at the git commit will have no idea about any of that background context...

Commit messages should answer "why?" not just "what?"

@laanwj
Copy link
Member Author

laanwj commented Jul 6, 2015

Ok, don't feel like this discussion, closing...

@laanwj laanwj closed this Jul 6, 2015
@jonasschnelli
Copy link
Member

jonasschnelli commented Jul 6, 2015

Hmm.. i think this is useful.
For the new wallet i did implement something similar (val.setNumStr(val.get_str()); see -> https://github.com/jonasschnelli/bitcoin/blob/2015/05/corewallet/src/corewallet/corewallet_rpc.cpp#L109).

This would allow users to post JSON content with encoded numbers like {"value": "0.00000001"} instead of "value": {0.00000001} which some php/python encoders wrap into 1e-8.

@laanwj
Copy link
Member Author

laanwj commented Jul 6, 2015

@jonasschnelli Yes, that's exactly the idea.

@laanwj laanwj reopened this Jul 6, 2015
@laanwj laanwj force-pushed the laanwj:2015_07_json_monetary_strings branch Jul 6, 2015
@luke-jr
Copy link
Member

luke-jr commented Jul 7, 2015

As a reminder, the conclusion from #3759 was to use satoshi/integer Numbers, not to stringify...

@laanwj
Copy link
Member Author

laanwj commented Jul 7, 2015

@luke-jr But that would be a major API change, while this could be used today and does not introduce any 1 satoshi versus 1 BTC magnitude risk.

@jonasschnelli
Copy link
Member

jonasschnelli commented Jul 7, 2015

I think using int Satoshis for all values in RPC would make sense. But as @laanwj said, this would be a major API change with many risks. And i also think that people are "thinking" in BTC rather then in Satoshis. Somehow this is a legacy we have to deal with.

Nevertheless this PR would also be a slightly API change. In the current releases (before UniValue) and in the current master, numbers encapsulated in string did made bitcoind throwing an exception.

UniValue internally stores everything as a string, that's why i think this PR makes sense and can be seen as a save solution. The only risk i see is that it could be possible that API users/apps expect that sending a string where a btc value is required should throw an error.

@morcos
Copy link
Member

morcos commented Jul 7, 2015

I like this pull as well. We fixed the JSON parsing in #6379 , but what's to say somebody won't be dealing with a broken JSON composer on their end. This provides a nice safe alternative.

ACK

@jgarzik
Copy link
Contributor

jgarzik commented Jul 7, 2015

ACK. Thanks for updating the commit.

@sipa
Copy link
Member

sipa commented Jul 9, 2015

ACK. We shouldn't forget to mention this in release notes if it gets merged.

@laanwj
Copy link
Member Author

laanwj commented Jul 10, 2015

Yes, and needs to be exercised at least once in the RPC tests as well.

@jonasschnelli
Copy link
Member

jonasschnelli commented Jul 14, 2015

Tested ACK.
RPC test extension for wallet.py : jonasschnelli@39b4437

@laanwj
Copy link
Member Author

laanwj commented Jul 15, 2015

Cherry-picked @jonasschnelli 's test. Thanks!

laanwj and others added 3 commits Jul 6, 2015
Accept strings containing decimal values, in addition to bare values.

Useful from JSON-RPC implementations where it's not possible to have
direct control over the text of numbers (e.g. where numbers are always
doubles), and it's still desired to send an exact value.

This would allow users to post JSON content with numbers encoded like
`{"value": "0.00000001"}` instead of `{"value": 0.00000001}` which some
php/python encoders wrap into 1e-8, or worse.
Add a section "low level RPC API changes" so that the changes with
regard to error codes can be added later.
@laanwj laanwj force-pushed the laanwj:2015_07_json_monetary_strings branch to 9127e97 Jul 27, 2015
@laanwj
Copy link
Member Author

laanwj commented Jul 27, 2015

Added small mention in release notes.

@jonasschnelli
Copy link
Member

jonasschnelli commented Jul 27, 2015

reACK

@laanwj laanwj merged commit 9127e97 into bitcoin:master Jul 27, 2015
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
laanwj added a commit that referenced this pull request Jul 27, 2015
9127e97 doc: Mention RPC strings for monetary amounts in release notes (Wladimir J. van der Laan)
7d226b7 [QA] add testcases for parsing strings as values (Jonas Schnelli)
614601b rpc: Accept strings in AmountFromValue (Wladimir J. van der Laan)
laanwj added a commit to laanwj/bitcoin that referenced this pull request Mar 28, 2016
Avoid an infinite loop in encoding, by ensuring EncodeDecimal
returns a string. round(Decimal) used to convert it to
float, but it no longer does in python 3.x. Strings are
supported since bitcoin#6380, so just use that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.