Implement customtype's (Cosmos Scalars)

[aaronc]

• cosmos_proto.scalar proto option that applies string and bytes types
• Mapping YAML config file between scalars (language agnostic) and golang types
  Ex:

scalars:
  cosmos.Int: github.com/cosmos/cosmos-sdk/types.Int

• define CustomType golang interface that all golang scalar type implementations need to implement (ex. Int), this is what we're using for gogo proto and can maybe reuse:

type CustomType interface {
	Marshal() ([]byte, error)
	MarshalTo(data []byte) (n int, err error)
	Unmarshal(data []byte) error
	Size() int

	MarshalJSON() ([]byte, error)
	UnmarshalJSON(data []byte) error
}

An alternative that just deals with string and bytes types could be:

type CustomStringType interface {
  MarshalString() (string, error)
  UnmarshalString(string) error
}

type CustomBytesTypes interface {
  MarshalBytes() ([]byte, error)
  UnmarshalBytes([]byte) error
}

Depending on whether cosmos_proto.scalar is used on a string or bytes type, the corresponding interface would be used.

• generate protoreflect.Message handling
• generate fast marshaling handling

[aaronc]

Here's one alternate idea I had for custom types:

type Coin struct {
    Denom string
    Amount string
    amountInt cosmos.Int
    amountIntLastStr string
}

func (c *Coin) GetAmountInt() (cosmos.Int, error) {
    if c.Amount == c.amountIntLastStr {
        return c.amountInt, nil
    }
    var err error
    c.amountInt, err = cosmos.NewIntFromString(c.Amount)
    if err != nil {
        ...
    }
    c.amountIntLastStr = c.Amount
    return c.amountInt, nil
}

func (c *Coin) SetAmountInt(x cosmos.Int) {
  c.amountInt = x
  c.Amount = x.String()
  c.amountIntLastStr = c.Amount
}

Here are a few pros of this approach:

• no panic in protoreflect.Message.Set
• not-breaking to add a cosmos_proto.scalar annotation
• caching

I talked about it with @fdymylja and he said it might actually be harder to handle in codegen and the panic in Set probably isn't a big deal, but just wanted to present this as one option. Let's make a choice and handle this one way or the other.

[aaronc]

I'm thinking this alternate approach might also be more performant for marshaling with the ORM. For instance, say we have a primary key field with a custom type - in the ORM we would Get it with the FieldDescriptor. If we just have the custom type this will require serializing say an Int to a string. If we have both the Int + string we can skip this step. Basically this alternate approach caches both the custom type and string representation. Does that make sense?

[aaronc]

Going to follow up on some of the discussion from #45 (comment).

The current proposal there I think is:

type CustomType interface {
	UnmarshalBytes(b []byte) error
	MarshalBytes() ([]byte, error)
	Size() int
	Set(value protoreflect.Value)
	Get() protoreflect.Value
	Clear()
	IsSet() bool
}

One issue here is that you can't support custom types with different proto encodings. For instance, you can't have an Int with both a string and bytes proto encodings. Because then when you call Get or Set with a protoreflect.Value what type is expected string or bytes? So you would need to have a separate Int custom type depending on the proto encoding.

With the Get/Set approach, you just need two methods per proto type - so to support string you would just need a NewIntFromString constructor and a String method. For bytes support, you'd add NewIntFromBytes and a Bytes method.

[fdymylja]

Going to follow up on some of the discussion from #45 (comment).

The current proposal there I think is:

type CustomType interface {
	UnmarshalBytes(b []byte) error
	MarshalBytes() ([]byte, error)
	Size() int
	Set(value protoreflect.Value)
	Get() protoreflect.Value
	Clear()
	IsSet() bool
}

One issue here is that you can't support custom types with different proto encodings. For instance, you can't have an Int with both a string and bytes proto encodings. Because then when you call Get or Set with a protoreflect.Value what type is expected string or bytes? So you would need to have a separate Int custom type depending on the proto encoding.

With the Get/Set approach, you just need two methods per proto type - so to support string you would just need a NewIntFromString constructor and a String method. For bytes support, you'd add NewIntFromBytes and a Bytes method.

sounds good to me, we just need to decide on the interface we want.

I was thinking one interface for each proto kind.

ex:

CustomTypeString interface {
  UnmarshalString(string) error
  MarshalString() (string, error)
  ValueString() string
}

CustomTypeInt64 interface { 
  UnmarshalInt64(in64) error
  MarshalInt64() (int64, error)
  ValueInt64() int64
}

We should also decide on the skeleton of the interface implementation (what methods are needed?). Also I don't think we should use constructors or anything, all the implementation should be methods of the custom type, so to pulsar we can just pass the go import path for the custom type instead of multiple functions from the pkg of the custom type.

[aaronc]

