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

Check that the amount precision isn't more than the size of the amount #309

Merged
merged 1 commit into from Aug 12, 2019

Conversation

@elichai
Copy link
Contributor

commented Aug 6, 2019

This should fix #300
Basically if the precision is longer than the length of the number than it's too precise.

This should also solve another unwanted behavior I thought of,
which is:
if last_n==s.len().
then s will be "".
which will make this loop: for c in s.chars() to never run(chars is size 0).
decimals will be unwrapped to 0, max_decimals is also 0.
so scale_factor is now zero (let scale_factor = max_decimals - decimals.unwrap_or(0);)
so for _ in 0..scale_factor won't run either.

which means the function will return with the default value of value(0)

Edit: Fixes #311 too

Show resolved Hide resolved src/util/amount.rs Outdated
@dpc

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Seems OK to me, but I have little experience with dealing this atrocious idea of using floats for balances. I'm surprised this isn't already phased out.

@elichai

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

Travis is failing for unrelated errors.

@apoelstra

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

@dpc this floating point code is new actually. It can't be phased out because serde-json has no other way to support non-integer numbers such as the ones that Bitcoin Core's RPC outputs.

@apoelstra
Copy link
Member

left a comment

ack, even without fixing the nit

@dpc

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

@apoelstra I meant in Bitcoin Core. My understanding why float number conversions are needed in Bitcoin libraries is because Bitcoin Core uses them for balances in the API, and the reason for that is (maybe) because JS/JSON by default has precision problems in big integers. If you ask me, Bitcoin Core should just start returning integers in satoshis (maybe stringified, if the precision is a problem), side by side old float number balances, deprecate the old ones, and target their removal in some future version.

@elichai

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

@dpc I don't know a lot about the history of the json api but this sounds like a conversation for an issue on bitcoin/bitcoin github

@apoelstra

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@dpc Bitcoin Core is not going to break nearly every wallet-centric RPC for the sake of precision errors in json libraries, which should not be an issue anyway since f64 has enough precision to uniquely specify (by rounding, if necessary) every number from 0 to 21 million with 8 decimal digits.

@elichai elichai force-pushed the elichai:2019-08-amount-precision branch from 44994ed to c7930a7 Aug 8, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Aug 8, 2019

Codecov Report

Merging #309 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #309      +/-   ##
==========================================
+ Coverage   81.72%   81.75%   +0.02%     
==========================================
  Files          38       38              
  Lines        6808     6819      +11     
==========================================
+ Hits         5564     5575      +11     
  Misses       1244     1244
Impacted Files Coverage Δ
src/util/amount.rs 86.79% <100%> (+0.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a1830c...a9e65f3. Read the comment docs.

@apoelstra

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Can you add a unit test for #311

@elichai elichai force-pushed the elichai:2019-08-amount-precision branch from c7930a7 to a9e65f3 Aug 9, 2019

assert_eq!(sp("000 msat"), Err(E::TooPrecise));
assert_eq!(sp("-000 msat"), Err(E::TooPrecise));
assert_eq!(p("0 msat"), Err(E::TooPrecise));
assert_eq!(p("-0 msat"), Err(E::TooPrecise));

This comment has been minimized.

Copy link
@elichai

elichai Aug 9, 2019

Author Contributor

Should this be TooPrecise or Negative?

This comment has been minimized.

Copy link
@stevenroose

stevenroose Aug 12, 2019

Collaborator

Yeah I had this dilemma as well. I decided that if two things are wrong, it doesn't matter which one you report; so one should just return the first one that is detected. In this case the negative check is deferred until later, so it's faster to return the TooPrecise case.

This comment has been minimized.

Copy link
@stevenroose

stevenroose Aug 12, 2019

Collaborator

To make the test more correct one could expect either a Negative or TooPrecise error, so that the tests don't break if the implementation would change the check order.

This comment has been minimized.

Copy link
@apoelstra

apoelstra Aug 12, 2019

Member

Nah, we can fix the test when/if it breaks. It's good to notice changed behavior like this anyway, in case users are depending on it.

@elichai

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Done

@@ -129,6 +129,11 @@ impl error::Error for ParseAmountError {
}
}


fn is_too_precise(s: &str, precision: usize) -> bool {

This comment has been minimized.

Copy link
@dongcarl

dongcarl Aug 9, 2019

Member

Please add a comment here describing this check

This comment has been minimized.

Copy link
@stevenroose

stevenroose Aug 12, 2019

Collaborator

This could even be inlined again IMO, but if taken out as a method, please provide some docs for context. Implementation seems correct.

@stevenroose

This comment has been minimized.

Copy link
Collaborator

commented Aug 12, 2019

Thanks for fixing this!

@apoelstra apoelstra merged commit cc0f114 into rust-bitcoin:master Aug 12, 2019

1 check passed

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

@elichai elichai deleted the elichai:2019-08-amount-precision branch Aug 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.