-
Notifications
You must be signed in to change notification settings - Fork 614
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(depinject): support for capability #6158
feat(depinject): support for capability #6158
Conversation
Important Auto Review SkippedAuto 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 You can disable this status message by setting the 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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configration File (
|
I had to delete manually a pg.go file that gets generated. Would there be a way to skip its generation? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty much good to go so far. I left a couple of nits and a comment about the MemoryStoreKey
Happy to also push some updates from comments myself today if I get to it!
proto/buf.gen.gogo.yaml
Outdated
@@ -2,7 +2,7 @@ version: v1 | |||
plugins: | |||
- name: gocosmos | |||
out: .. | |||
opt: plugins=grpc,Mgoogle/protobuf/any.proto=github.com/cosmos/cosmos-sdk/codec/types | |||
opt: plugins=grpc,Mgoogle/protobuf/any.proto=github.com/cosmos/cosmos-sdk/codec/types,Mcosmos/app/v1alpha1/module.proto=cosmossdk.io/api/cosmos/app/v1alpha1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what enables the custom cosmos module options, right?
i.e.
option (cosmos.app.v1alpha1.module) = {
go_import: "github.com/cosmos/ibc-go/modules/capability"
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that's my understanding.
Dockerfile
Outdated
@@ -20,6 +20,7 @@ COPY Makefile . | |||
|
|||
COPY go.mod . | |||
COPY go.sum . | |||
COPY ./api /go/api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we try doing ADD api api
on L13 instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attemped to move this to ADD
, looks like docker image built successfully. But we are seeing this sonar test failure on all PR workflows at the moment, I think it is outside of our control, assuming somebody somewhere will fix
I don't understand why we need to delete that file? Was it just in a single commit where something was funky? The diffs against the target branch don't show a deleted file there so I'm assuming its still in place |
When I run |
…upport-capability # Conflicts: # modules/capability/go.mod
Eeek, I was a little too quick on slapping the approval. Looks like build failures due to go version discrepancy. Might be worth just bumping all to go1.22 for this work. What do you think? |
Yes, I think it will be the shortest path... |
modules/capability/go.mod
Outdated
@@ -1,6 +1,6 @@ | |||
module github.com/cosmos/ibc-go/modules/capability | |||
|
|||
go 1.21 | |||
go 1.22.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh, so workflow file doesn't need to be bumped? That's odd, I thought this directive enforced something about the version of go
you're running, apparently not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can downgrade this to go1.21 again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! love to see everything building fine.
Quality Gate passed for 'ibc-go'Issues Measures |
Description
Adding support for capability package.
ref: #3560
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.
docs/
).godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.