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

Any <> interface Conversion #5

Closed
3 tasks
aaronc opened this issue Jul 14, 2021 · 4 comments
Closed
3 tasks

Any <> interface Conversion #5

aaronc opened this issue Jul 14, 2021 · 4 comments
Assignees

Comments

@aaronc
Copy link
Member

aaronc commented Jul 14, 2021

This will reuse the existing cosmos_proto.implement_interface and cosmos_proto.accepts_interface: https://github.com/regen-network/cosmos-proto/blob/master/cosmos.proto

We want a mapping from proto interface names to golang interface names, ex:

interfaces:
  cosmos.gov.Content: github.com/cosmos/cosmos-sdk/x/gov/types.Content

Then the generated code should use the interface and not Any, ex:

type MsgSubmitProposal struct {
	Content        Content
	InitialDeposit []*Coin
	Proposer       string                                   
}

protoreflect.Message and fast marshaling handling will need to be generated

@technicallyty technicallyty self-assigned this Sep 9, 2021
@technicallyty
Copy link
Contributor

technicallyty commented Sep 9, 2021

not sure if the protogen plugin library recognizes brackets to do stuff like string foo = 1 [(implements_interface: bar]);- https://pkg.go.dev/google.golang.org/protobuf/compiler/[email protected]#Field

One way around this is to use comments instead. each field gives us access to the comments attached to it either on top of it, or trailing it. so we could do stuff like:

string foo = 1; // @implements_interface: cosmos.gov.Content
or similarly

// @implements_interface: cosmos.gov.Content
string foo = 1; 

also, the type of the field on the message struct is generated by protoc-gen-go, so we'd have to manipulate the AST again to achieve this, or consider forking their plugin again

just out of curiosity, was using enums ever considered for the Any problem? judging by Interface Encoding and Usage of Any section of the docs, i feel, in the example, it could be more straightfoward to just have an Enum field with each account type as an option.

@aaronc
Copy link
Member Author

aaronc commented Sep 9, 2021

There is a way to do it, you just need to dig into the API. From a quick glance I think it's Options(). We don't want to use comments.

Yes we will definitely need to manipulate the struct codegen. Maybe we can just override that part of the AST?

We did consider oneof's quite extensively and we're not going to open that can of worms again lol

@robert-zaremba
Copy link
Collaborator

@technicallyty the problem with enums / oneof is that it's closed for extensibility. If you add a new type which implements an interface X, then you need to re-generate any other type which depends on X.

@aaronc
Copy link
Member Author

aaronc commented Jan 6, 2022

Seems like we can close as "won't fix" for now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

4 participants