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: full api module building alongside gogo proto files with buf schema registry support #10669

Merged
merged 20 commits into from
Jan 6, 2022

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Dec 2, 2021

Description

This PR makes the following changes:

  • two protobuf code generation templates are now applied:
    • gogo proto code generation is applied only to existing proto files already used in the SDK
    • an api module has pulsar/golang v2 code generation applied to all cosmos and tendermint proto files including new proto files which are never had nor will have a gogo proto representation. The buf managed mode is used for this generation
  • third_party/proto/tendermint is moved to proto/tendermint and made part of the "cosmos" proto API (@marbar3778 let's coordinate on applying versioning)
  • confio proto files were removed (not used)
  • gogoproto, googleapis proto types are now retrieved as buf schema registry dependencies

All of the cosmos-sdk proto files are now present in the buf schema registry with auto-generated documentation here: https://buf.build/cosmos/cosmos-sdk

The main files to look at here are protocgen.sh and the various buf.yaml files.


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)

@codecov
Copy link

codecov bot commented Dec 2, 2021

Codecov Report

Merging #10669 (5216e67) into master (074c95b) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10669      +/-   ##
==========================================
- Coverage   64.91%   64.87%   -0.05%     
==========================================
  Files         632      628       -4     
  Lines       59901    59689     -212     
==========================================
- Hits        38885    38722     -163     
+ Misses      18816    18770      -46     
+ Partials     2200     2197       -3     
Impacted Files Coverage Δ
baseapp/abci.go 60.53% <ø> (-0.37%) ⬇️
simapp/app.go 82.54% <ø> (+0.15%) ⬆️
types/module/module.go 70.58% <ø> (+3.13%) ⬆️
x/feegrant/filtered_fee.go 57.74% <ø> (+57.74%) ⬆️
x/distribution/simulation/operations.go 80.54% <0.00%> (-9.73%) ⬇️
errors/abci.go
errors/stacktrace.go
errors/handle.go
errors/errors.go
... and 4 more

@aaronc aaronc changed the title feat: full api module building alongside gogo proto files with full buf schema registry support feat: full api module building alongside gogo proto files with buf schema registry support Dec 3, 2021
@github-actions github-actions bot added C:CLI C:Cosmovisor Issues and PR related to Cosmovisor C:Store and removed C:x/staking labels Jan 5, 2022
@github-actions github-actions bot removed C:CLI C:Store C:Cosmovisor Issues and PR related to Cosmovisor labels Jan 5, 2022
@aaronc aaronc marked this pull request as ready for review January 5, 2022 19:38
@github-actions github-actions bot added the C:orm label Jan 5, 2022
Copy link
Contributor

@fdymylja fdymylja left a comment

Choose a reason for hiding this comment

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

finally... a dream coming true...

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

lgtm. the docker image just needs an updated version of bug correct?

@fdymylja fdymylja self-requested a review January 6, 2022 12:10
Copy link
Contributor

@fdymylja fdymylja left a comment

Choose a reason for hiding this comment

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

There's a bug, coming most likely from pulsar that needs to be fixed.

Importing signing causes a panic at init() phase.

IIRC this came from wrong ordering of some protobuf descriptor init data.

@fdymylja
Copy link
Contributor

fdymylja commented Jan 6, 2022

Once cosmos/cosmos-proto#56 is merged and protofiles are rebuilt I think we can merge this... Good we caught this bug though...

cd proto
proto_dirs=$(find ./cosmos -path -prune -o -name '*.proto' -print0 | xargs -0 -n1 dirname | sort | uniq)
for dir in $proto_dirs; do
for file in $(find "${dir}" -maxdepth 1 -name '*.proto'); do
Copy link
Member

Choose a reason for hiding this comment

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

do you need this for loop? for reference in tendermint we do https://github.com/tendermint/tendermint/blob/master/scripts/protocgen.sh#L32

Copy link
Member Author

@aaronc aaronc Jan 6, 2022

Choose a reason for hiding this comment

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

it is needed because we only generate proto files with a go_package option explicitly specified with gogo, and we run grep on each file to figure that out. if no go_package is set we assume it is pulsar only

@aaronc
Copy link
Member Author

aaronc commented Jan 6, 2022

Updated this code with your pulsar fix @fdymylja

@aaronc aaronc requested a review from fdymylja January 6, 2022 19:32
@aaronc
Copy link
Member Author

aaronc commented Jan 6, 2022

I guess let's merge this 🎉

@aaronc aaronc merged commit 0cb7fd0 into master Jan 6, 2022
@aaronc aaronc deleted the aaronc/bsr-setup branch January 6, 2022 19:57
@aaronc aaronc mentioned this pull request Jan 6, 2022
19 tasks
mergify bot pushed a commit that referenced this pull request Jan 11, 2022
## Description

seems like I didn't get the pulsar alpha6 protogen right in #10669  

---

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants