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

docs: add docs on proto annotations #15548

Merged
merged 8 commits into from
Sep 7, 2023
Merged

docs: add docs on proto annotations #15548

merged 8 commits into from
Sep 7, 2023

Conversation

tac0turtle
Copy link
Member

Description

ref: #14682

this pr describes various protobuf annotations that have been added over time to make working with protobuf easier for application developers


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)


## Scalars

(cosmos_proto.scalar) = "cosmos.AddressString";
Copy link
Member

Choose a reason for hiding this comment

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

Just a note to also describe AddressBytes, ValidatorAddressString/Bytes and ConsensusAddressString/Bytes. Basically we want to cover all of what is in the global config and sdk.{Acc/Val/Cons}Address

@tac0turtle
Copy link
Member Author

Closing due to inactivity. I will pick this up after abci++ is merged

@tac0turtle tac0turtle marked this pull request as ready for review September 6, 2023 15:10
@tac0turtle tac0turtle requested a review from a team as a code owner September 6, 2023 15:10
@github-prbot github-prbot requested review from a team, kocubinski and likhita-809 and removed request for a team September 6, 2023 15:11

The amino codec was removed in 0.50.0, this means there is not a need register `legacyAminoCodec`. To replace the amino codec, Amino protobuf annotations are used to provide information to the amino codec on how to encode and decode protobuf messages.

:::Note
Copy link
Member

Choose a reason for hiding this comment

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

Typo here.

(cosmos_proto.scalar) = "cosmos.AddressString"
```

There are a few options for what can be provided as a scalar: cosmos.AddressString, cosmos.ValidatorAddressString, cosmos.ConsensusAddressString, cosmos.Int, cosmos.Dec.
Copy link
Member

Choose a reason for hiding this comment

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

Can we maybe give an example? It is quite important that the scalar annotations are correct for the addresses as AutoCLI relies on it (otherwise it will just be considered as a string without any validation)

Copy link
Member Author

Choose a reason for hiding this comment

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

i think you should assume it wont be correct and/or they wont be implemented at all. I can add it but we shouldnt assume its used

Copy link
Member

@julienrbrt julienrbrt Sep 7, 2023

Choose a reason for hiding this comment

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

I feel like it is the role of developer removing their CLI for AutoCLI to verify the annotation. Hence I thought a small mention was useful.

However, AutoCLI will still work without it. Addresses will simply not be verified (if you have an annotation there will be a field verification that you input a valid address).

Copy link
Member Author

Choose a reason for hiding this comment

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

for sure, added it. It will be hard to get people to adopt this. Many times with proto its write once and forget till you need a change. Adding annotations will be harder to for people to integrate

Copy link
Member

Choose a reason for hiding this comment

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

It should be more in building modules imho.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

lgtm

@tac0turtle tac0turtle added the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label Sep 7, 2023
@tac0turtle tac0turtle added this pull request to the merge queue Sep 7, 2023
Merged via the queue into main with commit c86a6dc Sep 7, 2023
42 checks passed
@tac0turtle tac0turtle deleted the marko/14682 branch September 7, 2023 09:27
mergify bot pushed a commit that referenced this pull request Sep 7, 2023
tac0turtle added a commit that referenced this pull request Sep 7, 2023
@faddat faddat mentioned this pull request Nov 8, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants