implement Coin.Multiply overflow check #351
Conversation
husio
force-pushed
the
coins_overflow_issue_326
branch
from
7649fa3 to
6e65a14
Compare
| @@ -76,7 +76,11 @@ func (c Coin) Divide(pieces int64) (Coin, Coin, error) { | |||
| } | |||
|
|
|||
| // Multiply returns the result of a coin value multiplication. | |||
| func (c Coin) Multiply(times int64) Coin { | |||
| func (c Coin) Multiply(times int64) (Coin, error) { | |||
There was a problem hiding this comment.
If we enforce int32, I think we guarantee to have no overflows ever....
But we don't have to do so
x/coin.go
Outdated
| return 0, true | ||
| } | ||
| c := a * b | ||
| if (c < 0) == ((a < 0) != (b < 0)) { |
There was a problem hiding this comment.
I think you only need the inner check, c/b == a
I was just pasting that answer I got from https://stackoverflow.com/a/1815371
There was a problem hiding this comment.
Wow, this is a smart check! c := a * b therefore c / b == a must be true. I will upgrade.
x/coin.go
Outdated
|
|
||
| // Borrowed from | ||
| // https://github.com/JohnCGriffin/overflow/blob/master/overflow_impl.go#L336 | ||
| func mul64(a, b int64) (int64, bool) { |
There was a problem hiding this comment.
Please state that the bool is true if valid, false if overflow
There was a problem hiding this comment.
I will change the bool to error. It won't require a comment then 💪
|
|
||
| // Normalize if fractional value overflows. | ||
| if frac > FracUnit { | ||
| whole += frac / FracUnit | ||
| if n := whole + frac/FracUnit; n < whole { |
There was a problem hiding this comment.
Nice check here... gotta have a clever test that multiply doesn't overflow until we carry the fractional, but good to cover this edge
There was a problem hiding this comment.
This comment has been minimized.
This comment has been minimized.
implement Coin.Multiply overflow check
resolve #326