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

[RFC] Add Amount type #192

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@stevenroose
Copy link
Collaborator

stevenroose commented Nov 10, 2018

The Amount type is generic to the minimum precision it supports,
defaulting to Satoshi.

List of things to consider:

  • currently FromStr parses a bitcoin-denominated value
    • alternatively, we could make it parse a value and a denomination suffix, which would make it compatible with Display
  • serde deserialization just deserialized the inner type; I personally don't like this (taken from serde), but not sure what to do. should we just pass to FromStr?

The diff to support i128 as Inner and the max ZeptoSatoshi precision behind a feature is very small:
stevenroose@b823519

@stevenroose stevenroose changed the title Add Amount type [RFC] Add Amount type Nov 10, 2018

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
}
}

impl<P: Precision> ops::Add for Amount<P> {

This comment was marked as resolved.

Copy link
@sgeisler

sgeisler Nov 10, 2018

Member

A stretch goal would be implementing the operators (or at least some form of checked_*) for Lhs and Rhs with different Ps.

This comment was marked as resolved.

Copy link
@stevenroose

stevenroose Nov 10, 2018

Author Collaborator

I don't believe there is any value in doing things cross-P, tbh. A whole project should always use the same P and if they get external values in other P's they should rescale as soon as they can.

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Nov 10, 2018

One thing that troubles me is that the Precision trait also is used as a Denomination type implicitly. Not sure if we should name it differently.

@stevenroose stevenroose force-pushed the stevenroose:amount branch 2 times, most recently from 2013c9a to 258dd4b Nov 10, 2018

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

impl<'a, F: Precision, T: Precision> IntoAmount<F, T> for &'a f64 {

This comment was marked as outdated.

Copy link
@sgeisler

sgeisler Nov 10, 2018

Member

Why is this also implemented for &f64 and not only for f64? Since f64 is Copy that shouldn't be much of a problem. I think auto deref should take care of making the appropriate copies.

This comment was marked as outdated.

Copy link
@stevenroose

stevenroose Nov 10, 2018

Author Collaborator

I just copied this from rust-amount. I figured if it existed there, might have been a reason for it..

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
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

@stevenroose stevenroose force-pushed the stevenroose:amount branch from e32616d to 0293cda Nov 10, 2018

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Nov 10, 2018

Not sure if we can make max_value and min_value const/static for them to be more efficient.

@sgeisler

This comment has been minimized.

Copy link
Member

sgeisler commented Nov 10, 2018

Iirc rust 1.14.0 doesn't allow associated constants unfortunately and static doesn't seem to be the right modifier imo.

@stevenroose stevenroose force-pushed the stevenroose:amount branch from 8b071c9 to 7349642 Nov 10, 2018

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Nov 10, 2018

I'm not 100% convinced we should have FromStr implemented the way it is not, because we fix it to meaning "BTC". We could parse the denominated string that Display outputs, so that it roundtrips..

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 branch 2 times, most recently from 620799e to eafb79a Nov 11, 2018

@sgeisler

This comment has been minimized.

Copy link
Member

sgeisler commented Nov 11, 2018

Changing the max precision via features like stevenroose/rust-bitcoin@b823519 might have unwanted consequences for interoperability between crates using rust-bitcoin with different features (How would Amount i64 <-> Amount i128 conversion work? We don't have both amount versions in one crate so we can't really implement this.)

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Nov 11, 2018

There are conversion methods, but I'm not familiar enough with features to know what might happen. If a dependent crate returns an Amount<Satoshi> and I use Amount<PicoSatoshi> in my own crate, I can use .into_rescaled() to rescale to the new precision. But I'm not entirely sure if that would work if the crates use different Inner types internally. I can try to set something up.

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Nov 16, 2018

Ok I'm having second thoughts about the serde implementations. I don't think we should pick a default serialization or deserialization. I will create a submodule serde with two submodules as_btc and as_satoshi that can be used with #[serde(with = "bitcoin::amount::serde::as_btc")].

@stevenroose stevenroose force-pushed the stevenroose:amount branch from a4fd0f6 to 91ba2d0 Nov 16, 2018

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Nov 16, 2018

Ok so after that change, the only things we decide for the user that he can't change is that the fmt::Display trait picks Bitcoin by default. Since there is to_string_in, I guess that's fine and users should not rely on stability of Display too much in this case.

Also, I'm awaiting some feedback on rust-lang/rust#56008 to see what to do with the Copy error it produces now.

@stevenroose stevenroose force-pushed the stevenroose:amount branch 2 times, most recently from 7bb79d4 to 25660e0 Nov 16, 2018

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

This comment has been minimized.

Copy link
Member

TheBlueMatt commented Nov 21, 2018

Why go down the route of making the underlying storage generic? No one wants to divide an Amount, so could easily just always store in msat and make the access API safe. Also, why not only make the type either Satoshi (for normal things) and MilliSatoshi (for lightning) and provide a way to get it written as a string in different ways but always have satoshi precision, in other words, is there really a use-case for a BTC-precision Amount?

@sgeisler

This comment has been minimized.

Copy link
Member

sgeisler commented Nov 21, 2018

@TheBlueMatt IIRC it all started with me proposing an extension to the Amount type developed by @jeandudey. I proposed to parametrize the precision using a type parameter to be able to represent pico BTC amounts for lightning while still being able to represent 21M BTC and not needing more memory in case sub-satoshi amounts aren't needed (log2(21*10**(6+12)) = 64.187). This type parameter could either be a ZST or contain an integer to represent the sub-sat amount.

I'd have to look up what the BOLTs say about the smallest possible amount, but right now I base everything on pBTC in lightning-invoice. If msat is sufficient we could remove all that generic stuff and just provide two accessors: sat(&self) giving you the amount in satoshis rounded down and msat(&self) giving you the sub-satoshi amount (a bit like Duration does it).

There is no BTC precision anymore. Just a denomination enum in case you want to format something as whole BTC only.

@TheBlueMatt

This comment has been minimized.

Copy link
Member

TheBlueMatt commented Nov 21, 2018

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Nov 22, 2018

@TheBlueMatt

This comment has been minimized.

Copy link
Member

TheBlueMatt commented Nov 22, 2018

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Nov 22, 2018

@sgeisler

This comment has been minimized.

Copy link
Member

sgeisler commented Nov 22, 2018

I suppose it doesn’t really make sense to hold msat as dividing by 1000 every time you want to do anything under the hood is needlessly inefficient

Although rust doesn't support bit fields we could define bit 0..52 as amount in sat and bits 52..62 as the precision extension in msat. In that way we only need to do an AND operation with a fixed bitmask to get the amount in sat and only an AND and SHIFT to get the msat. Both operations should be much cheaper than division.

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Nov 22, 2018

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Nov 22, 2018

@sgeisler

This comment has been minimized.

Copy link
Member

sgeisler commented Nov 22, 2018

Wait would you be able to add those raw and just check for overflow of the lower precision part and then add one sat?

That's genius! At first I was skeptical because we need to do bounds checks for the sat amount, but since we know the result of any addition of two amounts can be two times Amount::MAX at most we can leave one bit free between sat and msat to check for overflows of sat. I think we have three bits left, so we can spare the two overflow padding bits for sat and msat.

EDIT: just to be clear, we still need to look at the whole sat/msat range plus the overflow bit imo. Just looking at the overflow bit won't work.

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Nov 22, 2018

@sgeisler

This comment has been minimized.

Copy link
Member

sgeisler commented Nov 22, 2018

Also, how would be indicate that a user doesn't want msat?

Why exactly do we need to know this? Sure, we could save some CPU cycles here and there, but I don't know how relevant that is. If we wanted to do this I'd say do it at compile time by keeping our Precision argument.

Somehow I have the feeling we should have specified what we actually want to achieve, I have so many conflicting design goals in my head by now that it becomes really hard to come up with consistent ideas.

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Nov 22, 2018

I think my own main argument is that (even though I haven't implement them (yet?)), I see value in operations like Amount / i64 -> Amount or equivalent Amount * f64 -> Amount. And to have those, it should be impossible for a regular satoshi-only user to accidentally get sub-satoshi amounts that can end up giving incorrect results if multiplied again.

@TheBlueMatt

This comment has been minimized.

Copy link
Member

TheBlueMatt commented Nov 22, 2018

Wait would you be able to add those raw and just check for overflow of the lower precision part and then add one sat?

Still not efficiently, though. You'd have to shift + add the two together any time you want to use msat and shift any time you want sat, you'd just make the conversion itself free. The interface would look similar since you still have a variant that rounds to satoshis and one that rounds to msat, so might as well just make the general case efficient and take the mul/div by 1000 hit only on conversion between types of Amount.

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Nov 23, 2018

Yeah for the msat case, to get the i64 in msat, you'd need a binary shift, decimal shift and add, which is quite bad.

@stevenroose stevenroose force-pushed the stevenroose:amount branch 2 times, most recently from 5bf5292 to 7d4e285 Nov 23, 2018

@sgeisler

This comment has been minimized.

Copy link
Member

sgeisler commented Nov 24, 2018

Thinking more about our changed goals (known minimum amount instead of infinite extensibility) the easiest way would be to drop the type parameter completely and just have two amount types implementing an amount trait like this:

trait Amount {
  /// Amount in sat rounded down if more precise
  fn sat(&self) -> u64;
  /// Amount in milli sat
  fn msat(&self) -> u64;
}

struct ChainAmount(u64);

impl Amount for ChainAmount {
  fn sat(&self) -> u64 {
    self.0
  }

  fn msat(&self) -> u64 {
    self.0 * 1000
  }
}

struct LightningAmount(u64);

impl Amount for LightningAmount {
  fn sat(&self) -> u64 {
    self.0 / 1000
  }

  fn msat(&self) -> u64 {
    self.0
  }
}

impl From<ChainAmount> for LightningAmount { ... }
impl From<LightningAmount> for ChainAmount { ... }

As long as we state our rounding behavior this should be safe to use and if someone really cares about the rounding they can always get the msat amount and do it themselves.

I'm sorry for all that back and forth @stevenroose , if you want I can implement that proposal myself tomorrow.

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Nov 24, 2018

@sgeisler

This comment has been minimized.

Copy link
Member

sgeisler commented Nov 24, 2018

What's wrong with the current impl?

Nothing. I'm just thinking about the trade-off between complexity, redundancy and functionality. The current design avoids redundancy in trait impls, which is nice. But it also needs special casing based on it's type parameter. I'll think a bit more about it.

You are gonna have amounts in structs everywhere and then you have to either pick one or make everything generic.

But if you want to have different precision options for an Amount field in a struct then you need to make it generic over the precision you want to use. Making it generic over the amount type just removed a layer of indirection. The only way to avoid this would be to box the precision struct but that doesn't make any sense (would be equal to just including an enum that tells us what how to interpret the u64). And it should be clear where to use which precision anyway (transactions will always use sat, LN will always use msat)

Indont understand your goal "know minimum amount".

It's not a goal, but an observation which removes one "moving part" and makes the design easier [1].
As @TheBlueMatt pointed out we won't need anything more precise than msat (unless there is a major change in BTC or LN). Thus we actually know the whole range of precisions to support and don't need to make it user extensible at all. rust-bitcoin can use the ChainAmount type for storage and rust-lightning can use the LightningAmount for storage. Both can opt for accepting all amount types in function calls by making these functions generic.

[1] My initial reason for using a type parameter for the precision was to be able to put as much memory in there as needed for precisions without a known bound. But even back then it would have probably been better to use multiple types.

@stevenroose stevenroose force-pushed the stevenroose:amount branch from 7d4e285 to ba551ee Nov 30, 2018

Add Amount type
The Amount type is generic to the minimum precision it supports,
defaulting to Satoshi.

@stevenroose stevenroose force-pushed the stevenroose:amount branch from ba551ee to 0b4fa0a Dec 2, 2018

@stevenroose

This comment has been minimized.

Copy link
Collaborator Author

stevenroose commented Feb 14, 2019

Is there any existing benchmark that can be used to test the performance implications of this addition?

@stevenroose stevenroose referenced this pull request Apr 3, 2019

Open

Add Amount type #252

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.