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

Post reaction registration and limiting #94

Closed
RiccardoM opened this issue Feb 4, 2020 · 5 comments · Fixed by #120
Closed

Post reaction registration and limiting #94

RiccardoM opened this issue Feb 4, 2020 · 5 comments · Fixed by #120
Assignees
Labels
kind/new-feature Propose the addition of a new feature that does not yet exist
Milestone

Comments

@RiccardoM
Copy link
Contributor

RiccardoM commented Feb 4, 2020

Context

Currently when a user adds a reaction to a post, he has the freedom of adding whatever reaction he prefers best. This allows a freedom of expression, and results in post having different kinds of reactions:

  • single emoji ones (e.g. "👍:", here)
  • the ones containing multiple emojis (e.g."😃😃😃" like this one)
  • the ones containing a text (e.g. "like", here)

While this allows users to better express themselves, it makes reactions representation a lot harded inside mobile application, which will most certainly be the way users will be able to use them.

For example, in order to present and allow users to easily use post reactions, Mooncake currently only shows single Unicode characters emojis, thus filtering out the second and third type described above.

The problem with such approach is that it causes an incoherent visualization of post reactions across the different platform. To make it more clear why, let's suppose that the following reaction have been added to a post:

  1. "👍"
  2. "😃😃😃"
  3. "like"

What would happen inside the explorer is that all of the reaction are going to be visualized properly. However, inside Mooncake only the first one will be shown, resulting in a conflicting and most probably confusing situation for the end user that sees two different things on two different platforms.

To solve this problem all over the Desmos client implementation, a unified approach should be taken.

Implementation proposal

After a brief discussion between me and @kwunyeung we found a possible solution that heavily imitates what currently happens inside Github, Slack, Discord and other channels: using emojis shortcodes.

According to this approach, all reactions will be identified by a string that has the following format:

:<value>:

Such string, also referenced as "emoji short code", is supported by most platforms and uniquely identifies an emoji. Examples of such shortcodes are the following:

  • :+1: -> 👍
  • :small_airplane: -> 🛩️
  • :smile: -> 😄

In order to allow users to express their opinions the best way the can, we have thought of also implementing custom shotcodes to allow for custom reactions registrations.

In order to implement such things, a list of supported shortcodes should be kept on-chain, and each time a new reaction is added to a post, that reaction should be a registered shortcode. If it is not, the adding should not proceed.

Implementation details

To implement this the best way possible, I suggest we create a new module called reactions which contains the following messaged handling and data storing logic.

Types

The current Reaction type should be renamed to PostReaction, while a new Reaction should be created:

// Reaction represents a registered reaction that can be referenced 
// by its shortcode inside post reactions
type Reaction struct { 
    Shortcode string
    Value string
}

Such type should also have a Validate method that checks the following constraints:

  • Shortcode must be a string starting and ending with a colon (:) and not containing any white space, thus satisfying the following regex: :[a-z]([a-z\d_])*:
  • Value must be either a single unicode emoji or a URL starting with http(s). This is to allow users to add gifs and custom pics as reactions too

Keeper

All the current methods regarding post reactions should be kept.

The following new methods should be created as well:

// RegisterReaction allows to register a new reaction for later reference
func (k Keeper) RegisterReaction(ctx sdk.Context, reaction types.Reaction) {}

// DoesReactionForShortcodeExist checks whether a reaction already exists for the given shortcode, returning it if it does.
func (k Keeper) DoesReactionForShortcodeExist(ctx sdk.Context, shortcode string) (types.Reaction, bool) {}

// ListReactions returns all the registered reactions
func (k Keeper) ListReactions(ctx sdk.Context) []types.Reaction {}

Messages

The current MsgAddReaction and MsgRemoveReaction types should be moved here.

An additional MsgRegisterReaction should be created:

// MsgRegisterReaction represents the message that must be used when wanting 
// to register a new reaction shortcode and the associated value
type MsgRegisterReaction struct {
    Shortcode string `json:"shortcode"`
    Value string     `json:"value"`
}

Such message should have the following constraints:

  • Shortcode must be a string starting and ending with a column (:) and not containing any white space, thus satisfying the following regex: :(\S)*:
  • Value must be either a single unicode emoji or a URL starting with http(s). This is to allow users to add gifs and custom pics as reactions too

Handler

The currently existing handler methods that deal with post reactions should be moved inside the reactions module handler.

A new method handling MsgRegisterPostReaction should be created. Such method should act as follows:

  1. Checks whether a reaction already exists for the specified msg.Shortcode.
  2. If the reaction exists, returns an error.
  3. If the reaction does not exist, registers the new reaction properly.

Querier

The currently present post reactions-related queries should be moved inside this new querier.

A new query should also be implemented, allowing a user to get all the registered reactions.

