From 89a094891709fe8e7f21d9648a31e2378da6f560 Mon Sep 17 00:00:00 2001 From: Emmanuel T Odeke Date: Tue, 13 Sep 2022 14:13:53 -0700 Subject: [PATCH] fix: types: correctly coalesce coins even with repeated denominations & simplify logic This code correctly coalesces coins with repeated denominations which can come from mistakenly duplicated or split up coins being passed in. However, the code MUST always function correctly regardless of assumptions that no one should ever pass in duplicated denominations. While here simplified the prior clever but hard to understand code, by making the code directly implementing deduplicating the coalescing. Credit to the Ingenuity/Quicksilver codebase which exposed this problem from an observation we made. Fixes #13234 --- types/coin.go | 57 ++++++++++++---------------------------------- types/coin_test.go | 48 ++++++++++++++++++++++++++++++-------- 2 files changed, 54 insertions(+), 51 deletions(-) diff --git a/types/coin.go b/types/coin.go index 52155a6edd13..68d604105ca5 100644 --- a/types/coin.go +++ b/types/coin.go @@ -319,7 +319,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() { @@ -329,51 +329,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 02b1d8b9f434..7172c771989b 100644 --- a/types/coin_test.go +++ b/types/coin_test.go @@ -6,6 +6,7 @@ import ( "testing" "cosmossdk.io/math" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "github.com/cosmos/cosmos-sdk/codec" @@ -536,21 +537,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) } }