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

Make JSONMarshaler methods require proto.Message #6982

Closed
4 tasks
aaronc opened this issue Aug 7, 2020 · 0 comments · Fixed by #7054
Closed
4 tasks

Make JSONMarshaler methods require proto.Message #6982

aaronc opened this issue Aug 7, 2020 · 0 comments · Fixed by #7054
Assignees

Comments

@aaronc
Copy link
Member

aaronc commented Aug 7, 2020

Summary

JSONMarshaler Marshal/UnmarshalJSON methods should take proto.Message instead of interface{}. The lack of this is causing runtime errors specifically with non-pointer versions of proto types (GenesisState vs `*GenesisState).

Problem Definition

When we first started the protobuf migration, we had use cases for JSONMarshaler for types that we never intended to fully migrate to protobuf such as legacy REST requests/responses. HybridCodec was created along those lines of reasoning and all of the JSONMarshaler methods accept interface{} and do not require proto.Message.

However, proto JSON marshaling will fail for any type that does implement proto.Message. This is especially problematic with pointer types. *GenesisState does implement proto.Message but GenesisState does not and with the current JSONMarshaler this will only show up at runtime and this is problematic.

Proposal

  1. Make JSONMarshaler methods require proto.Message. Amino JSON marshaling will still be possible but this ensures proto json marshaling doesn't unexpectedly fail at runtime.
  2. Use *codec.LegacyAmino directly anywhere we specifically just want to use amino - these are the legacy queriers and REST endpoints. That code will need to be refactored to not use JSONMarshaler at all and just be amino only and eventually deprecated altogether.
  3. *codec.LegacyAmino should not implement JSONMarshaler. Its MarshalJSON* methods should still accept interface{}. Where we need to use amino we use *codec.LegacyAmino. Were we want a choice between proto and amino we use JSONMarshaler

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
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.

2 participants