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

Add Amount type #252

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@stevenroose
Copy link
Collaborator

stevenroose commented Apr 3, 2019

Retake of #192 with a simpler structure for simple satoshi-precision amounts.

@stevenroose stevenroose force-pushed the stevenroose:amount-simple branch 2 times, most recently from eca2175 to cb387f0 Apr 3, 2019

@stevenroose stevenroose force-pushed the stevenroose:amount-simple branch 3 times, most recently from 576975d to 610efd8 Apr 4, 2019

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Apr 4, 2019

Rebased this on top of #250 so I could use serde_json in tests.

@stevenroose stevenroose force-pushed the stevenroose:amount-simple branch from 610efd8 to 7ff1150 Apr 4, 2019

Show resolved Hide resolved src/util/amount.rs Outdated
@sgeisler
Copy link
Member

sgeisler left a comment

This PR feels much cleaner than the previous one. But I wonder if we could save ourselves some pain by declaring parsing and serialization of amounts with suffixes an application responsibility. That would probably avoid the custom parsing and the pressure to acommodate all kinds of weird scaling suffixes (sat, satoshi, $ as some people proposed).

///
/// Note: This only parses the value string. If you want to parse a value with denomination,
/// use `FromStr`.
pub fn parse_denom(mut s: &str, denom: Denomination) -> Result<Amount, ParseAmountError> {

This comment has been minimized.

Copy link
@sgeisler

sgeisler Apr 4, 2019

Member

this feels like it should be fuzz tested against a regex implementation doing the same

This comment has been minimized.

Copy link
@sgeisler

sgeisler Apr 4, 2019

Member

I also wonder if we couldn't use f64 here if we defined the maximum valid range as -21*10^14 sat till 21*10^14 sat. log(21*10^(6+8), 2) ≈ 50.9 and a f64 has 52 mantissa bits, that should be enough to not have any losses imo (but I'd need to test this as well, but with only 2^51 combinations even exhaustive testing seems feasible).

This comment has been minimized.

Copy link
@stevenroose

stevenroose Apr 7, 2019

Author Collaborator

Hmm, I never touched the fuzz tests. I can try to look into this. Could also create random amounts (uniform i64 values) and string and destring for all denominations etc.

This comment has been minimized.

Copy link
@apoelstra

apoelstra Apr 11, 2019

Member

f64 in principle has enough precision to represent any bitcoin amount, yes. (This is probably one reason for Satoshi choosing the "max 21mil with 8 decimal places" limits he did.) But in practice it's hard to write a parser that doesn't do things like dividing by 10 that create rounding errors and result in wrong numbers being parsed anyway.

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

@stevenroose stevenroose referenced this pull request Apr 6, 2019

Open

[WIP] Fix listunspent #35

@stevenroose stevenroose force-pushed the stevenroose:amount-simple branch 2 times, most recently from d7f64e7 to 0e7ba7c Apr 7, 2019

@jonasnick
Copy link

jonasnick left a comment

I'm not familiar with the history of this PR but I've rewritten some code with it and basic functionality like converting denominations and from and to f64's works fine for me.

Travis prints a few unused warnings.

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

@stevenroose stevenroose force-pushed the stevenroose:amount-simple branch 2 times, most recently from 4913e3c to cc8c556 Apr 9, 2019

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Apr 9, 2019

I added checked_* and some other arithmetic methods (one block in the end of impl Amount).

@stevenroose stevenroose force-pushed the stevenroose:amount-simple branch 2 times, most recently from 24ead71 to 162706b Apr 9, 2019

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

@stevenroose stevenroose force-pushed the stevenroose:amount-simple branch from 162706b to 20950bf Apr 11, 2019

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Apr 11, 2019

Ok so I updated the PR to use u64 instead of i64 as discussed on IRC. I also removed float arithmetic, and changed the std::ops implementation to panic on overflow, which I think is a good middle ground between safety and convenience.

The one last thing I'm not fully content with is the from_float_denom implementation. I want to return TooPrecise when the amount has some significant digits, but still avoid failing to convert 1.0 + 3.0 because internally it's 4.00000000000000012 or so. I didn't feel comfortable to introduce arbitrary constants like sats.fract() > 1000.0 * f64::EPSILON.
Suggestions welcome. I think it's valuable to have the method. But in principle I could just implement it as stringing the float and parsing it.

@stevenroose stevenroose force-pushed the stevenroose:amount-simple branch from 20950bf to 7ff0572 Apr 11, 2019

@jonasnick
Copy link

jonasnick left a comment

Thanks for the update. The from_float_denom doesn't seem to work, for example

assert_eq!(f(0.0001234, D::Bitcoin), Err(ParseAmountError::TooPrecise));

should be 12340 satoshi instead.

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

dpc left a comment

Partial. Will finish later.

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

@stevenroose stevenroose force-pushed the stevenroose:amount-simple branch 3 times, most recently from d94494d to 2b30255 Apr 12, 2019

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Apr 12, 2019

I made some updates:

  • rebased off master instead of off #250 (the no-strason PR)
  • removed the IntoBtc trait, which removes the serde_json non-dev dependency
  • from_btc now just takes f64
  • removed denom from all method names, the convention is now (from|to)_*_in
  • from_float_in: replaced direct float interpretation with parsing the to_string impl of f64; anyone willing to come up with a sane way to do f64 -> Amount conversion is very much encouraged to contribute so

@stevenroose stevenroose force-pushed the stevenroose:amount-simple branch from 2b30255 to 3a419d0 Apr 12, 2019

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

This comment has been minimized.

Copy link
Contributor

dpc commented Apr 12, 2019

I see that you removed IntoBtc while I was commenting on it. Pasting it below, though it is probably not important now.

Nit, but "A trait used to" is redundant. Reader sees that it is a trait. :) "Can be converted to ..." is shorter and to the point.

This "into btc" is confusing. Shouldn't this be just "TryIntoAmount"? Also - shouldn't the error be associated type? Why would it always had to be "Parse"? That would imply using for just parseable stuff like strings.

TryInto etc. was just stabilized, but rust-bitcoin won't be able to use it for a while, but we could just copy that trait literally, and in the future impl<T> TryInto<Amount> for T where T: TryIntoAmount, and eventually deprecate TryIntoAmount.

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Apr 12, 2019

Lol meta-discussion. The IntoBtc name meant that the value was supposed to be interpreted as "BTC". A TryIntoAmount for f64 wouldn't tell you how many satoshi's you would get if you used it for 4.2, f.e..

But well, it's removed because it was not super valuable and caused confusion apparently.

@stevenroose stevenroose force-pushed the stevenroose:amount-simple branch from 3a419d0 to 172fb78 Apr 12, 2019

@jonasnick
Copy link

jonasnick left a comment

Can confirm that I can use this module in my code without issues.

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

@stevenroose stevenroose force-pushed the stevenroose:amount-simple branch from 4f6c114 to e7df221 Apr 15, 2019

@sgeisler
Copy link
Member

sgeisler left a comment

Some final, mostly minor, nits. Looks good otherwise.

