feat(depinject): add buf gen pulsar #5968

crodriguezvega · 2024-03-12T09:24:27Z

Description

I welcome ideas on how to call the new module instead of api.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

coderabbitai · 2024-03-12T09:24:35Z

Important

Auto Review Skipped

Auto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

crodriguezvega · 2024-03-12T22:22:58Z

We could also just use this api module also for what we want to do here.

damiannolan · 2024-04-15T17:06:53Z

Is the feat/depinject target branch clean? And up to date with main?

crodriguezvega · 2024-04-15T18:40:25Z

Is the feat/depinject target branch clean? And up to date with main?

Yes and yes!

scripts/protocgen-pulsar.sh

damiannolan · 2024-04-16T15:35:39Z

2024/04/16 15:30:37 Message ibc.applications.interchain_accounts.v1.InterchainAccountPacketData contains the reserved field name ibc.applications.interchain_accounts.v1.InterchainAccountPacketData.type which conflicts with protoreflect.Message interface implementation.
This field will be suffixed with an underscore '_'.
If you can change the message field name, please do so.
In a future iteration of pulsar we may make a breaking change to this practice in order to be compliant with field naming of the original golang protobuf implementation.

This msg is displayed in my shell when I run make proto-all. AFAIK the sdk team will be removing the need for pulsar(and protov2 spec) with the next release. They want to make it work with gogoproto which means the codegen will change and I think this problem/error msg will go away. The depinject apis will remain the same tho.

damiannolan · 2024-04-16T15:39:57Z

proto/buf.gen.pulsar.yaml

As I mentioned in #5968 (comment), we should no longer need pulsar with the next release. My thinking is that we can go ahead with this for now and add the pulsar codegen but when it works with gogoproto we just remove what's not needed and swap it out - should produce diffs to the codegen (i.e. removing the protov2 msg APIs and back to v1 with gogoproto - from what I understand this relates the reflection apis which are used in protobuf and sdk)

DimitrisJim

200k diff, quick skim, lgtm

writting comment now

DimitrisJim · 2024-04-16T23:07:20Z

Actually, scratch that. Can't get this to build locally still. Is cd-ing into api and doing go build ./... working for others? I would assume it should?

Looking into this more, some imports don't seem to be generated correctly? E.g, the import for the base account our interchain account wraps points to github.com/cosmos-sdk/x/auth/types and this type does not implement a ProtoReflect (what the go build step starts screaming about, among other things 😄 ).

The correct type is apparently located in cosmossdk.io/api/cosmos/auth/v1beta1.

go build complains about others too. The generation for some reason might have mucked things up?

damiannolan · 2024-04-17T09:26:44Z

Actually, scratch that. Can't get this to build locally still. Is cd-ing into api and doing go build ./... working for others? I would assume it should?

Same experience for me.

go build complains about others too. The generation for some reason might have mucked things up?

Very possible, I can try to look into this a bit more.

DimitrisJim · 2024-04-17T10:00:57Z

we should add a workflow for api/ that at least tries to go build it. Linting could be added later if we decide to use api to include non-generated files.

crodriguezvega · 2024-04-17T10:02:12Z

go build complains about others too. The generation for some reason might have mucked things up?

Oh, I hadn't even tried that... Does building work in the api folder of the SDK?

DimitrisJim · 2024-04-17T10:50:25Z

Does building work in the api folder of the SDK?

yup! I think our go.mod for api/ should probably have no reference to github.com/cosmos-sdk, only cosmossdk.io/api

api/ibc/applications/transfer/v1/tx.pulsar.go

…modifications.

…being.

DimitrisJim · 2024-04-17T18:59:11Z

proto/buf.gen.pulsar.yaml

+plugins:
+  - name: go-pulsar
+    out: ../api
+    opt: Mcosmos/app/v1alpha1/module.proto=cosmossdk.io/api/cosmos/app/v1alpha1,Mcosmos/ics23/v1/proofs.proto=github.com/cosmos/ics23/go/api


seems to be the way to link the api module?

yeah, I think this is how you do overriding.

maybe @julienrbrt could give us a more detailed description so we have a better understanding.

api/go.mod

DimitrisJim

I'd be perfectly happy to merge. Since we know where the build issues were, this can be tested locally for the time being.

damiannolan

LGTM!

.github/workflows/api_module.yml

api/go.mod

damiannolan · 2024-04-18T08:24:23Z

proto/buf.gen.pulsar.yaml

+plugins:
+  - name: go-pulsar
+    out: ../api
+    opt: Mcosmos/app/v1alpha1/module.proto=cosmossdk.io/api/cosmos/app/v1alpha1,Mcosmos/ics23/v1/proofs.proto=github.com/cosmos/ics23/go/api


yeah, I think this is how you do overriding.

maybe @julienrbrt could give us a more detailed description so we have a better understanding.

api/go.mod

sonarqubecloud · 2024-04-18T08:31:40Z

Quality Gate passed for 'ibc-go'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

crodriguezvega · 2024-04-18T09:56:56Z

Great work taking this to the finish line, @damiannolan and @DimitrisJim. 👏

Carlos Rodriguez added 9 commits July 7, 2023 15:20

add generation of pulsar files

bb25487

remove wrong import

91d963b

Merge branch 'feat/depinject-support' into carlos/add-buf-gen-pulsar

0d97046

remove code that should come in a different PR

98a354f

Merge branch 'main' into carlos/add-buf-gen-pulsar

