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

types: update coin regex #7027

Merged
merged 19 commits into from
Aug 14, 2020
Merged

types: update coin regex #7027

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
fbec784
types: update coin regex
fedekunze Aug 12, 2020
497e011
more tests
fedekunze Aug 12, 2020
2b22419
fix test case
fedekunze Aug 12, 2020
9df2c67
increase coverage
fedekunze Aug 12, 2020
431ede8
changelog
fedekunze Aug 12, 2020
afe2627
fix tests
fedekunze Aug 12, 2020
9d4c70e
support unicode letters and numbers
fedekunze Aug 12, 2020
216ebc8
remove ToUpper
fedekunze Aug 12, 2020
595a9d2
Update types/coin.go
fedekunze Aug 12, 2020
cbb88a0
Validate function
fedekunze Aug 13, 2020
88a6874
fix ICS20 test
fedekunze Aug 13, 2020
0bd648f
Merge branch 'master' of github.com:cosmos/cosmos-sdk into update-coi…
fedekunze Aug 13, 2020
e7c09f7
table tests and revert unicodes
fedekunze Aug 13, 2020
94fb4a9
Merge branch 'master' into update-coin-regex
fedekunze Aug 13, 2020
549e121
add test case
fedekunze Aug 13, 2020
12eece3
Merge branch 'update-coin-regex' of github.com:cosmos/cosmos-sdk into…
fedekunze Aug 13, 2020
9945305
Merge branch 'master' of github.com:cosmos/cosmos-sdk into update-coi…
fedekunze Aug 14, 2020
e12e38d
add test cases and comment
fedekunze Aug 14, 2020
928099c
Merge branch 'master' into update-coin-regex
fedekunze Aug 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,9 @@ 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 all caps in denominations
* (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.
Expand Down
42 changes: 28 additions & 14 deletions types/coin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,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 {
Expand All @@ -101,7 +101,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 {
Expand Down Expand Up @@ -200,8 +200,14 @@ func (coins Coins) IsValid() bool {
}

lowDenom := coins[0].Denom
seenDenoms := make(map[string]bool)
seenDenoms[lowDenom] = true

for _, coin := range coins[1:] {
if strings.ToLower(coin.Denom) != coin.Denom {
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
if seenDenoms[coin.Denom] {
return false
}
if err := ValidateDenom(coins[0].Denom); err != nil {
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
return false
}
if coin.Denom <= lowDenom {
Expand All @@ -213,6 +219,7 @@ func (coins Coins) IsValid() bool {

// we compare each coin against the last denom
lowDenom = coin.Denom
seenDenoms[coin.Denom] = true
}

return true
Expand Down Expand Up @@ -463,7 +470,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)

Expand Down Expand Up @@ -560,13 +567,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
Expand All @@ -576,8 +588,8 @@ 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.
reDnmString = `\p{L}[\p{L}\p{N}/]{2,127}`
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
reAmt = `[[:digit:]]+`
reDecAmt = `[[:digit:]]*\.[[:digit:]]+`
reSpc = `[[:space:]]*`
Expand Down Expand Up @@ -661,18 +673,20 @@ type findDupDescriptor interface {
Len() int
}

// findDup works on the assumption that coins is sorted
// findDup iterates over all the coins and returns the index on the first occurrence of a duplicated
// denomination. If no duplicated coin is found it returns -1.
func findDup(coins findDupDescriptor) int {
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
if coins.Len() <= 1 {
return -1
}

prevDenom := coins.GetDenomByIndex(0)
for i := 1; i < coins.Len(); i++ {
if coins.GetDenomByIndex(i) == prevDenom {
seenDenoms := make(map[string]bool)
for i := 0; i < coins.Len(); i++ {
coin := coins.GetDenomByIndex(i)
if seenDenoms[coin] {
return i
}
prevDenom = coins.GetDenomByIndex(i)
seenDenoms[coin] = true
}

return -1
Expand Down
71 changes: 58 additions & 13 deletions types/coin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -57,17 +57,26 @@ 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{"Atom", NewInt(1)}, true},
{Coin{"ATOM", NewInt(1)}, true},
{Coin{"a", NewInt(1)}, false},
{Coin{"a very long coin denom", NewInt(1)}, false},
{Coin{"atOm", NewInt(1)}, false},
{Coin{loremIpsum, NewInt(1)}, false},
{Coin{"atOm", NewInt(1)}, true},
{Coin{" ", NewInt(1)}, false},
}

Expand Down Expand Up @@ -391,10 +400,15 @@ func TestCoins(t *testing.T) {
{"mineral", NewInt(1)},
{"tree", NewInt(1)},
}
goodCaps := Coins{
{"GAS", NewInt(1)},
{"MINERAL", NewInt(1)},
{"TREE", NewInt(1)},
}
mixedCase1 := Coins{
{"gAs", NewInt(1)},
{"MineraL", NewInt(1)},
{"TREE", NewInt(1)},
{"gAs", NewInt(1)},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this test enforces the ordering by denom as published.

I just think if you normalize cases one place, they should be normalized everywhere.

Also, what if we have bal := Coins{{"atom", NewInt(10)}} and then bal.Sub(Coins{{"ATOM", NewInt(3)}}). What is the expected behavior?

Either "atom" == "ATOM" everywhere or nowhere - if they are considered dups, they should be treated the same by all math functions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought "atom" != "ATOM" everywhere? Is this not correct @fedekunze ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I removed it now

}
mixedCase2 := Coins{
{"gAs", NewInt(1)},
Expand All @@ -403,7 +417,25 @@ func TestCoins(t *testing.T) {
mixedCase3 := Coins{
{"gAs", NewInt(1)},
}
unicodeLinearBAndRoman := Coins{
{"𐀀𐀆𐀉Ⅲ", NewInt(1)},
}
unicodeEmoji := Coins{
{"🤑😋🤔", NewInt(1)},
}
multipleIBCDenoms := Coins{
{"ibc/7F1D3FCF4AE79E1554D670D1AD949A9BA4E4A3C76C63093E17E446A46061A7A2", NewInt(1)},
{"ibc/876563AAAACF739EB061C67CDB5EDF2B7C9FD4AA9D876450CC21210807C2820A", NewInt(2)},
}
allCaps := Coins{
{"ATOM", NewInt(1)},
}
empty := NewCoins()
invalidDenom := Coins{
{"MineraL", NewInt(1)},
{"0TREE", NewInt(1)},
{"gAs", NewInt(1)},
}
badSort1 := Coins{
{"tree", NewInt(1)},
{"gas", NewInt(1)},
Expand All @@ -421,20 +453,29 @@ func TestCoins(t *testing.T) {
{"tree", NewInt(0)},
{"mineral", NewInt(1)},
}
dup := Coins{
dup1 := Coins{
{"gas", NewInt(1)},
{"gas", NewInt(1)},
{"mineral", NewInt(1)},
}
notDup := Coins{
{"GAS", NewInt(1)},
{"gAs", NewInt(1)},
}
neg := Coins{
{"gas", NewInt(-1)},
{"mineral", NewInt(1)},
}

fedekunze marked this conversation as resolved.
Show resolved Hide resolved
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, goodCaps.IsValid(), "Coins all caps are valid")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: usually this is some sort of table test, no?

Personally, I like the named table test:

cases := map[string]struct { 
  someTestData string
}{
  "all upper case": { "FOO" },
  "mixed case": {"fOO"},
}

for name, tc := range cases {
  t.Run(name, func(t *testing.T) {
    // whatever you want to test
    assert.True(IsValid(tc.someTestData)
  }
}

They are all run separately and each failure has the name from the upper test case map.

assert.True(t, unicodeLinearBAndRoman.IsValid(), "Unicode characters")
assert.False(t, unicodeEmoji.IsValid(), "Unicode emoji")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😭

assert.True(t, mixedCase1.IsValid(), "Coins denoms contain upper case characters")
assert.True(t, mixedCase2.IsValid(), "First Coins denoms contain upper case characters")
assert.True(t, mixedCase3.IsValid(), "Single denom in Coins contains upper case characters")
assert.True(t, allCaps.IsValid(), "Coins denom contains all uppercase characters")
assert.True(t, multipleIBCDenoms.IsValid(), "IBC denominations as per ADR001 should be valid")
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)
Expand All @@ -443,7 +484,9 @@ func TestCoins(t *testing.T) {
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, invalidDenom.IsValid(), "Invalid coin")
assert.False(t, dup1.IsValid(), "Duplicate coin")
assert.True(t, notDup.IsValid(), "Same coin with uppercase")
assert.False(t, neg.IsValid(), "Negative first-denom coin")
}

Expand Down Expand Up @@ -514,7 +557,7 @@ func TestParse(t *testing.T) {
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)
}
Expand Down Expand Up @@ -609,7 +652,7 @@ 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) {
Expand Down Expand Up @@ -678,6 +721,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
Expand All @@ -689,6 +733,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
Expand Down
26 changes: 20 additions & 6 deletions types/dec_coin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ func NewDecCoins(decCoins ...DecCoin) DecCoins {
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))
Expand Down Expand Up @@ -518,8 +518,15 @@ func (coins DecCoins) IsValid() bool {
}

lowDenom := coins[0].Denom
seenDenoms := make(map[string]bool)
seenDenoms[strings.ToUpper(lowDenom)] = true

for _, coin := range coins[1:] {
if strings.ToLower(coin.Denom) != coin.Denom {
denomUpper := strings.ToUpper(coin.Denom)
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
if seenDenoms[denomUpper] {
return false
}
if err := ValidateDenom(coins[0].Denom); err != nil {
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
return false
}
if coin.Denom <= lowDenom {
Expand Down Expand Up @@ -570,11 +577,18 @@ 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) }

// Less implements sort.Interface for DecCoins. It compares the denominations in uppercase characters.
func (coins DecCoins) Less(i, j int) bool {
return strings.ToUpper(coins[i].Denom) < strings.ToUpper(coins[j].Denom)
}

func (coins DecCoins) Len() int { return len(coins) }
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 {
Expand Down
Loading