From 68d461052bc3a96184524c4f3fc7484b14078ff8 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Wed, 5 May 2021 17:55:28 +0200 Subject: [PATCH] add sorted check for the coins sub/add fun parameter (#9240) * add sorted check for the coins sub/add fun parameter * adding internal tests * fix tests * docs update * add self sorted check * add unit test for self sorted check * adding a comment * review updates Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> --- types/coin.go | 21 +++++++++++++++++ types/coin_internal_test.go | 37 ++++++++++++++++++++++++++++++ types/coin_test.go | 45 ++++++++++++++++++++----------------- 3 files changed, 82 insertions(+), 21 deletions(-) create mode 100644 types/coin_internal_test.go diff --git a/types/coin.go b/types/coin.go index 352dcf6ac5..c6675a1c7a 100644 --- a/types/coin.go +++ b/types/coin.go @@ -243,6 +243,15 @@ func (coins Coins) Validate() error { } } +func (coins Coins) isSorted() bool { + for i := 1; i < len(coins); i++ { + if coins[i-1].Denom > coins[i].Denom { + return false + } + } + return true +} + // IsValid calls Validate and returns true when the Coins are sorted, have positive amount, with a // valid and unique denomination (i.e no duplicates). func (coins Coins) IsValid() bool { @@ -260,6 +269,7 @@ func (coins Coins) IsValid() bool { // // CONTRACT: Add will never return Coins where one Coin has a non-positive // amount. In otherwords, IsValid will always return true. +// The function panics if `coins` or `coinsB` are not sorted (ascending). func (coins Coins) Add(coinsB ...Coin) Coins { return coins.safeAdd(coinsB) } @@ -269,7 +279,17 @@ func (coins Coins) Add(coinsB ...Coin) Coins { // other set is returned. Otherwise, the coins are compared in order of their // 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 { + // probably the best way will be to make Coins and interface and hide the structure + // definition (type alias) + if !coins.isSorted() { + panic("Coins (self) must be sorted") + } + if !coinsB.isSorted() { + panic("Wrong argument: coins must be sorted") + } + sum := ([]Coin)(nil) indexA, indexB := 0, 0 lenA, lenB := len(coins), len(coinsB) @@ -354,6 +374,7 @@ func (coins Coins) Sub(coinsB Coins) Coins { // SafeSub performs the same arithmetic as Sub but returns a boolean if any // negative coin amount was returned. +// The function panics if `coins` or `coinsB` are not sorted (ascending). func (coins Coins) SafeSub(coinsB Coins) (Coins, bool) { diff := coins.safeAdd(coinsB.negative()) return diff, diff.IsAnyNegative() diff --git a/types/coin_internal_test.go b/types/coin_internal_test.go new file mode 100644 index 0000000000..709683795c --- /dev/null +++ b/types/coin_internal_test.go @@ -0,0 +1,37 @@ +package types + +import ( + "testing" + + "github.com/stretchr/testify/suite" +) + +func TestCoinTestSuite(t *testing.T) { + suite.Run(t, new(coinInternalSuite)) +} + +type coinInternalSuite struct { + suite.Suite +} + +func (s *coinInternalSuite) TestIsSorted() { + v := NewInt(1) + cases := []struct { + coins Coins + expected bool + }{ + {Coins{}, true}, + {Coins{{"1", v}}, true}, + {Coins{{"1", v}, {"1", v}}, true}, + {Coins{{"1", v}, {"2", v}}, true}, + {Coins{{"1", v}, {"2", v}, {"2", v}}, true}, + + {Coins{{"1", v}, {"0", v}}, false}, + {Coins{{"1", v}, {"0", v}, {"2", v}}, false}, + {Coins{{"1", v}, {"1", v}, {"0", v}}, false}, + } + assert := s.Assert() + for i, tc := range cases { + assert.Equal(tc.expected, tc.coins.isSorted(), "testcase %d failed", i) + } +} diff --git a/types/coin_test.go b/types/coin_test.go index 1f7af54734..9a5d83fea9 100644 --- a/types/coin_test.go +++ b/types/coin_test.go @@ -18,6 +18,7 @@ var ( type coinTestSuite struct { suite.Suite + ca0, ca1, ca2, cm0, cm1, cm2 sdk.Coin } func TestCoinTestSuite(t *testing.T) { @@ -26,6 +27,11 @@ func TestCoinTestSuite(t *testing.T) { func (s *coinTestSuite) SetupSuite() { s.T().Parallel() + zero := sdk.NewInt(0) + one := sdk.OneInt() + two := sdk.NewInt(2) + s.ca0, s.ca1, s.ca2 = sdk.Coin{testDenom1, zero}, sdk.Coin{testDenom1, one}, sdk.Coin{testDenom1, two} + s.cm0, s.cm1, s.cm2 = sdk.Coin{testDenom2, zero}, sdk.Coin{testDenom2, one}, sdk.Coin{testDenom2, two} } // ---------------------------------------------------------------------------- @@ -409,20 +415,16 @@ func (s *coinTestSuite) TestEqualCoins() { } func (s *coinTestSuite) TestAddCoins() { - zero := sdk.NewInt(0) - one := sdk.OneInt() - two := sdk.NewInt(2) - cases := []struct { inputOne sdk.Coins inputTwo sdk.Coins expected sdk.Coins }{ - {sdk.Coins{{testDenom1, one}, {testDenom2, one}}, sdk.Coins{{testDenom1, one}, {testDenom2, one}}, sdk.Coins{{testDenom1, two}, {testDenom2, two}}}, - {sdk.Coins{{testDenom1, zero}, {testDenom2, one}}, sdk.Coins{{testDenom1, zero}, {testDenom2, zero}}, sdk.Coins{{testDenom2, one}}}, - {sdk.Coins{{testDenom1, two}}, sdk.Coins{{testDenom2, zero}}, sdk.Coins{{testDenom1, two}}}, - {sdk.Coins{{testDenom1, one}}, sdk.Coins{{testDenom1, one}, {testDenom2, two}}, sdk.Coins{{testDenom1, two}, {testDenom2, two}}}, - {sdk.Coins{{testDenom1, zero}, {testDenom2, zero}}, sdk.Coins{{testDenom1, zero}, {testDenom2, zero}}, sdk.Coins(nil)}, + {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)}, } for tcIndex, tc := range cases { @@ -433,31 +435,32 @@ func (s *coinTestSuite) TestAddCoins() { } func (s *coinTestSuite) TestSubCoins() { - zero := sdk.NewInt(0) - one := sdk.OneInt() - two := sdk.NewInt(2) - testCases := []struct { inputOne sdk.Coins inputTwo sdk.Coins expected sdk.Coins shouldPanic bool }{ - {sdk.Coins{{testDenom1, two}}, sdk.Coins{{testDenom1, one}, {testDenom2, two}}, sdk.Coins{{testDenom1, one}, {testDenom2, two}}, true}, - {sdk.Coins{{testDenom1, two}}, sdk.Coins{{testDenom2, zero}}, sdk.Coins{{testDenom1, two}}, false}, - {sdk.Coins{{testDenom1, one}}, sdk.Coins{{testDenom2, zero}}, sdk.Coins{{testDenom1, one}}, false}, - {sdk.Coins{{testDenom1, one}, {testDenom2, one}}, sdk.Coins{{testDenom1, one}}, sdk.Coins{{testDenom2, one}}, false}, - {sdk.Coins{{testDenom1, one}, {testDenom2, one}}, sdk.Coins{{testDenom1, two}}, sdk.Coins{}, true}, + // denoms are not sorted - should panic + {sdk.Coins{s.ca2}, sdk.Coins{s.cm2, s.ca1}, sdk.Coins{}, true}, + {sdk.Coins{s.cm2, s.ca2}, sdk.Coins{s.ca1}, sdk.Coins{}, true}, + // test cases for sorted denoms + {sdk.Coins{s.ca2}, sdk.Coins{s.ca1, s.cm2}, sdk.Coins{s.ca1, s.cm2}, true}, + {sdk.Coins{s.ca2}, sdk.Coins{s.cm0}, sdk.Coins{s.ca2}, false}, + {sdk.Coins{s.ca1}, sdk.Coins{s.cm0}, sdk.Coins{s.ca1}, false}, + {sdk.Coins{s.ca1, s.cm1}, sdk.Coins{s.ca1}, sdk.Coins{s.cm1}, false}, + {sdk.Coins{s.ca1, s.cm1}, sdk.Coins{s.ca2}, sdk.Coins{}, true}, } + assert := s.Assert() for i, tc := range testCases { tc := tc if tc.shouldPanic { - s.Require().Panics(func() { tc.inputOne.Sub(tc.inputTwo) }) + assert.Panics(func() { tc.inputOne.Sub(tc.inputTwo) }) } else { res := tc.inputOne.Sub(tc.inputTwo) - s.Require().True(res.IsValid()) - s.Require().Equal(tc.expected, res, "sum of coins is incorrect, tc #%d", i) + assert.True(res.IsValid()) + assert.Equal(tc.expected, res, "sum of coins is incorrect, tc #%d", i) } } }