Add NewCoins constructor

Replaces literal sdk.Coins{}, guarantees coins are sorted and panics if there are duplicates.
cosmos · Mar 6, 2019 · 7b71d73 · 7b71d73
1 parent bf7cbbb
commit 7b71d73
Show file tree

Hide file tree

Showing 20 changed files with 237 additions and 159 deletions.
diff --git a/PENDING.md b/PENDING.md
@@ -53,6 +53,9 @@ tags.
 
 * [\#3719](https://github.com/cosmos/cosmos-sdk/issues/3719) DBBackend can now be set at compile time.
   Defaults: goleveldb. Supported: cleveldb.
+* [\3813](https://github.com/cosmos/cosmos-sdk/pull/3813) New sdk.NewCoins safe constructor to replace bare
+  sdk.Coins{} declarations.
+
 
 ### Tendermint
 

diff --git a/cmd/gaia/app/genesis_test.go b/cmd/gaia/app/genesis_test.go
@@ -41,7 +41,7 @@ func makeGenesisState(t *testing.T, genTxs []auth.StdTx) GenesisState {
 		msg := msgs[0].(staking.MsgCreateValidator)
 
 		acc := auth.NewBaseAccountWithAddress(sdk.AccAddress(msg.ValidatorAddress))
-		acc.Coins = sdk.Coins{sdk.NewInt64Coin(defaultBondDenom, 150)}
+		acc.Coins = sdk.NewCoins(sdk.NewInt64Coin(defaultBondDenom, 150))
 		genAccs[i] = NewGenesisAccount(&acc)
 		stakingData.Pool.NotBondedTokens = stakingData.Pool.NotBondedTokens.Add(sdk.NewInt(150)) // increase the supply
 	}
@@ -55,7 +55,7 @@ func TestToAccount(t *testing.T) {
 	priv := ed25519.GenPrivKey()
 	addr := sdk.AccAddress(priv.PubKey().Address())
 	authAcc := auth.NewBaseAccountWithAddress(addr)
-	authAcc.SetCoins(sdk.Coins{sdk.NewInt64Coin(defaultBondDenom, 150)})
+	authAcc.SetCoins(sdk.NewCoins(sdk.NewInt64Coin(defaultBondDenom, 150)))
 	genAcc := NewGenesisAccount(&authAcc)
 	acc := genAcc.ToAccount()
 	require.IsType(t, &auth.BaseAccount{}, acc)

diff --git a/cmd/gaia/init/genesis_accts_test.go b/cmd/gaia/init/genesis_accts_test.go
@@ -32,8 +32,8 @@ func TestAddGenesisAccount(t *testing.T) {
 			args{
 				app.GenesisState{},
 				addr1,
-				sdk.Coins{},
-				sdk.Coins{},
+				sdk.NewCoins(),
+				sdk.NewCoins(),
 				0,
 				0,
 			},
@@ -44,8 +44,8 @@ func TestAddGenesisAccount(t *testing.T) {
 			args{
 				app.GenesisState{Accounts: []app.GenesisAccount{{Address: addr1}}},
 				addr1,
-				sdk.Coins{},
-				sdk.Coins{},
+				sdk.NewCoins(),
+				sdk.NewCoins(),
 				0,
 				0,
 			},
@@ -56,8 +56,8 @@ func TestAddGenesisAccount(t *testing.T) {
 			args{
 				app.GenesisState{},
 				addr1,
-				sdk.Coins{sdk.NewInt64Coin("stake", 50)},
-				sdk.Coins{sdk.NewInt64Coin("stake", 100)},
+				sdk.NewCoins(sdk.NewInt64Coin("stake", 50)),
+				sdk.NewCoins(sdk.NewInt64Coin("stake", 100)),
 				0,
 				0,
 			},
@@ -68,8 +68,8 @@ func TestAddGenesisAccount(t *testing.T) {
 			args{
 				app.GenesisState{},
 				addr1,
-				sdk.Coins{sdk.NewInt64Coin("stake", 50)},
-				sdk.Coins{sdk.NewInt64Coin("stake", 50)},
+				sdk.NewCoins(sdk.NewInt64Coin("stake", 50)),
+				sdk.NewCoins(sdk.NewInt64Coin("stake", 50)),
 				1654668078,
 				1554668078,
 			},

diff --git a/types/coin.go b/types/coin.go
@@ -130,6 +130,30 @@ func (coin Coin) IsNegative() bool {
 // Coins is a set of Coin, one per currency
 type Coins []Coin
 
+// NewCoins constructs a new coin set.
+func NewCoins(coins ...Coin) Coins {
+	// remove zeroes
+	newCoins := removeZeroCoins(Coins(coins))
+	if len(newCoins) == 0 {
+		return Coins{}
+	}
+
+	// sort
+	newCoins.Sort()
+
+	// detect duplicate Denoms
+	if dupIndex := findDup(newCoins); dupIndex != -1 {
+		panic(fmt.Errorf("find duplicate denom: %s", newCoins[dupIndex]))
+	}
+
+	// validate
+	if !newCoins.IsValid() {
+		panic(fmt.Errorf("invalid coin set: %s", newCoins))
+	}
+
+	return newCoins
+}
+
 func (coins Coins) String() string {
 	if len(coins) == 0 {
 		return ""
@@ -552,3 +576,19 @@ func ParseCoins(coinsStr string) (coins Coins, err error) {
 
 	return coins, nil
 }
+
+// findDup works on the assumption that coins is sorted
+func findDup(coins Coins) int {
+	if len(coins) <= 1 {
+		return -1
+	}
+
+	prevDenom := coins[0]
+	for i := 1; i < len(coins); i++ {
+		if coins[i] == prevDenom {
+			return i
+		}
+	}
+
+	return -1
+}
diff --git a/types/coin_test.go b/types/coin_test.go
@@ -519,3 +519,34 @@ func TestCoinsIsAnyGTE(t *testing.T) {
 	assert.True(t, Coins{{testDenom1, one}, {testDenom2, one}}.IsAnyGTE(Coins{{testDenom1, one}, {testDenom2, two}}))
 	assert.True(t, Coins{{"xxx", one}, {"yyy", one}}.IsAnyGTE(Coins{{testDenom2, one}, {"ccc", one}, {"yyy", one}, {"zzz", one}}))
 }
+
+func TestNewCoins(t *testing.T) {
+	tenatom := NewInt64Coin("atom", 10)
+	tenbtc := NewInt64Coin("btc", 10)
+	zeroeth := NewInt64Coin("eth", 0)
+	type args struct {
+		coins []Coin
+	}
+	tests := []struct {
+		name      string
+		args      args
+		want      Coins
+		wantPanic bool
+	}{
+		{"empty args", args{[]Coin{}}, Coins{}, false},
+		{"one coin", args{[]Coin{tenatom}}, Coins{tenatom}, false},
+		{"sort after create", args{[]Coin{tenbtc, tenatom}}, Coins{tenatom, tenbtc}, false},
+		{"sort and remove zeroes", args{[]Coin{zeroeth, tenbtc, tenatom}}, Coins{tenatom, tenbtc}, false},
+		{"panic on dups", args{[]Coin{tenatom, tenatom}}, Coins{}, true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.wantPanic {
+				require.Panics(t, func() { NewCoins(tt.args.coins...) })
+				return
+			}
+			got := NewCoins(tt.args.coins...)
+			require.True(t, got.IsEqual(tt.want))
+		})
+	}
+}
diff --git a/x/auth/ante_test.go b/x/auth/ante_test.go
@@ -300,18 +300,18 @@ func TestAnteHandlerFees(t *testing.T) {
 	tx = newTestTx(ctx, msgs, privs, accnums, seqs, fee)
 	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeInsufficientFunds)
 
-	acc1.SetCoins(sdk.Coins{sdk.NewInt64Coin("atom", 149)})
+	acc1.SetCoins(sdk.NewCoins(sdk.NewInt64Coin("atom", 149)))
 	input.ak.SetAccount(ctx, acc1)
 	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeInsufficientFunds)
 
 	require.True(t, input.fck.GetCollectedFees(ctx).IsEqual(emptyCoins))
 	require.True(t, input.ak.GetAccount(ctx, addr1).GetCoins().AmountOf("atom").Equal(sdk.NewInt(149)))
 
-	acc1.SetCoins(sdk.Coins{sdk.NewInt64Coin("atom", 150)})
+	acc1.SetCoins(sdk.NewCoins(sdk.NewInt64Coin("atom", 150)))
 	input.ak.SetAccount(ctx, acc1)
 	checkValidTx(t, anteHandler, ctx, tx, false)
 
-	require.True(t, input.fck.GetCollectedFees(ctx).IsEqual(sdk.Coins{sdk.NewInt64Coin("atom", 150)}))
+	require.True(t, input.fck.GetCollectedFees(ctx).IsEqual(sdk.NewCoins(sdk.NewInt64Coin("atom", 150))))
 	require.True(t, input.ak.GetAccount(ctx, addr1).GetCoins().AmountOf("atom").Equal(sdk.NewInt(0)))
 }
 
@@ -333,24 +333,24 @@ func TestAnteHandlerMemoGas(t *testing.T) {
 	var tx sdk.Tx
 	msg := newTestMsg(addr1)
 	privs, accnums, seqs := []crypto.PrivKey{priv1}, []uint64{0}, []uint64{0}
-	fee := NewStdFee(0, sdk.Coins{sdk.NewInt64Coin("atom", 0)})
+	fee := NewStdFee(0, sdk.NewCoins(sdk.NewInt64Coin("atom", 0)))
 
 	// tx does not have enough gas
 	tx = newTestTx(ctx, []sdk.Msg{msg}, privs, accnums, seqs, fee)
 	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeOutOfGas)
 
 	// tx with memo doesn't have enough gas
-	fee = NewStdFee(801, sdk.Coins{sdk.NewInt64Coin("atom", 0)})
+	fee = NewStdFee(801, sdk.NewCoins(sdk.NewInt64Coin("atom", 0)))
 	tx = newTestTxWithMemo(ctx, []sdk.Msg{msg}, privs, accnums, seqs, fee, "abcininasidniandsinasindiansdiansdinaisndiasndiadninsd")
 	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeOutOfGas)
 
 	// memo too large
-	fee = NewStdFee(9000, sdk.Coins{sdk.NewInt64Coin("atom", 0)})
+	fee = NewStdFee(9000, sdk.NewCoins(sdk.NewInt64Coin("atom", 0)))
 	tx = newTestTxWithMemo(ctx, []sdk.Msg{msg}, privs, accnums, seqs, fee, strings.Repeat("01234567890", 500))
 	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeMemoTooLarge)
 
 	// tx with memo has enough gas
-	fee = NewStdFee(9000, sdk.Coins{sdk.NewInt64Coin("atom", 0)})
+	fee = NewStdFee(9000, sdk.NewCoins(sdk.NewInt64Coin("atom", 0)))
 	tx = newTestTxWithMemo(ctx, []sdk.Msg{msg}, privs, accnums, seqs, fee, strings.Repeat("0123456789", 10))
 	checkValidTx(t, anteHandler, ctx, tx, false)
 }
@@ -721,28 +721,28 @@ func TestEnsureSufficientMempoolFees(t *testing.T) {
 		input      StdFee
 		expectedOK bool
 	}{
-		{NewStdFee(200000, sdk.Coins{sdk.NewInt64Coin("photino", 5)}), false},
-		{NewStdFee(200000, sdk.Coins{sdk.NewInt64Coin("stake", 1)}), false},
-		{NewStdFee(200000, sdk.Coins{sdk.NewInt64Coin("stake", 2)}), true},
-		{NewStdFee(200000, sdk.Coins{sdk.NewInt64Coin("photino", 10)}), true},
+		{NewStdFee(200000, sdk.NewCoins(sdk.NewInt64Coin("photino", 5))), false},
+		{NewStdFee(200000, sdk.NewCoins(sdk.NewInt64Coin("stake", 1))), false},
+		{NewStdFee(200000, sdk.NewCoins(sdk.NewInt64Coin("stake", 2))), true},
+		{NewStdFee(200000, sdk.NewCoins(sdk.NewInt64Coin("photino", 10))), true},
 		{
 			NewStdFee(
 				200000,
-				sdk.Coins{
+				sdk.NewCoins(
 					sdk.NewInt64Coin("photino", 10),
 					sdk.NewInt64Coin("stake", 2),
-				},
+				),
 			),
 			true,
 		},
 		{
 			NewStdFee(
 				200000,
-				sdk.Coins{
+				sdk.NewCoins(
 					sdk.NewInt64Coin("atom", 5),
 					sdk.NewInt64Coin("photino", 10),
 					sdk.NewInt64Coin("stake", 2),
-				},
+				),
 			),
 			true,
 		},

diff --git a/x/auth/feekeeper.go b/x/auth/feekeeper.go
@@ -33,10 +33,11 @@ func (fck FeeCollectionKeeper) GetCollectedFees(ctx sdk.Context) sdk.Coins {
 	store := ctx.KVStore(fck.key)
 	bz := store.Get(collectedFeesKey)
 	if bz == nil {
-		return sdk.Coins{}
+		return sdk.NewCoins()
 	}
 
-	feePool := &(sdk.Coins{})
+	emptyFees := sdk.NewCoins()
+	feePool := &emptyFees
 	fck.cdc.MustUnmarshalBinaryLengthPrefixed(bz, feePool)
 	return *feePool
 }
@@ -57,5 +58,5 @@ func (fck FeeCollectionKeeper) AddCollectedFees(ctx sdk.Context, coins sdk.Coins
 
 // ClearCollectedFees - clear the fee pool
 func (fck FeeCollectionKeeper) ClearCollectedFees(ctx sdk.Context) {
-	fck.setCollectedFees(ctx, sdk.Coins{})
+	fck.setCollectedFees(ctx, sdk.NewCoins())
 }
diff --git a/x/auth/feekeeper_test.go b/x/auth/feekeeper_test.go
@@ -9,9 +9,9 @@ import (
 )
 
 var (
-	emptyCoins = sdk.Coins{}
-	oneCoin    = sdk.Coins{sdk.NewInt64Coin("foocoin", 1)}
-	twoCoins   = sdk.Coins{sdk.NewInt64Coin("foocoin", 2)}
+	emptyCoins = sdk.NewCoins()
+	oneCoin    = sdk.NewCoins(sdk.NewInt64Coin("foocoin", 1))
+	twoCoins   = sdk.NewCoins(sdk.NewInt64Coin("foocoin", 2))
 )
 
 func TestFeeCollectionKeeperGetSet(t *testing.T) {

diff --git a/x/auth/genesis.go b/x/auth/genesis.go
@@ -22,7 +22,7 @@ func NewGenesisState(collectedFees sdk.Coins, params Params) GenesisState {
 
 // DefaultGenesisState - Return a default genesis state
 func DefaultGenesisState() GenesisState {
-	return NewGenesisState(sdk.Coins{}, DefaultParams())
+	return NewGenesisState(sdk.NewCoins(), DefaultParams())
 }
 
 // InitGenesis - Init store state from genesis data

diff --git a/x/auth/stdtx.go b/x/auth/stdtx.go
@@ -141,7 +141,7 @@ func (fee StdFee) Bytes() []byte {
 	// (in the lcd_test, client side its null,
 	// server side its [])
 	if len(fee.Amount) == 0 {
-		fee.Amount = sdk.Coins{}
+		fee.Amount = sdk.NewCoins()
 	}
 	bz, err := msgCdc.MarshalJSON(fee) // TODO
 	if err != nil {

diff --git a/x/auth/test_utils.go b/x/auth/test_utils.go
@@ -55,7 +55,7 @@ func newTestMsg(addrs ...sdk.AccAddress) *sdk.TestMsg {
 
 func newStdFee() StdFee {
 	return NewStdFee(50000,
-		sdk.Coins{sdk.NewInt64Coin("atom", 150)},
+		sdk.NewCoins(sdk.NewInt64Coin("atom", 150)),
 	)
 }
 

diff --git a/x/bank/keeper.go b/x/bank/keeper.go
@@ -211,7 +211,7 @@ func (keeper BaseViewKeeper) Codespace() sdk.CodespaceType {
 func getCoins(ctx sdk.Context, am auth.AccountKeeper, addr sdk.AccAddress) sdk.Coins {
 	acc := am.GetAccount(ctx, addr)
 	if acc == nil {
-		return sdk.Coins{}
+		return sdk.NewCoins()
 	}
 	return acc.GetCoins()
 }
@@ -255,7 +255,7 @@ func subtractCoins(ctx sdk.Context, ak auth.AccountKeeper, addr sdk.AccAddress,
 		return nil, nil, sdk.ErrInvalidCoins(amt.String())
 	}
 
-	oldCoins, spendableCoins := sdk.Coins{}, sdk.Coins{}
+	oldCoins, spendableCoins := sdk.NewCoins(), sdk.NewCoins()
 
 	acc := getAccount(ctx, ak, addr)
 	if acc != nil {