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 scientific notation for monetary amounts in JSON #6379

Merged
merged 1 commit into from Jul 10, 2015

Conversation

@laanwj
Copy link
Member

laanwj commented Jul 6, 2015

Add a function ParseFixedPoint that parses numbers according to the JSON number specification and returns a 64-bit integer.

Then use this in AmountFromValue, rather than ParseMoney.

Also add lots of tests (thanks to @jonasschnelli for some of them).

Fixes issue #6297.

@laanwj laanwj added the RPC/REST/ZMQ label Jul 6, 2015
@jonasschnelli
Copy link
Member

jonasschnelli commented Jul 6, 2015

Tested ACK (reviewed code/tests, ran tests, successfully ran smartfees.py, some curl scientific notation tests).

jonasschnelli$ curl --user bitcoinrpc:DP6DvqZtqXarpeNWyN3LZTFchCCyCUuHwNF7E8pX99x1 --data-binary '{"jsonrpc": "1.0", "id" : "1", "method": "sendtoaddress", "params": ["myWgdwSvL2z2WxUbc7wKnRnWnbR925YDaH", 1e-4] }' http://127.0.0.1:18332
{"result":"2ebf06579a36aa8a452924a071460de05d5d87ffc9266d35190ebc42e8476bf9","error":null,"id":"1"}

jonasschnelli$ ./src/bitcoin-cli --regtest gettransaction 2ebf06579a36aa8a452924a071460de05d5d87ffc9266d35190ebc42e8476bf9
{
  "amount": 0.00000000,
  "fee": -0.00000226,
  "confirmations": 0,
  "txid": "2ebf06579a36aa8a452924a071460de05d5d87ffc9266d35190ebc42e8476bf9",
  "walletconflicts": [
  ],
  "time": 1436184739,
  "timereceived": 1436184739,
  "details": [
    {
      "account": "",
      "address": "myWgdwSvL2z2WxUbc7wKnRnWnbR925YDaH",
      "category": "send",
      "amount": -0.00010000,
...
@morcos
morcos reviewed Jul 6, 2015
View changes
src/test/rpc_tests.cpp Outdated
BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.01e-6")), COIN/100000000);
BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.0000000000000000000000000000000000000000000000000000000000000000000000000001e+68")), COIN/100000000);
BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("10000000000000000000000000000000000000000000000000000000000000000e-64")), COIN);
BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("10000000000000000000000000000000000000000000000000000000000000000e-64")), COIN);

This comment has been minimized.

Copy link
@morcos

morcos Jul 6, 2015

Member

Isn't this a duplicate of the above line?

This comment has been minimized.

Copy link
@laanwj

laanwj Jul 6, 2015

Author Member

Yes, will remove (or if you have another test to suggest, please do :-).

This comment has been minimized.

Copy link
@morcos

morcos Jul 6, 2015

Member

maybe add some zeroes after the decimal point. you have an excessive number before and and excessive number after, try a test with an excessive number of zeroes both before and after....

This comment has been minimized.

Copy link
@laanwj

laanwj Jul 6, 2015

Author Member

Ok, done

@jgarzik
Copy link
Contributor

jgarzik commented Jul 6, 2015

ut ACK

@morcos
Copy link
Member

morcos commented Jul 6, 2015

ACK (code review and tested, but not exhaustively)

@laanwj laanwj force-pushed the laanwj:2015_07_fixedpoint_iterations branch Jul 6, 2015
@sipa
sipa reviewed Jul 9, 2015
View changes
src/utilstrencodings.cpp Outdated
@@ -538,3 +538,118 @@ int atoi(const std::string& str)
{
return atoi(str.c_str());
}

/** Upper bound for mantissa.
* 10^18-1, largest arbitrary decimal that will fit in 64 bit.

This comment has been minimized.

Copy link
@sipa

sipa Jul 9, 2015

Member

Hypernit: this comment seems inaccurate. There are definitely larger integers that fit in 64 bit.

This comment has been minimized.

Copy link
@laanwj

laanwj Jul 10, 2015

Author Member

it's worded somewhat ackwardly: my point is that larger 64-bit signed integers cannot consist of arbitrary combinations of 0-9:

 999999999999999999   1^18-1
9223372036854775807  (1<<63)-1 (max)
9999999999999999999  would not fit

Wouldn't be impossible to handle it, but I it would complicate bounds checking and don't see the point in our case.
I'll update the comment.

@sipa
Copy link
Member

sipa commented Jul 9, 2015

ACK. Carefully reviewed the code, but didn't test. The included test cases are convincing.

Add a function `ParseFixedPoint` that parses numbers according
to the JSON number specification and returns a 64-bit integer.

Then this in `AmountFromValue`, rather than `ParseMoney`.

Also add lots of tests (thanks to @jonasschnelli for some of them).

Fixes issue #6297.
@laanwj laanwj force-pushed the laanwj:2015_07_fixedpoint_iterations branch to 9cc9152 Jul 10, 2015
@laanwj laanwj merged commit 9cc9152 into bitcoin:master Jul 10, 2015
laanwj added a commit that referenced this pull request Jul 10, 2015
9cc9152 rpc: Accept scientific notation for monetary amounts in JSON (Wladimir J. van der Laan)
zkbot added a commit to zcash/zcash that referenced this pull request Feb 10, 2017
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Jul 6, 2020
830eadf Solving rpc_fundrawtransaction.py amount rounding issue. (furszy)
6cf9778 ATMP: Do not try to get the inputs in zc tx. (furszy)
38efbb9 sync_blocks: Increase debugging to hunt down mempool_reorg intermittent failure (furszy)
dd7855c sync_blocks: check that peer is connected when calling sync_*. (furszy)
bc7d542 functional tests, sync_blocks only cleanup. (furszy)
1ae04e7 Fix mempool package tracking edge case (Suhas Daftuar)
f4052aa Add test showing bug in mempool packages (furszy)
691749f rpc: Accept scientific notation for monetary amounts in JSON (furszy)
b0f9051 Fix removeForReorg to use MedianTimePast (Suhas Daftuar)
92583c6 Don't call removeForReorg if DisconnectTip fails (Suhas Daftuar)
bb77808 Track coinbase spends in CTxMemPoolEntry (furszy)
f607f18 Change GetPriority calculation. (furszy)
ac3c0f1 Remove default arguments for CTxMemPoolEntry() (furszy)
07eae9f Modify variable names for entry height and priority (furszy)
4356b31 Fix bug in mempool_tests unit test (Alex Morcos)
f35ebe3 removeForReorg calls once-per-disconnect-> once-per-reorg (furszy)
7f5737f Fix comment in removeForReorg (furszy)
8ad82ca Fix removal of time-locked transactions during reorg (furszy)
de74ab3 Move ProcessBlockFound reserveKey to optional. (furszy)

Pull request description:

  More back ports and adaptations over the RPC values parsing and mempool. We need to get closer in this area :) .

  * bitcoin#6379
  * bitcoin#6715
  * bitcoin#6915
  * bitcoin#7007
  * bitcoin#7008
  * bitcoin#12643
  * bitcoin#18474
  * bitcoin#18704

ACKs for top commit:
  Fuzzbawls:
    ACK 830eadf
  random-zebra:
    ACK 830eadf and merging...

Tree-SHA512: 014b31008aaf09ebf838e21da59379a45565df440a77d66a0cd53e824a6d69673d6975d08243a43fb5a7ebd1a35c07d1d07412a87216b42e9d6f17a1c0bc5708
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

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