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

RFC: Add Emoji support to denom #6744

Closed
wants to merge 13 commits into from
Closed

RFC: Add Emoji support to denom #6744

wants to merge 13 commits into from

Conversation

okwme
Copy link
Contributor

@okwme okwme commented Jul 16, 2020

Description

This PR adds support for emojis to the coin type's denom. It adds a whitelist of UTF8 character ranges that encompass the basic unicode 3.0 emoji list (http://unicode.org/Public/emoji/3.0/emoji-data.txt).

I'm not sure what the original reasoning was for limiting denoms to lowercase alphanumeric characters between 3 and 16 in length. This maintains that limitation while adding exceptions for emojis. I know this is a silly PR but would be curious to hear exactly why this might not be a bad idea before closing it. Otherwise if there are no strong reasons it could benefit the SDK to be emoji compatible as a PR stunt or to encourage more playful tutorials (currently working on a burner chain that uses emojis as a currency).

closes: #XXXX


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
Copy link

codecov bot commented Jul 16, 2020

Codecov Report

❗ No coverage uploaded for pull request base (release/v0.38.4@1c917e2). Click here to learn what that means.
The diff coverage is n/a.

@@                Coverage Diff                 @@
##             release/v0.38.4    #6744   +/-   ##
==================================================
  Coverage                   ?   51.50%           
==================================================
  Files                      ?      337           
  Lines                      ?    20617           
  Branches                   ?        0           
==================================================
  Hits                       ?    10619           
  Misses                     ?     9195           
  Partials                   ?      803           

@fedekunze
Copy link
Collaborator

fedekunze commented Jul 16, 2020

@okwme this needs to be rebased to master, and once merged, cherry-picked to the release branch

@alessio
Copy link
Contributor

alessio commented Jul 16, 2020

@fedekunze dixit:

this needs to be rebased to master, and once merged, cherry-picked to the release branch

True that. Plus, I'd like to see some test cases.

It's a fun idea, but perhaps instead of amending the regular expression we should finally find a way to allow client apps to implement their own denom validation functions.

@fedekunze fedekunze changed the base branch from rc0/v0.39.0 to master July 16, 2020 16:55
@okwme
Copy link
Contributor Author

okwme commented Jul 16, 2020

It's a fun idea, but perhaps instead of amending the regular expression we should finally find a way to allow client apps to implement their own denom validation functions.

ooh i like that idea @alessio. What do you think that would ideally look like?

@alessio
Copy link
Contributor

alessio commented Jul 16, 2020

diff --git a/types/coin.go b/types/coin.go
index 96b9c71b2..9c7ec9cdb 100644
--- a/types/coin.go
+++ b/types/coin.go
@@ -589,15 +589,18 @@ var (
        reDecCoin   = regexp.MustCompile(fmt.Sprintf(`^(%s)%s(%s)$`, reDecAmt, reSpc, reDnmString))
 )
 
-// ValidateDenom validates a denomination string returning an error if it is
-// invalid.
-func ValidateDenom(denom string) error {
+// DefaultValidateDenom is the default validation function for Coin.Denom.
+func DefaultValidateDenom(denom string) error {
        if !reDnm.MatchString(denom) {
                return fmt.Errorf("invalid denom: %s", denom)
        }
        return nil
 }
 
+// ValidateDenom validates a denomination string returning an error if it is
+// invalid.
+var ValidateDenom = DefaultValidateDenom
+
 func mustValidateDenom(denom string) {
        if err := ValidateDenom(denom); err != nil {
                panic(err)

Something like that would allow the client app to customise the ValidateDenom.

@okwme okwme changed the base branch from master to rc0/v0.39.0 July 17, 2020 05:04
@okwme okwme changed the base branch from rc0/v0.39.0 to release/v0.38.4 July 17, 2020 05:04
@okwme
Copy link
Contributor Author

okwme commented Jul 17, 2020

Closing in favor of a PR for custom validation against master

@okwme okwme closed this Jul 17, 2020
@okwme okwme mentioned this pull request Jul 17, 2020
9 tasks
@alessio
Copy link
Contributor

alessio commented Oct 6, 2020

Reopening this as it's not yet fixed (yet it will be so soon, see #6755 and #7450)

@alessio alessio reopened this Oct 6, 2020
@alessio
Copy link
Contributor

alessio commented Oct 6, 2020

My fault, this is a PR - not an issue. Hence closing again.

@alessio alessio closed this Oct 6, 2020
alessio pushed a commit that referenced this pull request Oct 6, 2020
Allow ValidateDenom to be customized per application.

closes: #6744
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants