Support for media posts

[kwunyeung]

Propose adding a Media and MediaType field in Post module for storing a media file in a post. Similar to an image of a Twitter tweet or an image post of Instagram.

This field is optional.
If it exists, validate if the Media value is a valid url. MediaType should be an option from a list of mime types.

[RiccardoM]

Media post proposal

Current status

Currently (inside #40) the Post object is represented as follows:

type Post struct {
    PostID     PostID         `json:"id"`
    ParentID   PostID         `json:"parent_id"`
    Message    string         `json:"message"`
    Created    int64          `json:"created"`     // Block height at which the post has been created
    LastEdited int64          `json:"last_edited"` // Block height at which the post has been edited the last time
    Owner      sdk.AccAddress `json:"owner"`
}

Media post implementation

Instead of adding multiple fields into the above Post definition, what I would advise is adding a single new field that contains the data of the media(s) associated to such post:

// PostMedia represents a media file associated to a post
type PostMedia struct {
    Uri      string `json:"uri"`
    MimeType string `json:"mime_type"`
}

// Post is a struct of a Magpie post
type Post struct {
    PostID     PostID         `json:"id"`
    ParentID   PostID         `json:"parent_id"`
    Message    string         `json:"message"`
    Medias     []PostMedia    `json:"medias"`  // List of medias associated to this post
    Created    int64          `json:"created"`     
    LastEdited int64          `json:"last_edited"`
    Owner      sdk.AccAddress `json:"owner"`
}

In this way we can easily support multiple medias being added to a single post without any problem.

Validity checks

During the Post creation, the following checks should be implemented:

1. The Medias field should either be empty or contain only valid PostMedia objects.
2. Each PostMedia should have:
   1. A non-empty URI value
   2. A non-empty MimeType value

About the URI

Having such a generic field named Uri can raise some questions, such as:

1. How can the user specify the provider associated to the URI?
   Suppose he uses an IPFS file identifier such as QmT9qk3CRYbFDWpDFYeAv8T8H1gnongwKhh5J68NLkLir6. How can he specify that in order to download the content of this media the user needs to use IPFS and not (i.e) Amazon AWS?
2. How can we validate this URI?
   Let's take the same example as above where the user inserts an IPFS file identifier. How can we validate such data? Should it be considered valid or should we require him to prepend the https://ipfs.io/ipfs prefix to transform it into a Web2 link?

To solve these proplems, another field could be added:

type PostMedia struct {
    Provider string `json:"provider"`
    Uri      string `json:"uri"`
    MimeType string `json:"mime_type"`
}

In this way the user would have to select from a list of providers before inputting the Uri value. Such providers could be:

Provider	Associated URI value	Associated RegExp
web2	A common Web 2 https link	^(?:https:\/\/)[\w.-]+(?:\.[\w\.-]+)+[\w\-\._~:/?#[\]@!\$&'\(\)\*\+,;=.]+$
ipfs	An IPFS file identifier (Qm...)	Qm[a-zA-Z0-9]*

Also, new providers could later be added from the governance specifying the identifier and the regex to be used during the message validation.

About the MIME type

I'm currently not sure whether we should check the content of the MimeType field or not. Here are my pros and cons of validating it.

Pros

• We can make sure that the field is not used as a memo field of some sort. All the values inserted will be at least taken from a preset list of mime types.

Cons

• We cannot guarantee that the real mime type of the file is the one specified.
  Suppose we have a user that links a PDF and sets the mime type to text/plain. How can we check the real time? Full nodes do not have internet access and thus we cannot do it.

Conclusions

I'd personally go with an implementation such as

// PostMedia represents a media file associated to a post
type PostMedia struct {
    Provider string `json:"provider"`
    Uri      string `json:"uri"`
    MimeType string `json:"mime_type"`
}

// Post is a struct of a Magpie post
type Post struct {
    PostID     PostID         `json:"id"`
    ParentID   PostID         `json:"parent_id"`
    Message    string         `json:"message"`
    Medias     []PostMedia    `json:"medias"`  // List of medias associated to this post
    Created    int64          `json:"created"`     
    LastEdited int64          `json:"last_edited"`
    Owner      sdk.AccAddress `json:"owner"`
}

Where each possible valid Provider and MimeType value is specified during genesis as:

type Provider struct {
    Id    string `json:"id"`
    RegEx string `json:"regex"`
}

type GenesisState struct {
    Providers []Provider `json:"providers"`
    MimeType  []string `json:"mime_types"`
}

Such lists can be used as reference to know what providers and MIME types are supported.
They can also be used by the governance to add a new provider or a new MIME type to the supported list.

[RiccardoM]

After reviewing this implementation and testing some other ones with the Amino encoder, I think we can go another way too.

Second Media Post implementation proposal

Instead of adding a new field into the original Post object like so:

// Post is a struct of a Magpie post
type Post struct {
    PostID     PostID         `json:"id"`
    ParentID   PostID         `json:"parent_id"`
    Message    string         `json:"message"`
    Medias     []PostMedia    `json:"medias"`  // List of medias associated to this post
    Created    int64          `json:"created"`     
    LastEdited int64          `json:"last_edited"`
    Owner      sdk.AccAddress `json:"owner"`
}

What we can do is define a new type, called MediaPost, formed as such:

type MediaPost struct {
    Post
    Medias []PostMedia `json:"medias"`
}

The PostMedia will always be defined as

// PostMedia represents a media file associated to a post
type PostMedia struct {
    Provider string `json:"provider"`
    Uri      string `json:"uri"`
    MimeType string `json:"mime_type"`
}

Pros and cons

Pros

• Usage of Go's composition principle
• Avoid adding useless Medias field to normal posts
• Better separation of concepts
• Easier to create other post types/compose posts types

Cons

• Need to specify the way Amino encodes this

  cdc.RegisterConcrete(Post{}, "desmos/Post", nil)
  cdc.RegisterConcrete(MediaPost{}, "desmos/MediaPost", nil)

• Need to define custom serialization for types encapsulating Post:

  func (mp MediaPost) MarshalJSON() ([]byte, error) {
     type mediaPostJSON MediaPost
     return json.Marshal(mediaPostJSON(mp))
  }

• Need to define a custom deserialization for types encapsulating Post:

  func (mp *MediaPost) UnmarshalJSON(data []byte) error {
     type mediaPostJSON MediaPost
     var temp mediaPostJSON
     if err := json.Unmarshal(data, &temp); err != nil {
         return err
     }
     *mp = MediaPost(temp)
     return nil
  }

[kwunyeung]

I'm up for the 2nd proposal. Building custom marshal/unmarshal methods is not a big deal. It makes the code cleaner and no media file is added to normal post. This is how the VestingAccount working in the SDK (https://github.com/cosmos/cosmos-sdk/blob/82a2c5d6d86ffd761f0162b93f0aaa57b7f66fe7/x/auth/vesting/types/vesting_account.go#L35).

We cannot and not necessary to verify the mime types on-chain. It depends on how the applications work and specify it as the uploading and rendering parts won't be done on-chain. The URI should be a valid URI which would have direct access to the file. The chain doesn't have to understand the protocol of the URI. For example, if the application uploaded a file to IPFS and get a hash, the application might want to store https://www.cloudflare.com/ipfs/{hash} in the URI field. If the application uploaded to AWS, then it might store https://{bucket}.s3.aws.com/{file}, etc. Storing the mime types are for the applications to know how the files should be rendered. So I don't think the Provider is necessary in any cases.

[RiccardoM]

The URI should be a valid URI which would have direct access to the file. The chain doesn't have to understand the protocol of the URI.

Should we then use the generic Web 2 URI RegEx to verify if it's a valid URI?

^(?:https:\/\/)[\w.-]+(?:\.[\w\.-]+)+[\w\-\._~:/?#[\]@!\$&'\(\)\*\+,;=.]+$

This only makes sure the URI starts with https (I would avoid supporting http anymore) and accepts any domain of sort

[kwunyeung]

And IPFS if they don't use web gateway?

[RiccardoM]

And IPFS if they don't use web gateway?

🤔 This is going to be hard then. We have two options then:

1. Required the URI to always start with https://
   This would mean having the users to use https://ipfs.io/ipfs/Qm... for IPFS files references
2. Do not validate the URI endpoint at all

I'd personally go for the first one, as it might be better for mobile clients that most of the time do not have other access to media resources other than the standard Web2.
For example, if you reference the plain IPFS link, a mobile app needs to perform other actions to get that file (either contact the IPFS public API or other stuff), the same would happen with other kind of providers.

[kwunyeung]

Ok just keep everything with https. It opens the opportunities for the application developers to choose any kinds of storage they want. Even if they use IPFS, they can always reference to the web gateways.

[leobragaz]

Let me recap if I understood everything properly:

1. Implementing media posts following the 2nd proposal;
2. Check that URI's field always starts with https:// by using the following RegEx : ^(?:https:\/\/)[\w.-]+(?:\.[\w\.-]+)+[\w\-\._~:/?#[\]@!\$&'\(\)\*\+,;=.]+$

Everything right?

[RiccardoM]

Let me recap if I understood everything properly:
1 - Implementing media posts following the 2nd proposal;
2- Check that URI's field always starts with https:// by using the following RegEx :
^(?:https:\/\/)[\w.-]+(?:\.[\w\.-]+)+[\w\-\._~:/?#[\]@!\$&'\(\)\*\+,;=.]+$
Everything right?

Yes, correct. Please note that that RegEx might not be enough/correct. We should search online for other options and create a proper test suite to test it.

[leobragaz]

Yes, correct. Please note that that RegEx might not be enough/correct. We should search online for other options and create a proper test suite to test it.

Cool. I'll look deeper for that.

[leobragaz]

Do you see any useful thing inside this: https://mathiasbynens.be/demo/url-regex?

I've also discovered that using IntelliJ you can check regEx directly from the IDE (but i'm not so sure about how it works).

Do I have also to implement a new type of message? Or just the type for now?

[RiccardoM]

Do you see any useful thing inside this?
https://mathiasbynens.be/demo/url-regex

Probably the gruber implementation is the best one in our case. Also, you might want to even use the one I posted initially.

I've also discovered that using IntelliJ you can check regEx directly from the IDE (but i'm not so sure about how it works).

I don't know how it works, I've never used it. I usually refer to regex101 to test regex before using them.

Do I have also to implement a new type of message? Or just the type for now?

Yes, a new message must be implemented to allow users to create such post types.
It should be called desmos/MsgCreateMediaPost or something similar, and documentation should be written accordingly.

[leobragaz]

Also, you might want to even use the one I posted initially.

I'm already implemented it with yours. But it might be useful to test some of them. I will give a try to gruber's one.

It should be called desmos/MsgCreateMediaPost or something similar, and documentation should be written accordingly.

All right! 👍

[leobragaz]

I was thinking about how to properly save MediaPosts and I've came up with two possibile solutions, which one do you think is the best one?:

1. Reuse the current method

Reuse SavePost function to save the Post member of a MediaPost and then save into the KVStore the couple of (k,v) which key = PostID and value = Medias[]

func (k Keeper) SaveMedia(ctx sdk.Context, mediaPost types.MediaPost)  {
	store := ctx.KVStore(k.StoreKey)

	//Save the post inside mediaPost
	k.SavePost(ctx, mediaPost.Post)

	//Save the medias associated with mediaPost.Post ID
	store.Set(
           []byte(types.MediaPostStorePrefix+mediaPost.Post.PostID.String()), 
           k.Cdc.MustMarshalBinaryBare(mediaPost.Medias)
        )

}

PROS

• Pursues the DRY principle by let you use basically any other already written post's function.

CONS

• Mixes Post and MediaPost together in the same KVStore.

2. Create a new method

Don't reuse and save the entire MediaPost inside the KVStore using PostID appended to the prefix:

func (k Keeper) SaveMedia(ctx sdk.Context, mediaPost types.MediaPost) {
	store := ctx.KVStore(k.StoreKey)
	
	store.Set([]byte(types.MediaPostStorePrefix+mediaPost.Post.PostID.String()), k.Cdc.MustMarshalBinaryBare(&mediaPost))

	// Set the last post id only if the current post has a greater one than the last one stored
	if mediaPost.Post.PostID > k.GetLastPostID(ctx) {
		store.Set([]byte(types.LastPostIDStoreKey), k.Cdc.MustMarshalBinaryBare(&mediaPost.Post.PostID))
	}

	// Save the comments to the parent post, if it is valid
	if mediaPost.Post.ParentID.Valid() {
		parentCommentsKey := []byte(types.PostCommentsStorePrefix + mediaPost.Post.ParentID.String())

		var commentsIDs types.PostIDs
		k.Cdc.MustUnmarshalBinaryBare(store.Get(parentCommentsKey), &commentsIDs)

		if editedIDs, appended := commentsIDs.AppendIfMissing(mediaPost.Post.PostID); appended {
			store.Set(parentCommentsKey, k.Cdc.MustMarshalBinaryBare(&editedIDs))
		}
	}
}

PROS

• Allows a better posts division.

CONS

• Need to write more (of the same) code.

I'd personally go for the First one, as it seems to me a more reasonable approach.

[RiccardoM]

@bragaz I personally think there is a more elegant and clean way of handling this, much like it's being done inside Tendermint's crypto package: using interfaces.

Implementation through interfaces

What we could do is define the following interface:

// Represents the interface that each post should implement
type PostI interface {
    Validate() error     // Tells if the post is valid or not
    GetID() PostID       // Returns the ID of the post
    GetParentID() PostID // Returns the ParentID of the post
    Equals(PostI) bool   // Tells if this post has the same contents of the one given
}

Then what we would have to do is implementing such interface in both Post

type Post struct { ... }

func NewPost (...) Post { ... }

// GetID implements PostI
func (p Post) GetID() PostID {
    return p.PostID
}

// GetParentID implements PostI
func (p Post) GetParentID() PostID {
    return p.ParentID
}

// Validate implements validator
func (p Post) Validate() error { ... }

func (p Post) Equals(other PostI) bool {
    // Cast and delegate
    if otherPost, ok := other.(Post); ok {
        return checkPostsEqual(p, otherPost)
    } else {
        return false
    }
}

func checkPostsEqual(first, second Post) bool {
    equalsOptionalData := len(first.OptionalData) == len(second.OptionalData)
    if equalsOptionalData {
        for key := range first.OptionalData {
            equalsOptionalData = equalsOptionalData && first.OptionalData[key] == second.OptionalData[key]
        }
    }

    return first.PostID.Equals(second.PostID) &&
        first.ParentID.Equals(second.ParentID) &&
        first.Message == second.Message &&
        first.Created.Equal(second.Created) &&
        first.LastEdited.Equal(second.LastEdited) &&
        first.AllowsComments == second.AllowsComments &&
        first.Subspace == second.Subspace &&
        equalsOptionalData &&
        first.Creator.Equals(second.Creator)
}

and MediaPost

type MediaPost struct { }

func NewMediaPost ( ... ) MediaPost { ... } 

// GetID implements PostI
func (mp MediaPost) GetID() PostID {
    return mp.PostID
}

// GetParentID implements PostI
func (mp MediaPost) GetParentID() PostID {
    return mp.ParentID
}

func (mp MediaPost) Validate() error { ... }

func (mp MediaPost) Equals(other PostI) bool {
    // Cast and delegate
    if otherMp, ok := other.(MediaPost); ok {
        return checkMediaPostEquals(mp, otherMp)
    } else {
        return false
    }
}

func checkMediaPostEquals(first, second MediaPost) bool {
    if !first.Post.Equals(second.Post) {
        return false
    }

    if len(first.Medias) != len(second.Medias) {
        return false
    }

    for index, media := range first.Medias {
        if media != second.Medias[index] {
            return false
        }
    }

    return true
}

Codec changes

Due to how Amino works, we could simply register the interface and the concrete types inside the codec.go file:

func RegisterCodec(cdc *codec.Codec) {
    // ...
    cdc.RegisterInterface((*PostI)(nil), nil)
    cdc.RegisterConcrete(Post{}, "text", nil)
    cdc.RegisterConcrete(MediaPost{}, "media", nil)
}

Keeper changes

With this trick, we can use just the SavePost method, editing the type of the second parameter as well as the current post.PostID (changed in post.GetID()) and post.ParentID (changed in post.GetParentID()):

func (k Keeper) SavePost(ctx sdk.Context, post types.PostI) {
    store := ctx.KVStore(k.StoreKey)

    // Save the post
    store.Set([]byte(types.PostStorePrefix+post.GetID().String()), k.Cdc.MustMarshalBinaryBare(&post))

    // Set the last post id only if the current post has a greater one than the last one stored
    if id := post.GetID(); id > k.GetLastPostID(ctx) {
        store.Set([]byte(types.LastPostIDStoreKey), k.Cdc.MustMarshalBinaryBare(&id))
    }

    // Save the comments to the parent post, if it is valid
    if post.GetParentID().Valid() {
        parentCommentsKey := []byte(types.PostCommentsStorePrefix + post.GetParentID().String())

        var commentsIDs types.PostIDs
        k.Cdc.MustUnmarshalBinaryBare(store.Get(parentCommentsKey), &commentsIDs)

        if editedIDs, appended := commentsIDs.AppendIfMissing(post.GetID()); appended {
            store.Set(parentCommentsKey, k.Cdc.MustMarshalBinaryBare(&editedIDs))
        }
    }
}

Reading of posts

Please note that the GetPost method should change its return type accordingly:

func (k Keeper) GetPost(ctx sdk.Context, id types.PostID) (post types.PostI, found bool) {
    store := ctx.KVStore(k.StoreKey)

    key := k.getPostStoreKey(id)
    if !store.Has(key) {
        return types.Post{}, false
    }

    k.Cdc.MustUnmarshalBinaryBare(store.Get(key), &post)
    return post, true
}

⚠️ Warnings ⚠️

Due to the fact that we are changing the GetPost method (see here) we will have to change also the followings:

• handler.go
• querier.go

And all the associated tests. This should be done with particular carefulness as it might break something, and tests should be written using both Post and MediaPost types to be sure everything works properly.

Also, please note that the PostI interface should be changed accordingly if something is missing from its methods. Personally, I would also add the String() string method inside it, inherited from the fmt.Stringer interface.

Naming

Due to the current state of the code, I proposed PostI as the interface name. However, I really think that the following names should be adopted instead:

• Post should become TextPost (basic post)
• PostI should become Post (interface)

Conclusions

@bragaz Please let me know what you think about such implementation, if the explanation is clear and/or something is missing.

[leobragaz]

The whole solution seems reasonable and better than the others to me, using interfaces is the way.
I've only got some doubts about the returning type in GetPost function that I would discuss with you in Zoom or Skype.

However, I really think that the following names should be adopted instead:

• Post should become TextPost (basic post)
• PostI should become Post (interface)

I agree more with this naming because it's more natural and easier to understand than something like PostI. Using PostI is like having HamburgerI Hamburger Cheeseburger instead of Burger Hamburger Cheeseburger, it feels unnatural, almost forced.