I think a more consistent name is from_sats_checked like I added here: #3703

I think from_sats_checked is related to addressing #3696. This is just a private function created so the code is not repeated for the min/max check in the various other functions. But if the public function is created it could be used for the check as well I guess.

But if we add a check for MIN to from_sats_checked, then we don't need check_min_max.

As a private helper function this looks fine to me. Since its private I'd shorten the docs though. Also checking min first seems a bit cleaner to me

    /// Checks the amount is within the allowed range.
    const fn check_min_max(self) -> Option<SignedAmount> {
        if self.0 < Self::MIN.0 || self.0 > Self::MAX.0 {
            None
        } else {
            Some(self)
        }
    }

If we are going to enforce a lower bound that's not i64::MIN , should that be MIN_MONEY? Not sure since core doesn't.

I have no strong opinion either way on adding a MIN_MONEY or leaving as it is. But lean towards not adding one since there was already discussion of removing MAX_MONEY.

-1 to adding MIN_MONEY

Yeah I'm inclined to agree

I don't think this makes sense to check twice. There's a way to do this where we don't need to check self.0.checked_add(rhs.0) as well as .check_min_max(). right? same for the other operations.

In other words, do a checked add, and if the results exceed MAX_MONEY, then NONE else Some(signed_amount)

I don't understand what you mean by check twice. The first checked_add() will only fail if it overflows, it has nothing to do with our MAX invariant. There is only one check for MIN and MAX, the .check_min_max() private function. I made it this way so that the check for all the public functions are not repeated and are done by one function which returns the Option.

The first checked_add() will only fail if it overflows

Right. This check doesn't need to happen is what I'm saying. If two numbers are added together and the result is less than MAX_MONEY like what you do in check_min_max then you don't also need to check that the two numbers when added together don't overflow. IE less than MAX_MONEY implies not overflow.

But I still need to handle the case where the addition overflows. I can't check if the sum is greater than MAX if adding the numbers overflows, therefore I use i64::checked_add

this could be verified with an assertion added to a test. My point is that checked_add should check that adding two numbers is not greater than MAX_MONEY, then if is not greater than MAX_MONEY you don't need to check that it overflows do you?

This change looks correct to me.

It's not incorrect to check both that it is not greater than MAX_MONEY and not greater than i64::MAX just unnecessary.

\me says a little prayer "please lord give me patience"

same here, if checked_sub is greater than MIN, then return Some else None.

ditto

I had already put in a PR for this, if that commits first you'll need to rebase.

Rebased to remove conflict. I kept it as MAX.0 and not .to_sats() to be consistent with the other changes

This change is unnecessary.

-1 to adding MIN_MONEY

This change is unnecessary.

Perhaps this better expresses what this match is doing

            (true, sat) if sat > SignedAmount::MIN.to_sat().unsigned_abs() => Err(ParseAmountError(
                ParseAmountErrorInner::OutOfRange(OutOfRangeError::too_small()),
            )),

Yes, thanks that is clearer. It did seem odd to check a min against a max even though it's the same thing.

This change looks correct to me.

As a private helper function this looks fine to me. Since its private I'd shorten the docs though. Also checking min first seems a bit cleaner to me

    /// Checks the amount is within the allowed range.
    const fn check_min_max(self) -> Option<SignedAmount> {
        if self.0 < Self::MIN.0 || self.0 > Self::MAX.0 {
            None
        } else {
            Some(self)
        }
    }