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

proto: Investigate migrating to Google's Go code generator #731

Closed
thanethomson opened this issue Apr 19, 2023 · 2 comments
Closed

proto: Investigate migrating to Google's Go code generator #731

thanethomson opened this issue Apr 19, 2023 · 2 comments
Assignees
Labels
protobuf Protocol buffers-related spike A time-boxed research spike of uncertain outcome
Milestone

Comments

@thanethomson
Copy link
Contributor

thanethomson commented Apr 19, 2023

At present we make use of https://github.com/cosmos/gogoproto to generate our Go code from our Protobuf definitions. Even though the project from which it was forked (https://github.com/gogo/protobuf) has been deprecated some time ago, and the same for the project from which gogoproto was forked (https://github.com/golang/protobuf), the Cosmos gogoproto fork is maintained by the SDK team.

It would, however, be valuable to investigate the impact of migrating to Google's official Go code generator (https://pkg.go.dev/google.golang.org/protobuf). See https://go.dev/blog/protobuf-apiv2 for context.

See also cosmos/cosmos-sdk#7488 for the SDK's deliberations on this front - it provides valuable context.

Definition of Done

When we have a feature branch that demonstrates the pragmatic impact of migrating to Google's Go code generator. We especially need to quantify the impact on:

  1. Our Go APIs
  2. Our RPC and JSON serialization
  3. Which user(s) this affects

Once we have quantified this impact, and if the impact is manageable, follow-up work should involve coordinating the release of this change - especially with the Cosmos SDK team.

@thanethomson thanethomson added protobuf Protocol buffers-related spike A time-boxed research spike of uncertain outcome labels Apr 19, 2023
@thanethomson thanethomson added this to the 2023-Q2 milestone Apr 19, 2023
@thanethomson thanethomson changed the title proto: Migrate to Google's Go code generator proto: Investigate migrating to Google's Go code generator Apr 19, 2023
@thanethomson
Copy link
Contributor Author

thanethomson commented May 3, 2023

I've spent a little time investigating what this entails here, and it appears as though gogoproto is, unfortunately, very deeply embedded throughout the codebase:

  • Many parts of the codebase rely on functions generated specifically by gogoproto (comparators, equality operations, etc.). We would need to manually re-implement this functionality across many different types. For example:
    option (gogoproto.equal_all) = true;
  • In cases where we have message fields whose types are other messages, the gogoproto annotations control whether the generated code refers to a value or a pointer. Google's Go code generator actually produces structs that do not allow themselves to be copied - on purpose. This, imho, is the correct way of making use of these structs. But blindly changing our usage here by value to pointer may introduce new race conditions, and re-evaluating all the usages of these structs may take quite some time. But maybe the race detector will help us here?
  • There are a handful of places where we explicitly make use of gogo types in marshalling/unmarshalling data. We would need to re-implement these types manually. For example:
    h := gogotypes.Int64Value{Value: ev.Height()}

This doesn't even take into consideration the impact of this change on the JSON-RPC responses. In many cases, using Google's Go code generator here would entail several breaking changes in the RPC.

@thanethomson thanethomson self-assigned this May 3, 2023
@thanethomson
Copy link
Contributor Author

Marking the spike/investigation as done here. If we want to actually do the migration, we should open a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
protobuf Protocol buffers-related spike A time-boxed research spike of uncertain outcome
Projects
No open projects
Status: Done
Development

No branches or pull requests

1 participant