types: update coin regex by fedekunze · Pull Request #7027 · cosmos/cosmos-sdk

fedekunze · 2020-08-12T18:15:14Z

Description

Split coin changes from #6871 for ease of review
closes #6716

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

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.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2020-08-12T18:19:40Z

Codecov Report

Merging #7027 into master will increase coverage by 0.97%.
The diff coverage is 91.78%.

@@            Coverage Diff             @@
##           master    #7027      +/-   ##
==========================================
+ Coverage   60.99%   61.96%   +0.97%     
==========================================
  Files         380      520     +140     
  Lines       24753    32022    +7269     
==========================================
+ Hits        15097    19843    +4746     
- Misses       8467    10551    +2084     
- Partials     1189     1628     +439

types/coin.go

ethanfrey

It needs to be clear if atom, ATOM, and Atom are the same or different tokens.
And then make that stick in all code - subtracting, adding, ordering. Not just for the dup checks.

I am find with either decision (those 3 denoms are same or different), but it should be consistent

types/coin.go

ethanfrey · 2020-08-12T20:09:45Z

types/coin_test.go

-		{"gAs", NewInt(1)},
 		{"MineraL", NewInt(1)},
 		{"TREE", NewInt(1)},
+		{"gAs", NewInt(1)},


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.

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

yes, I removed it now

ethanfrey · 2020-08-12T20:10:22Z

types/coin_test.go

-	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")


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.

types/coin.go

Co-authored-by: Aaron Craelius <aaron@regen.network>

AdityaSripal

A little confused as to whether the case is ignored or not in denominations

types/coin.go

AdityaSripal · 2020-08-12T22:39:42Z

types/coin_test.go

-		{"gAs", NewInt(1)},
 		{"MineraL", NewInt(1)},
 		{"TREE", NewInt(1)},
+		{"gAs", NewInt(1)},


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

types/dec_coin.go

okwme · 2020-08-13T08:13:46Z

types/coin_test.go

-	assert.False(t, mixedCase3.IsValid(), "Single denom in Coins contains upper case characters")
+	assert.True(t, goodCaps.IsValid(), "Coins all caps are valid")
+	assert.True(t, unicodeLinearBAndRoman.IsValid(), "Unicode characters")
+	assert.False(t, unicodeEmoji.IsValid(), "Unicode emoji")


okwme · 2020-08-13T08:17:31Z

This looks good to me but I'm still a little confused about how customization would fare in an IBC context. For example I would go into main.go of my application and overwrite the Coin.Validate method to use custom regex that allows emoji in the denom. If I were to transfer one of these Coins to the Hub via IBC, would it cause an error on the Hub side?

colin-axner · 2020-08-13T08:53:33Z

This looks good to me but I'm still a little confused about how customization would fare in an IBC context. For example I would go into main.go of my application and overwrite the Coin.Validate method to use custom regex that allows emoji in the denom. If I were to transfer one of these Coins to the Hub via IBC, would it cause an error on the Hub side?

no, ADR 1 specifically considers this issue. The transfer denom is internally represented as ibc/<hash>

where

