perf: Refactor Coins/Validate to avoid unnecessary map (backport #14163…

…) (#14182) Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>
cosmos · Dec 8, 2022 · 4997976 · 4997976
1 parent 50f8de4
commit 4997976
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 8 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -46,6 +46,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 * (genesis) [#14149](https://github.com/cosmos/cosmos-sdk/pull/14149) Add `simd genesis` command, which contains all genesis-related sub-commands.
 
+### Improvements
+
+* (types) [#14163](https://github.com/cosmos/cosmos-sdk/pull/14163) Refactor `(coins Coins) Validate()` to avoid unnecessary map.
+
 ## [v0.47.0-alpha2](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.0-alpha2) - 2022-12-06
 
 ### Improvements

diff --git a/types/coin.go b/types/coin.go
@@ -247,26 +247,23 @@ func (coins Coins) Validate() error {
 		}
 
 		lowDenom := coins[0].Denom
-		seenDenoms := make(map[string]bool)
-		seenDenoms[lowDenom] = true
 
 		for _, coin := range coins[1:] {
-			if seenDenoms[coin.Denom] {
-				return fmt.Errorf("duplicate denomination %s", coin.Denom)
-			}
 			if err := ValidateDenom(coin.Denom); err != nil {
 				return err
 			}
-			if coin.Denom <= lowDenom {
+			if coin.Denom < lowDenom {
 				return fmt.Errorf("denomination %s is not sorted", coin.Denom)
 			}
+			if coin.Denom == lowDenom {
+				return fmt.Errorf("duplicate denomination %s", coin.Denom)
+			}
 			if !coin.IsPositive() {
 				return fmt.Errorf("coin %s amount is not positive", coin.Denom)
 			}
 
 			// we compare each coin against the last denom
 			lowDenom = coin.Denom
-			seenDenoms[coin.Denom] = true
 		}
 
 		return nil

diff --git a/types/coin_test.go b/types/coin_test.go
@@ -778,6 +778,15 @@ func (s *coinTestSuite) TestCoins_Validate() {
 			},
 			false,
 		},
+		{
+			"bad sort (3)",
+			sdk.Coins{
+				{"gas", math.OneInt()},
+				{"tree", math.OneInt()},
+				{"gas", math.OneInt()},
+			},
+			false,
+		},
 		{
 			"non-positive amount (1)",
 			sdk.Coins{
@@ -797,14 +806,32 @@ func (s *coinTestSuite) TestCoins_Validate() {
 			false,
 		},
 		{
-			"duplicate denomination",
+			"duplicate denomination (1)",
 			sdk.Coins{
 				{"gas", math.OneInt()},
 				{"gas", math.OneInt()},
 				{"mineral", math.OneInt()},
 			},
 			false,
 		},
+		{
+			"duplicate denomination (2)",
+			sdk.Coins{
+				{"gold", math.OneInt()},
+				{"gold", math.OneInt()},
+			},
+			false,
+		},
+		{
+			"duplicate denomination (3)",
+			sdk.Coins{
+				{"gas", math.OneInt()},
+				{"mineral", math.OneInt()},
+				{"silver", math.OneInt()},
+				{"silver", math.OneInt()},
+			},
+			false,
+		},
 	}
 
 	for _, tc := range testCases {