Remove zero coins from ParseCoins and ParseDecCoins (cosmos#7348)

* Remove zero coins from ParseCoins and ParseDecCoins * Implement requested changes * Update types/coin_test.go * Revert * Align the behaviour of NewCoins and ParseCoins (respectively NewDecCoins and ParseDecCoins) * Amend comments * Update types/dec_coin.go Co-authored-by: Alessio Treglia <[email protected]> Co-authored-by: Alessio Treglia <[email protected]> Co-authored-by: SaReN <[email protected]> Co-authored-by: colin axnér <[email protected]>
arnabmitra · Sep 25, 2020 · 30f9862 · 30f9862
1 parent d16fe57
commit 30f9862
Show file tree

Hide file tree

Showing 5 changed files with 57 additions and 40 deletions.
diff --git a/go.sum b/go.sum
@@ -488,8 +488,6 @@ github.com/prometheus/common v0.7.0/go.mod h1:DjGbpBbp5NYNiECxcL/VnbXCCaQpKd3tt2
 github.com/prometheus/common v0.9.1/go.mod h1:yhUN8i9wzaXS3w1O07YhxHEBxD+W35wd8bs7vj7HSQ4=
 github.com/prometheus/common v0.10.0 h1:RyRA7RzGXQZiW+tGMr7sxa85G1z0yOpM1qq5c8lNawc=
 github.com/prometheus/common v0.10.0/go.mod h1:Tlit/dnDKsSWFlCLTWaA1cyBgKHSMdTB80sz/V91rCo=
-github.com/prometheus/common v0.13.0 h1:vJlpe9wPgDRM1Z+7Wj3zUUjY1nr6/1jNKyl7llliccg=
-github.com/prometheus/common v0.13.0/go.mod h1:U+gB1OBLb1lF3O42bTCL+FK18tX9Oar16Clt/msog/s=
 github.com/prometheus/common v0.14.0 h1:RHRyE8UocrbjU+6UvRzwi6HjiDfxrrBU91TtbKzkGp4=
 github.com/prometheus/common v0.14.0/go.mod h1:U+gB1OBLb1lF3O42bTCL+FK18tX9Oar16Clt/msog/s=
 github.com/prometheus/procfs v0.0.0-20181005140218-185b4288413d/go.mod h1:c3At6R/oaqEKCNdg8wHV1ftS6bRYblBhIjjI8uT2IGk=

diff --git a/types/coin.go b/types/coin.go
@@ -135,23 +135,26 @@ func (coin Coin) IsNegative() bool {
 // Coins is a set of Coin, one per currency
 type Coins []Coin
 
-// NewCoins constructs a new coin set.
+// NewCoins constructs a new coin set. The provided coins will be sanitized by removing
+// zero coins and sorting the coin set. A panic will occur if the coin set is not valid.
 func NewCoins(coins ...Coin) Coins {
-	// remove zeroes
-	newCoins := removeZeroCoins(Coins(coins))
-	if len(newCoins) == 0 {
-		return Coins{}
-	}
-
-	newCoins.Sort()
-
+	newCoins := sanitizeCoins(coins)
 	if err := newCoins.Validate(); err != nil {
 		panic(fmt.Errorf("invalid coin set %s: %w", newCoins, err))
 	}
 
 	return newCoins
 }
 
+func sanitizeCoins(coins []Coin) Coins {
+	newCoins := removeZeroCoins(coins)
+	if len(newCoins) == 0 {
+		return Coins{}
+	}
+
+	return newCoins.Sort()
+}
+
 type coinsJSON Coins
 
 // MarshalJSON implements a custom JSON marshaller for the Coins type to allow
@@ -644,8 +647,11 @@ func ParseCoin(coinStr string) (coin Coin, err error) {
 	return NewCoin(denomStr, amount), nil
 }
 
-// ParseCoins will parse out a list of coins separated by commas. If nothing is provided, it returns
-// nil Coins. If the coins aren't valid they return an error. Returned coins are sorted.
+// ParseCoins will parse out a list of coins separated by commas. If the parsing is successuful,
+// the provided coins will be sanitized by removing zero coins and sorting the coin set. Lastly
+// a validation of the coin set is executed. If the check passes, ParseCoins will return the sanitized coins.
+// Otherwise it will return an error.
+// If an empty string is provided to ParseCoins, it returns nil Coins.
 // Expected format: "{amount0}{denomination},...,{amountN}{denominationN}"
 func ParseCoins(coinsStr string) (Coins, error) {
 	coinsStr = strings.TrimSpace(coinsStr)
@@ -664,13 +670,10 @@ func ParseCoins(coinsStr string) (Coins, error) {
 		coins[i] = coin
 	}
 
-	// sort coins for determinism
-	coins.Sort()
-
-	// validate coins before returning
-	if err := coins.Validate(); err != nil {
+	newCoins := sanitizeCoins(coins)
+	if err := newCoins.Validate(); err != nil {
 		return nil, err
 	}
 
-	return coins, nil
+	return newCoins, nil
 }
diff --git a/types/coin_test.go b/types/coin_test.go
@@ -612,7 +612,7 @@ func TestCoinsLTE(t *testing.T) {
 	assert.True(t, Coins{}.IsAllLTE(Coins{{testDenom1, one}}))
 }
 
-func TestParse(t *testing.T) {
+func TestParseCoins(t *testing.T) {
 	one := OneInt()
 
 	cases := []struct {
@@ -621,7 +621,10 @@ func TestParse(t *testing.T) {
 		expected Coins // if valid is true, make sure this is returned
 	}{
 		{"", true, nil},
+		{"0stake", true, Coins{}}, // remove zero coins
+		{"0stake,1foo,99bar", true, Coins{{"bar", NewInt(99)}, {"foo", one}}}, // remove zero coins
 		{"1foo", true, Coins{{"foo", one}}},
+		{"10btc,1atom,20btc", false, nil},
 		{"10bar", true, Coins{{"bar", NewInt(10)}}},
 		{"99bar,1foo", true, Coins{{"bar", NewInt(99)}, {"foo", one}}},
 		{"98 bar , 1 foo  ", true, Coins{{"bar", NewInt(98)}, {"foo", one}}},

diff --git a/types/dec_coin.go b/types/dec_coin.go
@@ -158,23 +158,27 @@ func (coin DecCoin) IsValid() bool {
 type DecCoins []DecCoin
 
 // NewDecCoins constructs a new coin set with with decimal values
-// from DecCoins.
+// from DecCoins. The provided coins will be sanitized by removing
+// zero coins and sorting the coin set. A panic will occur if the coin set is not valid.
 func NewDecCoins(decCoins ...DecCoin) DecCoins {
-	// remove zeroes
-	newDecCoins := removeZeroDecCoins(DecCoins(decCoins))
-	if len(newDecCoins) == 0 {
-		return DecCoins{}
-	}
-
-	newDecCoins.Sort()
-
+	newDecCoins := sanitizeDecCoins(decCoins)
 	if err := newDecCoins.Validate(); err != nil {
 		panic(fmt.Errorf("invalid coin set %s: %w", newDecCoins, err))
 	}
 
 	return newDecCoins
 }
 
+func sanitizeDecCoins(decCoins []DecCoin) DecCoins {
+	// remove zeroes
+	newDecCoins := removeZeroDecCoins(decCoins)
+	if len(newDecCoins) == 0 {
+		return DecCoins{}
+	}
+
+	return newDecCoins.Sort()
+}
+
 // NewDecCoinsFromCoins constructs a new coin set with decimal values
 // from regular Coins.
 func NewDecCoinsFromCoins(coins ...Coin) DecCoins {
@@ -630,32 +634,33 @@ func ParseDecCoin(coinStr string) (coin DecCoin, err error) {
 	return NewDecCoinFromDec(denomStr, amount), nil
 }
 
-// ParseDecCoins will parse out a list of decimal coins separated by commas. If nothing is provided,
-// it returns nil DecCoins. If the coins aren't valid they return an error. Returned coins are sorted.
+// ParseDecCoins will parse out a list of decimal coins separated by commas. If the parsing is successuful,
+// the provided coins will be sanitized by removing zero coins and sorting the coin set. Lastly
+// a validation of the coin set is executed. If the check passes, ParseDecCoins will return the sanitized coins.
+// Otherwise it will return an error.
+// If an empty string is provided to ParseDecCoins, it returns nil Coins.
+// Expected format: "{amount0}{denomination},...,{amountN}{denominationN}"
 func ParseDecCoins(coinsStr string) (DecCoins, error) {
 	coinsStr = strings.TrimSpace(coinsStr)
 	if len(coinsStr) == 0 {
 		return nil, nil
 	}
 
 	coinStrs := strings.Split(coinsStr, ",")
-	coins := make(DecCoins, len(coinStrs))
+	decCoins := make(DecCoins, len(coinStrs))
 	for i, coinStr := range coinStrs {
 		coin, err := ParseDecCoin(coinStr)
 		if err != nil {
 			return nil, err
 		}
 
-		coins[i] = coin
+		decCoins[i] = coin
 	}
 
-	// sort coins for determinism
-	coins.Sort()
-
-	// validate coins before returning
-	if err := coins.Validate(); err != nil {
+	newDecCoins := sanitizeDecCoins(decCoins)
+	if err := newDecCoins.Validate(); err != nil {
 		return nil, err
 	}
 
-	return coins, nil
+	return newDecCoins, nil
 }
diff --git a/types/dec_coin_test.go b/types/dec_coin_test.go
@@ -348,7 +348,8 @@ func TestParseDecCoins(t *testing.T) {
 		{"", nil, false},
 		{"4stake", nil, true},
 		{"5.5atom,4stake", nil, true},
-		{"0.0stake", nil, true},
+		{"0.0stake", DecCoins{}, false}, // remove zero coins
+		{"10.0btc,1.0atom,20.0btc", nil, true},
 		{
 			"0.004STAKE",
 			DecCoins{NewDecCoinFromDec("STAKE", NewDecWithPrec(4000000000000000, Precision))},
@@ -367,6 +368,13 @@ func TestParseDecCoins(t *testing.T) {
 			},
 			false,
 		},
+		{"0.0stake,0.004stake,5.04atom", // remove zero coins
+			DecCoins{
+				NewDecCoinFromDec("atom", NewDecWithPrec(5040000000000000000, Precision)),
+				NewDecCoinFromDec("stake", NewDecWithPrec(4000000000000000, Precision)),
+			},
+			false,
+		},
 	}
 
 	for i, tc := range testCases {