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

Merged file descriptors from gogo and protoreflect #14693

Closed
1 of 4 tasks
amaury1093 opened this issue Jan 19, 2023 · 10 comments · Fixed by #14886
Closed
1 of 4 tasks

Merged file descriptors from gogo and protoreflect #14693

amaury1093 opened this issue Jan 19, 2023 · 10 comments · Fixed by #14886
Assignees

Comments

@amaury1093
Copy link
Contributor

amaury1093 commented Jan 19, 2023

Update

Latest proposal is to put gogo+apiv2 registries merging inside cosmos/gogoproto, see #14693 (comment)

Summary

Create codec/v2 go module

Problem Definition

From @aaronc: We should have a standardized way of collecting both gogo and protoreflect file descriptors into a single global registry as that will be needed in many places now (textual #14647, autocli, replacing getsigners, runtime's reflection endpoint etc.).

Proposal

Create a codec/v2 go module that will host this logic.

In the future (out of scope of this issue), we can add the following items too to this module:

@aaronc
Copy link
Member

aaronc commented Jan 19, 2023

As part of codec/v2 I would like to have a testing package which generates random proto messages based on the message descriptors. It could leverage the annotations in implements_interface/accepts_interface to properly populate Anys. We could use this for generative testing and maybe simulations in the future.

@aaronc
Copy link
Member

aaronc commented Jan 19, 2023

One thing we need to be careful about here is that this global registry should be built after all go packages have had a chance to initialize and register themselves with the protoregistry and gogo proto global registries. Otherwise, this composite registry will be missing files.

@testinginprod
Copy link
Contributor

As part of codec/v2 I would like to have a testing package which generates random proto messages based on the message descriptors. It could leverage the annotations in implements_interface/accepts_interface to properly populate Anys. We could use this for generative testing and maybe simulations in the future.

we already do have something similar: https://github.com/cosmos/cosmos-proto/blob/main/internal/fuzz/message.go

@aaronc
Copy link
Member

aaronc commented Jan 19, 2023

As part of codec/v2 I would like to have a testing package which generates random proto messages based on the message descriptors. It could leverage the annotations in implements_interface/accepts_interface to properly populate Anys. We could use this for generative testing and maybe simulations in the future.

we already do have something similar: https://github.com/cosmos/cosmos-proto/blob/main/internal/fuzz/message.go

Right I forgot about that. Could we make it general purpose and also use generics? Do other see this as a useful tool for testing generally?

@testinginprod
Copy link
Contributor

As part of codec/v2 I would like to have a testing package which generates random proto messages based on the message descriptors. It could leverage the annotations in implements_interface/accepts_interface to properly populate Anys. We could use this for generative testing and maybe simulations in the future.

we already do have something similar: https://github.com/cosmos/cosmos-proto/blob/main/internal/fuzz/message.go

Right I forgot about that. Could we make it general purpose and also use generics? Do other see this as a useful tool for testing generally?

Yes we can I guess. Feel free to open an issue and assign me.

@aaronc
Copy link
Member

aaronc commented Jan 19, 2023

Yes we can I guess. Feel free to open an issue and assign me.

Added #14704

@aaronc
Copy link
Member

aaronc commented Jan 24, 2023

I actually think the gogo/protoreflect global registry merging should live in our fork of gogo. Its utility will hopefully be short lived as I imagine there will be a time when everything is protoreflect.

Regarding amino json, what if we put that in x/tx with other sign mode stuff?

As we discussed in today's call, more generic stuff like random msg generation, unknown field filtering, and runtime interface linting can live in cosmos-proto.

@amaury1093
Copy link
Contributor Author

I like this idea better than creating a new codec/v2.

@robert-zaremba
Copy link
Collaborator

Yes, adding registry in cosmos/cosmos-proto makes more sense -- someone will be able to use it directly in tools, without relaying on `cosmos-sdk/codec.

Side note: if that would be in codec, why it would require bump to v2? Isn't it just an iterative addon?

@amaury1093 amaury1093 self-assigned this Jan 31, 2023
@amaury1093
Copy link
Contributor Author

@robert-zaremba Just to clarify, afaiu @aaronc's proposal is to put in cosmos/gogoproto not cosmos/cosmos-proto. Basically cosmos/cosmos-proto is the clean api v2 repo, and cosmos/gogoproto provides registry merging as a compatibility layer.

@github-project-automation github-project-automation bot moved this to 📝 Todo in Cosmos-SDK Feb 1, 2023
@amaury1093 amaury1093 moved this from 📝 Todo to 💪 In Progress in Cosmos-SDK Feb 1, 2023
@amaury1093 amaury1093 changed the title Create codec/v2 go module Merged file descriptors from gogo and protoreflect Feb 15, 2023
@github-project-automation github-project-automation bot moved this from 💪 In Progress to 👏 Done in Cosmos-SDK Feb 20, 2023
@tac0turtle tac0turtle removed this from Cosmos-SDK May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants