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.

Sign up for GitHub

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

Jump to bottom

feat(depinject): support for core #5976

Merged
merged 42 commits into from
Apr 18, 2024
Merged

feat(depinject): support for core #5976

merged 42 commits into from
Apr 18, 2024

Conversation

crodriguezvega
Copy link
Contributor

Description

Adding support for ibc 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.

  • Targeted PR against the correct branch (see CONTRIBUTING.md).
  • Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
  • Code follows the module structure standards and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/).
  • Added relevant godoc comments.
  • Provide a conventional commit message to follow the repository standards.
  • Include a descriptive changelog entry when appropriate. This may be left to the discretion of the PR reviewers. (e.g. chores should be omitted from changelog)
  • Re-reviewed Files changed in the GitHub PR explorer.
  • Review SonarCloud Report in the comment section below once CI passes.

Carlos Rodriguez added 27 commits June 16, 2023 11:13
wip: depinject ibc core module configuration
f51ade4
fix import cosmos/app/v1alpha1 import
719f387
Merge branch 'main' into carlos/3560-support-sdk-dependency-injection…
0bf15b9 
…-for-v2-apps
Merge branch 'main' into carlos/3560-support-sdk-dependency-injection…
bf6ec6a 
…-for-v2-apps

# Conflicts:
#	modules/core/module.go
fix wrong merge
b6a96ce
add generation of pulsar files
bb25487
Merge branch 'carlos/add-buf-gen-pulsar' into carlos/3560-support-sdk…
a822c5c 
…-dependency-injection-for-v2-apps

# Conflicts:
#	proto/ibc/core/module/v1/module.proto
remove previously generated proto file
89d961c
add api import in go.mod
33b4141
add api import to e2e
f727360
remove wrong import
91d963b
Merge branch 'carlos/add-buf-gen-pulsar' into carlos/3560-support-sdk…
1e87904 
…-dependency-injection-for-v2-apps

# Conflicts:
#	go.mod
Merge branch 'feat/depinject-support' into carlos/add-buf-gen-pulsar
0d97046
Merge branch 'carlos/add-buf-gen-pulsar' into carlos/3560-support-sdk…
381b58c 
…-dependency-injection-for-v2-apps
some go.mod chores
c772207
remove code that should come in a different PR
98a354f
Merge branch 'carlos/add-buf-gen-pulsar' into carlos/3560-depinject-s…
fbb1296 
…upport-code

# Conflicts:
#	go.mod
#	modules/core/module.go
Merge branch 'main' into carlos/add-buf-gen-pulsar
d41e1fa
Merge branch 'carlos/add-buf-gen-pulsar' into carlos/3560-depinject-s…
5c1943e 
…upport-code

# Conflicts:
#	e2e/go.mod
#	go.mod
#	go.sum
#	modules/core/module.go
Merge branch 'feat/depinject-support' into carlos/add-buf-gen-pulsar
dce72db
Merge branch 'carlos/add-buf-gen-pulsar' into carlos/3560-depinject-s…
5dc6368 
…upport-code

# Conflicts:
#	e2e/go.mod
#	go.mod
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
Merge branch 'carlos/add-buf-gen-pulsar' into carlos/3560-depinject-s…
32d8899 
…upport-core
move to depinject.go
9617b9d
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed 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 crodriguezvega marked this pull request as draft March 12, 2024 22:21
@crodriguezvega crodriguezvega mentioned this pull request Mar 13, 2024
Open
3 tasks
crodriguezvega
crodriguezvega commented Mar 13, 2024
View reviewed changes
Dockerfile Outdated Show resolved Hide resolved
@crodriguezvega crodriguezvega marked this pull request as ready for review March 13, 2024 08:14
damiannolan
damiannolan approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

Nice work! Thanks for keeping these PRs clean.

modules/core/depinject.go
Comment on lines +25 to +28
appmodule.Register(
&modulev1.Module{},
appmodule.Provide(ProvideModule),
)
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking that we may need something like InvokeAddRoutes here which will register []porttypes.IBCModule on the router..

Something similar to x/gov maybe: https://github.com/cosmos/cosmos-sdk/blob/main/x/gov/depinject.go#L100

Hopefully we can use this as some kind of example. Happy to work on this once the initial PRs are merged.

DimitrisJim
DimitrisJim reviewed Apr 17, 2024
View reviewed changes
proto/buf.gen.gogo.yaml
@@ -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
Copy link
Contributor

