Change Amount Debug impl to BTC with 8 decimals

[stevenroose]

I've been working a bit with Amounts and the satoshi notation is really confusing if you're dealing with 1 BTC+ amounts. The Debug trait is meant to as easily as possible give the developer an idea of the amount, so adding a decimal point gives strictly more information. Like this it always shows 8 digits.

This should not be considered a breaking change, IMO. The Debug trait is not intended to be parseable.

[elichai]

Why not use the Display trait if you want BTC amounts?
I personally prefer have the Debug method as close as possible to the actual type in the struct, because this is what you usually need when debugging(which is why I almost always derive it).

Right now: display: 432.43243256 BTC, debug: Amount(43243243256 satoshi)
After the change: display: 432.43243256 BTC, debug: Amount(432.43243256 BTC)

[apoelstra]

I think it's easier to read with the decimal point in place. As Steven says, it's still showing exactly the same data, but with a . stuck in, so there's no loss of information.

[apoelstra]

utack

[elichai]

I think it's easier to read with the decimal point in place. As Steven says, it's still showing exactly the same data, but with a . stuck in, so there's no loss of information.

My only problem with it is that it's a lot more complexity for a Debug impl, ie if there's some bug in fmt_satoshi_in you can't add intermediate println!("{:?}, a); or dbg!(a); because then you'll recursively call the debug impl which will call fmt_satoshi_in etc.

The obvious easy solution would be println!("{:?}", a.0); so if no one else agrees with me then I'm OK with this change

[dr-orlovsky]

BTW, what about renaming Amount into Satoshi? Also will help compatibility when rust-bitcoin will be used with Liquid Confidential & RGB assets.

[apoelstra]

I'd prefer to keep Amount to be analogous to Core's 'CAmount`.

I don't fully understand your comment about RGB or Confidential Assets. We've been using rust-elements with CA for quite a while now and this hasn't been a point of difficulty.

[stevenroose]

@elichai I do understand your concern. It might be tricky to use the (admittedly suspicious) floating point trickery in a Debug impl. Though I think the Debug impl isn't relied on in the Amount unit tests itself. In these unit tests, the line where a failure happens should give you insight in exactly what amount you're dealing with.

Apart from that, I think external users of Amount should assume the implementations are correct, no?

Citing std::fmt::Debug

Debug should format the output in a programmer-facing, debugging context.

That said, I'd be ok with accepting the status quo. I just experienced that satoshi values are often hard to read. Like 100000000 sats and 1000000000 sats are 1 and 10 btc and are really hard to distinguish. While 1.00000000 and 10.00000000 are immediately recognizable.

[dr-orlovsky]

Do not understand why CI has stuck blocking merging. I do not have an option of restarting GitHub actions

[sanket1729]

utACK 6186ee6. This PR was raised before we migrated to actions. So, the CI won't pick it up. A rebase should fixup everything.

[Kixunil]

How about using # to conditionally format it? # usually means "pretty print" so that'd be expected. It could also be dependent on the value if # is used (e.g. less than 0.01 - show in sats).

Like this:

if f.alternate() && self.0 >= 1000000 {
    write!(f, "Amount({:.8} BTC)", self.as_btc())
} else {
    write!(f, "Amount({} satoshi)", self.as_sat())
}

[sgeisler]

I won't pretend this is any cleaner, but if your main concern is the float arithmetic:

impl fmt::Debug for Amount {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        let mut str_repr = self.as_sat().to_string();
        if str_repr.len() > 8 {
            str_repr.insert(str_repr.len() - 8, '.');
        } else {
            let amt_zeros = 8 - str_repr.len();
            str_repr = "0.".chars()
                .chain(std::iter::repeat('0').take(amt_zeros))
                .chain(str_repr.chars())
                .collect::<String>();
        }

        write!(f, "Amount({} BTC)", str_repr)
    }
}

(I actually had to use a similar atrocity for thousands separators, that's where I got the idea from 😄 )

[Kixunil]

Please use math, not allocations:

    write!(f, "Amount({}.", self.as_sat() / 100_000_000);
    let fract = self.as_sat() % 100_000_000;
    let mut mul = 10_000_000;
    while mul > fract && mul > 1 {
        write!(f, "0");
        mul /= 10;
    }
    write!("{} BTC)", fract % 100_000_000);

I'm leaving optimizing dynamic dispatch as an exercise for the reader. :)