Fix for 6786 removeZeroCoins mutates original slice (#6811)

* added test cases to show errors identified in 6786

* mods to fix remove zero coin mutation in slice

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

types/coin.go

@@ -546,18 +546,15 @@ func (coins Coins) negative() Coins {
 
 // removeZeroCoins removes all zero coins from the given coin set in-place.
 func removeZeroCoins(coins Coins) Coins {
-	i, l := 0, len(coins)
+	result := make([]Coin, 0, len(coins))
-	for i < l {
+
-		if coins[i].IsZero() {
+	for _, coin := range coins {
-			// remove coin
+		if !coin.IsZero() {
-			coins = append(coins[:i], coins[i+1:]...)
+			result = append(result, coin)
-			l--
-		} else {
-			i++
 		}
 	}
 
-	return coins[:i]
+	return result
 }
 
 //-----------------------------------------------------------------------------

types/coin_test.go

@@ -191,6 +191,61 @@ func TestCoinIsZero(t *testing.T) {
 	require.False(t, res)
 }
 
+func TestFilteredZeroCoins(t *testing.T) {
+	cases := []struct {
+		name     string
+		input    Coins
+		original string
+		expected string
+	}{
+		{
+			name: "all greater than zero",
+			input: Coins{
+				{"testa", NewInt(1)},
+				{"testb", NewInt(2)},
+				{"testc", NewInt(3)},
+				{"testd", NewInt(4)},
+				{"teste", NewInt(5)},
+			},
+			original: "1testa,2testb,3testc,4testd,5teste",
+			expected: "1testa,2testb,3testc,4testd,5teste",
+		},
+		{
+			name: "zero coin in middle",
+			input: Coins{
+				{"testa", NewInt(1)},
+				{"testb", NewInt(2)},
+				{"testc", NewInt(0)},
+				{"testd", NewInt(4)},
+				{"teste", NewInt(5)},
+			},
+			original: "1testa,2testb,0testc,4testd,5teste",
+			expected: "1testa,2testb,4testd,5teste",
+		},
+		{
+			name: "zero coin end (unordered)",
+			input: Coins{
+				{"teste", NewInt(5)},
+				{"testc", NewInt(3)},
+				{"testa", NewInt(1)},
+				{"testd", NewInt(4)},
+				{"testb", NewInt(0)},
+			},
+			original: "5teste,3testc,1testa,4testd,0testb",
+			expected: "1testa,3testc,4testd,5teste",
+		},
+	}
+
+	for _, tt := range cases {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			undertest := NewCoins(tt.input...)
+			require.Equal(t, tt.expected, undertest.String(), "NewCoins must return expected results")
+			require.Equal(t, tt.original, tt.input.String(), "input must be unmodified and match original")
+		})
+	}
+}
+
 // ----------------------------------------------------------------------------
 // Coins tests
 

types/dec_coin.go

@@ -556,18 +556,15 @@ func (coins DecCoins) IsAllPositive() bool {
 }
 
 func removeZeroDecCoins(coins DecCoins) DecCoins {
-	i, l := 0, len(coins)
+	result := make([]DecCoin, 0, len(coins))
-	for i < l {
+
-		if coins[i].IsZero() {
+	for _, coin := range coins {
-			// remove coin
+		if !coin.IsZero() {
-			coins = append(coins[:i], coins[i+1:]...)
+			result = append(result, coin)
-			l--
-		} else {
-			i++
 		}
 	}
 
-	return coins[:i]
+	return result
 }
 
 //-----------------------------------------------------------------------------

types/dec_coin_test.go

@@ -96,6 +96,61 @@ func TestAddDecCoins(t *testing.T) {
 	}
 }
 
+func TestFilteredZeroDecCoins(t *testing.T) {
+	cases := []struct {
+		name     string
+		input    DecCoins
+		original string
+		expected string
+	}{
+		{
+			name: "all greater than zero",
+			input: DecCoins{
+				{"testa", NewDec(1)},
+				{"testb", NewDec(2)},
+				{"testc", NewDec(3)},
+				{"testd", NewDec(4)},
+				{"teste", NewDec(5)},
+			},
+			original: "1.000000000000000000testa,2.000000000000000000testb,3.000000000000000000testc,4.000000000000000000testd,5.000000000000000000teste",
+			expected: "1.000000000000000000testa,2.000000000000000000testb,3.000000000000000000testc,4.000000000000000000testd,5.000000000000000000teste",
+		},
+		{
+			name: "zero coin in middle",
+			input: DecCoins{
+				{"testa", NewDec(1)},
+				{"testb", NewDec(2)},
+				{"testc", NewDec(0)},
+				{"testd", NewDec(4)},
+				{"teste", NewDec(5)},
+			},
+			original: "1.000000000000000000testa,2.000000000000000000testb,0.000000000000000000testc,4.000000000000000000testd,5.000000000000000000teste",
+			expected: "1.000000000000000000testa,2.000000000000000000testb,4.000000000000000000testd,5.000000000000000000teste",
+		},
+		{
+			name: "zero coin end (unordered)",
+			input: DecCoins{
+				{"teste", NewDec(5)},
+				{"testc", NewDec(3)},
+				{"testa", NewDec(1)},
+				{"testd", NewDec(4)},
+				{"testb", NewDec(0)},
+			},
+			original: "5.000000000000000000teste,3.000000000000000000testc,1.000000000000000000testa,4.000000000000000000testd,0.000000000000000000testb",
+			expected: "1.000000000000000000testa,3.000000000000000000testc,4.000000000000000000testd,5.000000000000000000teste",
+		},
+	}
+
+	for _, tt := range cases {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			undertest := NewDecCoins(tt.input...)
+			require.Equal(t, tt.expected, undertest.String(), "NewDecCoins must return expected results")
+			require.Equal(t, tt.original, tt.input.String(), "input must be unmodified and match original")
+		})
+	}
+}
+
 func TestIsValid(t *testing.T) {
 	tests := []struct {
 		coin       DecCoin