<hash> = Sha256Hash( {portIDN}/{channelIDN/.../.../{portID0}/{channelID0}/{baseDenom} )

Since the baseDenom is included in the hash the SDK does not care what it is. By updating the validation in this pr, any zone or hub that uses the SDK will be compatible with the SDK's ibc-transfer module. Furthermore, any zone or hub could mint denominations in any format except empty strings and transfer them to the cosmos hub where internally the are represented by the hash.

This mapping is only internally represented on the SDK. Zones or Hubs implementing different versions of ibc-transfer could internally represent the denomination in any way as long as they can send and recv packets properly and with the correct info required by the ICS20 specification.

If a zone or hub where to enforce stricter denomination requirements than the default in the SDK, if they did not support:

lower/upper case characters
integers
/
length of ibc/<hash>

then it would not be compatible with the ibc-transfer module and would return an error. These are pretty easy requirements to fulfill though

cwgoes

Agreed that the tests should be converted to table-form.

Regarding unicode characters, I think we need to be a little careful - are there any two characters which look very similar / identical but are actually represented differently (i.e., are unequal)? This can be an attack vector (where some malicious agent purposefully creates a denom that looks almost exactly like a real one - e.g. atom - but is actually unequal).

colin-axner

Overall LGTM. I think we should take the time to document some key design decisions/rational while everyone is viewing these changes. coin.go has always been a poorly documented mystery to me without explanations for important code decisions. Sounds to me like we should open an issue for an ADR for coin.go and DecCoins

types/coin.go

types/coin_test.go

colin-axner · 2020-08-13T09:25:22Z

types/coin.go


-	if amount.IsNegative() {
-		return fmt.Errorf("negative coin amount: %v", amount)
+	if coin.Amount.IsNegative() {


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

types/dec_coin.go

types/dec_coin_test.go

fedekunze · 2020-08-13T15:19:21Z

Thanks for your reviews. Regarding the attack vectors and Unicode, I've decided to undo the changes that add Unicode letters support until these changes are documented in an ADR as @colin-axner suggested. This PR was only meant for adding the coin changes from the already approved ADR001, so I guess it makes sense to scope the PR changes to that only.

…n-regex

fedekunze · 2020-08-13T16:25:47Z

I addressed all the requested changes. I also opened issues for out of scope discussions about the Coins and tests #7046 #7031

… update-coin-regex

colin-axner

ACK

AdityaSripal

Can there be tests confirming that adding atom + ATOM yield 2 coins and not 1 or something similar?

AdityaSripal · 2020-08-13T17:51:55Z

types/coin.go

-	// 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 ('/').


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

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

…n-regex

addressed

* types: update coin regex * more tests * fix test case * increase coverage * changelog * fix tests * support unicode letters and numbers * remove ToUpper * Update types/coin.go Co-authored-by: Aaron Craelius <aaron@regen.network> * Validate function * fix ICS20 test * table tests and revert unicodes * add test case * add test cases and comment Co-authored-by: Aaron Craelius <aaron@regen.network>

types: update coin regex

fbec784

fedekunze added the C:Types label Aug 12, 2020

fedekunze added 3 commits August 12, 2020 20:48

more tests

497e011

fix test case

2b22419

increase coverage

9df2c67

fedekunze marked this pull request as ready for review August 12, 2020 19:01

fedekunze requested review from aaronc, alessio, alexanderbez, clevinson and jgimeno as code owners August 12, 2020 19:01

fedekunze requested review from AdityaSripal, cwgoes, ethanfrey, tac0turtle and zmanian August 12, 2020 19:01

changelog

431ede8

alexanderbez approved these changes Aug 12, 2020

View reviewed changes

fedekunze commented Aug 12, 2020

View reviewed changes

types/coin.go Show resolved Hide resolved

aaronc reviewed Aug 12, 2020

View reviewed changes

types/coin.go Show resolved Hide resolved

fix tests

afe2627

ethanfrey reviewed Aug 12, 2020

View reviewed changes

fedekunze added 2 commits August 12, 2020 22:26

support unicode letters and numbers

9d4c70e

remove ToUpper

216ebc8

aaronc reviewed Aug 12, 2020

View reviewed changes

types/coin.go Outdated Show resolved Hide resolved

Update types/coin.go

595a9d2

Co-authored-by: Aaron Craelius <aaron@regen.network>

AdityaSripal suggested changes Aug 12, 2020

View reviewed changes

Validate function

cbb88a0

fedekunze requested a review from AdityaSripal August 13, 2020 07:00

fedekunze requested review from amaury1093, anilcse, atheeshp and blushi August 13, 2020 08:08

okwme reviewed Aug 13, 2020

View reviewed changes

cwgoes reviewed Aug 13, 2020

View reviewed changes

colin-axner suggested changes Aug 13, 2020

View reviewed changes

fedekunze added 2 commits August 13, 2020 17:20

Merge branch 'master' of github.com:cosmos/cosmos-sdk into update-coi…

0bd648f

…n-regex

table tests and revert unicodes

e7c09f7

fedekunze requested a review from colin-axner August 13, 2020 16:11

fedekunze and others added 2 commits August 13, 2020 18:11

Merge branch 'master' into update-coin-regex

94fb4a9

add test case

549e121

fedekunze mentioned this pull request Aug 13, 2020

Write ADR for Coins #7046

Closed

4 tasks

Merge branch 'update-coin-regex' of github.com:cosmos/cosmos-sdk into…

12eece3

… update-coin-regex

colin-axner approved these changes Aug 13, 2020

View reviewed changes

AdityaSripal previously requested changes Aug 13, 2020

View reviewed changes

fedekunze added 2 commits August 14, 2020 10:51

Merge branch 'master' of github.com:cosmos/cosmos-sdk into update-coi…

9945305

…n-regex

add test cases and comment

e12e38d

Merge branch 'master' into update-coin-regex

928099c

fedekunze added the automerge label Aug 14, 2020

mergify bot merged commit 1b08e10 into master Aug 14, 2020

mergify bot deleted the update-coin-regex branch August 14, 2020 09:09

webmaster128 mentioned this pull request Aug 18, 2020

Update Fee tables cosmos/cosmjs#399

Merged

ig-shaun mentioned this pull request Jun 13, 2021

docs: ADR-043 Base NFT Module #9329

Merged

9 tasks

Conversation

fedekunze commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

codecov bot commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

ethanfrey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AdityaSripal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

okwme commented Aug 13, 2020

Uh oh!

colin-axner commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cwgoes left a comment

Choose a reason for hiding this comment

Uh oh!

colin-axner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fedekunze commented Aug 13, 2020

Uh oh!

fedekunze commented Aug 13, 2020

Uh oh!

colin-axner left a comment

Choose a reason for hiding this comment

Uh oh!

AdityaSripal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

fedekunze commented Aug 12, 2020 •

edited

Loading

codecov bot commented Aug 12, 2020 •

edited

Loading

colin-axner commented Aug 13, 2020 •

edited

Loading

AdityaSripal left a comment •

edited

Loading