-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Protobuf Golang v2 Migration #7488
Comments
To aid in this decision an outline of the used/needed features of gogoproto should be made. Many of the features can be recreated without using gogoproto. |
Here are the extensions we are using (based on the ripgrep command below): ❯ rg -o -I "\(gogoproto\..*?\)" proto | sort | uniq
(gogoproto.castrepeated)
(gogoproto.casttype)
(gogoproto.customname)
(gogoproto.customtype)
(gogoproto.description)
(gogoproto.embed)
(gogoproto.enumvalue_customname)
(gogoproto.equal)
(gogoproto.equal_all)
(gogoproto.goproto_enum_prefix)
(gogoproto.goproto_getters)
(gogoproto.goproto_getters_all)
(gogoproto.goproto_stringer)
(gogoproto.goproto_stringer_all)
(gogoproto.goproto_unrecognized)
(gogoproto.jsontag)
(gogoproto.moretags)
(gogoproto.nullable)
(gogoproto.stdduration)
(gogoproto.stdtime)
(gogoproto.stringer)
(gogoproto.stringer_all) |
Do we know what is required and what is being used because it's available? Differentiating what is modifying struct and what is not would be helpful as well. The items that are just methods we can implement as a side process similar to https://github.com/marbar3778/go-proto-wrapper/ |
So if we could only choose one thing from that list I would say |
When talking to the gogoproto maintainer we agreed rewriting gogoproto was the best approach. Customtype will require a decent amount of work, we will need to write a simple compiler that adheres to the proto v2 interface. |
Yes, I've been thinking the same thing. And I believe the proto v2 approach is in some ways a lot more flexible. If there are decent code generators it could be pretty good. |
+1 for moving away from gogoproto. This might be an unfrequent use case, but gogoproto also breaks protobuf service reflection. ref: https://jbrandhorst.com/post/gogoproto/#reflection |
It wasn't that hard to write a custom protobuf service code generator: regen-network/regen-ledger#207 (comment). So that implementing and calling this interfaces is more ergonomic in the SDK. I just forked the official grpc codegen, deleted a bunch of stuff for streaming which we don't need, and tweaked the types in a few places. I imagine writing a proper go proto v2 code generator that does custom types and hides |
This was discussed on today's architecture call:
The general consensus from this call was that a basic protobuf service code generator should probably live directly within the SDK repo (upstreamed from the work @aaronc recently did in Regen Ledger). As for a more general golang proto v2 code generator, attendees were largely in favor of us writing our own code generator. We decided it should most likely live under its own repository (in the Cosmos org), so it can be used my multiple projects within the cosmos ecosystem (e.g. both cosmos-sdk and tendermint). There was some discussion of potentially treating this as a more general purpose project for the larger Golang community, but we were not convinced that taking on such a project would be worth the potential additional maintenance overhead of satisfying the larger Golang community's user needs. |
Awesome to see this work. Generally not a fan of forking like this as it can easily become burdensome to keep track of upstream changes if we modify things. This is what happened to gogoproto. |
@marbar3778 the idea is that we implement code generators compatible with the golang proto v2 spec. Especially for service code generators, the protobuf spec actually recommends custom code generators. I'm also not sure that's what happened to gogo proto - they tried to do a bunch of stuff that diverged pretty heavily from upstream. Regarding golang proto v2 in general, I don't see a way around a custom code generator because the default implementation is 100% reflection based. So I would like us to talk about soon what it looks like to have a custom golang v2 generator that supports a very limited set of annotations instead of the whole grab bag that gogo proto supports. |
Small update: we are spinning out a small working group to align on a specification for a golang v2 code generator with the tendermint core team. |
how can I help with this? |
@faddat see In Progress and Ready columns here: https://github.com/orgs/cosmos/projects/10 |
Summary
Given the precarious future of gogo protobuf, we should assess our dependence on it before reaching v1.0, and establish a working group to come up with a concrete design.
Problem Definition
When the Stargate protobuf migration began, https://github.com/gogo/protobuf appeared to be the clearly superior implementation compared to https://github.com/golang/protobuf. It is both faster and allows for more canonical go structures through extensions which we used extensively especially initially.
In March, the official go protobuf implementation announced a new API: https://blog.golang.org/protobuf-apiv2 and in May gogo protobuf announced that they were looking for new maintainers: gogo/protobuf#691.
As of now the future of gogo protobuf looks precarious and it seems likely that it will fall into disuse in spite of being the superior implementation.
We are using
gogoproto
annotations quite extensively in our proto files currently and switching to the official go proto would likely mean needing to abandon any sort of amino support (#6190). Also, the official go proto implementation is still slower than gogo proto.On other hand, as we are thinking towards a v1.0 (#7421), it would be ideal to release our stable API based on a protobuf library that is going to be maintained.
Proposal
For now, let's not do anything and wait and see how the community responds to the gogo protobuf situation. If gogo protobuf finds an active maintainer in the meantime, we may not need to do anything.
If it doesn't find a maintainer, before we release a v1.0 we should either:
For Admin Use
The text was updated successfully, but these errors were encountered: