R4R: Added IsValid to Coin #4558

colin-axner · 2019-06-14T23:04:42Z

closes #4556

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added a relevant changelog entry: clog add [section] [stanza] [message]
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

types/coin.go

codecov · 2019-06-14T23:29:01Z

Codecov Report

Merging #4558 into master will increase coverage by 0.45%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #4558      +/-   ##
=========================================
+ Coverage   53.04%   53.5%   +0.45%     
=========================================
  Files         258     257       -1     
  Lines       16288   16177     -111     
=========================================
+ Hits         8640    8655      +15     
+ Misses       7002    6876     -126     
  Partials      646     646

fedekunze

Let’s add a blank string test with more than 3 spaces

fedekunze

Sweet ! Thanks Colin !

alessio

IsValid() was removed for two reasons:

developers must use NewCoin() to initialise coins
we wanted validation to live in one place only

I believe this PR requires further discussion before merging.

fedekunze · 2019-06-15T20:19:35Z

@alessio I agree that users should use the constructors, but we can't enforce them to do it. We should then support these sorts of functions to increase the security of other chains on the SDK. Also, this might be useful to verify single coin parameters such as #4472 .

alexanderbez · 2019-06-16T08:44:10Z

types/coin.go

@@ -51,6 +51,15 @@ func (coin Coin) String() string {
 	return fmt.Sprintf("%v%v", coin.Amount, coin.Denom)
 }

+// IsValid asserts that the Coin has a positive amount and the Denom does not contain
+// upper case characters and has a length of 3 ~ 16 characters.


I would simply update this godoc to state:

// IsValid asserts that the Coin has a positive amount and the Denom is valid.

alexanderbez

ACK

alessio · 2019-06-16T08:48:16Z

but we can't enforce them to do it.

We can by not providing IsValid(). The Coin type is guaranteed to be safe as long as you comply with the interface (and dev docs). In other words, if you don't initialise a Coin structure via a NewCoin call then you are on your own and can't blame us :)

alessio · 2019-06-16T08:50:02Z

Plus, validation should really live in one place only. It does not feel right to duplicate validation logic in multiple places...

alessio · 2019-06-16T09:10:42Z

func NewCoin(denom string, amount Int) Coin {
	if err := validate(denom, amount); err != nil {
		panic(err)
	} 
}


func validate(denom string, amount Int) error {
	if err := validateDenom(c.Denom); err != nil {
		return err
	}

	if amount.LT(ZeroInt()) {
		return fmt.Errorf("negative coin amount: %v", amount)
	}

	return nil
}

func (c Coin) IsValid() bool {
	if err := validate(denom, amount); err != nil {
		return true
	}
	return false
}

fedekunze · 2019-06-16T09:42:07Z

@colin-axner just had a quick chat with @alessio. What he's saying is that for sdk.NewCoin a 0 amount coin is valid but on the IsValid it's not. Can we change it so it's consistent in both implementations?

@alexanderbez and I agree on having the 0 amount coin to be invalid in both funcs.

alexanderbez · 2019-06-16T09:43:02Z

Agreed. We should not allow 0 coins as input anywhere. We should have a single public validation function that we use.

fedekunze

~~Can we also change the coins.IsValid() func to just iterate over the coins and call the coin.IsValid func() ?~~

EDIT: completely forgot about the sort, please ignore the comment

…on 0 amount

colin-axner · 2019-06-17T18:02:35Z

Should validateDenom be made public? I could see use cases where a Msg requires a valid denom but no coin(s) and therefore would require a public validation function separate from the one used for Coin

colin-axner · 2019-06-17T20:26:44Z

I have been going through trying to update tests and code to reflect 0 amount coin as invalid, but the usage of 0 coins seems to be deeply ingrained in the code/tests. For example: in minter it appears it should be valid to have block provisions of zero. Abiding by the updated constructor for Coin would force me to use Coin{} which seems risky/messy.

A lot of the simulations/tests also use empty/zero coins to test various functionality, originally I was changing these to use Coin{} to bypass the panic but would it be better to eliminate the usage of zero coins entirely?

Also DecCoin should also probably go through the same validation as Coin but to get all validation to live under the same func, I would probably need to add some type of interface for Int and Dec and pass that in as the amount param in Validate.

This is starting to be a time sink and causing me a lot of trouble. What direction should I take this? I still think it is very important to introduce some sort of public validation function coin(s)/denoms can use, though I starting to wonder if removal of zero coins should be done in a separate pr.

Thoughts? @fedekunze @alexanderbez @alessio

alessio · 2019-06-18T08:21:14Z

I did remember that in few places we do take 0 as valid Coin amount.
All Coins operations remove zero coins before doing work, so 0-amount Coin are already taken into account.
IMHO we should continue accepting 0 as valid coin amount for both the constructor and IsValid() function. IsZero() is the function that clients can call that to check against zero.

alexanderbez

ACK, I think the changes are fine for now @colin-axner.

alexanderbez · 2019-06-18T12:20:31Z

