R4R: Make coins denoms case insensitive #3092

alessio · 2018-12-12T17:51:50Z

Closes: #3064

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
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)

codecov · 2018-12-12T18:01:05Z

Codecov Report

Merging #3092 into develop will increase coverage by 0.02%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #3092      +/-   ##
===========================================
+ Coverage    55.01%   55.04%   +0.02%     
===========================================
  Files          133      133              
  Lines         9435     9434       -1     
===========================================
+ Hits          5191     5193       +2     
+ Misses        3927     3924       -3     
  Partials       317      317

types/coin.go

alexanderbez

Changes look good but I agree that I think it should be lowercase.

cwgoes

I don't think we should do any runtime conversion - that's unnecessary and a waste of compute - but validating genesis / CLI / REST input for only-uppercase or only-lowercase makes sense.

alessio · 2018-12-13T08:58:04Z

@fedekunze @jackzampolin @alexanderbez

@rigelrozanski dixit:

I agree that it makes sense to prevent the either lowercase or uppercase - I guess making it all caps makes sense as tokens are usually listed as all caps

I agree with Rigel here - yet I'm open to change it lowercase if we reach a wider consensus

alessio · 2018-12-13T09:04:02Z

@cwgoes the safest way to do this is to normalize the denom on NewCoin() call. AmountOf() could assume that denom is already uppercase though, even though I don't really believe that a single call to ToUpper() will have such a big impact. Thoughts?

alessio · 2018-12-13T09:12:07Z

Furthermore, IMHO denom should never be empty. Thoughts on that?

cwgoes · 2018-12-13T11:20:39Z

the safest way to do this is to normalize the denom on NewCoin() call

Unfortunately, I think we don't use NewCoin uniformly - we also use sdk.Coin{} (maybe we could lint for that). Even if we did, I don't think we should implement this style of fix. It's not too expensive, you're right, but I would still prefer to fix other parts of our code instead of performing conversion at runtime - the latter costs us forever long-term, where the former just requires that we find the occurrences of incorrect capitalization now.

What would be fine, I think, is for NewCoin (and AmountOf) to panic upon receiving an uppercase denom - what do you think about that? That should help find any miscreant callers.

alessio · 2018-12-13T13:44:41Z

@cwgoes

What would be fine, I think, is for NewCoin (and AmountOf) to panic upon receiving an uppercase denom - what do you think about that? That should help find any miscreant callers.

I like that 👍

alessio · 2018-12-13T17:19:27Z

@cwgoes amended, please review

jackzampolin · 2018-12-13T17:34:00Z

I like this idea:

Unfortunately, I think we don't use NewCoin uniformly - we also use sdk.Coin{} (maybe we could lint for that).

Grepping through the code revels that we aren't using sdk.Coin{} in many places at all. Mainly in []sdk.Coin{sdk.NewCoin()} calls.

alessio · 2018-12-13T19:51:35Z

Unfortunately, I think we don't use NewCoin uniformly

