support denoms custom validation #7450

okwme · 2020-10-04T08:26:02Z

Backport of #6755

Closes: #6744
Origin: #6755

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-10-04T08:40:24Z

Codecov Report

❗ No coverage uploaded for pull request base (launchpad/backports@203876e). Click here to learn what that means.
The diff coverage is n/a.

@@                  Coverage Diff                   @@
##             launchpad/backports    #7450   +/-   ##
======================================================
  Coverage                       ?   50.23%           
======================================================
  Files                          ?      338           
  Lines                          ?    17548           
  Branches                       ?        0           
======================================================
  Hits                           ?     8815           
  Misses                         ?     7945           
  Partials                       ?      788

alessio · 2020-10-04T14:26:29Z

Hey! Just rebase this against launchpad/backports - that will do it

alessio · 2020-10-04T14:29:54Z

@ethanfrey @clevinson
Hello fellow SRMs!
I love this PR and it's good-to-go from me (pending rebase to the right target branch). I also believe that this does not require Stable Release Exception - this introduces neither API nor ABI changes.

ethanfrey

The code looks clean enough to me. And all that change are private (package-internal) variables, just adding some public variables.

I guess you could set this in app.go to allow different denoms, but I am unclear if that is actually useful? Keeping them strict and consistent is nice in the lead-up to IBC. Why do we want people to allow emoji denoms? (Show me the use case and I may approve it)

ethanfrey · 2020-10-04T17:44:04Z

types/coin_test.go

@@ -70,6 +70,31 @@ func TestCoinIsValid(t *testing.T) {
 	}
 }

+func TestCustomValidation(t *testing.T) {


This part I get

ethanfrey · 2020-10-04T17:45:43Z

types/coin.go

 )