types/coin.go

+		return err
+	}
+
+	if amount.LTE(ZeroInt()) {


@colin-axner lets go with LT for now so we can get the build passing and as this was the original behavior.

alessio · 2019-06-18T13:50:38Z

types/coin.go

@@ -51,6 +49,28 @@ func (coin Coin) String() string {
 	return fmt.Sprintf("%v%v", coin.Amount, coin.Denom)
 }

+// Validate returns an error if the Coin has a nonpositive amount or if
+// the Denom is invalid.
+func Validate(denom string, amount Int) error {


Let's not export this one please

alessio · 2019-06-18T13:50:47Z

types/coin.go

@@ -51,6 +49,28 @@ func (coin Coin) String() string {
 	return fmt.Sprintf("%v%v", coin.Amount, coin.Denom)
 }

+// Validate returns an error if the Coin has a nonpositive amount or if
+// the Denom is invalid.
+func Validate(denom string, amount Int) error {


Suggested change

func Validate(denom string, amount Int) error {

func validate(denom string, amount Int) error {

alessio · 2019-06-18T13:50:59Z

types/coin.go

-
-	if amount.LT(ZeroInt()) {
-		panic(fmt.Errorf("negative coin amount: %v", amount))
+	if err := Validate(denom, amount); err != nil {


Suggested change

if err := Validate(denom, amount); err != nil {

if err := validate(denom, amount); err != nil {

alessio · 2019-06-18T13:51:13Z

types/coin.go

+
+// IsValid asserts that the Coin has a positive amount and the Denom is vaild.
+func (coin Coin) IsValid() bool {
+	if err := Validate(coin.Denom, coin.Amount); err != nil {


Suggested change

if err := Validate(coin.Denom, coin.Amount); err != nil {

if err := validate(coin.Denom, coin.Amount); err != nil {

alessio · 2019-06-18T13:52:36Z

types/coin_test.go

@@ -56,7 +77,7 @@ func TestAddCoin(t *testing.T) {
 		shouldPanic bool
 	}{
 		{NewInt64Coin(testDenom1, 1), NewInt64Coin(testDenom1, 1), NewInt64Coin(testDenom1, 2), false},
-		{NewInt64Coin(testDenom1, 1), NewInt64Coin(testDenom1, 0), NewInt64Coin(testDenom1, 1), false},
+		{NewInt64Coin(testDenom1, 1), Coin{testDenom1, NewInt(0)}, NewInt64Coin(testDenom1, 1), false},


I'd revert this

alessio · 2019-06-18T13:52:52Z

types/coin_test.go

@@ -80,7 +101,7 @@ func TestSubCoin(t *testing.T) {
 		{NewInt64Coin(testDenom1, 1), NewInt64Coin(testDenom2, 1), NewInt64Coin(testDenom1, 1), true},
 		{NewInt64Coin(testDenom1, 10), NewInt64Coin(testDenom1, 1), NewInt64Coin(testDenom1, 9), false},
 		{NewInt64Coin(testDenom1, 5), NewInt64Coin(testDenom1, 3), NewInt64Coin(testDenom1, 2), false},
-		{NewInt64Coin(testDenom1, 5), NewInt64Coin(testDenom1, 0), NewInt64Coin(testDenom1, 5), false},
+		{NewInt64Coin(testDenom1, 5), Coin{testDenom1, NewInt(0)}, NewInt64Coin(testDenom1, 5), false},


ditto - I'd revert this

alessio · 2019-06-18T13:52:58Z

types/coin_test.go

@@ -133,7 +154,7 @@ func TestIsLTCoin(t *testing.T) {
 	}{
 		{NewInt64Coin(testDenom1, 1), NewInt64Coin(testDenom1, 1), false, false},
 		{NewInt64Coin(testDenom1, 2), NewInt64Coin(testDenom1, 1), false, false},
-		{NewInt64Coin(testDenom1, 0), NewInt64Coin(testDenom2, 1), false, true},
+		{Coin{testDenom1, NewInt(0)}, NewInt64Coin(testDenom2, 1), false, true},


alessio · 2019-06-18T13:53:07Z

types/coin_test.go

@@ -150,7 +171,7 @@ func TestIsLTCoin(t *testing.T) {
 }

 func TestCoinIsZero(t *testing.T) {
-	coin := NewInt64Coin(testDenom1, 0)
+	coin := Coin{testDenom1, NewInt(0)}


ditto - and so on

alessio · 2019-06-18T13:55:05Z

types/coin.go

+		return err
+	}
+
+	if amount.LTE(ZeroInt()) {


Suggested change

if amount.LTE(ZeroInt()) {

if amount.LT(ZeroInt()) {

alessio · 2019-06-18T13:56:21Z

types/coin.go

+	return nil
+}
+
+// IsValid asserts that the Coin has a positive amount and the Denom is vaild.


Suggested change

// IsValid asserts that the Coin has a positive amount and the Denom is vaild.

// IsValid returns true if the Coin has a non-negative amount and the denom is valid.

Comments addressed or taken back (#4558 (review))

alessio

Good one!

codecov · 2019-06-18T16:03:36Z

Codecov Report

Merging #4558 into master will increase coverage by 0.45%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #4558      +/-   ##
=========================================
+ Coverage   53.04%   53.5%   +0.45%     
=========================================
  Files         258     257       -1     
  Lines       16288   16177     -111     
=========================================
+ Hits         8640    8655      +15     
+ Misses       7002    6876     -126     
  Partials      646     646

colin-axner added 2 commits June 14, 2019 16:01

added isvalid and test to coin

35e59dd

add pending log

02887cb

colin-axner requested review from alessio, alexanderbez, jackzampolin and rigelrozanski as code owners June 14, 2019 23:04

golangcibot reviewed Jun 14, 2019

View reviewed changes

types/coin.go Outdated Show resolved Hide resolved

fix misspell

cdc2a44

fedekunze reviewed Jun 15, 2019

View reviewed changes

colin-axner added 2 commits June 14, 2019 17:24

added empty denom with spaces test

3e8bb1a

added more spaces for good measure

76ba8c9

fedekunze approved these changes Jun 15, 2019

View reviewed changes

alessio suggested changes Jun 15, 2019

View reviewed changes

alexanderbez reviewed Jun 16, 2019

View reviewed changes

alexanderbez approved these changes Jun 16, 2019

View reviewed changes

fedekunze previously requested changes Jun 16, 2019

View reviewed changes

added public validation function for coin and coin constructor panic …

6bc231e

…on 0 amount

alexanderbez reviewed Jun 18, 2019

View reviewed changes

alessio reviewed Jun 18, 2019

View reviewed changes

colin-axner added 2 commits June 18, 2019 08:34

revert changes, unexport validate

7b82512

Revet denom_test

44cfee5

alessio approved these changes Jun 18, 2019

View reviewed changes

alessio merged commit 314af42 into cosmos:master Jun 18, 2019

colin-axner mentioned this pull request Sep 16, 2020

Write ADR for Coins #7046

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Added IsValid to Coin #4558

R4R: Added IsValid to Coin #4558

colin-axner commented Jun 14, 2019

codecov bot commented Jun 14, 2019 •

edited

Loading

fedekunze left a comment •

edited

Loading

fedekunze left a comment •

edited

Loading

alessio left a comment

fedekunze commented Jun 15, 2019

alexanderbez Jun 16, 2019

alexanderbez left a comment

alessio commented Jun 16, 2019

alessio commented Jun 16, 2019

alessio commented Jun 16, 2019 •

edited

Loading

fedekunze commented Jun 16, 2019

alexanderbez commented Jun 16, 2019

fedekunze left a comment •

edited

Loading

colin-axner commented Jun 17, 2019

colin-axner commented Jun 17, 2019

alessio commented Jun 18, 2019 •

edited

Loading

alexanderbez left a comment

alexanderbez Jun 18, 2019

alessio Jun 18, 2019

alessio Jun 18, 2019

alessio Jun 18, 2019

alessio Jun 18, 2019

alessio Jun 18, 2019

alessio Jun 18, 2019

alessio Jun 18, 2019

alessio Jun 18, 2019

alessio Jun 18, 2019

alessio Jun 18, 2019

alessio left a comment

codecov bot commented Jun 18, 2019

	func Validate(denom string, amount Int) error {
	func validate(denom string, amount Int) error {

	if err := Validate(denom, amount); err != nil {
	if err := validate(denom, amount); err != nil {

	if err := Validate(coin.Denom, coin.Amount); err != nil {
	if err := validate(coin.Denom, coin.Amount); err != nil {

	// IsValid asserts that the Coin has a positive amount and the Denom is vaild.
	// IsValid returns true if the Coin has a non-negative amount and the denom is valid.

R4R: Added IsValid to Coin #4558

R4R: Added IsValid to Coin #4558

Conversation

colin-axner commented Jun 14, 2019

codecov bot commented Jun 14, 2019 • edited Loading

Codecov Report

fedekunze left a comment • edited Loading

Choose a reason for hiding this comment

fedekunze left a comment • edited Loading

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

fedekunze commented Jun 15, 2019

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

alessio commented Jun 16, 2019

alessio commented Jun 16, 2019

alessio commented Jun 16, 2019 • edited Loading

fedekunze commented Jun 16, 2019

alexanderbez commented Jun 16, 2019

fedekunze left a comment • edited Loading

Choose a reason for hiding this comment

colin-axner commented Jun 17, 2019

colin-axner commented Jun 17, 2019

alessio commented Jun 18, 2019 • edited Loading

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 18, 2019

Codecov Report

codecov bot commented Jun 14, 2019 •

edited

Loading

fedekunze left a comment •

edited

Loading

fedekunze left a comment •

edited

Loading

alessio commented Jun 16, 2019 •

edited

Loading

fedekunze left a comment •

edited

Loading

alessio commented Jun 18, 2019 •

edited

Loading