fix DecCoins constructor (cosmos#5410)

* fix decCoins

* minor changes for standardization

* changelog

* add test cases

CHANGELOG.md

@@ -223,6 +223,7 @@ to detail this new feature and how state transitions occur.
 * (rest) [\#5212](https://github.com/cosmos/cosmos-sdk/issues/5212) Fix pagination in the `/gov/proposals` handler.
 * (baseapp) [\#5350](https://github.com/cosmos/cosmos-sdk/issues/5350) Allow a node to restart successfully after a `halt-height` or `halt-time`
   has been triggered.
+* (types) [\#5408](https://github.com/cosmos/cosmos-sdk/issues/5408) `NewDecCoins` constructor now sorts the coins.
 
 ## [v0.37.4] - 2019-11-04
 

types/dec_coin.go

@@ -11,17 +11,16 @@ import (
 // ----------------------------------------------------------------------------
 // Decimal Coin
 
-// Coins which can have additional decimal points
+// DecCoin defines a coin which can have additional decimal points
 type DecCoin struct {
 	Denom  string `json:"denom"`
 	Amount Dec    `json:"amount"`
 }
 
+// NewDecCoin creates a new DecCoin instance from an Int.
 func NewDecCoin(denom string, amount Int) DecCoin {
-	mustValidateDenom(denom)
-
-	if amount.IsNegative() {
-		panic(fmt.Sprintf("negative coin amount: %v\n", amount))
+	if err := validate(denom, amount); err != nil {
+		panic(err)
 	}
 
 	return DecCoin{
@@ -30,6 +29,7 @@ func NewDecCoin(denom string, amount Int) DecCoin {
 	}
 }
 
+// NewDecCoinFromDec creates a new DecCoin instance from a Dec.
 func NewDecCoinFromDec(denom string, amount Dec) DecCoin {
 	mustValidateDenom(denom)
 
@@ -43,12 +43,10 @@ func NewDecCoinFromDec(denom string, amount Dec) DecCoin {
 	}
 }
 
+// NewDecCoinFromCoin creates a new DecCoin from a Coin.
 func NewDecCoinFromCoin(coin Coin) DecCoin {
-	if coin.Amount.IsNegative() {
-		panic(fmt.Sprintf("negative decimal coin amount: %v\n", coin.Amount))
-	}
-	if strings.ToLower(coin.Denom) != coin.Denom {
-		panic(fmt.Sprintf("denom cannot contain upper case characters: %s\n", coin.Denom))
+	if err := validate(coin.Denom, coin.Amount); err != nil {
+		panic(err)
 	}
 
 	return DecCoin{
@@ -97,28 +95,32 @@ func (coin DecCoin) IsEqual(other DecCoin) bool {
 	return coin.Amount.Equal(other.Amount)
 }
 
-// Adds amounts of two coins with same denom
+// Add adds amounts of two decimal coins with same denom.
 func (coin DecCoin) Add(coinB DecCoin) DecCoin {
 	if coin.Denom != coinB.Denom {
 		panic(fmt.Sprintf("coin denom different: %v %v\n", coin.Denom, coinB.Denom))
 	}
 	return DecCoin{coin.Denom, coin.Amount.Add(coinB.Amount)}
 }
 
-// Subtracts amounts of two coins with same denom
+// Sub subtracts amounts of two decimal coins with same denom.
 func (coin DecCoin) Sub(coinB DecCoin) DecCoin {
 	if coin.Denom != coinB.Denom {
 		panic(fmt.Sprintf("coin denom different: %v %v\n", coin.Denom, coinB.Denom))
 	}
-	return DecCoin{coin.Denom, coin.Amount.Sub(coinB.Amount)}
+	res := DecCoin{coin.Denom, coin.Amount.Sub(coinB.Amount)}
+	if res.IsNegative() {
+		panic("negative decimal coin amount")
+	}
+	return res
 }
 
 // TruncateDecimal returns a Coin with a truncated decimal and a DecCoin for the
 // change. Note, the change may be zero.
 func (coin DecCoin) TruncateDecimal() (Coin, DecCoin) {
 	truncated := coin.Amount.TruncateInt()
 	change := coin.Amount.Sub(truncated.ToDec())
-	return NewCoin(coin.Denom, truncated), DecCoin{coin.Denom, change}
+	return NewCoin(coin.Denom, truncated), NewDecCoinFromDec(coin.Denom, change)
 }
 
 // IsPositive returns true if coin amount is positive.
@@ -141,18 +143,30 @@ func (coin DecCoin) String() string {
 	return fmt.Sprintf("%v%v", coin.Amount, coin.Denom)
 }
 
+// IsValid returns true if the DecCoin has a non-negative amount and the denom is vaild.
+func (coin DecCoin) IsValid() bool {
+	if err := ValidateDenom(coin.Denom); err != nil {
+		return false
+	}
+	return !coin.IsNegative()
+}
+
 // ----------------------------------------------------------------------------
 // Decimal Coins
 
-// coins with decimal
+// DecCoins defines a slice of coins with decimal values
 type DecCoins []DecCoin
 
+// NewDecCoins constructs a new coin set with decimal values
+// from regular Coins.
 func NewDecCoins(coins Coins) DecCoins {
-	dcs := make(DecCoins, len(coins))
-	for i, coin := range coins {
-		dcs[i] = NewDecCoinFromCoin(coin)
+	decCoins := make(DecCoins, len(coins))
+	newCoins := NewCoins(coins...)
+	for i, coin := range newCoins {
+		decCoins[i] = NewDecCoinFromCoin(coin)
 	}
-	return dcs
+
+	return decCoins
 }
 
 // String implements the Stringer interface for DecCoins. It returns a
@@ -177,7 +191,7 @@ func (coins DecCoins) TruncateDecimal() (truncatedCoins Coins, changeCoins DecCo
 	for _, coin := range coins {
 		truncated, change := coin.TruncateDecimal()
 		if !truncated.IsZero() {
-			truncatedCoins = truncatedCoins.Add(Coins{truncated})
+			truncatedCoins = truncatedCoins.Add(NewCoins(truncated))
 		}
 		if !change.IsZero() {
 			changeCoins = changeCoins.Add(DecCoins{change})
@@ -404,7 +418,7 @@ func (coins DecCoins) Empty() bool {
 	return len(coins) == 0
 }
 
-// returns the amount of a denom from deccoins
+// AmountOf returns the amount of a denom from deccoins
 func (coins DecCoins) AmountOf(denom string) Dec {
 	mustValidateDenom(denom)
 
@@ -452,7 +466,7 @@ func (coins DecCoins) IsEqual(coinsB DecCoins) bool {
 	return true
 }
 
-// return whether all coins are zero
+// IsZero returns whether all coins are zero
 func (coins DecCoins) IsZero() bool {
 	for _, coin := range coins {
 		if !coin.Amount.IsZero() {

types/dec_coin_test.go

@@ -96,6 +96,111 @@ func TestAddDecCoins(t *testing.T) {
 	}
 }
 
+func TestIsValid(t *testing.T) {
+	tests := []struct {
+		coin       DecCoin
+		expectPass bool
+		msg        string
+	}{
+		{
+			NewDecCoin("mytoken", NewInt(10)),
+			true,
+			"valid coins should have passed",
+		},
+		{
+			DecCoin{Denom: "BTC", Amount: NewDec(10)},
+			false,
+			"invalid denoms",
+		},
+		{
+			DecCoin{Denom: "BTC", Amount: NewDec(-10)},
+			false,
+			"negative amount",
+		},
+	}
+
+	for _, tc := range tests {
+		tc := tc
+		if tc.expectPass {
+			require.True(t, tc.coin.IsValid(), tc.msg)
+		} else {
+			require.False(t, tc.coin.IsValid(), tc.msg)
+		}
+	}
+}
+
+func TestSubDecCoin(t *testing.T) {
+	tests := []struct {
+		coin       DecCoin
+		expectPass bool
+		msg        string
+	}{
+		{
+			NewDecCoin("mytoken", NewInt(20)),
+			true,
+			"valid coins should have passed",
+		},
+		{
+			NewDecCoin("othertoken", NewInt(20)),
+			false,
+			"denom mismatch",
+		},
+		{
+			NewDecCoin("mytoken", NewInt(9)),
+			false,
+			"negative amount",
+		},
+	}
+
+	decCoin := NewDecCoin("mytoken", NewInt(10))
+
+	for _, tc := range tests {
+		tc := tc
+		if tc.expectPass {
+			equal := tc.coin.Sub(decCoin)
+			require.Equal(t, equal, decCoin, tc.msg)
+		} else {
+			require.Panics(t, func() { tc.coin.Sub(decCoin) }, tc.msg)
+		}
+	}
+}
+
+func TestSubDecCoins(t *testing.T) {
+	tests := []struct {
+		coins      DecCoins
+		expectPass bool
+		msg        string
+	}{
+		{
+			NewDecCoins(Coins{NewCoin("mytoken", NewInt(10)), NewCoin("btc", NewInt(20)), NewCoin("eth", NewInt(30))}),
+			true,
+			"sorted coins should have passed",
+		},
+		{
+			DecCoins{NewDecCoin("mytoken", NewInt(10)), NewDecCoin("btc", NewInt(20)), NewDecCoin("eth", NewInt(30))},
+			false,
+			"unorted coins should panic",
+		},
+		{
+			DecCoins{DecCoin{Denom: "BTC", Amount: NewDec(10)}, NewDecCoin("eth", NewInt(15)), NewDecCoin("mytoken", NewInt(5))},
+			false,
+			"invalid denoms",
+		},
+	}
+
+	decCoins := NewDecCoins(Coins{NewCoin("btc", NewInt(10)), NewCoin("eth", NewInt(15)), NewCoin("mytoken", NewInt(5))})
+
+	for _, tc := range tests {
+		tc := tc
+		if tc.expectPass {
+			equal := tc.coins.Sub(decCoins)
+			require.Equal(t, equal, decCoins, tc.msg)
+		} else {
+			require.Panics(t, func() { tc.coins.Sub(decCoins) }, tc.msg)
+		}
+	}
+}
+
 func TestSortDecCoins(t *testing.T) {
 	good := DecCoins{
 		NewInt64DecCoin("gas", 1),