Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement Coin.Multiply overflow check #351

Merged
merged 3 commits into from
Feb 25, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ var (
// ErrExpired stands for expired entities, normally has to do with block height expirations
ErrExpired = Register(15, "expired")

// ErrOverflow s returned when a computation cannot be completed
// because the result value exceeds the type.
ErrOverflow = Register(16, "an operation cannot be completed due to value overflow")

// ErrPanic is only set when we recover from a panic, so we know to redact potentially sensitive system info
ErrPanic = Register(111222, "panic")
)
Expand Down
42 changes: 36 additions & 6 deletions x/coin.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,22 +75,52 @@ func (c Coin) Divide(pieces int64) (Coin, Coin, error) {
return one, rest, nil
}

// Multiply returns the result of a coin value multiplication.
func (c Coin) Multiply(times int64) Coin {
whole := c.Whole * times
frac := c.Fractional * times
// Multiply returns the result of a coin value multiplication. This method can
// fail if the result would overflow maximum coin value.
func (c Coin) Multiply(times int64) (Coin, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we enforce int32, I think we guarantee to have no overflows ever....
But we don't have to do so

if times == 0 || (c.Whole == 0 && c.Fractional == 0) {
return Coin{Ticker: c.Ticker}, nil
}

whole, err := mul64(c.Whole, times)
if err != nil {
return Coin{}, err

}
frac, err := mul64(c.Fractional, times)
if err != nil {
return Coin{}, err
}

// Normalize if fractional value overflows.
if frac > FracUnit {
whole += frac / FracUnit
if n := whole + frac/FracUnit; n < whole {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice check here... gotta have a clever test that multiply doesn't overflow until we carry the fractional, but good to cover this edge

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return Coin{}, errors.ErrOverflow
} else {
whole = n
}
frac = frac % FracUnit
}

return Coin{
res := Coin{
Ticker: c.Ticker,
Whole: whole,
Fractional: frac,
}
return res, nil
}

// mul64 multiplies two int64 numbers. If the result overflows the int64 size
// the ErrOverflow is returned.
func mul64(a, b int64) (int64, error) {
if a == 0 || b == 0 {
return 0, nil
}
c := a * b
if c/a != b {
return c, errors.ErrOverflow
}
return c, nil
}

// Add combines two coins.
Expand Down
57 changes: 50 additions & 7 deletions x/coin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package x

import (
"fmt"
"math"
"testing"

"github.com/iov-one/weave/errors"
Expand Down Expand Up @@ -340,16 +341,17 @@ func TestCoinDivide(t *testing.T) {

func TestCoinMultiply(t *testing.T) {
cases := map[string]struct {
coin Coin
times int64
want Coin
coin Coin
times int64
want Coin
wantErr error
}{
"zero value coin": {
coin: NewCoin(0, 0, "DOGE"),
times: 666,
want: NewCoin(0, 0, "DOGE"),
},
"multiply": {
"simple multiply": {
husio marked this conversation as resolved.
Show resolved Hide resolved
coin: NewCoin(1, 0, "DOGE"),
times: 3,
want: NewCoin(3, 0, "DOGE"),
Expand All @@ -369,12 +371,53 @@ func TestCoinMultiply(t *testing.T) {
times: -2,
want: NewCoin(-2, -2, "DOGE"),
},
"overflow of a negative and a positive value": {
coin: NewCoin(math.MaxInt64, 0, "DOGE"),
times: -math.MaxInt64,
wantErr: errors.ErrOverflow,
},
"overflow of two negative values": {
coin: NewCoin(-math.MaxInt64, 0, "DOGE"),
times: -math.MaxInt64,
wantErr: errors.ErrOverflow,
},
"overflow of two positive values": {
coin: NewCoin(math.MaxInt64, 0, "DOGE"),
times: math.MaxInt64,
wantErr: errors.ErrOverflow,
},
"overflow with a big multiply": {
coin: NewCoin(1000, 0, "DOGE"),
times: math.MaxInt64 / 10,
wantErr: errors.ErrOverflow,
},
"overflow with a small multiply": {
coin: NewCoin(math.MaxInt64/10, 0, "DOGE"),
times: 1000,
wantErr: errors.ErrOverflow,
},
"overflow when normalizing": {
coin: NewCoin(math.MaxInt64-1, math.MaxInt64-1, "DOGE"),
times: 1,
wantErr: errors.ErrOverflow,
},
"overflow when normalizing 2": {
coin: NewCoin(1, 230000000, "DOGE"),
times: 10,
want: NewCoin(12, 300000000, "DOGE"),
},
}
for testName, tc := range cases {
t.Run(testName, func(t *testing.T) {
got := tc.coin.Multiply(tc.times)
if !got.Equals(tc.want) {
t.Fatalf("got %v", got)
got, err := tc.coin.Multiply(tc.times)
if !errors.Is(tc.wantErr, err) {
t.Logf("got coin: %+v", got)
t.Fatalf("got error %v", err)
}
if tc.wantErr == nil {
if !got.Equals(tc.want) {
t.Fatalf("got %v", got)
}
}
})
}
Expand Down
5 changes: 4 additions & 1 deletion x/distribution/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,10 @@ func distribute(db weave.KVStore, ctrl CashController, source weave.Address, rec
}

for _, r := range recipients {
amount := one.Multiply(int64(r.Weight / div))
amount, err := one.Multiply(int64(r.Weight / div))
if err != nil {
return errors.Wrap(err, "cannot multiply chunk")
}
// Chunk is too small to be distributed.
if amount.IsZero() {
continue
Expand Down