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

Utility for dynamicpb.NewMessageType + known types #104

Open
amaury1093 opened this issue Mar 15, 2023 · 1 comment
Open

Utility for dynamicpb.NewMessageType + known types #104

amaury1093 opened this issue Mar 15, 2023 · 1 comment
Labels
enhancement New feature or request

Comments

@amaury1093
Copy link
Contributor

amaury1093 commented Mar 15, 2023

Summary

Write a new utililty function that's the same as dynamicpb.NewMessageType, but when a nested field is a known type, use that resolved type, instead of a new dynamicpb.Message.

ref: cosmos/cosmos-sdk#15302 (comment)

Problem

Consider a custom Msg, suppose it's only registered in gogo, and not in protov2:

message MsgCustom {
  google.protobuf.Any a = 1;
}

Then when our Unpack utility receives this message, it sees MsgCustom and will use dynamicpb.NewMessageType to generate the whole proto.Message, including the nested field. MsgCustom.A will have type dynamicpb.Message.

Is this expected, or is it more helpful to have MsgCustom.A be an anypb.Any?

Proposal 1: Keep using official dynamicpb.NewMessageType

Using this, we know that proto.Messages created with dynamicpb are dynamicpb all the way down. There are no statically-created types used.

However, this also means that callers in general cannot type-cast. For example, in Textual, we use a switch case to handle both cases (dynamicpb or known type): https://github.com/cosmos/cosmos-sdk/blob/9a83c202cd545265f54d15090c6b150dc692f0f6/x/tx/signing/textual/any.go#L134-L148

We probably can move some utility functions like this to cosmos-proto if they are repeated.

Proposal 2: Write an utility function that does dynamicpb.NewMessageType with known types

We should pass a file resolver / type resolver to this new function, but it shouldn't be too hard to write it. The positive point is that callers can always type-cast to known types.

@amaury1093
Copy link
Contributor Author

Currently, in Textual, we decided to use a coerceToMessage utility, see here.

Usage is like this:

// Assume `givenCoin` is a dynamicpb.Message representing a Coin
coin := &basev1beta1.Coin{}
err := coerceToMessage(givenCoin, coin)
if err != nil { /* handler error */ }
fmt.Println(coin) // Will have the same field values as `givenCoin`

This way, after coercing, Textual always handles concrete types in its code. If coerceToMessage is useful for other packages, I propose to move it here, as it's really SDK-independent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant