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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
145 changes: 71 additions & 74 deletions types/coin.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we document the rational to why a single coin of ZeroInt is valid but a set of coins with a coin of ZeroInt is invalid? Seems counter-intuitive to me and if we support this it, the reason should be well documented. I'd be in favor of only considering positive amount Coins valid

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
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
return false
if seenDenoms[coin.Denom] {
return fmt.Errorf("duplicate denomination %s", coin.Denom)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}
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.
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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
Expand All @@ -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 ('/').
Copy link
Member

Choose a reason for hiding this comment

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

Note: There's the potential for confusing UI here.

Suppose I create a denom atom10, the following Coins string representation is ambigious to an end user

5atom10,300btc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unfortunately, there's no workaround for this. I added a test case and documented the expected format on the ParseCoin(s) func

reDnmString = `[a-zA-Z][a-zA-Z0-9/]{2,127}`
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
reAmt = `[[:digit:]]+`
reDecAmt = `[[:digit:]]*\.[[:digit:]]+`
reSpc = `[[:space:]]*`
Expand All @@ -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)

Expand All @@ -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 {
Expand All @@ -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
}
Loading