CustomTypeString interface {
  UnmarshalString(string) error
  MarshalString() (string, error)
  ValueString() string
}

CustomTypeInt64 interface { 
  UnmarshalInt64(in64) error
  MarshalInt64() (int64, error)
  ValueInt64() int64
}

We should also decide on the skeleton of the interface implementation (what methods are needed?). Also I don't think we should use constructors or anything, all the implementation should be methods of the custom type, so to pulsar we can just pass the go import path for the custom type instead of multiple functions from the pkg of the custom type.

One issue with defining a method like UnmarshalString directly on a type like Int, is that we actually want Int to be immutable. If you don't like using a constructor, we could have a method Int.NewFromString. Why do we need both MarshalString + ValueString? Not sure why there would be an error

[fdymylja]

CustomTypeString interface {
  UnmarshalString(string) error
  MarshalString() (string, error)
  ValueString() string
}

CustomTypeInt64 interface { 
  UnmarshalInt64(in64) error
  MarshalInt64() (int64, error)
  ValueInt64() int64
}

We should also decide on the skeleton of the interface implementation (what methods are needed?). Also I don't think we should use constructors or anything, all the implementation should be methods of the custom type, so to pulsar we can just pass the go import path for the custom type instead of multiple functions from the pkg of the custom type.

One issue with defining a method like UnmarshalString directly on a type like Int, is that we actually want Int to be immutable. If you don't like using a constructor, we could have a method Int.NewFromString. Why do we need both MarshalString + ValueString? Not sure why there would be an error

ValueInt64 is for reflection to check equality between private and exported field in the struct. Does mutability matter though in this case?

[aaronc]

But how is Value different from Marshal then?

Unmarshal would make it mutable.

I'm also not sure why the constructor is a problem. That can easily be derived from type name: foo/bar.Int -> foo.bar/NewIntFromString.

[aaronc]

Would be great to have a decision here. I'm mostly leaning towards the approach in #2 (comment) for the reasons outlined there. I also think it will be quite easy to implement.

The big downside I think is that you can't do something like Coin{Denom: "foo", Amount: someInt} and would need to instead write coin := Coin{Denom: "foo"}; coin.SetAmountInt(someInt) which is different from gogo proto.

Other than that, I think this approach makes implementation simpler for both codegen and custom types and has the advantages of built-in caching and gradual upgradability.

[technicallyty]

Would be great to have a decision here. I'm mostly leaning towards the approach in #2 (comment) for the reasons outlined there. I also think it will be quite easy to implement.

The big downside I think is that you can't do something like Coin{Denom: "foo", Amount: someInt} and would need to instead write coin := Coin{Denom: "foo"}; coin.SetAmountInt(someInt) which is different from gogo proto.

Other than that, I think this approach makes implementation simpler for both codegen and custom types and has the advantages of built-in caching and gradual upgradability.

maybe the downside could be skirted a bit by codegenning some constructors? also, i forgot where we left off with this. is Frojdi still handling it? or should i take over?

[fdymylja]

IIRC, we were awaiting on community feedback for this specific feature.

[aaronc]

Would be great to have a decision here. I'm mostly leaning towards the approach in #2 (comment) for the reasons outlined there. I also think it will be quite easy to implement.
The big downside I think is that you can't do something like Coin{Denom: "foo", Amount: someInt} and would need to instead write coin := Coin{Denom: "foo"}; coin.SetAmountInt(someInt) which is different from gogo proto.
Other than that, I think this approach makes implementation simpler for both codegen and custom types and has the advantages of built-in caching and gradual upgradability.

maybe the downside could be skirted a bit by codegenning some constructors? also, i forgot where we left off with this. is Frojdi still handling it? or should i take over?

constructors is an interesting idea, but it introduces surface area for incompatible breaing changes, because constructors generally use positional parameters

IIRC, we were awaiting on community feedback for this specific feature.

yes, that is where we left it. but either way, things should be decided mostly on technical pros/cons and their relative impact which we should be able to assess.

@marbar3778 would be great to have you chime in here

[elias-orijtech]

Gentle ping :) This is marked as a 1.0 blocker, so a decision here is needed to move forward with implementation.

[aaronc]

Thanks @elias-orijtech. Likely the whole code generator will get rewritten before 1.0 in light of cosmos/cosmos-sdk#15404 so that will likely affect this feature (as well as everything else) pretty substantially.

[tac0turtle]

would it be the same code path or potentially a different go mod to seaprate proto codegen from zero copy

[aaronc]

would it be the same code path or potentially a different go mod to seaprate proto codegen from zero copy

I think we'd want them to be the same codegen if we want performant message passing. The zero-copy design would be proto compatible. I'm thinking we'd be encouraging people to migrate directly to that instead of just dropping gogo for the current pulsar codegen

[tac0turtle]

lets talk about this on the team call today, would be good to have the path laid out