Default type behavior for arithmetic

[yancyribbens]

Soliciting some discussion about how the type behavior for arithmetic such as Addition and Subtraction should work. For example, Weight and Amount have different semantics for Addition and Subtraction.

Behold the add trait for Weight:

impl Add for Weight {
    type Output = Weight;

    fn add(self, rhs: Weight) -> Self::Output { Weight(self.0 + rhs.0) }
}

By contrast, the add trait for Amount:

impl ops::Add for Amount {
    type Output = Amount;

    fn add(self, rhs: Amount) -> Self::Output {
        self.checked_add(rhs).expect("Amount addition error")
    }
}

The two behaviors are different; one of them is a checked_add while the other is unchecked (raw?). Is there any good reason why these defaults should be different? To try and focus the discussion, I'm primarily asking:

1. Should the ops type behavior be the same for Weight, Amount and other types?
2. What should that behavior be for release mode and debug mode? unchecked vs checked? monadic or not?

Even though there's an argument for panic in release mode being safer than allowing a possible overflow, does having the possibility of a panic preclude monodic error handling?

Lastly, there is performance implications for checked arithmetic that personally I'd like a way to "opt out of" in release mode for Amount and SignedAmount as talked about here: #2434

[tcharding]

@yancyribbens please be cognizant that this is not a trivial issue to solve and requires quite a lot of brain power for the dev and the reviewers. We are pretty much overstretched at the moment (like all projects) and are working like mad at crate smashing and trying to stabalize. I do think the unit types need work but I think it can/should be done when units crate is released and a bit closer to stable. Thanks for raising the issue though.

[yancyribbens]

Alright, I think I can maintain a branch of units that has the unchecked behavior I need until it can be worked on.

[Kixunil]

I disagree that we should put off this work after units is released. I would like units to be as ready for stabilization as possible and I think it is possible to do it in time. However I wouldn't excessively prolong the release just to get everything perfect.

[apoelstra]

I would like units to be as ready for stabilization as possible

This is not consistent with us having a release out next month.

If we want to iterate until we're perfect before having anything tangible that the public can use then (a) we'll fail at creating a usable API, and (b) we'll burn out before having any API at all.

Maybe you personally think you can clean up the overflow behavior across units in time, in which case I'd be happy to review your code, though I have already had push_decode queued for several weeks. But I think this would be a much simpler thing that I could review quickly.

[Kixunil]

This is not consistent with us having a release out next month.

Why not? Get it as close as possible and if we fail, not a huge deal (but in that case I'd love to have a major release sooner).

If we want to iterate until we're perfect before having anything tangible that the public can use then

Yes but I think we're very close.

[yancyribbens]

I disagree that we should put off this work after units is released. I would like units to be as ready for stabilization as possible and I think it is possible to do it in time. However I wouldn't excessively prolong the release just to get everything perfect.

+1 coin-selection ought to be supported by the API imo

Maybe you personally think you can clean up the overflow behavior across units in time, in which case I'd be happy to review your code, though I have already had push_decode queued for several weeks. But I think this would be a much simpler thing that I could review quickly.

Happy to also pitch in.

[Kixunil]

The reason Amount behaves this way is that the consequences of an overflow in Amount are vastly more disastrous than any other overflow. This seeming inconsistency is warranted.

[apoelstra]

This is fine, but whatever we decide it should be in the README of units.