Require valid amino.* annotations

[amaury1093]

Summary

Validate amino.* annotations:

• Require that amino.name and amino.oneof_name proto annotations matches go-amino's RegisterConcrete name.
• Make sure that all chains update their proto files to use:
  • amino.encoding, amino.message_encoding
  • amino.dont_omitempty, amino.field_name

ref: #13405 (comment)

Problem Definition

With #14353, SDK msgs can have an amino.name proto annotation to define how they are encoded in Anys in amino. This is the same functionality as aminoCdc.RegisterConcrete. We should:

1. (ideal solution) either remove aminoCdc.RegisterConcrete altogether, but this needs Implement protoreflect based amino JSON encoder #10993
2. (okay solution) or have some sort of validation that checks that the proto annotation string matches the string in RegisterConcrete

Solution

• Validate amino.name against aminoCdc.RegisterConcrete registry
• Validate amino.oneof_name against aminoCdc.RegisterConcrete registry
• Validate amino.dont_omitempty, amino.field_name against existing gogoproto annotations.

[kocubinski]

The test suite in https://github.com/cosmos/cosmos-sdk/blob/e9478df1619d7d0845dab3308852a9e2948269ac/tests/integration/aminojson/aminojson_test.go seems robust enough to catch the consistencies described in this issue as in the case of #15165

This also raises the question for me; is fuzz testing (the approach in the test suite) good enough or do we need a set of deterministic rules?

[amaury1093]

This also raises the question for me; is fuzz testing (the approach in the test suite) good enough or do we need a set of deterministic rules?

For a set of deterministic rules, we need a spec. However, go-amino implementation is currently the amino spec. So fuzz testing is IMO the best way to test that the new amino encoder is "spec"-compliant.

[amaury1093]

My biggest concern here is not our own types, but making sure other chains adopt the new amino.* annotations correctly. I don't see each chain creating such a comprehensive test suite like ours.

Maybe an entry in the UPGRADING.md guide is enough. Or maybe it's a script that we write, that each chain runs before updating, which will modify the *.proto files to add the correct amino.name,dont_omitempty,message_encoding annotations.

The original issue proposed a startup check, which may not be the ideal UX, but I believe is still the simplest solution.

[tac0turtle]

For a time being its either/or right, Its not one or the other?

[kocubinski]

Maybe the first step to tackling this issue ought to be just defining the spec of valid amino annotations as docs? Also if that already exists let me know where.

[amaury1093]

I think that's a good idea 👍. There are no such docs currently, the only thing we have written are the proto comments.

[kocubinski]

Related work: #13793

[amaury1093]

Related work: #13793

Also see the higher-level tracking issue #13310

[tac0turtle]

closing as we added this to documentation