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

ICA EncodingType: String vs Enum vs Type Alias #4036

Open
3 tasks
srdtrk opened this issue Jul 7, 2023 · 0 comments
Open
3 tasks

ICA EncodingType: String vs Enum vs Type Alias #4036

srdtrk opened this issue Jul 7, 2023 · 0 comments
Labels
27-interchain-accounts change: api breaking Issues or PRs that break Go API (need to be release in a new major version) needs discussion Issues that need discussion before they can be worked on nice-to-have

Comments

@srdtrk
Copy link
Member

srdtrk commented Jul 7, 2023

Summary

This discussion was started by @chatton in this thread. It has gathered some support (@crodriguezvega ).

Problem Definition

Currently, during the channel handshake, the exchanged version metadata includes an encoding field, serialised as a string. The different values this string could take are defined in modules/apps/27-interchain-accounts/types/metadata.go:

const (
	// EncodingProtobuf defines the protocol buffers proto3 encoding format
	EncodingProtobuf = "proto3"
	// EncodingProto3JSON defines the proto3 JSON encoding format
	EncodingProto3JSON = "proto3json"

	// ...
)

We have three possible options:

  • Option 1: Leave as string (non-breaking)
  • Option 2: Use a proto enum (breaking)
  • Option 3: Use a type alias (non-breaking?)

Option 2: Use an Enum

The reason why this is listed as breaking is, even if the enum is defined in a way such that the .String() is non-broken such as:

// Encoding defines the supported codec formats for interchain account transactions
enum Encoding {
  option (gogoproto.goproto_enum_prefix) = false;

  // Default zero value enumeration
  ENCODING_UNKNOWN_UNSPECIFIED = 0 [(gogoproto.enumvalue_customname) = "UNKNOWN"];
  // proto3 defines the proto3 prut otobuf encoding
  proto3 = 1 [(gogoproto.enumvalue_customname) = "PROTO3"];
  // proto3json defines the proto3 json encoding
  proto3json = 2 [(gogoproto.enumvalue_customname) = "PROTO3JSON"];
}

This is still breaking because inherently, proto enums are defined as int32 in go. Which means, when using json to encode the version, it will be converted to a number rather than the string proto3.

Option 3: Type Alias

I think this might be the more sensible option here. This would keep the inherent type as string, but not allow any string to be passed in (unless the string is defined in metadata.go as a constant). This solution would look like:

type EncodingType string

const (
	// EncodingProtobuf defines the protocol buffers proto3 encoding format
	EncodingProtobuf EncodingType = "proto3"
	// EncodingProto3JSON defines the proto3 JSON encoding format
	EncodingProto3JSON EncodingType = "proto3json"

	// ...
)

I'm not sure if we can use proto extensions to use our alias in protobuf.

Proposal

Take Option 3


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@srdtrk srdtrk added needs discussion Issues that need discussion before they can be worked on nice-to-have 27-interchain-accounts change: api breaking Issues or PRs that break Go API (need to be release in a new major version) labels Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
27-interchain-accounts change: api breaking Issues or PRs that break Go API (need to be release in a new major version) needs discussion Issues that need discussion before they can be worked on nice-to-have
Projects
None yet
Development

No branches or pull requests

1 participant