Rounding mode names

[PaulRBerg]

This issue is about the Rounding enum part of the math library:

openzeppelin-contracts/contracts/utils/math/Math.sol

Lines 10 to 14 in 8d633cb

	enum Rounding {
	Down, // Toward negative infinity
	Up, // Toward infinity
	Zero // Toward zero
	}

I'd like to provide some feedback:

• Given that the math library in OZ exclusively supports unsigned integers, I see no need for both Rounding.Down and Rounding.Zero. They essentially mean the same thing for positive numbers.
• Independent of the first point, the term Rounding.Zero is ambiguous. To understand that it means "toward zero," one must examine the source code, as it could be mistaken for "away from zero."
• The words "down" and "up" can create confusion. As stated by Wikipedia:

For positive x, round-down is equivalent to round-toward-zero, while round-up is equivalent to round-away-from-zero. For negative x, round-down corresponds to round-away-from-zero, and round-up corresponds to round-toward-zero.

Complicating the matter, the ICU has further butchered these terms by defining "down" as "rounded towards the next smaller absolute value" (which Wikipedia refers to as "toward zero"), and "up" as "rounded towards the next greater absolute value" (which Wikipedia refers to as "away from zero"). This can be misleading, as "down" typically implies "toward negative infinity," and "up" implies "toward positive infinity."

To address all of these issues, I recommend adhering to the Intl.NumberFormat V3 standard, which replaces the terms "down" and "up" with "trunc" and "expand", respectively:

enum Rounding {
    Expand, // away from zero
    Trunc // toward zero
}

Or like this (more verbose, but clear):

enum Rounding {
    AwayFromZero,
    TowardZero
}

Or like this (given that only unsigned numbers are supported):

enum Rounding {
    Ceil, // toward positive infinity
    Floor, // toward negative infinity
}

However, I wouldn't go with the last option if you plan on supporting signed numbers in the future.

A good related discussion about this topic can be found here: tc39/proposal-intl-numberformat-v3#7

[ernestognw]

Thanks for the feedback @PaulRBerg, we're reviewing this as part of 5.0. I have a few comments:

• I personally see the Rounding as a handy util on its own. In that sense, even if Rounding.Down is not used in Math.sol, it is used in ERC4626 so I would consider keeping both
• I agree Zero is ambiguous. It's actually not used in any place in the library so I think we can remove it without issues if we decide to

So far I agree the current naming may not be clear and require to see the code to verify. Given that I see Rounding as a util, I'd stick to Ceil and Floor for more general use cases, although let's consider that EIP-4626 also mentions up and down.

[Amxx]

When we added this enum, the objective was to "future-proof" it. This is why we distinguished Zero from Up and Down in case we ever add rounding for signed integer functions.

I personally believe that, we should keep the 3 options, and in the same order, so that we minimize the changes.
That being said, I would be open to reconsidering the naming in a way that is more explicit and remains valid if we add rounding of signed values:

• Down → TowardNegativeInfinity
• Up → TowardInfinity
• Zero → TowardZero

[frangio]

In principle I like the suggestion to follow Intl.NumberFormat V3. This suggestion would be:

• Ceil (toward +∞)
• Floor (toward -∞)
• Trunc (toward 0)

Trunc is the only one that I find really cryptic. I would not remove the Zero/Trunc option, because if we ever do want to use it, it's better to have had it from the beginning to avoid issues from assuming that the other two were the only options.

The alternative I would favor is something like the verbose option, but TowardNegativeInfinity is a little too much. Perhaps:

• TowardNegInfinity
• TowardPosInfinity
• TowardZero

I am fine with both sets of options in this comment.

[ernestognw]

I also find Trunc very confusing, I'll favor:

• Down → TowardNegativeInfinity
• Up → TowardInfinity
• Zero → TowardZero

I also want to bump that we can remove the Zero because it's unused. I'd be fine also with that.

[PaulRBerg]

The Toward options are good, too. I would go with any option other than Up and Down, really.

However, because the two following things have been said:

• Future-proof the rounding mode (potential signed support)
• You agree that Intl.NumberFormatV3 is a good standard to follow

You should also be open to using Expand instead of Ceil, since the former encompasses the signed domain.

[Amxx]

It feels to me that

• TowardNegInfinity
• TowardPosInfinity
• TowardZero

Is the winning option here. Any opposition ?

[frangio]

Starting to realize that it doesn't make sense to try to future-proof for signed numbers. The point that is evident in Intl.NumberFormat V3 is that once you consider signed numbers, 3 rounding modes are not really enough, you need all the 4 rounding modes: ceil (toward +∞), floor (toward -∞), expand (away from 0), trunc (toward zero), depending on what you want to do with the result.

So I now think we should only cater to unsigned numbers, and remove Zero. If we ever need signed numbers we can use a separate enum.

With this consideration I think Ceil and Floor would be clear enough.

[ernestognw]

Considering Zero is removed, I'm more in favor of Ceil and Floor. We just need confirmation from @Amxx.

[Amxx]

Starting to realize that it doesn't make sense to try to future-proof for signed numbers. The point that is evident in Intl.NumberFormat V3 is that once you consider signed numbers, 3 rounding modes are not really enough, you need all the 4 rounding modes: ceil (toward +∞), floor (toward -∞), expand (away from 0), trunc (toward zero), depending on what you want to do with the result.

Then I would add the 4th mode instead of removing zero.

Considering Zero is removed

Please don't consider it remove before I'm able to say anything following frangio's message. I have a right to disagree here.

[Amxx]

Also, if we decide to remove the Zero part, I would strongly insist we make sure that the naming we use for the remaining 2 makes clear sens for both signed and unsigned rounding so that we don't have to change it again if we want to introduce signed math with rounding.

Changing the name again would be a breaking change, I don't want us to commit to not making signed math with rounding in 5.0.