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

chore (api!): removal of capabilites module and all references to it. #7279

Merged
merged 4 commits into from
Sep 10, 2024
Merged

chore (api!): removal of capabilites module and all references to it. #7279

merged 4 commits into from
Sep 10, 2024

Conversation

bznein
Copy link
Contributor

@bznein bznein commented Sep 10, 2024 •
edited
Loading

Description

ref: #7273

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.

@bznein
Copy link
Contributor Author

bznein commented Sep 10, 2024

Opening review to trigger E2E

@bznein bznein marked this pull request as ready for review September 10, 2024 15:36
colin-axner
colin-axner approved these changes Sep 10, 2024
View reviewed changes
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

ACK pending green e2e's

What a beautiful day! 🌞 So happy to see this module removed

DimitrisJim
DimitrisJim reviewed Sep 10, 2024
View reviewed changes
e2e/go.sum
@@ -389,6 +389,8 @@ github.com/cosmos/gogoproto v1.7.0 h1:79USr0oyXAbxg3rspGh/m4SWNyoz/GLaAh0QlCe2fr
github.com/cosmos/gogoproto v1.7.0/go.mod h1:yWChEv5IUEYURQasfyBW5ffkMHR/90hiHgbNgrtp4j0=
github.com/cosmos/iavl v1.2.0 h1:kVxTmjTh4k0Dh1VNL046v6BXqKziqMDzxo93oh3kOfM=
github.com/cosmos/iavl v1.2.0/go.mod h1:HidWWLVAtODJqFD6Hbne2Y0q3SdxByJepHUOeoH4LiI=
github.com/cosmos/ibc-go/modules/capability v1.0.1 h1:ibwhrpJ3SftEEZRxCRkH0fQZ9svjthrX2+oXdZvzgGI=
Copy link
Contributor

Choose a reason for hiding this comment

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

hm, what is bringing this in as an indirect? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

go mod why -m github.com/cosmos/ibc-go/modules/capability
# github.com/cosmos/ibc-go/modules/capability
github.com/cosmos/ibc-go/e2e/testsuite
github.com/strangelove-ventures/interchaintest/v8/chain/cosmos
github.com/cosmos/ibc-go/modules/capability

Stack overflow brought me here, now I need to understand what it means :D

Copy link
Contributor Author

@bznein bznein Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

https://github.com/strangelove-ventures/interchaintest/blob/main/chain/cosmos/codec.go#L23

It comes from here, and we import that module from two different files in our e2e/testsuite dir:
https://github.com/cosmos/ibc-go/blob/main/e2e/testsuite/testsuite.go#L15
https://github.com/cosmos/ibc-go/blob/main/e2e/testsuite/tx.go#L12

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, ofc its interchaintest 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

no worries, was just curious!

@bznein bznein changed the title [WIP] chore (api!): removal of capabilites module and all references to it. chore (api!): removal of capabilites module and all references to it. Sep 10, 2024
DimitrisJim
DimitrisJim approved these changes Sep 10, 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.

beautiful chunky red diff to end your day. what a time to be alive!

@bznein bznein enabled auto-merge September 10, 2024 16:22
@bznein bznein added this pull request to the merge queue Sep 10, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Sep 10, 2024

Quality Gate Passed Quality Gate passed for 'ibc-go'

Issues
149 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Merged via the queue into main with commit 524ff0d Sep 10, 2024
73 checks passed
@bznein bznein deleted the bznein/capabilitiesfinalremoval branch September 10, 2024 16:30
@crodriguezvega crodriguezvega mentioned this pull request Sep 30, 2024
Closed
@colin-axner colin-axner mentioned this pull request Oct 9, 2024
Closed
3 tasks
@womensrights womensrights added this to the v10.0.0 milestone Dec 5, 2024
@womensrights womensrights mentioned this pull request Dec 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colin-axner colin-axner colin-axner approved these changes

@DimitrisJim DimitrisJim DimitrisJim approved these changes

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

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

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

@chatton chatton Awaiting requested review from chatton

@crodriguezvega crodriguezvega Awaiting requested review from crodriguezvega

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v10.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@bznein @DimitrisJim @colin-axner @womensrights