alessio@bangalter:~/.../cosmos/cosmos-sdk$ grep -rw 'Coin{' 
types/coin_test.go:		{NewInt64Coin("a", 1), NewInt64Coin("a", 5), Coin{}, true},
types/coin.go:	return Coin{
types/coin.go:		res = append(res, Coin{
types/coin.go:		return Coin{}, fmt.Errorf("invalid coin expression: %s", coinStr)
types/coin.go:		return Coin{}, fmt.Errorf("failed to parse coin amount: %s", amountStr)
types/coin.go:		return Coin{}, fmt.Errorf("denom cannot contain upper case characters: %s", denomStr)
x/auth/client/txbuilder/txbuilder.go:	fee := sdk.Coin{}

alessio · 2018-12-14T13:00:24Z

@cwgoes amended as requested. Please review

cwgoes

One more requested change. Note also I think we'll need to sanitize input wherever we accept sdk.Coins directly in user-input messages (mostly in x/bank) to ensure maliciously crafted mixed-case denoms can't cause panics or unusual behaviour.

cwgoes · 2018-12-14T23:33:33Z

types/coin.go

@@ -320,13 +323,14 @@ func (coins Coins) Empty() bool {

 // Returns the amount of a denom from coins
 func (coins Coins) AmountOf(denom string) Int {
+	lowercaseDenom := strings.ToLower(denom)


In this function, can we also check the case like we do on line 464? I think that would be preferable to just converting to lowercase - we should be enforcing that all "denoms" are lowercase.

AmountOf at the moment is just converting denom to lowercase and process it (basically implementing real case insensitivity). I couldn't quite get what you would like to see instead - maybe you would prefer AmountOf to panic if a non-all-lowcase string is encountered?

Yes, exactly (instead of converting). Just want to ensure we don't accidentally have mixed-case somewhere in our code, which is implicitly converted in AmountOf - better to panic (and find such places if they exist).

amended @cwgoes, please review

Closes: #3064

cwgoes · 2018-12-17T22:10:33Z

Your changes look fine but now many tests seem to fail - are we panicking because of non-lowercase denominations somewhere?

alessio · 2018-12-17T22:24:03Z

@cwgoes no, I needed to merge develop in

cwgoes

Tested ACK

A reminder that we'll need to keep in mind the necessity of sanitizing user input (calling IsValid on every JSON-parsed sdk.Coins in ValidateBasic, basically).

jackzampolin · 2018-12-17T23:23:57Z

@cwgoes should that change be made with this PR or should we open an issue for that?

cwgoes · 2018-12-18T01:22:47Z

@cwgoes should that change be made with this PR or should we open an issue for that?

Oh - it's already fine in x/bank, I just mean as a reminder for the future.

cwgoes · 2018-12-18T01:23:14Z

(this needs a second review & approval, and it is a breaking change)

alexanderbez

Changes LGTM -- as long as we're consistent and use the constructors (NewCoin, NewInt64Coin) 👍

alessio added the ready-for-review label Dec 12, 2018

alessio requested review from alexanderbez and faboweb December 12, 2018 17:51

alessio requested review from cwgoes, ebuchman, rigelrozanski and zramsay as code owners December 12, 2018 17:51

alessio requested a review from jaekwon December 12, 2018 18:13

fedekunze reviewed Dec 12, 2018

View reviewed changes

types/coin.go Outdated Show resolved Hide resolved

alexanderbez reviewed Dec 12, 2018

View reviewed changes

cwgoes suggested changes Dec 12, 2018

View reviewed changes

alessio force-pushed the alessio/3064-case-insensitive-coins branch from 160ca1e to d932fdb Compare December 13, 2018 19:50

cwgoes self-assigned this Dec 14, 2018

cwgoes suggested changes Dec 14, 2018

View reviewed changes

Make coins denoms case insensitive

e5667f0

Closes: #3064

alessio force-pushed the alessio/3064-case-insensitive-coins branch from 5192b4c to e5667f0 Compare December 16, 2018 19:10

Alessio Treglia added 2 commits December 17, 2018 21:14

AmountOf must fail if denom contains upper case chars

198d06b

Test panic

5a85879

Merge branch 'develop' into alessio/3064-case-insensitive-coins

1034dda

Merge branch 'develop' into alessio/3064-case-insensitive-coins

c5b6e46

cwgoes approved these changes Dec 17, 2018

View reviewed changes

jackzampolin mentioned this pull request Dec 17, 2018

Release 0.29 Tracking #3139

Closed

jackzampolin added the T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). label Dec 18, 2018

alexanderbez approved these changes Dec 18, 2018

View reviewed changes

cwgoes mentioned this pull request Dec 18, 2018

"Red Wedding" (first GoS hard fork) master to-do list #3079

Closed

7 tasks

jackzampolin merged commit 500fa2b into develop Dec 18, 2018

jackzampolin deleted the alessio/3064-case-insensitive-coins branch December 18, 2018 19:14

jbowen93 mentioned this pull request May 12, 2022

Support core nodes with single or multiple validators celestiaorg/ephemeral-cluster#22

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Make coins denoms case insensitive #3092

R4R: Make coins denoms case insensitive #3092

alessio commented Dec 12, 2018 •

edited

Loading

codecov bot commented Dec 12, 2018 •

edited

Loading

alexanderbez left a comment

cwgoes left a comment

alessio commented Dec 13, 2018

alessio commented Dec 13, 2018

alessio commented Dec 13, 2018

cwgoes commented Dec 13, 2018 •

edited

Loading

alessio commented Dec 13, 2018

alessio commented Dec 13, 2018

jackzampolin commented Dec 13, 2018 •

edited

Loading

alessio commented Dec 13, 2018

alessio commented Dec 14, 2018

cwgoes left a comment

cwgoes Dec 14, 2018

alessio Dec 16, 2018 •

edited

Loading

cwgoes Dec 17, 2018

alessio Dec 17, 2018

cwgoes commented Dec 17, 2018

alessio commented Dec 17, 2018

cwgoes left a comment

jackzampolin commented Dec 17, 2018

cwgoes commented Dec 18, 2018

cwgoes commented Dec 18, 2018

alexanderbez left a comment

R4R: Make coins denoms case insensitive #3092

R4R: Make coins denoms case insensitive #3092

Conversation

alessio commented Dec 12, 2018 • edited Loading

codecov bot commented Dec 12, 2018 • edited Loading

Codecov Report

alexanderbez left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

alessio commented Dec 13, 2018

alessio commented Dec 13, 2018

alessio commented Dec 13, 2018

cwgoes commented Dec 13, 2018 • edited Loading

alessio commented Dec 13, 2018

alessio commented Dec 13, 2018

jackzampolin commented Dec 13, 2018 • edited Loading

alessio commented Dec 13, 2018

alessio commented Dec 14, 2018

cwgoes left a comment

Choose a reason for hiding this comment

cwgoes Dec 14, 2018

Choose a reason for hiding this comment

alessio Dec 16, 2018 • edited Loading

Choose a reason for hiding this comment

cwgoes Dec 17, 2018

Choose a reason for hiding this comment

alessio Dec 17, 2018

Choose a reason for hiding this comment

cwgoes commented Dec 17, 2018

alessio commented Dec 17, 2018

cwgoes left a comment

Choose a reason for hiding this comment

jackzampolin commented Dec 17, 2018

cwgoes commented Dec 18, 2018

cwgoes commented Dec 18, 2018

alexanderbez left a comment

Choose a reason for hiding this comment

alessio commented Dec 12, 2018 •

edited

Loading

codecov bot commented Dec 12, 2018 •

edited

Loading

cwgoes commented Dec 13, 2018 •

edited

Loading

jackzampolin commented Dec 13, 2018 •

edited

Loading

alessio Dec 16, 2018 •

edited

Loading