increase auction maths safety

x/auction/genesis_test.go

@@ -9,17 +9,20 @@ import (
 
 	abci "github.com/tendermint/tendermint/abci/types"
 
+	sdk "github.com/cosmos/cosmos-sdk/types"
+
 	"github.com/kava-labs/kava/app"
 	"github.com/kava-labs/kava/x/auction"
 )
 
+var _, testAddrs = app.GeneratePrivKeyAddressPairs(2)
 var testTime = time.Date(1998, 1, 1, 0, 0, 0, 0, time.UTC)
 var testAuction = auction.NewCollateralAuction(
 	"seller",
 	c("lotdenom", 10),
 	testTime,
 	c("biddenom", 1000),
-	auction.WeightedAddresses{},
+	auction.WeightedAddresses{Addresses: testAddrs, Weights: []sdk.Int{sdk.OneInt(), sdk.OneInt()}},
 	c("debt", 1000),
 ).WithID(3).(auction.GenesisAuction)
 

x/auction/keeper/auctions.go

@@ -343,7 +343,7 @@ func (k Keeper) PlaceReverseBidCollateral(ctx sdk.Context, a types.CollateralAuc
 	}
 
 	// Decrease in lot is sent to weighted addresses (normally the CDP depositors)
-	// TODO: paying out rateably to cdp depositors is vulnerable to errors compounding over multiple bids - check this can't be gamed.
+	// Note: splitting an integer amount across weighted buckets results in small errors.
 	lotPayouts, err := splitCoinIntoWeightedBuckets(a.Lot.Sub(lot), a.LotReturns.Weights)
 	if err != nil {
 		return a, err

x/auction/keeper/math.go

@@ -7,41 +7,52 @@ import (
 )
 
 // splitIntIntoWeightedBuckets divides an initial +ve integer among several buckets in proportion to the buckets' weights
-// It uses the largest remainder method:
+// It uses the largest remainder method: https://en.wikipedia.org/wiki/Largest_remainder_method
-// https://en.wikipedia.org/wiki/Largest_remainder_method
+// See also: https://stackoverflow.com/questions/13483430/how-to-make-rounded-percentages-add-up-to-100
-// see also: https://stackoverflow.com/questions/13483430/how-to-make-rounded-percentages-add-up-to-100
 func splitIntIntoWeightedBuckets(amount sdk.Int, buckets []sdk.Int) []sdk.Int {
-	// Limit input to +ve numbers as algorithm hasn't been designed to work with -ve numbers.
+	// Limit input to +ve numbers as algorithm hasn't been scoped to work with -ve numbers.
 	if amount.IsNegative() {
 		panic("negative amount")
 	}
+	if len(buckets) < 1 {
+		panic("no buckets")
+	}
 	for _, bucket := range buckets {
 		if bucket.IsNegative() {
 			panic("negative bucket")
 		}
 	}
 
-	totalWeights := totalInts(buckets...)
+	// 1) Split the amount by weights, recording whole number part and remainder
+
+	totalWeights := totalInts(buckets...)
+	if !totalWeights.IsPositive() {
+		panic("total weights must sum to > 0")
+	}
 
-	// split amount by weights, recording whole number part and remainder
 	quotients := make([]quoRem, len(buckets))
 	for i := range buckets {
+		// amount * ( weight/total_weight )
 		q := amount.Mul(buckets[i]).Quo(totalWeights)
 		r := amount.Mul(buckets[i]).Mod(totalWeights)
 		quotients[i] = quoRem{index: i, quo: q, rem: r}
 	}
 
-	// apportion left over to buckets with the highest remainder (to minimize error)
+	// 2) Calculate total left over from remainders, and apportion it to buckets with the highest remainder (to minimize error)
+
+	// sort by decreasing remainder order
 	sort.Slice(quotients, func(i, j int) bool {
-		return quotients[i].rem.GT(quotients[j].rem) // decreasing remainder order
+		return quotients[i].rem.GT(quotients[j].rem)
 	})
 
+	// calculate total left over from remainders
 	allocated := sdk.ZeroInt()
 	for _, qr := range quotients {
 		allocated = allocated.Add(qr.quo)
 	}
 	leftToAllocate := amount.Sub(allocated)
 
+	// apportion according to largest remainder
 	results := make([]sdk.Int, len(quotients))
 	for _, qr := range quotients {
 		results[qr.index] = qr.quo

x/auction/keeper/math_test.go

@@ -10,19 +10,97 @@ import (
 
 func TestSplitIntIntoWeightedBuckets(t *testing.T) {
 	testCases := []struct {
-		name    string
+		name        string
-		amount  sdk.Int
+		amount      sdk.Int
-		buckets []sdk.Int
+		buckets     []sdk.Int
-		want    []sdk.Int
+		want        []sdk.Int
+		expectPanic bool
 	}{
-		{"2split1,1", i(2), is(1, 1), is(1, 1)},
+		{
-		{"100split1,9", i(100), is(1, 9), is(10, 90)},
+			name:        "0split0",
-		{"7split1,2", i(7), is(1, 2), is(2, 5)},
+			amount:      i(0),
-		{"17split1,1,1", i(17), is(1, 1, 1), is(6, 6, 5)},
+			buckets:     is(0),
+			expectPanic: true,
+		},
+		{
+			name:        "5splitnil",
+			amount:      i(5),
+			buckets:     is(),
+			expectPanic: true,
+		},
+		{
+			name:        "-2split1,1",
+			amount:      i(-2),
+			buckets:     is(1, 1),
+			expectPanic: true,
+		},
+		{
+			name:        "2split1,-1",
+			amount:      i(2),
+			buckets:     is(1, -1),
+			expectPanic: true,
+		},
+		{
+			name:    "0split0,0,0,1",
+			amount:  i(0),
+			buckets: is(0, 0, 0, 1),
+			want:    is(0, 0, 0, 0),
+		},
+		{
+			name:    "2split1,1",
+			amount:  i(2),
+			buckets: is(1, 1),
+			want:    is(1, 1),
+		},
+		{
+			name:    "100split1,9",
+			amount:  i(100),
+			buckets: is(1, 9),
+			want:    is(10, 90),
+		},
+		{
+			name:    "100split9,1",
+			amount:  i(100),
+			buckets: is(9, 1),
+			want:    is(90, 10),
+		},
+		{
+			name:    "7split1,2",
+			amount:  i(7),
+			buckets: is(1, 2),
+			want:    is(2, 5),
+		},
+		{
+			name:    "17split1,1,1",
+			amount:  i(17),
+			buckets: is(1, 1, 1),
+			want:    is(6, 6, 5),
+		},
+		{
+			name:    "10split1000000,1",
+			amount:  i(10),
+			buckets: is(1000000, 1),
+			want:    is(10, 0),
+		},
+		{
+			name:    "334733353split730777,31547",
+			amount:  i(334733353),
+			buckets: is(730777, 31547),
+			want:    is(320881194, 13852159),
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			got := splitIntIntoWeightedBuckets(tc.amount, tc.buckets)
+			var got []sdk.Int
+			run := func() {
+				got = splitIntIntoWeightedBuckets(tc.amount, tc.buckets)
+			}
+			if tc.expectPanic {
+				require.Panics(t, run)
+			} else {
+				require.NotPanics(t, run)
+			}
+
 			require.Equal(t, tc.want, got)
 		})
 	}

x/auction/types/auctions.go

@@ -287,15 +287,23 @@ func NewWeightedAddresses(addrs []sdk.AccAddress, weights []sdk.Int) (WeightedAd
 	return wa, nil
 }
 
-// Validate checks for that the weights are not negative and that lengths match.
+// Validate checks for that the weights are not negative, not all zero, and the lengths match.
 func (wa WeightedAddresses) Validate() error {
+	if len(wa.Weights) < 1 {
+		return fmt.Errorf("must be at least 1 weighted address")
+	}
 	if len(wa.Addresses) != len(wa.Weights) {
 		return fmt.Errorf("number of addresses doesn't match number of weights, %d ≠ %d", len(wa.Addresses), len(wa.Weights))
 	}
+	totalWeight := sdk.ZeroInt()
 	for _, w := range wa.Weights {
 		if w.IsNegative() {
 			return fmt.Errorf("weights contain a negative amount: %s", w)
 		}
+		totalWeight = totalWeight.Add(w)
+	}
+	if !totalWeight.IsPositive() {
+		return fmt.Errorf("total weight must be positive")
 	}
 	return nil
 }

x/auction/types/auctions_test.go

@@ -1,6 +1,7 @@
 package types
 
 import (
+	"fmt"
 	"testing"
 	"time"
 
@@ -24,48 +25,65 @@ const (
 	TestAccAddress2         = "kava1pdfav2cjhry9k79nu6r8kgknnjtq6a7rcr0qlr"
 )
 
+func d(amount string) sdk.Dec               { return sdk.MustNewDecFromStr(amount) }
+func c(denom string, amount int64) sdk.Coin { return sdk.NewInt64Coin(denom, amount) }
+func i(n int64) sdk.Int                     { return sdk.NewInt(n) }
+func is(ns ...int64) (is []sdk.Int) {
+	for _, n := range ns {
+		is = append(is, sdk.NewInt(n))
+	}
+	return
+}
+
 func TestNewWeightedAddresses(t *testing.T) {
 
 	tests := []struct {
-		name       string
+		name        string
-		addresses  []sdk.AccAddress
+		addresses   []sdk.AccAddress
-		weights    []sdk.Int
+		weights     []sdk.Int
-		expectpass bool
+		expectedErr error
 	}{
 		{
-			"normal",
+			name: "normal",
-			[]sdk.AccAddress{
+			addresses: []sdk.AccAddress{
 				sdk.AccAddress([]byte(TestAccAddress1)),
 				sdk.AccAddress([]byte(TestAccAddress2)),
 			},
-			[]sdk.Int{
+			weights:     is(6, 8),
-				sdk.NewInt(6),
+			expectedErr: nil,
-				sdk.NewInt(8),
-			},
-			true,
 		},
 		{
-			"mismatched",
+			name: "mismatched",
-			[]sdk.AccAddress{
+			addresses: []sdk.AccAddress{
 				sdk.AccAddress([]byte(TestAccAddress1)),
 				sdk.AccAddress([]byte(TestAccAddress2)),
 			},
-			[]sdk.Int{
+			weights:     is(6),
-				sdk.NewInt(6),
+			expectedErr: fmt.Errorf("number of addresses doesn't match number of weights, %d ≠ %d", 2, 1),
-			},
-			false,
 		},
 		{
-			"negativeWeight",
+			name: "negativeWeight",
-			[]sdk.AccAddress{
+			addresses: []sdk.AccAddress{
 				sdk.AccAddress([]byte(TestAccAddress1)),
 				sdk.AccAddress([]byte(TestAccAddress2)),
 			},
-			[]sdk.Int{
+			weights:     is(6, -8),
-				sdk.NewInt(6),
+			expectedErr: fmt.Errorf("weights contain a negative amount: %s", i(-8)),
-				sdk.NewInt(-8),
+		},
+		{
+			name: "zero total weights",
+			addresses: []sdk.AccAddress{
+				sdk.AccAddress([]byte(TestAccAddress1)),
+				sdk.AccAddress([]byte(TestAccAddress2)),
 			},
-			false,
+			weights:     is(0, 0),
+			expectedErr: fmt.Errorf("total weight must be positive"),
+		},
+		{
+			name:        "no weights",
+			addresses:   nil,
+			weights:     nil,
+			expectedErr: fmt.Errorf("must be at least 1 weighted address"),
 		},
 	}
 
@@ -75,27 +93,16 @@ func TestNewWeightedAddresses(t *testing.T) {
 			// Attempt to instantiate new WeightedAddresses
 			weightedAddresses, err := NewWeightedAddresses(tc.addresses, tc.weights)
 
-			if tc.expectpass {
+			if tc.expectedErr != nil {
-				// Confirm there is no error
+				// Confirm the error
-				require.Nil(t, err)
+				require.EqualError(t, err, tc.expectedErr.Error())
-
+			} else {
+				require.NoError(t, err)
 				// Check addresses, weights
 				require.Equal(t, tc.addresses, weightedAddresses.Addresses)
 				require.Equal(t, tc.weights, weightedAddresses.Weights)
-			} else {
-				// Confirm that there is an error
-				require.NotNil(t, err)
-
-				switch tc.name {
-				case "mismatched":
-					require.Contains(t, err.Error(), "number of addresses doesn't match number of weights")
-				case "negativeWeight":
-					require.Contains(t, err.Error(), "weights contain a negative amount")
-				default:
-					// Unexpected error state
-					t.Fail()
-				}
 			}
+
 		})
 	}
 }

x/auction/types/msg_test.go

@@ -39,5 +39,3 @@ func TestMsgPlaceBid_ValidateBasic(t *testing.T) {
 		})
 	}
 }
-
-func c(denom string, amount int64) sdk.Coin { return sdk.NewInt64Coin(denom, amount) }

x/auction/types/params_test.go

@@ -5,8 +5,6 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
-
-	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
 func TestParams_Validate(t *testing.T) {
@@ -105,5 +103,3 @@ func TestParams_Validate(t *testing.T) {
 		})
 	}
 }
-
-func d(amount string) sdk.Dec { return sdk.MustNewDecFromStr(amount) }