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

feat: Add proto annotations for Amino JSON #13501

Merged
merged 54 commits into from
Nov 7, 2022
Merged

feat: Add proto annotations for Amino JSON #13501

merged 54 commits into from
Nov 7, 2022

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented Oct 11, 2022

Description

Closes: #13407

Goal of this PR

  • Make JS have all amino encoding infos via proto files
  • Make sure JS doesn't rely on gogoproto.* annotations at all. They will be removed.

Note for reviewers

This is a big PR in terms of line diff, but you only need to review *.proto files (~38 files). I also added some comments for (hopefully) easier navigation.

There are 4 new annotations, which can be found in legacy_amino/amino.proto (jump to file):

Addition 1: legacy_amino.name

e.g. bank MsgSend => cosmos.msg.v1.legacy_amino_name = "cosmos-sdk/MsgSend"

Addition 2: legacy_amino.field_name and dont_omitempty (jump to example)

These 2 new annotations are used to replace gogoproto.jsontag, which is used to:

  1. optionally set a custom field name
  2. optionally to set a field to omitempty or not (Note: we default to omitempty)

Example: if we have:

message Foo {
  string bar = 1 [(gogoproto.nullable) = X, (gogoproto.jsontag) = "<Y>"];
}

Then the algorithm is, when jsontag is the only annotation:

value of X value of <Y> legacy_amino.field_name value legacy_amino.dont_omitempty value
true empty not set not set
false empty not set true
* "bar,omitempty" not set not set
* "bar" not set true
* "baz" "baz" true
* "baz,omitempty" "baz" not set

Addition 3: legacy_amino.encoding (field option) and message_encoding (message option)

For use cases where a field or a whole struct has a custom encoding. (jump to example)


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@github-actions github-actions bot added C:CLI C:Cosmovisor Issues and PR related to Cosmovisor C:orm labels Oct 13, 2022
@amaury1093 amaury1093 enabled auto-merge (squash) November 3, 2022 21:19
@amaury1093
Copy link
Contributor Author

I'm putting automerge on, thanks for everyone's reviews here!

@ValarDragon would you mind taking another look or lifting your block, for automerge to merge this PR?

@pyramation pyramation mentioned this pull request Nov 4, 2022
54 tasks
proto/cosmos/auth/v1beta1/auth.proto Outdated Show resolved Hide resolved
Comment on lines 15 to 26
option (cosmos.msg.v1.legacy_amino_name) = "tendermint/PubKeyEd25519";
// The Amino encoding is simply the inner bytes field, and not the Amino
// encoding of the whole PubKey struct.
//
// Example (JSON):
// s := PubKey{Key: []byte{0x01}}
// out := AminoJSONEncoder(s)
//
// Then we have:
// out == `"MQ=="`
// out != `{"key":"MQ=="}`
option (cosmos.msg.v1.legacy_amino_encoding) = "bytes field";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any ideas what to put instead of bytes field here?

bytes_field sounds strange. I would use bytes, base64, etc...

@tac0turtle tac0turtle dismissed ValarDragon’s stale review November 7, 2022 16:10

talked with dev about dismissing this review. Got support

@pyramation
Copy link
Contributor

#13501 (comment)

agree, I like bytes, base64 better than bytes_field

@amaury1093 amaury1093 enabled auto-merge (squash) November 7, 2022 19:05
@amaury1093
Copy link
Contributor Author

agree, I like bytes, base64 better than bytes_field

It's amino.message_encoding="key_field" now. The meaning of this is that the encoding of the message is whatever the encoding of the key field (field number 1) is. I don't think bytes or base64 expresses that.

@robert-zaremba
Copy link
Collaborator

I understand the motivation, I don't think key_field as a value is clear enough. How about:

amino.message_encoding_field="key"  // use msg.key as the encoded value of the msg
// or
amino.message_encoding="flat"  // makes sense only for Msgs with one field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add amino JSON proto annotations
7 participants