+func returnDecCoin() *regexp.Regexp {


And so if I write the global CoinDenomRegex it will let me override all these checks done inside of Coin.

https://github.com/interchainberlin/pooltoy/blob/master/cmd/pooltoyd/main.go#L37

alessio · 2020-10-04T19:55:03Z

FTR @okwme, I've taken care of this ->

just rebase this against launchpad/backports - that will do it

👍

okwme · 2020-10-05T12:15:51Z

The code looks clean enough to me. And all that change are private (package-internal) variables, just adding some public variables.

I guess you could set this in app.go to allow different denoms, but I am unclear if that is actually useful? Keeping them strict and consistent is nice in the lead-up to IBC. Why do we want people to allow emoji denoms? (Show me the use case and I may approve it)

We made a kudos blockchain where users can mint one emoji denom coin a day: http://github.com/interchainberlin/pooltoy

I had to fork the SDK just to allow use of emojis. I can see why it's important for IBC denom management to be consistent but when the logic is just internal to the application it should be up to the application to make those design choices. We might go as far and actually make denom recommendations (in the form of default values maybe like now) but it's always going to be up to the app developer to make those final calls. We'd be better to give them a clean method of doing so rather than forcing them to fork the SDK and potentially fall out of sync with updates.

Do you think it's a strong enough security threat that we should be making it as difficult as possible to make custom denoms validation @ethanfrey ?

ethanfrey · 2020-10-05T13:05:12Z

Thank you for explaining the use case. Sounds like a fun toy.

I would just ask you to check with the IBC team on this one if it is an issue (or if they can add emoji support to ICS20). If they give it an okay, then I will too. This is more important for the merge to master, I guess this can go in, but then if we disable emojis in stargate, that would suck.

okwme · 2020-10-05T13:08:02Z

@cwgoes i think you’d confirmed previously that this wouldn’t be a problem, is that still correct?-- billyrennekamp.com

ethanfrey

Thinking a bit more, it shouldn't not stop an IBC transfer for an emoji from a chain to a non-emoji chain, as the tokens are described with hashes. It would just block displaying it properly on the remote chain. And that being that IBC doesn't add emoji support.

I actually see other use-cases changing lengths or casing or special chars per chain.

I just need to think on it a bit, but I approve this. Happy to see them pool toys

alessio

Good to go (pending merge of #6755 - that should go in first)

alessio

We need a changelog entry for this (e.g. https://github.com/cosmos/cosmos-sdk/pull/6755/files#diff-4ac32a78649ca5bdd8e0ba38b7006a1eR166)

…alidation

okwme · 2020-10-07T08:04:53Z

@alessio updated the changelog, is that the correct format?

alessio · 2020-10-07T08:10:08Z

@okwme looks good to me

okwme added 2 commits October 3, 2020 19:49

make coin evaluation into functions instead of static

c3cc14c

reset default regex

5b6cff4

okwme requested review from alessio, alexanderbez, fedekunze and jackzampolin as code owners October 4, 2020 08:26

ethanfrey reviewed Oct 4, 2020

View reviewed changes

alessio changed the base branch from okwme/v0.39.1-target to launchpad/backports October 4, 2020 19:53

alessio requested a review from clevinson as a code owner October 4, 2020 19:53

ethanfrey approved these changes Oct 6, 2020

View reviewed changes

alessio approved these changes Oct 6, 2020

View reviewed changes

alessio reviewed Oct 6, 2020

View reviewed changes

alessio added this to the 0.39.2 - The Launchpad Series milestone Oct 6, 2020

alessio mentioned this pull request Oct 6, 2020

RFC: Add Emoji support to denom #6744

Closed

9 tasks

alessio changed the title ~~Billy/6744 launchpad custom validation~~ support denoms' custom validation Oct 6, 2020

alessio changed the title ~~support denoms' custom validation~~ support denoms custom validation Oct 6, 2020

okwme added 3 commits October 7, 2020 10:00

Update CHANGELOG.md

726d57a

Update CHANGELOG.md

a74a519

Merge branch 'launchpad/backports' into billy/6744-launchpad-custom-v…

85d4291

…alidation

amaury1093 approved these changes Oct 7, 2020

View reviewed changes

okwme merged commit 3101928 into launchpad/backports Oct 7, 2020

okwme deleted the billy/6744-launchpad-custom-validation branch October 7, 2020 10:56

alessio mentioned this pull request Nov 21, 2020

types/Coin: compile and reuse Regexps to reduce massive RAM+CPU burn #8001

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support denoms custom validation #7450

support denoms custom validation #7450

okwme commented Oct 4, 2020 •

edited by alessio

Loading

codecov bot commented Oct 4, 2020 •

edited

Loading

alessio commented Oct 4, 2020

alessio commented Oct 4, 2020

ethanfrey left a comment

ethanfrey Oct 4, 2020

okwme Oct 5, 2020

ethanfrey Oct 4, 2020

okwme Oct 5, 2020

okwme Oct 5, 2020

alessio commented Oct 4, 2020

okwme commented Oct 5, 2020

ethanfrey commented Oct 5, 2020

okwme commented Oct 5, 2020 via email

ethanfrey left a comment •

edited

Loading

alessio left a comment

alessio left a comment

okwme commented Oct 7, 2020

alessio commented Oct 7, 2020

support denoms custom validation #7450

support denoms custom validation #7450

Conversation

okwme commented Oct 4, 2020 • edited by alessio Loading

codecov bot commented Oct 4, 2020 • edited Loading

Codecov Report

alessio commented Oct 4, 2020

alessio commented Oct 4, 2020

ethanfrey left a comment

Choose a reason for hiding this comment

ethanfrey Oct 4, 2020

Choose a reason for hiding this comment

okwme Oct 5, 2020

Choose a reason for hiding this comment

ethanfrey Oct 4, 2020

Choose a reason for hiding this comment

okwme Oct 5, 2020

Choose a reason for hiding this comment

okwme Oct 5, 2020

Choose a reason for hiding this comment

alessio commented Oct 4, 2020

okwme commented Oct 5, 2020

ethanfrey commented Oct 5, 2020

okwme commented Oct 5, 2020 via email

ethanfrey left a comment • edited Loading

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

okwme commented Oct 7, 2020

alessio commented Oct 7, 2020

okwme commented Oct 4, 2020 •

edited by alessio

Loading

codecov bot commented Oct 4, 2020 •

edited

Loading

ethanfrey left a comment •

edited

Loading