From 3988941942d9aa5f21696655c5cd07842f0ceb3f Mon Sep 17 00:00:00 2001 From: jaeseung-bae <119839167+jaeseung-bae@users.noreply.github.com> Date: Wed, 27 Mar 2024 20:38:34 +0900 Subject: [PATCH] fix: correctly coalesce coins even with repeated denominations(backport cosmos/cosmos-sdk#13265) (#1313) * fix: correctly coalesce coins even with repeated denominations * chore: update changelog (cherry picked from commit 01762150b8c857d62f76b172113129eb2c1b96e0) # Conflicts: # CHANGELOG.md --- CHANGELOG.md | 20 +++++++++ types/coin.go | 57 +++++++------------------ types/coin_test.go | 48 +++++++++++++++++---- x/auth/vesting/types/vesting_account.go | 2 +- x/gov/types/params.go | 2 +- 5 files changed, 76 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eb197ca3e7..575edc4f48 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,11 +43,31 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements ### Bug Fixes +<<<<<<< HEAD * (x/auth) [#1281](https://github.com/Finschia/finschia-sdk/pull/1281) `ModuleAccount.Validate` now reports a nil `.BaseAccount` instead of panicking. (backport #1274) * (x/foundation) [\#1283](https://github.com/Finschia/finschia-sdk/pull/1283) add init logic of foundation module accounts to InitGenesis in order to eliminate potential panic (backport #1277) * (x/collection) [\#1282](https://github.com/Finschia/finschia-sdk/pull/1282) eliminates potential risk for Insufficient Sanity Check of tokenID in Genesis (backport #1276) * (x/collection) [\#1290](https://github.com/Finschia/finschia-sdk/pull/1290) export x/collection params into genesis (backport #1268) * (x/foundation) [\#1295](https://github.com/Finschia/finschia-sdk/pull/1295) add missing error handling for migration +======= +* chore(deps) [\#1141](https://github.com/Finschia/finschia-sdk/pull/1141) Bump github.com/cosmos/ledger-cosmos-go from 0.12.2 to 0.13.2 to fix ledger signing issue +* (x/auth, x/slashing) [\#1179](https://github.com/Finschia/finschia-sdk/pull/1179) modify missing changes of converting to tendermint +* (x/auth) [#1274](https://github.com/Finschia/finschia-sdk/pull/1274) `ModuleAccount.Validate` now reports a nil `.BaseAccount` instead of panicking. +* (x/collection) [\#1276](https://github.com/Finschia/finschia-sdk/pull/1276) eliminates potential risk for Insufficient Sanity Check of tokenID in Genesis +* (x/foundation) [\#1277](https://github.com/Finschia/finschia-sdk/pull/1277) add init logic of foundation module accounts to InitGenesis in order to eliminate potential panic +* (x/collection, x/token) [\#1288](https://github.com/Finschia/finschia-sdk/pull/1288) use accAddress to compare in validatebasic function in collection & token modules +* (x/collection) [\#1268](https://github.com/Finschia/finschia-sdk/pull/1268) export x/collection params into genesis +* (x/collection) [\#1294](https://github.com/Finschia/finschia-sdk/pull/1294) reject NFT coins on FT APIs +* (sec) [\#1302](https://github.com/Finschia/finschia-sdk/pull/1302) remove map iteration non-determinism with keys + sorting +* (client) [\#1303](https://github.com/Finschia/finschia-sdk/pull/1303) fix possible overflow in BuildUnsignedTx +* (types) [\#1299](https://github.com/Finschia/finschia-sdk/pull/1299) add missing nil checks +* (x/staking) [\#1301](https://github.com/Finschia/finschia-sdk/pull/1301) Use bytes instead of string comparison in delete validator queue (backport cosmos/cosmos-sdk#12303) +* (x/gov) [\#1304](https://github.com/Finschia/finschia-sdk/pull/1304) fetch a failed proposal tally from proposal.FinalTallyResult in the gprc query +* (x/staking) [\#1306](https://github.com/Finschia/finschia-sdk/pull/1306) add validation for potential slashing evasion during re-delegation +* (client/keys) [#1312](https://github.com/Finschia/finschia-sdk/pull/1312) ignore error when key not found in `keys delete` +* (store) [\#1310](https://github.com/Finschia/finschia-sdk/pull/1310) fix app-hash mismatch if upgrade migration commit is interrupted(backport cosmos/cosmos-sdk#13530) +* (types) [\#1313](https://github.com/Finschia/finschia-sdk/pull/1313) fix correctly coalesce coins even with repeated denominations(backport cosmos/cosmos-sdk#13265) +>>>>>>> 01762150b (fix: correctly coalesce coins even with repeated denominations(backport cosmos/cosmos-sdk#13265) (#1313)) ### Removed diff --git a/types/coin.go b/types/coin.go index 01358f414b..ec9bcde3e2 100644 --- a/types/coin.go +++ b/types/coin.go @@ -290,7 +290,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() { @@ -300,51 +300,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 8f48e89fec..d36fba8396 100644 --- a/types/coin_test.go +++ b/types/coin_test.go @@ -5,6 +5,7 @@ import ( "strings" "testing" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "github.com/Finschia/finschia-sdk/codec" @@ -429,21 +430,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.Equal(want) { + t.Fatalf("Wrong result\n\tGot: %s\n\tWant: %s", got, want) } } diff --git a/x/auth/vesting/types/vesting_account.go b/x/auth/vesting/types/vesting_account.go index d3e385cab6..0f74902324 100644 --- a/x/auth/vesting/types/vesting_account.go +++ b/x/auth/vesting/types/vesting_account.go @@ -4,7 +4,7 @@ import ( "errors" "time" - yaml "gopkg.in/yaml.v2" + "gopkg.in/yaml.v2" cryptotypes "github.com/Finschia/finschia-sdk/crypto/types" sdk "github.com/Finschia/finschia-sdk/types" diff --git a/x/gov/types/params.go b/x/gov/types/params.go index c433fa8f63..fddd32ad17 100644 --- a/x/gov/types/params.go +++ b/x/gov/types/params.go @@ -4,7 +4,7 @@ import ( "fmt" "time" - yaml "gopkg.in/yaml.v2" + "gopkg.in/yaml.v2" sdk "github.com/Finschia/finschia-sdk/types" paramtypes "github.com/Finschia/finschia-sdk/x/params/types"