From 8e8166fbfd2294b93d239792f4caa000ae36ba32 Mon Sep 17 00:00:00 2001 From: Emmanuel T Odeke Date: Wed, 14 Sep 2022 06:29:45 -0700 Subject: [PATCH] fix: types: correctly coalesce coins even with repeated denominations & simplify logic (#13265) (cherry picked from commit 83f88a68208d0c80d525de1241820a3a77261c97) # Conflicts: # types/coin_test.go --- types/coin.go | 57 ++++++++++++---------------------------------- types/coin_test.go | 52 ++++++++++++++++++++++++++++++++++-------- 2 files changed, 58 insertions(+), 51 deletions(-) diff --git a/types/coin.go b/types/coin.go index acf3ba13b27d..12b373c59230 100644 --- a/types/coin.go +++ b/types/coin.go @@ -310,7 +310,7 @@ func (coins Coins) Add(coinsB ...Coin) Coins { // denomination and addition only occurs when the denominations match, otherwise // the coin is simply added to the sum assuming it's not zero. // The function panics if `coins` or `coinsB` are not sorted (ascending). -func (coins Coins) safeAdd(coinsB Coins) Coins { +func (coins Coins) safeAdd(coinsB Coins) (coalesced Coins) { // probably the best way will be to make Coins and interface and hide the structure // definition (type alias) if !coins.isSorted() { @@ -320,51 +320,24 @@ func (coins Coins) safeAdd(coinsB Coins) Coins { panic("Wrong argument: coins must be sorted") } - sum := ([]Coin)(nil) - indexA, indexB := 0, 0 - lenA, lenB := len(coins), len(coinsB) - - for { - if indexA == lenA { - if indexB == lenB { - // return nil coins if both sets are empty - return sum - } - - // return set B (excluding zero coins) if set A is empty - return append(sum, removeZeroCoins(coinsB[indexB:])...) - } else if indexB == lenB { - // return set A (excluding zero coins) if set B is empty - return append(sum, removeZeroCoins(coins[indexA:])...) + uniqCoins := make(map[string]Coins, len(coins)+len(coinsB)) + // Traverse all the coins for each of the coins and coinsB. + for _, cL := range []Coins{coins, coinsB} { + for _, c := range cL { + uniqCoins[c.Denom] = append(uniqCoins[c.Denom], c) } + } - coinA, coinB := coins[indexA], coinsB[indexB] - - switch strings.Compare(coinA.Denom, coinB.Denom) { - case -1: // coin A denom < coin B denom - if !coinA.IsZero() { - sum = append(sum, coinA) - } - - indexA++ - - case 0: // coin A denom == coin B denom - res := coinA.Add(coinB) - if !res.IsZero() { - sum = append(sum, res) - } - - indexA++ - indexB++ - - case 1: // coin A denom > coin B denom - if !coinB.IsZero() { - sum = append(sum, coinB) - } - - indexB++ + for denom, cL := range uniqCoins { + comboCoin := Coin{Denom: denom, Amount: NewInt(0)} + for _, c := range cL { + comboCoin = comboCoin.Add(c) + } + if !comboCoin.IsZero() { + coalesced = append(coalesced, comboCoin) } } + return coalesced.Sort() } // DenomsSubsetOf returns true if receiver's denom set diff --git a/types/coin_test.go b/types/coin_test.go index 8f2e6c92d023..2b908a167b54 100644 --- a/types/coin_test.go +++ b/types/coin_test.go @@ -5,6 +5,11 @@ import ( "strings" "testing" +<<<<<<< HEAD +======= + "cosmossdk.io/math" + "github.com/stretchr/testify/require" +>>>>>>> 83f88a682 (fix: types: correctly coalesce coins even with repeated denominations & simplify logic (#13265)) "github.com/stretchr/testify/suite" "github.com/cosmos/cosmos-sdk/codec" @@ -508,21 +513,50 @@ func (s *coinTestSuite) TestEqualCoins() { func (s *coinTestSuite) TestAddCoins() { cases := []struct { + name string inputOne sdk.Coins inputTwo sdk.Coins expected sdk.Coins }{ - {sdk.Coins{s.ca1, s.cm1}, sdk.Coins{s.ca1, s.cm1}, sdk.Coins{s.ca2, s.cm2}}, - {sdk.Coins{s.ca0, s.cm1}, sdk.Coins{s.ca0, s.cm0}, sdk.Coins{s.cm1}}, - {sdk.Coins{s.ca2}, sdk.Coins{s.cm0}, sdk.Coins{s.ca2}}, - {sdk.Coins{s.ca1}, sdk.Coins{s.ca1, s.cm2}, sdk.Coins{s.ca2, s.cm2}}, - {sdk.Coins{s.ca0, s.cm0}, sdk.Coins{s.ca0, s.cm0}, sdk.Coins(nil)}, + {"{1atom,1muon}+{1atom,1muon}", sdk.Coins{s.ca1, s.cm1}, sdk.Coins{s.ca1, s.cm1}, sdk.Coins{s.ca2, s.cm2}}, + {"{0atom,1muon}+{0atom,0muon}", sdk.Coins{s.ca0, s.cm1}, sdk.Coins{s.ca0, s.cm0}, sdk.Coins{s.cm1}}, + {"{2atom}+{0muon}", sdk.Coins{s.ca2}, sdk.Coins{s.cm0}, sdk.Coins{s.ca2}}, + {"{1atom}+{1atom,2muon}", sdk.Coins{s.ca1}, sdk.Coins{s.ca1, s.cm2}, sdk.Coins{s.ca2, s.cm2}}, + {"{0atom,0muon}+{0atom,0muon}", sdk.Coins{s.ca0, s.cm0}, sdk.Coins{s.ca0, s.cm0}, sdk.Coins(nil)}, } - for tcIndex, tc := range cases { - res := tc.inputOne.Add(tc.inputTwo...) - s.Require().True(res.IsValid()) - s.Require().Equal(tc.expected, res, "sum of coins is incorrect, tc #%d", tcIndex) + for _, tc := range cases { + s.T().Run(tc.name, func(t *testing.T) { + res := tc.inputOne.Add(tc.inputTwo...) + require.True(t, res.IsValid(), fmt.Sprintf("%s + %s = %s", tc.inputOne, tc.inputTwo, res)) + require.Equal(t, tc.expected, res, "sum of coins is incorrect") + }) + } +} + +// Tests that even if coins with repeated denominations are passed into .Add that they +// are correctly coalesced. Please see issue https://github.com/cosmos/cosmos-sdk/issues/13234 +func TestCoinsAddCoalescesDuplicateDenominations(t *testing.T) { + A := sdk.Coins{ + {"den", sdk.NewInt(2)}, + {"den", sdk.NewInt(3)}, + } + B := sdk.Coins{ + {"den", sdk.NewInt(3)}, + {"den", sdk.NewInt(2)}, + {"den", sdk.NewInt(1)}, + } + + A = A.Sort() + B = B.Sort() + got := A.Add(B...) + + want := sdk.Coins{ + {"den", sdk.NewInt(11)}, + } + + if !got.IsEqual(want) { + t.Fatalf("Wrong result\n\tGot: %s\n\tWant: %s", got, want) } }