d41e1fa

Merge branch 'feat/depinject-support' into carlos/add-buf-gen-pulsar

dce72db

Merge branch 'main' into carlos/add-buf-gen-pulsar

66b7e15

update pulsar files

34e6e1c

api go mod tidy

17ce7dd

Merge branch 'main' into carlos/add-buf-gen-pulsar

c9f1eb1

crodriguezvega changed the base branch from main to feat/depinject March 12, 2024 18:41

crodriguezvega mentioned this pull request Mar 13, 2024

Support SDK dependency injection for v2 apps #3560

Open

3 tasks

crodriguezvega marked this pull request as ready for review April 15, 2024 08:47

crodriguezvega requested review from colin-axner, AdityaSripal, charleenfei, damiannolan, chatton and DimitrisJim as code owners April 15, 2024 08:47

damiannolan reviewed Apr 16, 2024

View reviewed changes

scripts/protocgen-pulsar.sh Outdated Show resolved Hide resolved

chore: consolidate protogen scripts

b65ad37

damiannolan approved these changes Apr 16, 2024

View reviewed changes

DimitrisJim previously approved these changes Apr 16, 2024

View reviewed changes

Merge branch 'feat/depinject' into carlos/add-buf-gen-pulsar

e779ba0

damiannolan requested changes Apr 17, 2024

View reviewed changes

api/ibc/applications/transfer/v1/tx.pulsar.go Outdated Show resolved Hide resolved

damiannolan and others added 6 commits April 17, 2024 14:56

proto: add override to buf.gen.pulsar for cosmos-sdk

21e2f1b

chore: go mod tidy api

d7f02dd

proto: use protobuf/types/known/anypb

5253dce

chore: tidy api mod.

ae4d335

proto: rm go-grpc plugin for pulsar

a5e37eb

chore: go mod tidy api

2362ef6

This was referenced Apr 17, 2024

feat(depinject): support for core #5976

Merged

feat: add api folder containing pulsar generated code. cosmos/ics23#306

Closed

DimitrisJim added 2 commits April 17, 2024 19:43

github: add workflow to at least sanity check that api mod builds on …

2183ec1

…modifications.

wip: add opt for ics23, rebuild pulsar. Comment out imports for time …

e2b05e9

…being.

DimitrisJim reviewed Apr 17, 2024

View reviewed changes

api/go.mod Outdated Show resolved Hide resolved

DimitrisJim approved these changes Apr 17, 2024

View reviewed changes

deps: replace with go psuedoversion of pulsar ics23 temp branch

7f4c62c

damiannolan approved these changes Apr 18, 2024

View reviewed changes

Update api/go.mod

dcb615b

damiannolan reviewed Apr 18, 2024

View reviewed changes

api/go.mod Show resolved Hide resolved

crodriguezvega merged commit 71233e5 into feat/depinject Apr 18, 2024
74 checks passed

crodriguezvega deleted the carlos/add-buf-gen-pulsar branch April 18, 2024 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(depinject): add buf gen pulsar #5968

feat(depinject): add buf gen pulsar #5968

crodriguezvega commented Mar 12, 2024 •

edited

Loading

coderabbitai bot commented Mar 12, 2024 •

edited

Loading

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

crodriguezvega commented Mar 12, 2024

damiannolan commented Apr 15, 2024

crodriguezvega commented Apr 15, 2024

damiannolan commented Apr 16, 2024

damiannolan Apr 16, 2024

DimitrisJim left a comment

DimitrisJim commented Apr 16, 2024 •

edited

Loading

damiannolan commented Apr 17, 2024

DimitrisJim commented Apr 17, 2024

crodriguezvega commented Apr 17, 2024

DimitrisJim commented Apr 17, 2024

DimitrisJim Apr 17, 2024

damiannolan Apr 18, 2024

DimitrisJim left a comment

damiannolan left a comment

damiannolan Apr 18, 2024

sonarqubecloud bot commented Apr 18, 2024

crodriguezvega commented Apr 18, 2024

feat(depinject): add buf gen pulsar #5968

feat(depinject): add buf gen pulsar #5968

Conversation

crodriguezvega commented Mar 12, 2024 • edited Loading

Description

coderabbitai bot commented Mar 12, 2024 • edited Loading

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

crodriguezvega commented Mar 12, 2024

damiannolan commented Apr 15, 2024

crodriguezvega commented Apr 15, 2024

damiannolan commented Apr 16, 2024

damiannolan Apr 16, 2024

Choose a reason for hiding this comment

DimitrisJim left a comment

Choose a reason for hiding this comment

DimitrisJim commented Apr 16, 2024 • edited Loading

damiannolan commented Apr 17, 2024

DimitrisJim commented Apr 17, 2024

crodriguezvega commented Apr 17, 2024

DimitrisJim commented Apr 17, 2024

DimitrisJim Apr 17, 2024

Choose a reason for hiding this comment

damiannolan Apr 18, 2024

Choose a reason for hiding this comment

DimitrisJim left a comment

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

damiannolan Apr 18, 2024

Choose a reason for hiding this comment

sonarqubecloud bot commented Apr 18, 2024

Quality Gate passed for 'ibc-go'

crodriguezvega commented Apr 18, 2024

crodriguezvega commented Mar 12, 2024 •

edited

Loading

coderabbitai bot commented Mar 12, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)

DimitrisJim commented Apr 16, 2024 •

edited

Loading