diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e1dab849ecc..085535b8b0a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -283,6 +283,10 @@ Buffers for state serialization instead of Amino. ### Improvements +* (types) [\#7027](https://github.com/cosmos/cosmos-sdk/pull/7027) `Coin(s)` and `DecCoin(s)` updates: + * Bump denomination max length to 128 + * Allow unicode letters and numbers in denominations + * Added `Validate` function that returns a descriptive error * (baseapp) [\#6186](https://github.com/cosmos/cosmos-sdk/issues/6186) Support emitting events during `AnteHandler` execution. * (x/auth) [\#5702](https://github.com/cosmos/cosmos-sdk/pull/5702) Add parameter querying support for `x/auth`. * (types) [\#5581](https://github.com/cosmos/cosmos-sdk/pull/5581) Add convenience functions {,Must}Bech32ifyAddressBytes. diff --git a/types/coin.go b/types/coin.go index ad287a0a785b..00ba996271fa 100644 --- a/types/coin.go +++ b/types/coin.go @@ -12,16 +12,18 @@ import ( // Coin // NewCoin returns a new coin with a denomination and amount. It will panic if -// the amount is negative. +// the amount is negative or if the denomination is invalid. func NewCoin(denom string, amount Int) Coin { - if err := validate(denom, amount); err != nil { - panic(err) - } - - return Coin{ + coin := Coin{ Denom: denom, Amount: amount, } + + if err := coin.Validate(); err != nil { + panic(err) + } + + return coin } // NewInt64Coin returns a new coin with a denomination and amount. It will panic @@ -35,26 +37,23 @@ func (coin Coin) String() string { return fmt.Sprintf("%v%v", coin.Amount, coin.Denom) } -// validate returns an error if the Coin has a negative amount or if +// Validate returns an error if the Coin has a negative amount or if // the denom is invalid. -func validate(denom string, amount Int) error { - if err := ValidateDenom(denom); err != nil { +func (coin Coin) Validate() error { + if err := ValidateDenom(coin.Denom); err != nil { return err } - if amount.IsNegative() { - return fmt.Errorf("negative coin amount: %v", amount) + if coin.Amount.IsNegative() { + return fmt.Errorf("negative coin amount: %v", coin.Amount) } return nil } -// IsValid returns true if the Coin has a non-negative amount and the denom is vaild. +// IsValid returns true if the Coin has a non-negative amount and the denom is valid. func (coin Coin) IsValid() bool { - if err := validate(coin.Denom, coin.Amount); err != nil { - return false - } - return true + return coin.Validate() == nil } // IsZero returns if this represents no money @@ -91,7 +90,7 @@ func (coin Coin) IsEqual(other Coin) bool { return coin.Amount.Equal(other.Amount) } -// Adds amounts of two coins with same denom. If the coins differ in denom then +// Add adds amounts of two coins with same denom. If the coins differ in denom then // it panics. func (coin Coin) Add(coinB Coin) Coin { if coin.Denom != coinB.Denom { @@ -101,7 +100,7 @@ func (coin Coin) Add(coinB Coin) Coin { return Coin{coin.Denom, coin.Amount.Add(coinB.Amount)} } -// Subtracts amounts of two coins with same denom. If the coins differ in denom +// Sub subtracts amounts of two coins with same denom. If the coins differ in denom // then it panics. func (coin Coin) Sub(coinB Coin) Coin { if coin.Denom != coinB.Denom { @@ -146,13 +145,8 @@ func NewCoins(coins ...Coin) Coins { newCoins.Sort() - // detect duplicate Denoms - if dupIndex := findDup(newCoins); dupIndex != -1 { - panic(fmt.Errorf("find duplicate denom: %s", newCoins[dupIndex])) - } - - if !newCoins.IsValid() { - panic(fmt.Errorf("invalid coin set: %s", newCoins)) + if err := newCoins.Validate(); err != nil { + panic(fmt.Errorf("invalid coin set %s: %w", newCoins, err)) } return newCoins @@ -182,43 +176,61 @@ func (coins Coins) String() string { return out[:len(out)-1] } -// IsValid asserts the Coins are sorted, have positive amount, -// and Denom does not contain upper case characters. -func (coins Coins) IsValid() bool { +// Validate checks that the Coins are sorted, have positive amount, with a valid and unique +// denomination (i.e no duplicates). Otherwise, it returns an error. +func (coins Coins) Validate() error { switch len(coins) { case 0: - return true + return nil + case 1: if err := ValidateDenom(coins[0].Denom); err != nil { - return false + return err } - return coins[0].IsPositive() + if !coins[0].IsPositive() { + return fmt.Errorf("coin %s amount is not positive", coins[0]) + } + return nil + default: // check single coin case - if !(Coins{coins[0]}).IsValid() { - return false + if err := (Coins{coins[0]}).Validate(); err != nil { + return err } lowDenom := coins[0].Denom + seenDenoms := make(map[string]bool) + seenDenoms[lowDenom] = true + for _, coin := range coins[1:] { - if strings.ToLower(coin.Denom) != coin.Denom { - return false + if seenDenoms[coin.Denom] { + return fmt.Errorf("duplicate denomination %s", coin.Denom) + } + if err := ValidateDenom(coin.Denom); err != nil { + return err } if coin.Denom <= lowDenom { - return false + return fmt.Errorf("denomination %s is not sorted", coin.Denom) } if !coin.IsPositive() { - return false + return fmt.Errorf("coin %s amount is not positive", coin.Denom) } // we compare each coin against the last denom lowDenom = coin.Denom + seenDenoms[coin.Denom] = true } - return true + return nil } } +// 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 { + return coins.Validate() == nil +} + // Add adds two sets of coins. // // e.g. @@ -463,7 +475,7 @@ func (coins Coins) Empty() bool { return len(coins) == 0 } -// Returns the amount of a denom from coins +// AmountOf returns the amount of a denom from coins func (coins Coins) AmountOf(denom string) Int { mustValidateDenom(denom) @@ -560,13 +572,18 @@ func removeZeroCoins(coins Coins) Coins { //----------------------------------------------------------------------------- // Sort interface -func (coins Coins) Len() int { return len(coins) } +// Len implements sort.Interface for Coins +func (coins Coins) Len() int { return len(coins) } + +// Less implements sort.Interface for Coins func (coins Coins) Less(i, j int) bool { return coins[i].Denom < coins[j].Denom } -func (coins Coins) Swap(i, j int) { coins[i], coins[j] = coins[j], coins[i] } + +// Swap implements sort.Interface for Coins +func (coins Coins) Swap(i, j int) { coins[i], coins[j] = coins[j], coins[i] } var _ sort.Interface = Coins{} -// Sort is a helper function to sort the set of coins inplace +// Sort is a helper function to sort the set of coins in-place func (coins Coins) Sort() Coins { sort.Sort(coins) return coins @@ -576,8 +593,9 @@ func (coins Coins) Sort() Coins { // Parsing var ( - // Denominations can be 3 ~ 64 characters long. - reDnmString = `[a-z][a-z0-9/]{2,63}` + // Denominations can be 3 ~ 128 characters long and support letters, followed by either + // a letter, a number or a separator ('/'). + reDnmString = `[a-zA-Z][a-zA-Z0-9/]{2,127}` reAmt = `[[:digit:]]+` reDecAmt = `[[:digit:]]*\.[[:digit:]]+` reSpc = `[[:space:]]*` @@ -601,8 +619,9 @@ func mustValidateDenom(denom string) { } } -// ParseCoin parses a cli input for one coin type, returning errors if invalid. -// This returns an error on an empty string as well. +// ParseCoin parses a cli input for one coin type, returning errors if invalid or on an empty string +// as well. +// Expected format: "{amount}{denomination}" func ParseCoin(coinStr string) (coin Coin, err error) { coinStr = strings.TrimSpace(coinStr) @@ -619,15 +638,15 @@ func ParseCoin(coinStr string) (coin Coin, err error) { } if err := ValidateDenom(denomStr); err != nil { - return Coin{}, fmt.Errorf("invalid denom cannot contain upper case characters or spaces: %s", err) + return Coin{}, err } return NewCoin(denomStr, amount), nil } -// ParseCoins will parse out a list of coins separated by commas. -// If nothing is provided, it returns nil Coins. -// Returned coins are sorted. +// ParseCoins will parse out a list of coins separated by commas. If nothing is provided, it returns +// nil Coins. If the coins aren't valid they return an error. Returned coins are sorted. +// Expected format: "{amount0}{denomination},...,{amountN}{denominationN}" func ParseCoins(coinsStr string) (Coins, error) { coinsStr = strings.TrimSpace(coinsStr) if len(coinsStr) == 0 { @@ -649,31 +668,9 @@ func ParseCoins(coinsStr string) (Coins, error) { coins.Sort() // validate coins before returning - if !coins.IsValid() { - return nil, fmt.Errorf("parseCoins invalid: %#v", coins) + if err := coins.Validate(); err != nil { + return nil, err } return coins, nil } - -type findDupDescriptor interface { - GetDenomByIndex(int) string - Len() int -} - -// findDup works on the assumption that coins is sorted -func findDup(coins findDupDescriptor) int { - if coins.Len() <= 1 { - return -1 - } - - prevDenom := coins.GetDenomByIndex(0) - for i := 1; i < coins.Len(); i++ { - if coins.GetDenomByIndex(i) == prevDenom { - return i - } - prevDenom = coins.GetDenomByIndex(i) - } - - return -1 -} diff --git a/types/coin_test.go b/types/coin_test.go index 9d896861ab8a..9042093bfb42 100644 --- a/types/coin_test.go +++ b/types/coin_test.go @@ -22,8 +22,8 @@ var ( func TestCoin(t *testing.T) { require.Panics(t, func() { NewInt64Coin(testDenom1, -1) }) require.Panics(t, func() { NewCoin(testDenom1, NewInt(-1)) }) - require.Panics(t, func() { NewInt64Coin(strings.ToUpper(testDenom1), 10) }) - require.Panics(t, func() { NewCoin(strings.ToUpper(testDenom1), NewInt(10)) }) + require.Equal(t, NewInt(10), NewInt64Coin(strings.ToUpper(testDenom1), 10).Amount) + require.Equal(t, NewInt(10), NewCoin(strings.ToUpper(testDenom1), NewInt(10)).Amount) require.Equal(t, NewInt(5), NewInt64Coin(testDenom1, 5).Amount) require.Equal(t, NewInt(5), NewCoin(testDenom1, NewInt(5)).Amount) } @@ -57,18 +57,27 @@ func TestIsEqualCoin(t *testing.T) { } func TestCoinIsValid(t *testing.T) { + loremIpsum := `Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nam viverra dui vel nulla aliquet, non dictum elit aliquam. Proin consequat leo in consectetur mattis. Phasellus eget odio luctus, rutrum dolor at, venenatis ante. Praesent metus erat, sodales vitae sagittis eget, commodo non ipsum. Duis eget urna quis erat mattis pulvinar. Vivamus egestas imperdiet sem, porttitor hendrerit lorem pulvinar in. Vivamus laoreet sapien eget libero euismod tristique. Suspendisse tincidunt nulla quis luctus mattis. + Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos. Sed id turpis at erat placerat fermentum id sed sapien. Fusce mattis enim id nulla viverra, eget placerat eros aliquet. Nunc fringilla urna ac condimentum ultricies. Praesent in eros ac neque fringilla sodales. Donec ut venenatis eros. Quisque iaculis lectus neque, a varius sem ullamcorper nec. Cras tincidunt dignissim libero nec volutpat. Donec molestie enim sed metus venenatis, quis elementum sem varius. Curabitur eu venenatis nulla. + Cras sit amet ligula vel turpis placerat sollicitudin. Nunc massa odio, eleifend id lacus nec, ultricies elementum arcu. Donec imperdiet nulla lacus, a venenatis lacus fermentum nec. Proin vestibulum dolor enim, vitae posuere velit aliquet non. Suspendisse pharetra condimentum nunc tincidunt viverra. Etiam posuere, ligula ut maximus congue, mauris orci consectetur velit, vel finibus eros metus non tellus. Nullam et dictum metus. Aliquam maximus fermentum mauris elementum aliquet. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos. Etiam dapibus lectus sed tellus rutrum tincidunt. Nulla at dolor sem. Ut non dictum arcu, eget congue sem.` + + loremIpsum = strings.ReplaceAll(loremIpsum, " ", "") + loremIpsum = strings.ReplaceAll(loremIpsum, ".", "") + loremIpsum = strings.ReplaceAll(loremIpsum, ",", "") + cases := []struct { coin Coin expectPass bool }{ {Coin{testDenom1, NewInt(-1)}, false}, {Coin{testDenom1, NewInt(0)}, true}, - {Coin{testDenom1, NewInt(1)}, true}, - {Coin{"Atom", NewInt(1)}, false}, - {Coin{"a", NewInt(1)}, false}, - {Coin{"a very long coin denom", NewInt(1)}, false}, - {Coin{"atOm", NewInt(1)}, false}, - {Coin{" ", NewInt(1)}, false}, + {Coin{testDenom1, OneInt()}, true}, + {Coin{"Atom", OneInt()}, true}, + {Coin{"ATOM", OneInt()}, true}, + {Coin{"a", OneInt()}, false}, + {Coin{loremIpsum, OneInt()}, false}, + {Coin{"atOm", OneInt()}, true}, + {Coin{" ", OneInt()}, false}, } for i, tc := range cases { @@ -201,7 +210,7 @@ func TestFilteredZeroCoins(t *testing.T) { { name: "all greater than zero", input: Coins{ - {"testa", NewInt(1)}, + {"testa", OneInt()}, {"testb", NewInt(2)}, {"testc", NewInt(3)}, {"testd", NewInt(4)}, @@ -213,7 +222,7 @@ func TestFilteredZeroCoins(t *testing.T) { { name: "zero coin in middle", input: Coins{ - {"testa", NewInt(1)}, + {"testa", OneInt()}, {"testb", NewInt(2)}, {"testc", NewInt(0)}, {"testd", NewInt(4)}, @@ -227,7 +236,7 @@ func TestFilteredZeroCoins(t *testing.T) { input: Coins{ {"teste", NewInt(5)}, {"testc", NewInt(3)}, - {"testa", NewInt(1)}, + {"testa", OneInt()}, {"testd", NewInt(4)}, {"testb", NewInt(0)}, }, @@ -256,25 +265,23 @@ func TestCoins_String(t *testing.T) { expected string }{ { - name: "empty coins", - input: Coins{}, - expected: "", + "empty coins", + Coins{}, + "", }, { - name: "single coin", - input: Coins{ - {"tree", NewInt(1)}, - }, - expected: "1tree", + "single coin", + Coins{{"tree", OneInt()}}, + "1tree", }, { - name: "multiple coins", - input: Coins{ - {"tree", NewInt(1)}, - {"gas", NewInt(1)}, - {"mineral", NewInt(1)}, + "multiple coins", + Coins{ + {"tree", OneInt()}, + {"gas", OneInt()}, + {"mineral", OneInt()}, }, - expected: "1tree,1gas,1mineral", + "1tree,1gas,1mineral", }, } @@ -333,7 +340,7 @@ func TestEqualCoins(t *testing.T) { func TestAddCoins(t *testing.T) { zero := NewInt(0) - one := NewInt(1) + one := OneInt() two := NewInt(2) cases := []struct { @@ -357,7 +364,7 @@ func TestAddCoins(t *testing.T) { func TestSubCoins(t *testing.T) { zero := NewInt(0) - one := NewInt(1) + one := OneInt() two := NewInt(2) testCases := []struct { @@ -385,70 +392,185 @@ func TestSubCoins(t *testing.T) { } } -func TestCoins(t *testing.T) { - good := Coins{ - {"gas", NewInt(1)}, - {"mineral", NewInt(1)}, - {"tree", NewInt(1)}, - } - mixedCase1 := Coins{ - {"gAs", NewInt(1)}, - {"MineraL", NewInt(1)}, - {"TREE", NewInt(1)}, - } - mixedCase2 := Coins{ - {"gAs", NewInt(1)}, - {"mineral", NewInt(1)}, - } - mixedCase3 := Coins{ - {"gAs", NewInt(1)}, - } - empty := NewCoins() - badSort1 := Coins{ - {"tree", NewInt(1)}, - {"gas", NewInt(1)}, - {"mineral", NewInt(1)}, +func TestCoins_Validate(t *testing.T) { + testCases := []struct { + name string + coins Coins + expPass bool + }{ + { + "valid lowercase coins", + Coins{ + {"gas", OneInt()}, + {"mineral", OneInt()}, + {"tree", OneInt()}, + }, + true, + }, + { + "valid uppercase coins", + Coins{ + {"GAS", OneInt()}, + {"MINERAL", OneInt()}, + {"TREE", OneInt()}, + }, + true, + }, + { + "valid uppercase coin", + Coins{ + {"ATOM", OneInt()}, + }, + true, + }, + { + "valid lower and uppercase coins (1)", + Coins{ + {"GAS", OneInt()}, + {"gAs", OneInt()}, + }, + true, + }, + { + "valid lower and uppercase coins (2)", + Coins{ + {"ATOM", OneInt()}, + {"Atom", OneInt()}, + {"atom", OneInt()}, + }, + true, + }, + { + "mixed case (1)", + Coins{ + {"MineraL", OneInt()}, + {"TREE", OneInt()}, + {"gAs", OneInt()}, + }, + true, + }, + { + "mixed case (2)", + Coins{ + {"gAs", OneInt()}, + {"mineral", OneInt()}, + }, + true, + }, + { + "mixed case (3)", + Coins{ + {"gAs", OneInt()}, + }, + true, + }, + { + "unicode letters and numbers", + Coins{ + {"𐀀𐀆𐀉Ⅲ", OneInt()}, + }, + false, + }, + { + "emojis", + Coins{ + {"🤑😋🤔", OneInt()}, + }, + false, + }, + { + "IBC denominations (ADR 001)", + Coins{ + {"ibc/7F1D3FCF4AE79E1554D670D1AD949A9BA4E4A3C76C63093E17E446A46061A7A2", OneInt()}, + {"ibc/876563AAAACF739EB061C67CDB5EDF2B7C9FD4AA9D876450CC21210807C2820A", NewInt(2)}, + }, + true, + }, + { + "empty (1)", + NewCoins(), + true, + }, + { + "empty (2)", + Coins{}, + true, + }, + { + "invalid denomination (1)", + Coins{ + {"MineraL", OneInt()}, + {"0TREE", OneInt()}, + {"gAs", OneInt()}, + }, + false, + }, + { + "invalid denomination (2)", + Coins{ + {"-GAS", OneInt()}, + {"gAs", OneInt()}, + }, + false, + }, + { + "bad sort (1)", + Coins{ + {"tree", OneInt()}, + {"gas", OneInt()}, + {"mineral", OneInt()}, + }, + false, + }, + { + "bad sort (2)", + Coins{ + {"gas", OneInt()}, + {"tree", OneInt()}, + {"mineral", OneInt()}, + }, + false, + }, + { + "non-positive amount (1)", + Coins{ + {"gas", OneInt()}, + {"tree", NewInt(0)}, + {"mineral", OneInt()}, + }, + false, + }, + { + "non-positive amount (2)", + Coins{ + {"gas", NewInt(-1)}, + {"tree", OneInt()}, + {"mineral", OneInt()}, + }, + false, + }, { + "duplicate denomination", + Coins{ + {"gas", OneInt()}, + {"gas", OneInt()}, + {"mineral", OneInt()}, + }, + false, + }, } - // both are after the first one, but the second and third are in the wrong order - badSort2 := Coins{ - {"gas", NewInt(1)}, - {"tree", NewInt(1)}, - {"mineral", NewInt(1)}, - } - badAmt := Coins{ - {"gas", NewInt(1)}, - {"tree", NewInt(0)}, - {"mineral", NewInt(1)}, + for _, tc := range testCases { + err := tc.coins.Validate() + if tc.expPass { + require.NoError(t, err, tc.name) + } else { + require.Error(t, err, tc.name) + } } - dup := Coins{ - {"gas", NewInt(1)}, - {"gas", NewInt(1)}, - {"mineral", NewInt(1)}, - } - neg := Coins{ - {"gas", NewInt(-1)}, - {"mineral", NewInt(1)}, - } - - assert.True(t, good.IsValid(), "Coins are valid") - assert.False(t, mixedCase1.IsValid(), "Coins denoms contain upper case characters") - assert.False(t, mixedCase2.IsValid(), "First Coins denoms contain upper case characters") - assert.False(t, mixedCase3.IsValid(), "Single denom in Coins contains upper case characters") - assert.True(t, good.IsAllPositive(), "Expected coins to be positive: %v", good) - assert.False(t, empty.IsAllPositive(), "Expected coins to not be positive: %v", empty) - assert.True(t, good.IsAllGTE(empty), "Expected %v to be >= %v", good, empty) - assert.False(t, good.IsAllLT(empty), "Expected %v to be < %v", good, empty) - assert.True(t, empty.IsAllLT(good), "Expected %v to be < %v", empty, good) - assert.False(t, badSort1.IsValid(), "Coins are not sorted") - assert.False(t, badSort2.IsValid(), "Coins are not sorted") - assert.False(t, badAmt.IsValid(), "Coins cannot include 0 amounts") - assert.False(t, dup.IsValid(), "Duplicate coin") - assert.False(t, neg.IsValid(), "Negative first-denom coin") } func TestCoinsGT(t *testing.T) { - one := NewInt(1) + one := OneInt() two := NewInt(2) assert.False(t, Coins{}.IsAllGT(Coins{})) @@ -460,7 +582,7 @@ func TestCoinsGT(t *testing.T) { } func TestCoinsLT(t *testing.T) { - one := NewInt(1) + one := OneInt() two := NewInt(2) assert.False(t, Coins{}.IsAllLT(Coins{})) @@ -475,7 +597,7 @@ func TestCoinsLT(t *testing.T) { } func TestCoinsLTE(t *testing.T) { - one := NewInt(1) + one := OneInt() two := NewInt(2) assert.True(t, Coins{}.IsAllLTE(Coins{})) @@ -490,7 +612,7 @@ func TestCoinsLTE(t *testing.T) { } func TestParse(t *testing.T) { - one := NewInt(1) + one := OneInt() cases := []struct { input string @@ -508,13 +630,14 @@ func TestParse(t *testing.T) { {"2 3foo, 97 bar", false, nil}, // 3foo is invalid coin name {"11me coin, 12you coin", false, nil}, // no spaces in coin names {"1.2btc", false, nil}, // amount must be integer - {"5foo-bar", false, nil}, // once more, only letters in coin name + {"5foo:bar", false, nil}, // invalid separator + {"10atom10", true, Coins{{"atom10", NewInt(10)}}}, } for tcIndex, tc := range cases { res, err := ParseCoins(tc.input) if !tc.valid { - require.NotNil(t, err, "%s: %#v. tc #%d", tc.input, res, tcIndex) + require.Error(t, err, "%s: %#v. tc #%d", tc.input, res, tcIndex) } else if assert.Nil(t, err, "%s: %+v", tc.input, err) { require.Equal(t, tc.expected, res, "coin parsing was incorrect, tc #%d", tcIndex) } @@ -552,21 +675,35 @@ func TestSortCoins(t *testing.T) { } cases := []struct { - coins Coins - before, after bool // valid before/after sort + name string + coins Coins + validBefore, + validAfter bool }{ - {good, true, true}, - {empty, false, false}, - {badSort1, false, true}, - {badSort2, false, true}, - {badAmt, false, false}, - {dup, false, false}, + {"valid coins", good, true, true}, + {"empty coins", empty, false, false}, + {"bad sort (1)", badSort1, false, true}, + {"bad sort (2)", badSort2, false, true}, + {"zero value coin", badAmt, false, false}, + {"duplicate coins", dup, false, false}, } - for tcIndex, tc := range cases { - require.Equal(t, tc.before, tc.coins.IsValid(), "coin validity is incorrect before sorting, tc #%d", tcIndex) + for _, tc := range cases { + err := tc.coins.Validate() + if tc.validBefore { + require.NoError(t, err, tc.name) + } else { + require.Error(t, err, tc.name) + } + tc.coins.Sort() - require.Equal(t, tc.after, tc.coins.IsValid(), "coin validity is incorrect after sorting, tc #%d", tcIndex) + + err = tc.coins.Validate() + if tc.validAfter { + require.NoError(t, err, tc.name) + } else { + require.Error(t, err, tc.name) + } } } @@ -609,11 +746,11 @@ func TestAmountOf(t *testing.T) { assert.Equal(t, NewInt(tc.amountOfTREE), tc.coins.AmountOf("tree")) } - assert.Panics(t, func() { cases[0].coins.AmountOf("Invalid") }) + assert.Panics(t, func() { cases[0].coins.AmountOf("10Invalid") }) } func TestCoinsIsAnyGTE(t *testing.T) { - one := NewInt(1) + one := OneInt() two := NewInt(2) assert.False(t, Coins{}.IsAnyGTE(Coins{})) @@ -633,7 +770,7 @@ func TestCoinsIsAnyGTE(t *testing.T) { } func TestCoinsIsAllGT(t *testing.T) { - one := NewInt(1) + one := OneInt() two := NewInt(2) assert.False(t, Coins{}.IsAllGT(Coins{})) @@ -653,7 +790,7 @@ func TestCoinsIsAllGT(t *testing.T) { } func TestCoinsIsAllGTE(t *testing.T) { - one := NewInt(1) + one := OneInt() two := NewInt(2) assert.True(t, Coins{}.IsAllGTE(Coins{})) @@ -678,6 +815,7 @@ func TestNewCoins(t *testing.T) { tenatom := NewInt64Coin("atom", 10) tenbtc := NewInt64Coin("btc", 10) zeroeth := NewInt64Coin("eth", 0) + invalidCoin := Coin{"0ETH", OneInt()} tests := []struct { name string coins Coins @@ -689,6 +827,7 @@ func TestNewCoins(t *testing.T) { {"sort after create", []Coin{tenbtc, tenatom}, Coins{tenatom, tenbtc}, false}, {"sort and remove zeroes", []Coin{zeroeth, tenbtc, tenatom}, Coins{tenatom, tenbtc}, false}, {"panic on dups", []Coin{tenatom, tenatom}, Coins{}, true}, + {"panic on invalid coin", []Coin{invalidCoin, tenatom}, Coins{}, true}, } for _, tt := range tests { tt := tt @@ -710,44 +849,23 @@ func TestCoinsIsAnyGT(t *testing.T) { sixEth := NewInt64Coin("eth", 6) twoBtc := NewInt64Coin("btc", 2) - require.False(t, Coins{}.IsAnyGT(Coins{})) - - require.False(t, Coins{fiveAtom}.IsAnyGT(Coins{})) - require.False(t, Coins{}.IsAnyGT(Coins{fiveAtom})) - require.True(t, Coins{fiveAtom}.IsAnyGT(Coins{twoAtom})) - require.False(t, Coins{twoAtom}.IsAnyGT(Coins{fiveAtom})) - - require.True(t, Coins{twoAtom, sixEth}.IsAnyGT(Coins{twoBtc, fiveAtom, threeEth})) - require.False(t, Coins{twoBtc, twoAtom, threeEth}.IsAnyGT(Coins{fiveAtom, sixEth})) - require.False(t, Coins{twoAtom, sixEth}.IsAnyGT(Coins{twoBtc, fiveAtom})) -} - -func TestFindDup(t *testing.T) { - abc := NewInt64Coin("abc", 10) - def := NewInt64Coin("def", 10) - ghi := NewInt64Coin("ghi", 10) - - type args struct { - coins Coins - } tests := []struct { - name string - args args - want int + name string + coinsA Coins + coinsB Coins + expPass bool }{ - {"empty", args{NewCoins()}, -1}, - {"one coin", args{NewCoins(NewInt64Coin("xyz", 10))}, -1}, - {"no dups", args{Coins{abc, def, ghi}}, -1}, - {"dup at first position", args{Coins{abc, abc, def}}, 1}, - {"dup after first position", args{Coins{abc, def, def}}, 2}, + {"{} ≤ {}", Coins{}, Coins{}, false}, + {"{} ≤ 5atom", Coins{}, Coins{fiveAtom}, false}, + {"5atom > 2atom", Coins{fiveAtom}, Coins{twoAtom}, true}, + {"2atom ≤ 5atom", Coins{twoAtom}, Coins{fiveAtom}, false}, + {"2atom,6eth > 2btc,5atom,3eth", Coins{twoAtom, sixEth}, Coins{twoBtc, fiveAtom, threeEth}, true}, + {"2btc,2atom,3eth ≤ 5atom,6eth", Coins{twoBtc, twoAtom, threeEth}, Coins{fiveAtom, sixEth}, false}, + {"2atom,6eth ≤ 2btc,5atom", Coins{twoAtom, sixEth}, Coins{twoBtc, fiveAtom}, false}, } - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - if got := findDup(tt.args.coins); got != tt.want { - t.Errorf("findDup() = %v, want %v", got, tt.want) - } - }) + + for _, tc := range tests { + require.True(t, tc.expPass == tc.coinsA.IsAnyGT(tc.coinsB), tc.name) } } diff --git a/types/dec_coin.go b/types/dec_coin.go index 128db1cae311..89dc716037c2 100644 --- a/types/dec_coin.go +++ b/types/dec_coin.go @@ -13,13 +13,11 @@ import ( // NewDecCoin creates a new DecCoin instance from an Int. func NewDecCoin(denom string, amount Int) DecCoin { - if err := validate(denom, amount); err != nil { - panic(err) - } + coin := NewCoin(denom, amount) return DecCoin{ - Denom: denom, - Amount: amount.ToDec(), + Denom: coin.Denom, + Amount: coin.Amount.ToDec(), } } @@ -39,7 +37,7 @@ func NewDecCoinFromDec(denom string, amount Dec) DecCoin { // NewDecCoinFromCoin creates a new DecCoin from a Coin. func NewDecCoinFromCoin(coin Coin) DecCoin { - if err := validate(coin.Denom, coin.Amount); err != nil { + if err := coin.Validate(); err != nil { panic(err) } @@ -137,12 +135,20 @@ func (coin DecCoin) String() string { return fmt.Sprintf("%v%v", coin.Amount, coin.Denom) } -// IsValid returns true if the DecCoin has a non-negative amount and the denom is vaild. -func (coin DecCoin) IsValid() bool { +// Validate returns an error if the DecCoin has a negative amount or if the denom is invalid. +func (coin DecCoin) Validate() error { if err := ValidateDenom(coin.Denom); err != nil { - return false + return err } - return !coin.IsNegative() + if coin.IsNegative() { + return fmt.Errorf("decimal coin %s amount cannot be negative", coin) + } + return nil +} + +// IsValid returns true if the DecCoin has a non-negative amount and the denom is valid. +func (coin DecCoin) IsValid() bool { + return coin.Validate() == nil } // ---------------------------------------------------------------------------- @@ -162,19 +168,14 @@ func NewDecCoins(decCoins ...DecCoin) DecCoins { newDecCoins.Sort() - // detect duplicate Denoms - if dupIndex := findDup(newDecCoins); dupIndex != -1 { - panic(fmt.Errorf("find duplicate denom: %s", newDecCoins[dupIndex])) - } - - if !newDecCoins.IsValid() { - panic(fmt.Errorf("invalid coin set: %s", newDecCoins)) + if err := newDecCoins.Validate(); err != nil { + panic(fmt.Errorf("invalid coin set %s: %w", newDecCoins, err)) } return newDecCoins } -// NewDecCoinsFromCoin constructs a new coin set with decimal values +// NewDecCoinsFromCoins constructs a new coin set with decimal values // from regular Coins. func NewDecCoinsFromCoins(coins ...Coin) DecCoins { decCoins := make(DecCoins, len(coins)) @@ -498,45 +499,60 @@ func (coins DecCoins) IsZero() bool { return true } -// IsValid asserts the DecCoins are sorted, have positive amount, and Denom -// does not contain upper case characters. -func (coins DecCoins) IsValid() bool { +// Validate checks that the DecCoins are sorted, have positive amount, with a valid and unique +// denomination (i.e no duplicates). Otherwise, it returns an error. +func (coins DecCoins) Validate() error { switch len(coins) { case 0: - return true + return nil case 1: if err := ValidateDenom(coins[0].Denom); err != nil { - return false + return err } - return coins[0].IsPositive() - + if !coins[0].IsPositive() { + return fmt.Errorf("coin %s amount is not positive", coins[0]) + } + return nil default: // check single coin case - if !(DecCoins{coins[0]}).IsValid() { - return false + if err := (DecCoins{coins[0]}).Validate(); err != nil { + return err } lowDenom := coins[0].Denom + seenDenoms := make(map[string]bool) + seenDenoms[lowDenom] = true + for _, coin := range coins[1:] { - if strings.ToLower(coin.Denom) != coin.Denom { - return false + if seenDenoms[coin.Denom] { + return fmt.Errorf("duplicate denomination %s", coin.Denom) + } + if err := ValidateDenom(coin.Denom); err != nil { + return err } if coin.Denom <= lowDenom { - return false + return fmt.Errorf("denomination %s is not sorted", coin.Denom) } if !coin.IsPositive() { - return false + return fmt.Errorf("coin %s amount is not positive", coin.Denom) } // we compare each coin against the last denom lowDenom = coin.Denom + seenDenoms[coin.Denom] = true } - return true + return nil } } +// IsValid calls Validate and returns true when the DecCoins are sorted, have positive amount, with a +// valid and unique denomination (i.e no duplicates). +func (coins DecCoins) IsValid() bool { + return coins.Validate() == nil +} + // IsAllPositive returns true if there is at least one coin and all currencies // have a positive value. // @@ -570,11 +586,16 @@ func removeZeroDecCoins(coins DecCoins) DecCoins { //----------------------------------------------------------------------------- // Sorting -var _ sort.Interface = Coins{} +var _ sort.Interface = DecCoins{} + +// Len implements sort.Interface for DecCoins +func (coins DecCoins) Len() int { return len(coins) } -func (coins DecCoins) Len() int { return len(coins) } +// Less implements sort.Interface for DecCoins func (coins DecCoins) Less(i, j int) bool { return coins[i].Denom < coins[j].Denom } -func (coins DecCoins) Swap(i, j int) { coins[i], coins[j] = coins[j], coins[i] } + +// Swap implements sort.Interface for DecCoins +func (coins DecCoins) Swap(i, j int) { coins[i], coins[j] = coins[j], coins[i] } // Sort is a helper function to sort the set of decimal coins in-place. func (coins DecCoins) Sort() DecCoins { @@ -609,9 +630,8 @@ func ParseDecCoin(coinStr string) (coin DecCoin, err error) { return NewDecCoinFromDec(denomStr, amount), nil } -// ParseDecCoins will parse out a list of decimal coins separated by commas. -// If nothing is provided, it returns nil DecCoins. Returned decimal coins are -// sorted. +// ParseDecCoins will parse out a list of decimal coins separated by commas. If nothing is provided, +// it returns nil DecCoins. If the coins aren't valid they return an error. Returned coins are sorted. func ParseDecCoins(coinsStr string) (DecCoins, error) { coinsStr = strings.TrimSpace(coinsStr) if len(coinsStr) == 0 { @@ -633,8 +653,8 @@ func ParseDecCoins(coinsStr string) (DecCoins, error) { coins.Sort() // validate coins before returning - if !coins.IsValid() { - return nil, fmt.Errorf("parsed decimal coins are invalid: %#v", coins) + if err := coins.Validate(); err != nil { + return nil, err } return coins, nil diff --git a/types/dec_coin_test.go b/types/dec_coin_test.go index 598ea2f48610..821d29756672 100644 --- a/types/dec_coin_test.go +++ b/types/dec_coin_test.go @@ -14,7 +14,7 @@ func TestNewDecCoin(t *testing.T) { require.NotPanics(t, func() { NewInt64DecCoin(testDenom1, 0) }) - require.Panics(t, func() { + require.NotPanics(t, func() { NewInt64DecCoin(strings.ToUpper(testDenom1), 5) }) require.Panics(t, func() { @@ -29,7 +29,7 @@ func TestNewDecCoinFromDec(t *testing.T) { require.NotPanics(t, func() { NewDecCoinFromDec(testDenom1, ZeroDec()) }) - require.Panics(t, func() { + require.NotPanics(t, func() { NewDecCoinFromDec(strings.ToUpper(testDenom1), NewDec(5)) }) require.Panics(t, func() { @@ -44,7 +44,7 @@ func TestNewDecCoinFromCoin(t *testing.T) { require.NotPanics(t, func() { NewDecCoinFromCoin(Coin{testDenom1, NewInt(0)}) }) - require.Panics(t, func() { + require.NotPanics(t, func() { NewDecCoinFromCoin(Coin{strings.ToUpper(testDenom1), NewInt(5)}) }) require.Panics(t, func() { @@ -164,11 +164,16 @@ func TestIsValid(t *testing.T) { }, { DecCoin{Denom: "BTC", Amount: NewDec(10)}, - false, - "invalid denoms", + true, + "valid uppercase denom", }, { - DecCoin{Denom: "BTC", Amount: NewDec(-10)}, + DecCoin{Denom: "Bitcoin", Amount: NewDec(10)}, + true, + "valid mixed case denom", + }, + { + DecCoin{Denom: "btc", Amount: NewDec(-10)}, false, "negative amount", }, @@ -287,43 +292,50 @@ func TestSortDecCoins(t *testing.T) { } cases := []struct { + name string coins DecCoins before, after bool // valid before/after sort }{ - {good, true, true}, - {empty, false, false}, - {badSort1, false, true}, - {badSort2, false, true}, - {badAmt, false, false}, - {dup, false, false}, + {"valid coins", good, true, true}, + {"empty coins", empty, false, false}, + {"unsorted coins (1)", badSort1, false, true}, + {"unsorted coins (2)", badSort2, false, true}, + {"zero amount coins", badAmt, false, false}, + {"duplicate coins", dup, false, false}, } - for tcIndex, tc := range cases { - require.Equal(t, tc.before, tc.coins.IsValid(), "coin validity is incorrect before sorting, tc #%d", tcIndex) + for _, tc := range cases { + require.Equal(t, tc.before, tc.coins.IsValid(), "coin validity is incorrect before sorting; %s", tc.name) tc.coins.Sort() - require.Equal(t, tc.after, tc.coins.IsValid(), "coin validity is incorrect after sorting, tc #%d", tcIndex) + require.Equal(t, tc.after, tc.coins.IsValid(), "coin validity is incorrect after sorting; %s", tc.name) } } -func TestDecCoinsIsValid(t *testing.T) { +func TestDecCoinsValidate(t *testing.T) { testCases := []struct { - input DecCoins - expected bool + input DecCoins + expectedPass bool }{ {DecCoins{}, true}, {DecCoins{DecCoin{testDenom1, NewDec(5)}}, true}, {DecCoins{DecCoin{testDenom1, NewDec(5)}, DecCoin{testDenom2, NewDec(100000)}}, true}, {DecCoins{DecCoin{testDenom1, NewDec(-5)}}, false}, - {DecCoins{DecCoin{"AAA", NewDec(5)}}, false}, + {DecCoins{DecCoin{"BTC", NewDec(5)}}, true}, + {DecCoins{DecCoin{"0BTC", NewDec(5)}}, false}, {DecCoins{DecCoin{testDenom1, NewDec(5)}, DecCoin{"B", NewDec(100000)}}, false}, {DecCoins{DecCoin{testDenom1, NewDec(5)}, DecCoin{testDenom2, NewDec(-100000)}}, false}, {DecCoins{DecCoin{testDenom1, NewDec(-5)}, DecCoin{testDenom2, NewDec(100000)}}, false}, - {DecCoins{DecCoin{"AAA", NewDec(5)}, DecCoin{testDenom2, NewDec(100000)}}, false}, + {DecCoins{DecCoin{"BTC", NewDec(5)}, DecCoin{testDenom2, NewDec(100000)}}, true}, + {DecCoins{DecCoin{"0BTC", NewDec(5)}, DecCoin{testDenom2, NewDec(100000)}}, false}, } for i, tc := range testCases { - res := tc.input.IsValid() - require.Equal(t, tc.expected, res, "unexpected result for test case #%d, input: %v", i, tc.input) + err := tc.input.Validate() + if tc.expectedPass { + require.NoError(t, err, "unexpected result for test case #%d, input: %v", i, tc.input) + } else { + require.Error(t, err, "unexpected result for test case #%d, input: %v", i, tc.input) + } } } @@ -337,7 +349,11 @@ func TestParseDecCoins(t *testing.T) { {"4stake", nil, true}, {"5.5atom,4stake", nil, true}, {"0.0stake", nil, true}, - {"0.004STAKE", nil, true}, + { + "0.004STAKE", + DecCoins{NewDecCoinFromDec("STAKE", NewDecWithPrec(4000000000000000, Precision))}, + false, + }, { "0.004stake", DecCoins{NewDecCoinFromDec("stake", NewDecWithPrec(4000000000000000, Precision))}, @@ -478,7 +494,7 @@ func TestDecCoinsQuoDecTruncate(t *testing.T) { } func TestNewDecCoinsWithIsValid(t *testing.T) { - fake1 := append(NewDecCoins(NewDecCoin("mytoken", NewInt(10))), DecCoin{Denom: "BTC", Amount: NewDec(10)}) + fake1 := append(NewDecCoins(NewDecCoin("mytoken", NewInt(10))), DecCoin{Denom: "10BTC", Amount: NewDec(10)}) fake2 := append(NewDecCoins(NewDecCoin("mytoken", NewInt(10))), DecCoin{Denom: "BTC", Amount: NewDec(-10)}) tests := []struct { @@ -528,7 +544,7 @@ func TestDecCoins_AddDecCoinWithIsValid(t *testing.T) { "valid coins should have passed", }, { - NewDecCoins().Add(NewDecCoin("mytoken", NewInt(10))).Add(DecCoin{Denom: "BTC", Amount: NewDec(10)}), + NewDecCoins().Add(NewDecCoin("mytoken", NewInt(10))).Add(DecCoin{Denom: "0BTC", Amount: NewDec(10)}), false, "invalid denoms", }, diff --git a/x/bank/types/params_test.go b/x/bank/types/params_test.go index 3bcb76e4d8c4..f6f95d73737e 100644 --- a/x/bank/types/params_test.go +++ b/x/bank/types/params_test.go @@ -25,8 +25,8 @@ func Test_validateSendEnabledParam(t *testing.T) { {"valid denom send enabled", args{*NewSendEnabled(sdk.DefaultBondDenom, true)}, false}, {"valid denom send disabled", args{*NewSendEnabled(sdk.DefaultBondDenom, false)}, false}, - {"invalid denom send enabled", args{*NewSendEnabled("FOO", true)}, true}, - {"invalid denom send disabled", args{*NewSendEnabled("FOO", false)}, true}, + {"invalid denom send enabled", args{*NewSendEnabled("0FOO", true)}, true}, + {"invalid denom send disabled", args{*NewSendEnabled("0FOO", false)}, true}, } for _, tt := range tests { tt := tt diff --git a/x/ibc-transfer/types/msgs_test.go b/x/ibc-transfer/types/msgs_test.go index 8a089b8305c4..9c32b826d6bf 100644 --- a/x/ibc-transfer/types/msgs_test.go +++ b/x/ibc-transfer/types/msgs_test.go @@ -28,7 +28,7 @@ var ( emptyAddr sdk.AccAddress coin = sdk.NewCoin("atom", sdk.NewInt(100)) - invalidDenomCoin = sdk.Coin{Denom: "ato-m", Amount: sdk.NewInt(100)} + invalidDenomCoin = sdk.Coin{Denom: "0atom", Amount: sdk.NewInt(100)} negativeCoin = sdk.Coin{Denom: "atoms", Amount: sdk.NewInt(-100)} ) @@ -60,7 +60,7 @@ func TestMsgTransferValidation(t *testing.T) { {"too short channel id", NewMsgTransfer(validPort, invalidShortChannel, coin, addr1, addr2, 10, 0), false}, {"too long channel id", NewMsgTransfer(validPort, invalidLongChannel, coin, addr1, addr2, 10, 0), false}, {"channel id contains non-alpha", NewMsgTransfer(validPort, invalidChannel, coin, addr1, addr2, 10, 0), false}, - {"invalid amount", NewMsgTransfer(validPort, validChannel, invalidDenomCoin, addr1, addr2, 10, 0), false}, + {"invalid denom", NewMsgTransfer(validPort, validChannel, invalidDenomCoin, addr1, addr2, 10, 0), false}, {"negative coin", NewMsgTransfer(validPort, validChannel, negativeCoin, addr1, addr2, 10, 0), false}, {"missing sender address", NewMsgTransfer(validPort, validChannel, coin, emptyAddr, addr2, 10, 0), false}, {"missing recipient address", NewMsgTransfer(validPort, validChannel, coin, addr1, "", 10, 0), false}, @@ -70,7 +70,7 @@ func TestMsgTransferValidation(t *testing.T) { for i, tc := range testCases { err := tc.msg.ValidateBasic() if tc.expPass { - require.NoError(t, err, "valid test case %d failed: %v", i, err) + require.NoError(t, err, "valid test case %d failed: %s", i, tc.name) } else { require.Error(t, err, "invalid test case %d passed: %s", i, tc.name) }