Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: types: correctly coalesce coins even with repeated denominations & simplify logic #13265

Merged
merged 2 commits into from Sep 14, 2022
Merged

fix: types: correctly coalesce coins even with repeated denominations & simplify logic #13265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89a0948
fix: types: correctly coalesce coins even with repeated denominations…
odeke-em Sep 13, 2022
0e89a14
Merge branch 'main' into types-Coin.Add-properly-coalesce-repeated-de…
odeke-em Sep 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
57 changes: 15 additions & 42 deletions types/coin.go
View file
Open in desktop
Expand Up @@ -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() {
Expand All @@ -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
Expand Down
48 changes: 39 additions & 9 deletions types/coin_test.go
View file
Open in desktop
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
}
}

Expand Down