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

Don't go through double in AmountFromValue and ValueFromAmount #6239

Merged
merged 3 commits into from Jun 9, 2015

Conversation

Projects
None yet
4 participants
@laanwj
Member

laanwj commented Jun 5, 2015

My prime gripe with JSON spirit was that monetary values still had to be converted from and to floating point which can cause deviations (see #3759 and https://bitcoin.stackexchange.com/questions/22716/bitcoind-sendfrom-round-amount-error).

As UniValue stores internal values as strings, this is no longer necessary. By using fixed-point parsing and formatting already available, this avoids risky double-to-integer and integer-to-double conversions completely, and results in more elegant code to boot.

Also contains a related dead code cleanup commit for FormatMoney.

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

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jun 5, 2015

Contributor

Indeed - that was the intention RE stored as strings - great work @laanwj

"it works" tested ACK

Contributor

jgarzik commented Jun 5, 2015

Indeed - that was the intention RE stored as strings - great work @laanwj

"it works" tested ACK

throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount");
return nAmount;
if (!MoneyRange(amount))

This comment has been minimized.

@laanwj

laanwj Jun 5, 2015

Member

I just noticed that there is a slight change of semantics here. The old check dAmount <= 0.0 || dAmount > 21000000.0 did not allow zero amounts, whereas MoneyRange allows nValue >= 0 && nValue <= MAX_MONEY.

Unsure what to do here. The no-brainer solution would be to restore the old behavior, however

The alternative would be to handle zero-ness at the other call sites, which is feasible as they are few. It would be less surprising to handle specific constraints like this at the call side instead of the parser function. Whatever is decided there should be a test for this in rpc_parse_monetary_values.

@laanwj

laanwj Jun 5, 2015

Member

I just noticed that there is a slight change of semantics here. The old check dAmount <= 0.0 || dAmount > 21000000.0 did not allow zero amounts, whereas MoneyRange allows nValue >= 0 && nValue <= MAX_MONEY.

Unsure what to do here. The no-brainer solution would be to restore the old behavior, however

The alternative would be to handle zero-ness at the other call sites, which is feasible as they are few. It would be less surprising to handle specific constraints like this at the call side instead of the parser function. Whatever is decided there should be a test for this in rpc_parse_monetary_values.

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jun 5, 2015

Just wanted to mention that PaymentServer::verifyAmount() also uses MoneyRange().

Diapolo commented Jun 5, 2015

Just wanted to mention that PaymentServer::verifyAmount() also uses MoneyRange().

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 5, 2015

Member

@Diapolo True, but not relevant here, I'm not intending to change MoneyRange's semantics.

Member

laanwj commented Jun 5, 2015

@Diapolo True, but not relevant here, I'm not intending to change MoneyRange's semantics.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 5, 2015

Member

Nice!
Tested and reviewed ACK.

Member

jonasschnelli commented Jun 5, 2015

Nice!
Tested and reviewed ACK.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 5, 2015

Member

@jonasschnelli What do you think about the range issue mentioned in my comment above?

Member

laanwj commented Jun 5, 2015

@jonasschnelli What do you think about the range issue mentioned in my comment above?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 5, 2015

Member

@laanwj: If we take a closer look at a AmountFromValue, than i think, CAmount can be 0 so AmountFromValue() should also be able to handle 0ness. Even, a single function must not/should not represent the behavior of the whole system (if someone would define our system behavior to reject 0 values everywhere).
AmountFromValue is also used in createrawtransaction and there i could imaging usecases for 0 value in/outputs (to replace later by some hex/further manipulating, etc.).

Therefore ACK this solution (even if it's a slightly API change).
And yes, adding a test for a 0 value would probably be a good idea.

Member

jonasschnelli commented Jun 5, 2015

@laanwj: If we take a closer look at a AmountFromValue, than i think, CAmount can be 0 so AmountFromValue() should also be able to handle 0ness. Even, a single function must not/should not represent the behavior of the whole system (if someone would define our system behavior to reject 0 values everywhere).
AmountFromValue is also used in createrawtransaction and there i could imaging usecases for 0 value in/outputs (to replace later by some hex/further manipulating, etc.).

Therefore ACK this solution (even if it's a slightly API change).
And yes, adding a test for a 0 value would probably be a good idea.

laanwj added some commits Jun 4, 2015

Don't go through double in AmountFromValue and ValueFromAmount
My prime gripe with JSON spirit was that monetary values still had to be
converted from and to floating point which can cause deviations (see #3759
and https://bitcoin.stackexchange.com/questions/22716/bitcoind-sendfrom-round-amount-error).

As UniValue stores internal values as strings, this is no longer
necessary. This avoids risky double-to-integer and integer-to-double
conversions completely, and results in more elegant code to boot.
Get rid of fPlus argument to FormatMoney
It's never used with any other value than false, the default.
Changes necessary now that zero values accepted in AmountFromValue
- Add an accept test for zero amounts, and a reject test for negative
  amounts
- Remove ugly hack in `settxfee` that is no longer necessary
- Do explicit zero checks in wallet RPC functions
- Don't add a check for zero amounts in `createrawtransaction` - this
  could be seen as a feature
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 6, 2015

Member

Thanks. Pushed a commit that adds the checks to caller sites where needed, and adds tests.

Member

laanwj commented Jun 6, 2015

Thanks. Pushed a commit that adds the checks to caller sites where needed, and adds tests.

@laanwj laanwj merged commit 7d8ffac into bitcoin:master Jun 9, 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 9, 2015

Merge pull request #6239
7d8ffac Changes necessary now that zero values accepted in AmountFromValue (Wladimir J. van der Laan)
a04bdef Get rid of fPlus argument to FormatMoney (Wladimir J. van der Laan)
4b4b9a8 Don't go through double in AmountFromValue and ValueFromAmount (Wladimir J. van der Laan)

@str4d str4d referenced this pull request Feb 10, 2017

Merged

Add UniValue as subtree #2082

zkbot added a commit to zcash/zcash that referenced this pull request Feb 10, 2017

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