'.' => 1,
_ => 2,
};
if s.chars().skip(1).map(penalty).sum::<i32>() > 1 {

This comment has been minimized.

Copy link
@sgeisler

sgeisler Apr 15, 2019

Member

minor nit: technically that should be usize, if someone gives you a >1GiB string to parse, which might be possible if no other protocol restrictions apply, that might lead to a panic.

This comment has been minimized.

Copy link
@stevenroose

stevenroose Apr 18, 2019

Author Collaborator

How about an extra sanity length check? u64::max_value is 18446744073709551615 (20 digits). I'm inclined to just error InvalidFormat on anything that has size >50.
But agreed it should be usize.

This comment has been minimized.

Copy link
@sgeisler

sgeisler Apr 18, 2019

Member

Yes, a length check would avoid certain DoS scenarios and would make the code fail early before iterating over all characters.

if s.chars().skip(1).map(penalty).sum::<i32>() > 1 {
return Err(ParseAmountError::InvalidFormat);
}
return Err(ParseAmountError::Negative);

This comment has been minimized.

Copy link
@sgeisler

sgeisler Apr 15, 2019

Member

I understand that useful error messages are nice, but wouldn't it be easier to make - an invalid character if we disallow negative numbers anyway?

This comment has been minimized.

Copy link
@stevenroose

stevenroose Apr 18, 2019

Author Collaborator

@apoelstra Opinion? I could just have a small check that checks if size == 0 or size > 50 and return InvalidFormat and then skip the - check. If then a negative number is encountered from an RPC or some other place, it can be harder to debug. Personally I think a small performance hit in the error case it not a big deal.
But I like the simpler error handling. Less error cases to check for the caller.

Like this:

diff --git a/src/util/amount.rs b/src/util/amount.rs
index ef02413..0cf1d4d 100644
--- a/src/util/amount.rs
+++ b/src/util/amount.rs
@@ -193,23 +193,10 @@ impl Amount {
     /// Note: This only parses the value string.  If you want to parse a value
     /// with denomination, use [FromStr].
     pub fn from_str_in(mut s: &str, denom: Denomination) -> Result<Amount, ParseAmountError> {
-        if s.len() == 0 {
+        if s.len() == 0 || s.len() > 50 {
             return Err(ParseAmountError::InvalidFormat);
         }
 
-        if s.chars().nth(0).unwrap() == '-' {
-            // Quickly check whether the remainder of the string is only numbers and max one point.
-            let penalty = |c| match c {
-                '0'...'9' => 0,
-                '.' => 1,
-                _ => 2,
-            };
-            if s.len() == 1 || s.chars().skip(1).map(penalty).sum::<usize>() > 1 {
-                return Err(ParseAmountError::InvalidFormat);
-            }
-            return Err(ParseAmountError::Negative);
-        }
-
         let max_decimals = {
             // The difference in precision between native (satoshi)
             // and desired denomination.

This comment has been minimized.

Copy link
@sgeisler

sgeisler Apr 18, 2019

Member

Maybe there could be an InvalidCharacter(char) error that tells you what went wrong? That would not only help if an API returns a negative number but e.g. also if it returns a number with a comma instead of a decimal dot.

This comment has been minimized.

Copy link
@apoelstra

apoelstra Apr 20, 2019

Member

Yeah, I'd like InvalidCharacter(char). No strong opinions on also having Negative ... I wouldn't put the effort in to code that, but I don't mind reviewing it if somebody else does ;)

assert!(t.unwrap_err().to_string().contains(&ParseAmountError::Negative.to_string()));
let t: Result<T, serde_json::Error> = serde_json::from_str("{\"amt\": 1000000.000000001}");
assert!(t.unwrap_err().to_string().contains(&ParseAmountError::TooPrecise.to_string()));
//let t: Result<T, serde_json::Error> = serde_json::from_str("{\"amt\": 21000000.000000001}");

This comment has been minimized.

Copy link
@sgeisler

sgeisler Apr 15, 2019

Member

Can this be removed?

This comment has been minimized.

Copy link
@stevenroose

stevenroose Apr 18, 2019

Author Collaborator

Well, it's a test that should work, but the fact that serde forces us to do floating point makes it not work.


/// Checked addition.
/// Returns [None] if overflow occurred.
pub fn checked_add(self, rhs: Amount) -> Option<Amount> {

This comment has been minimized.

Copy link
@sgeisler

sgeisler Apr 15, 2019

Member

The checked_* stuff is great!

This comment has been minimized.

Copy link
@stevenroose

stevenroose Apr 18, 2019

Author Collaborator

I thought it was sad there are no traits for it, only in the num crate. But yeah it's good. We've been using it already internally.

This comment has been minimized.

Copy link
@sgeisler

sgeisler Apr 18, 2019

Member

We could have an optional dependency on num-traits, I'm using it for rust-lightning-invoiceand think it's an acceptable dependency in many cases.

@stevenroose stevenroose force-pushed the stevenroose:amount-simple branch from e7df221 to e7920d7 Apr 18, 2019

stevenroose added some commits Apr 12, 2019

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