Add methods to UInt128 for explicit overflow control #853

yihuang · 2021-03-29T02:04:34Z

Fixes #850

webmaster128

Nice start. Could you add tests for the newly added functions?

I think the best way to handle the operator implementation is deperate it and ask users to use the explicit methods.

webmaster128 · 2021-03-29T09:38:50Z

packages/std/src/errors/std_error.rs

+        operand2: String,
+        #[cfg(feature = "backtraces")]
+        backtrace: Backtrace,
+    },


We should generalize this and remove the term "underflow" because:

Storing values that are too low in an integer variable (e.g., attempting to store −1 in an unsigned integer) is properly referred to as integer overflow, or more broadly, integer wraparound. The term underflow normally refers to floating point numbers only, which is a separate issue. It is not possible in most floating-point designs to store a too-low value, as usually they are signed and have a negative infinity value.

https://en.wikipedia.org/wiki/Arithmetic_underflow

done ;D only OverflowError and DivideByZeroError now.

webmaster128 · 2021-03-29T09:44:40Z

packages/std/src/math.rs

+        self.0
+            .checked_add(other.0)
+            .map(Self)
+            .ok_or_else(|| StdError::overflow(self, other))


Can you create a specific

struct Uint128Overflow { // all the error information we need }

and implement From<Uint128Overflow> for StdError?

This would make clear there is only one error type that can happen (as expressed by the Option API from Rust) but at the same time make it easy for contract developers to just use ?.

yihuang · 2021-03-30T08:57:31Z

Nice start. Could you add tests for the newly added functions?

I think the best way to handle the operator implementation is deperate it and ask users to use the explicit methods.

added tests
change the Add operator to return Result.

ethanfrey

Nice to add these extra helpers.
I would just keep the default behavior of Add and Sum as they were.

ethanfrey · 2021-03-30T12:34:09Z

packages/std/src/math.rs

@@ -180,6 +180,80 @@ impl Uint128 {
    pub fn is_zero(&self) -> bool {
        self.0 == 0
    }
+
+    pub fn checked_add(self, other: Self) -> Result<Self, OverflowError> {


Nice to see these all added here

ethanfrey · 2021-03-30T12:37:53Z

packages/std/src/math.rs

-        self.0 += rhs.u128();
+    fn add(self, rhs: &'a Uint128) -> Self::Output {
+        let (op1, op2) = (self.u128(), rhs.u128());
+        op1.checked_add(op2)


Why change the behavior here?

There is the checked_add operator (and more), I would keep the same public API of + (even if it is something like self.checked_add(rhs).unwrap() to make "panic on overflow" explicit)

Also, why calling checked_add on the .u128() and not the Uint128 helper method.

But why Sub is "checked" by default, and Add is panic by default?

Because Sub will error in many normal cases (sending 13 tokens when I have 5), where Add will only error in some extreme case (entire ETH supply in wei can safely be added far from the Uint128 overflow)

The initial motivation for this change is for the use case here(https://github.com/yihuang/cosmwasm-plus/blob/cw1155-impl/contracts/cw1155-base/src/contract.rs#L110).
When the from is None(which means mint new tokens), the user inputted amount get passed to the + operator directly, I thought it's a security vulnerability at first. If we always make sure it panic at overflow, then it's not a security vulnerability, but still I find it a little bit confused why it's different from the Sub operator.

Let's keep the panic for add by default. This the behaviour of all Rust types as well and the only reason why Uint128 exists is JSON serialization/deserialization of u128. It should be almost identical to u128.

To me the behaviour of impl ops::Sub<Uint128> for Uint128 is what should get attention. Ideally we make it panic. However, I am a bit scared to just change the behaviour. This is why I suggested deprecating it.

ok, I'll revert the Add change.

Thanks!

Do you want to put deprecated attribute on both Add and Sub? there's maybe also legit use case for that too? How about keep it as it for now?

What about this: impl ops::Sub<Uint128> for Uint128 gets the following deprecate note: "The behaviour of the minus operator for Uint128 is likely to change in the future. Please consider using Uint128::checked_sub, Uint128::wrapping_sub or Uint128::saturating_sub."

When the from is None(which means mint new tokens), the user inputted amount get passed to the + operator directly, I thought it's a security vulnerability at first. If we always make sure it panic at overflow, then it's not a security vulnerability, but still I find it a little bit confused why it's different from the Sub operator.

Good point. And relying on compiler flags for such security critical behavior is sub-optimal.

I think a reimplementation using checked_add internally, but then panic on error (.unwrap()) will have the same external behavior but be very explicit and stable about how it handles overflow.

I think a reimplementation using checked_add internally, but then panic on error (.unwrap()) will have the same external behavior but be very explicit and stable about how it handles overflow.

Sounds good to me.

The behaviour of the minus operator for Uint128 is likely to change in the future. Please consider using Uint128::checked_sub, Uint128::wrapping_sub or Uint128::saturating_sub.

it seems unable to add deprecated annotation to trait impl: rust-lang/rust#39935

We could also remove the Sub trait for now and just force everyone to checked_sub.
v0.14.0 will have plenty of breaking changes, I'd rather do this sooner than later if we want to do it.

ethanfrey · 2021-03-30T12:39:30Z

packages/std/src/math.rs

-impl Sum<Uint128> for Uint128 {
-    fn sum<I: Iterator<Item = Uint128>>(iter: I) -> Self {
-        iter.fold(Uint128::zero(), ops::Add::add)
+impl Sum<Uint128> for Result<Uint128, OverflowError> {


Also seems more unwieldy to me. I agree with allowing opt-in, but why as default?
It makes the whole API more complex and doesn't give more security in this extreme edge case (contract panic will halt), just a better error message.

It makes the interface consistent with Sub, also make user aware of the overflow error case.

I see it as a case that is never hit in normal usage and impedes the usage experience for normal user.

If I am doing math for a bonding curve and squaring amounts and adding them, it is very important to use checked_add, and great to expose them as you do in this PR. But in this case, I assume the developer will use the methods not + (or quickly realize that on the first tests that overflows and panics)

API changes are @webmaster128 judgement, I just find it to be a bit more annoying to enforce checked add constantly. But if you both prefer it, I will use it.

restructure the error branches in StdError

yihuang · 2021-03-31T02:17:22Z

All suggestions are fixed, other than deprecating Sub operator, which seems don't work: rust-lang/rust#39935

ethanfrey

Looks good to me.
Only open question seems to be whether to remove Sub or leave it. I'm fine with both, but lean towards removing it.

I'll let Simon make the final call and merge this.

webmaster128 · 2021-03-31T15:53:02Z

Thank you.

Regarding

All suggestions are fixed, other than deprecating Sub operator, which seems don't work: rust-lang/rust#39935

and

Only open question seems to be whether to remove Sub or leave it. I'm fine with both, but lean towards removing it.

Let's remove the implementation. It is better to break the code from compiling than changing the implementation. Then we can wait a couple of releases and re-introduce a panicking - operator.

yihuang · 2021-04-01T07:04:45Z

Let's remove the implementation. It is better to break the code from compiling than changing the implementation. Then we can wait a couple of releases and re-introduce a panicking - operator.

Done

webmaster128 · 2021-04-01T08:15:11Z

packages/std/src/errors/std_error.rs

+    }
+}
+
+#[derive(Error, Debug, PartialEq, Eq)]


Ah, I just see this: This derive makes two error instances unequal if they have different backtraces. For StdError we avoided that using the manual PartialEq implementation.

But no worries, I'll fix this now.

webmaster128 · 2021-04-01T08:34:18Z

I pushed a CHANGELOG entry for this PR to main: fd4f9f4

Thank you!

yihuang changed the title ~~Add methods to Uint128 for explicit overflow control~~ Add methods to UInt128 for explicit overflow control Mar 29, 2021

yihuang force-pushed the fix-overflow branch from 5ae4707 to 2f209f6 Compare March 29, 2021 02:19

webmaster128 reviewed Mar 29, 2021

View reviewed changes

yihuang force-pushed the fix-overflow branch 3 times, most recently from 53df44b to 0a99ba2 Compare March 30, 2021 08:56

yihuang force-pushed the fix-overflow branch from 03950f1 to 2d88c6b Compare March 30, 2021 09:41

ethanfrey reviewed Mar 30, 2021

View reviewed changes

yihuang added 2 commits March 31, 2021 10:04

add methods for explicit overflow control

e005f15

restructure the error branches in StdError

force panic in Add operator

6f81c24

yihuang force-pushed the fix-overflow branch from 2d88c6b to 6f81c24 Compare March 31, 2021 02:15

ethanfrey approved these changes Mar 31, 2021

View reviewed changes

Remove Sub operator

b441203

webmaster128 approved these changes Apr 1, 2021

View reviewed changes

webmaster128 merged commit 53a7833 into CosmWasm:main Apr 1, 2021

webmaster128 reviewed Apr 1, 2021

View reviewed changes

webmaster128 mentioned this pull request Apr 1, 2021

Re-introduce ops::Sub<Uint128> for Uint128 #858

Closed

webmaster128 added a commit that referenced this pull request Apr 1, 2021

Add CHANGELOG entry for #853 [skip ci]

fd4f9f4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add methods to UInt128 for explicit overflow control #853

Add methods to UInt128 for explicit overflow control #853

yihuang commented Mar 29, 2021 •

edited

Loading

webmaster128 left a comment

webmaster128 Mar 29, 2021

yihuang Mar 30, 2021 •

edited

Loading

webmaster128 Mar 29, 2021

yihuang Mar 30, 2021

yihuang commented Mar 30, 2021

ethanfrey left a comment

ethanfrey Mar 30, 2021

ethanfrey Mar 30, 2021

yihuang Mar 30, 2021 •

edited

Loading

ethanfrey Mar 30, 2021

yihuang Mar 30, 2021 •

edited

Loading

webmaster128 Mar 30, 2021 •

edited

Loading

webmaster128 Mar 30, 2021

ethanfrey Mar 30, 2021

yihuang Mar 30, 2021

yihuang Mar 31, 2021

ethanfrey Mar 31, 2021

ethanfrey Mar 30, 2021

yihuang Mar 30, 2021 •

edited

Loading

ethanfrey Mar 30, 2021

ethanfrey Mar 30, 2021 •

edited

Loading

yihuang commented Mar 31, 2021

ethanfrey left a comment

webmaster128 commented Mar 31, 2021

yihuang commented Apr 1, 2021

webmaster128 Apr 1, 2021

webmaster128 Apr 1, 2021

webmaster128 commented Apr 1, 2021

Add methods to UInt128 for explicit overflow control #853

Add methods to UInt128 for explicit overflow control #853

Conversation

yihuang commented Mar 29, 2021 • edited Loading

webmaster128 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang commented Mar 30, 2021

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

webmaster128 Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

yihuang commented Mar 31, 2021

ethanfrey left a comment

Choose a reason for hiding this comment

webmaster128 commented Mar 31, 2021

yihuang commented Apr 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webmaster128 commented Apr 1, 2021

yihuang commented Mar 29, 2021 •

edited

Loading

yihuang Mar 30, 2021 •

edited

Loading

yihuang Mar 30, 2021 •

edited

Loading

yihuang Mar 30, 2021 •

edited

Loading

webmaster128 Mar 30, 2021 •

edited

Loading

yihuang Mar 30, 2021 •

edited

Loading

ethanfrey Mar 30, 2021 •

edited

Loading