Conclusion

I think that with such implementation we could fix the problem described above, maintaining a nice on-chain list of all the supported reactions. As always, I'd like your thoughts on this too @kwunyeung @bragaz

@RiccardoM RiccardoM added kind/new-feature Propose the addition of a new feature that does not yet exist status/specification This feature is currently in the specification process labels Feb 4, 2020
@RiccardoM RiccardoM added this to the v0.4.0 milestone Feb 4, 2020
@kwunyeung
Copy link
Contributor

I see the solution fine here. I have the following comments.

  1. In the shortcode, we should also exclude the - and force to use _ as concatenation. This avoid duplicates from :two_hearts: and :two-hearts:. It should start with a letter and better make sure all letters in the shortcode are lower case. So the rule I'm thinking is to only accept letter a-z, numbers and underscore (_). The regex will become :[a-z]([a-z\d_])*:. This also matches the format of Github emoji shortcode.
  2. We should record the owner of the reactions in the state when we let them register their own. Should we let them edit the reactions too?
  3. We need to have a tighter validation to the Value as well. Emoji unicode starts with U+1F600 and I guess it does not have an ending limit for now. This unicode range should already exclude all other language character ranges. The largest unicode range should be CJK and it's before the emoji range, so should be safe. If the Value is an url, we may not be able to check the mimetype but we should at least check if it's ending with (.gif|.jpg|.jpeg|.png).

@leobragaz
Copy link
Contributor

The implementation proposal is good. I agree with @kwunyeung's 1 comment.
To answer the 2nd one; we could register them with a prefix that includes the owner's address, so it will be easy later iterate on them by user. Also, should we also provide the deletion of them? And maybe set a limit on how many emojis a user can register.
3. I think checking the ending should be enough but maybe we could add also an omittable mime-type field that would be used when URL is provided.
What do you think? @RiccardoM @kwunyeung

@RiccardoM
Copy link
Contributor Author

@kwunyeung @bragaz I'm splitting my comments into different sections for easier reading.

RegEx update

Thanks @kwunyeung for the tip, I have updated the original regex to be the one you've written as I think too it is better.

Emoji ownership

I see no problem in saving who is the owner, but I see some problems in allowing him/her to edit the emoji value if we use the implementation proposed above.

Suppose that a user creates an emoji called :smile and associated to it a simple smiley emoji (:smile:). Now, this reaction is later used by thousands of people, and one day he changes it to be something more strange, like a cross :latin_cross:. This might cause some problems if the emoji is widely used between a lot of different apps based on Desmos.

To solve this issue I currently see two possible options:

  1. Do not allow emoji editing. In this way, once an emoji is created its value will not be changed whatsoever.

  2. Other than the creator of the emoji, save the subpsace for which the emoji has been created. This way users would be able to use the same shortcode for different emojis based on the app they are creating it for.
    An example might be the willing of using the :mooncake: shortcode differently based on the case we are inside our Mooncake app (and thus we want to associate its logo to it) or another app (in this case we might want to associated the chinese cake icon).

URL checking

I'm personally against this URL checking thing. While it is generically true that most of the images have URLs that terminate with the image extension itself, some don't. If the user starts grabbing images from Google Search, he might find ones like the followings:

In the first case, the extensions is available but it is not the last thing the URL has. In the second one, the extensions is not even visible.

Generally, when we refer to an image there is no need for the URL to have the extension inside it. We just have to make sure the request returns a proper Content-Type value.

What I would do, instead, is think more deeply how a possible desmoscli tx posts register-reaction CLI command might work. As this is executed locally, and we can perform HTTP requests while executing it, what I would do instead is trying to fetch the custom image the user is trying to use and verify the Content-Type returned is valid. This would allow to block invalid requests from the CLI.

For the REST part, however, there is no global solution as we should always consider the blockchain cannot perform any HTTP call and we should not force any URL form. If the user sets an invalid image, the worst thing would be the clients will not show it altogether.

@kwunyeung
Copy link
Contributor

@RiccardoM you are right on the Emoji ownership. Saving subspace is a better solution.

We can only request a range of the file and verify with the content-type from the URL.
https://stackoverflow.com/questions/27844307/how-to-download-only-the-beginning-of-a-large-file-with-go-http-client-and-app

@RiccardoM RiccardoM removed the status/specification This feature is currently in the specification process label Mar 6, 2020
@leobragaz
Copy link
Contributor

After a discussion with @RiccardoM about cyclic dependency of a possibile new reactions module with the posts module, we decided to keep all the reactions related code inside the posts module but splitting it in different files such as keeper_reactions to have a better clearness of the code itself.
We will open a new issue to discuss the possibile ways to separate reactions code from posts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/new-feature Propose the addition of a new feature that does not yet exist
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants