rpc: Accept strings in AmountFromValue #6380

pull laanwj wants to merge 3 commits into bitcoin:master from laanwj:2015_07_json_monetary_strings changing 3 files +57 −22

laanwj commented at 9:45 AM on July 6, 2015: member

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

note: untested
laanwj added the label RPC on Jul 6, 2015
jgarzik commented at 4:21 PM on July 6, 2015: contributor

Context? Zero explanation of "why"
laanwj commented at 4:57 PM on July 6, 2015: member

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 commented at 4:59 PM on July 6, 2015: member

See also #3759 for previous discussion - I hadn't thought any more context was necessary here.
jgarzik commented at 5:16 PM on July 6, 2015: contributor

@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 commented at 6:16 PM on July 6, 2015: member

Ok, don't feel like this discussion, closing...
laanwj closed this on Jul 6, 2015
jonasschnelli commented at 6:42 PM on July 6, 2015: contributor

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 commented at 7:19 PM on July 6, 2015: member

@jonasschnelli Yes, that's exactly the idea.
laanwj reopened this on Jul 6, 2015
laanwj force-pushed on Jul 6, 2015
luke-jr commented at 12:24 AM on July 7, 2015: member

As a reminder, the conclusion from #3759 was to use satoshi/integer Numbers, not to stringify...
laanwj commented at 7:54 AM on July 7, 2015: member

@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 commented at 8:22 AM on July 7, 2015: contributor

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 commented at 2:58 PM on July 7, 2015: member

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 commented at 3:00 PM on July 7, 2015: contributor

ACK. Thanks for updating the commit.
sipa commented at 8:37 PM on July 9, 2015: member

ACK. We shouldn't forget to mention this in release notes if it gets merged.
laanwj commented at 1:29 PM on July 10, 2015: member

Yes, and needs to be exercised at least once in the RPC tests as well.
jonasschnelli commented at 7:23 PM on July 14, 2015: contributor

Tested ACK. RPC test extension for wallet.py : https://github.com/jonasschnelli/bitcoin/commit/39b44374f78891839d1488133e9a2c3a60ad44e7
laanwj commented at 2:07 PM on July 15, 2015: member

Cherry-picked @jonasschnelli 's test. Thanks!

rpc: Accept strings in AmountFromValue

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.

614601be8f

[QA] add testcases for parsing strings as values 7d226b7ca0

doc: Mention RPC strings for monetary amounts in release notes

Add a section "low level RPC API changes" so that the changes with
regard to error codes can be added later.

9127e9766a

laanwj force-pushed on Jul 27, 2015
laanwj commented at 12:05 PM on July 27, 2015: member

Added small mention in release notes.
jonasschnelli commented at 12:06 PM on July 27, 2015: contributor

reACK
laanwj merged this on Jul 27, 2015
laanwj closed this on Jul 27, 2015
laanwj referenced this in commit 240b30eaf0 on Jul 27, 2015
laanwj referenced this in commit ec5241312b on Mar 27, 2016
laanwj referenced this in commit 5efe742695 on Mar 27, 2016
laanwj referenced this in commit 66622020e5 on Mar 27, 2016
laanwj referenced this in commit 28b1ec359c on Mar 28, 2016
laanwj referenced this in commit d7b80b54fb on Mar 28, 2016
makevoid referenced this in commit 8274f21ca2 on Jun 13, 2016
zkbot referenced this in commit 9af55822fb on Feb 15, 2017
zkbot referenced this in commit a7cf698873 on Mar 4, 2017
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#7751 test_framework: python3.4 authproxy compat