@DimitrisJim DimitrisJim Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ah, this is probably why we need Mcosmos/app/v1alpha1/module.proto=cosmossdk.io/api/cosmos/app/v1alpha1. Wonder if the override Damian added in #5968 should cover this.

Base automatically changed from carlos/add-buf-gen-pulsar to feat/depinject April 18, 2024 09:57
Carlos Rodriguez added 2 commits April 18, 2024 12:01
Merge branch 'feat/depinject' into carlos/3560-depinject-support-core
aa8ef9e 
# Conflicts:
#	api/capability/v1/capability.pulsar.go
#	api/capability/v1/genesis.pulsar.go
#	api/go.mod
#	api/go.sum
#	api/ibc/applications/fee/v1/fee.pulsar.go
#	api/ibc/applications/fee/v1/query.pulsar.go
#	api/ibc/applications/fee/v1/tx.pulsar.go
#	api/ibc/applications/interchain_accounts/controller/v1/tx.pulsar.go
#	api/ibc/applications/interchain_accounts/host/v1/host.pulsar.go
#	api/ibc/applications/interchain_accounts/host/v1/tx.pulsar.go
#	api/ibc/applications/interchain_accounts/v1/account.pulsar.go
#	api/ibc/applications/interchain_accounts/v1/packet.pulsar.go
#	api/ibc/applications/transfer/v1/authz.pulsar.go
#	api/ibc/applications/transfer/v1/genesis.pulsar.go
#	api/ibc/applications/transfer/v1/query.pulsar.go
#	api/ibc/applications/transfer/v1/tx.pulsar.go
#	api/ibc/core/channel/v1/query.pulsar.go
#	api/ibc/core/channel/v1/tx.pulsar.go
#	api/ibc/core/client/v1/client.pulsar.go
#	api/ibc/core/client/v1/query.pulsar.go
#	api/ibc/core/client/v1/tx.pulsar.go
#	api/ibc/core/commitment/v1/commitment.pulsar.go
#	api/ibc/core/connection/v1/query.pulsar.go
#	api/ibc/core/connection/v1/tx.pulsar.go
#	api/ibc/lightclients/solomachine/v2/solomachine.pulsar.go
#	api/ibc/lightclients/solomachine/v3/solomachine.pulsar.go
#	api/ibc/lightclients/tendermint/v1/tendermint.pulsar.go
#	api/ibc/lightclients/wasm/v1/query.pulsar.go
#	api/ibc/lightclients/wasm/v1/tx.pulsar.go
#	proto/buf.gen.pulsar.yaml
downgrade api module to go 1.21
b56e330
@crodriguezvega
Copy link
Contributor Author

I have downgraded the api module to use go 1.21 to avoid all kind of build issues. We can upgrade the whole repo to 1.22 when we tackle #5776.

@crodriguezvega
Copy link
Contributor Author

I have downgraded the api module to use go 1.21 to avoid all kind of build issues. We can upgrade the whole repo to 1.22 when we tackle #5776.

Nah, it doesn't work.

@DimitrisJim
Copy link
Contributor

nice @crodriguezvega! Forgot ics23's api/!

@crodriguezvega
Copy link
Contributor Author

nice @crodriguezvega! Forgot ics23's api/!

hehe, nice, it's cool that it builds. 😅

@crodriguezvega crodriguezvega mentioned this pull request Apr 18, 2024
Closed
10 tasks
@crodriguezvega
Copy link
Contributor Author

@DimitrisJim If the CI is green and you approve this one, I can rebase the other PRs and hopefully we can get them merged quickly.

DimitrisJim
DimitrisJim approved these changes Apr 18, 2024
View reviewed changes
Copy link
Contributor

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

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

lgtm, left one Q about some of the files but I could be mistaken

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed 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 crodriguezvega merged commit fdb29a2 into feat/depinject Apr 18, 2024
85 of 86 checks passed
@crodriguezvega crodriguezvega deleted the carlos/3560-depinject-support-core branch April 18, 2024 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DimitrisJim DimitrisJim DimitrisJim approved these changes

@damiannolan damiannolan damiannolan approved these changes

@colin-axner colin-axner Awaiting requested review from colin-axner

@AdityaSripal AdityaSripal Awaiting requested review from AdityaSripal AdityaSripal is a code owner

@chatton chatton Awaiting requested review from chatton

@charleenfei charleenfei Awaiting requested review from charleenfei

@srdtrk srdtrk Awaiting requested review from srdtrk srdtrk is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@crodriguezvega @DimitrisJim @damiannolan