chore (api)!: remove capabilities from channel handshakes #7232

bznein · 2024-08-30T10:09:32Z

Description

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.

bznein

This PR is definitely bigger than expected. I'm happy to split it into separate PRs if needed.

However, most of the diff is from removing stuff (usages of LookupModule.., tests that rely on capabilities, and usage of capabilities within other tets)

I also added as many comments to the PR as possible, to make review easier :)

bznein · 2024-08-30T10:31:33Z

modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go

@@ -128,26 +127,12 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() {
 		{
 			"success", func() {}, nil,
 		},
-		{
-			"ICA auth module does not claim channel capability", func() {


Capability-related test. No longer needed

bznein · 2024-08-30T10:36:39Z

modules/core/04-channel/keeper/handshake_test.go

 		}, false},
 		{"connection does not support ORDERED channels", func() {
 			path.SetupConnections()
 			path.SetChannelOrdered()
 			err := path.EndpointA.ChanOpenInit()
 			suite.Require().NoError(err)

-			// modify connA versions to only support UNORDERED channels
-			path.EndpointA.UpdateConnection(func(c *connectiontypes.ConnectionEnd) {
+			// modify connB versions to only support UNORDERED channels


This change was needed because the test was not correct. It comes from 29d9b59 (comments on #7009 explain it better)

bznein · 2024-08-30T10:37:54Z

modules/core/04-channel/keeper/handshake_test.go


 				// trigger upgradeInit on B which will bump the counterparty upgrade sequence.
-				path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion
-				err := path.EndpointB.ChanUpgradeInit()
+				path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion


Same re: 29d9b59

bznein · 2024-08-30T10:41:02Z

modules/core/05-port/keeper/keeper.go

@@ -87,5 +88,21 @@ func (k *Keeper) LookupModuleByPort(ctx context.Context, portID string) (string,
 // Route returns a IBCModule for a given module, and a boolean indicating
 // whether or not the route is present.
 func (k *Keeper) Route(module string) (types.IBCModule, bool) {
-	return k.Router.GetRoute(module)
+	routes, ok := k.Router.GetRoute(module)


This is basically the same as https://github.com/cosmos/ibc-go/pull/7009/files#r1700067369, with the added function "Keys" (and the underlying Keys() in router.go)

bznein · 2024-08-30T10:43:55Z

modules/core/keeper/msg_server.go

@@ -193,32 +193,24 @@ func (k *Keeper) ConnectionOpenConfirm(goCtx context.Context, msg *connectiontyp
 func (k *Keeper) ChannelOpenInit(goCtx context.Context, msg *channeltypes.MsgChannelOpenInit) (*channeltypes.MsgChannelOpenInitResponse, error) {


This is probably the file with most changes. They can be quite easily summarized as follows:

For each method that uses capability:

Remove usages of LookupModule... and replace with direct call to the (now changed) Route method.

Since we do not get the capability anymore from the function above, remove it as an argument to underlying functions

bznein · 2024-08-30T10:47:17Z

Note: I haven't run E2E tests yet, and I need to open the PR for review for that. If anything fails, I'll mark the PR as not ready :)

Edit: everything is Green!!!

DimitrisJim · 2024-09-02T09:40:27Z

modules/apps/27-interchain-accounts/controller/ibc_middleware.go

 	if err != nil {
 		return "", err
 	}

-	if err := im.keeper.ClaimCapability(ctx, chanCap, host.ChannelCapabilityPath(portID, channelID)); err != nil {


is this Path func in host in use anymore after this PR?

Undirectly in a lot of test cases. I plan to remove them as followup

DimitrisJim · 2024-09-02T09:45:40Z

modules/core/04-channel/keeper/packet.go

-	capName := host.ChannelCapabilityPath(packet.GetDestPort(), packet.GetDestChannel())
-	if !k.scopedKeeper.AuthenticateCapability(ctx, chanCap, capName) {
-		return "", errorsmod.Wrapf(
-			types.ErrInvalidChannelCapability,


can also be removed?

Which one do you mean exactly? If it is AuthenticateCapability (the definition of the method itself), I'd like to do that as followup as well

(or did you mean the error type? same answer anyway :) )

sgtm! just worried that a lot of these public symbols don't get caught by linters so would be nice to keep track of them. A grep for Capability in the end might do the trick though!

DimitrisJim · 2024-09-02T09:46:02Z

modules/core/04-channel/keeper/timeout.go

-	capName := host.ChannelCapabilityPath(packet.GetSourcePort(), packet.GetSourceChannel())
-	if !k.scopedKeeper.AuthenticateCapability(ctx, chanCap, capName) {
-		return errorsmod.Wrapf(
-			types.ErrChannelCapabilityNotFound,


also axing time?

colin-axner · 2024-09-02T10:21:31Z

modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go

-		suite.Require().NoError(err)
-
-		cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
+		cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(path.EndpointB.ChannelConfig.PortID)


not from your changes, but I think the test might have been incorrect. I feel like this should be EndpointB

Uhm, it is EndpointB. Did you mean A?

colin-axner · 2024-09-02T10:37:04Z

modules/core/05-port/keeper/keeper.go

@@ -87,5 +88,21 @@ func (k *Keeper) LookupModuleByPort(ctx context.Context, portID string) (string,
 // Route returns a IBCModule for a given module, and a boolean indicating
 // whether or not the route is present.
 func (k *Keeper) Route(module string) (types.IBCModule, bool) {
-	return k.Router.GetRoute(module)
+	routes, ok := k.Router.GetRoute(module)


we can probably rename this back to route

haha I meant the var routes but all good

modules/core/05-port/keeper/keeper.go

modules/core/04-channel/keeper/handshake_test.go

modules/core/keeper/msg_server.go

colin-axner

Amazing 🙌 thank you @bznein! I appreciate you doing this seemingly tedious work (twice!). It makes me quite happy to see over 800 lines of red code 😄

Co-authored-by: colin axnér <[email protected]>

chatton

LGTM! Thank you so much for doing this clean up!

DimitrisJim

thats what I call a sweet diff. thanks much for clean up!

sonarqubecloud · 2024-09-02T12:17:35Z

Quality Gate passed for 'ibc-go'

Issues
151 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

bznein added 7 commits August 30, 2024 10:59

almost all tests working

ad21454

fix last tests

936aaf7

Merge branch 'main' into bznein/removeCap/channelhandshakes

c316b30

solve merge error

bd29bb4

remove newline

4c56bc3

fix comment

9307fed

docstring

d340715

bznein commented Aug 30, 2024

View reviewed changes

bznein marked this pull request as ready for review August 30, 2024 10:46

bznein requested review from colin-axner, AdityaSripal, damiannolan, srdtrk, chatton, DimitrisJim, crodriguezvega and gjermundgaraba as code owners August 30, 2024 10:46

bznein added the priority PRs that need prompt reviews label Aug 30, 2024

docs

7fa1078

DimitrisJim reviewed Sep 2, 2024

View reviewed changes

colin-axner reviewed Sep 2, 2024

View reviewed changes

modules/core/05-port/keeper/keeper.go Outdated Show resolved Hide resolved

colin-axner reviewed Sep 2, 2024

View reviewed changes

modules/core/04-channel/keeper/handshake_test.go Outdated Show resolved Hide resolved

colin-axner reviewed Sep 2, 2024

View reviewed changes

modules/core/keeper/msg_server.go Outdated Show resolved Hide resolved

colin-axner approved these changes Sep 2, 2024

View reviewed changes

Nikolas De Giorgis and others added 3 commits September 2, 2024 12:14

Update modules/core/keeper/msg_server.go

9d4fca7

Co-authored-by: colin axnér <[email protected]>

pr feedback

475394a

change error message

8a24b07

forgot to save a file...

2b712b3

chatton approved these changes Sep 2, 2024

View reviewed changes

DimitrisJim approved these changes Sep 2, 2024

View reviewed changes

Nikolas De Giorgis added 2 commits September 2, 2024 13:01

Merge branch 'main' into bznein/removeCap/channelhandshakes

29e5503

merge error

554308a

bznein added this pull request to the merge queue Sep 2, 2024

Merged via the queue into main with commit eb9bcc3 Sep 2, 2024
72 of 73 checks passed

bznein deleted the bznein/removeCap/channelhandshakes branch September 2, 2024 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore (api)!: remove capabilities from channel handshakes #7232

chore (api)!: remove capabilities from channel handshakes #7232

bznein commented Aug 30, 2024 •

edited by crodriguezvega

Loading

bznein left a comment

bznein Aug 30, 2024

bznein Aug 30, 2024

bznein Aug 30, 2024

bznein Aug 30, 2024

bznein Aug 30, 2024

bznein commented Aug 30, 2024 •

edited

Loading

DimitrisJim Sep 2, 2024

bznein Sep 2, 2024

DimitrisJim Sep 2, 2024

bznein Sep 2, 2024

bznein Sep 2, 2024

DimitrisJim Sep 2, 2024

DimitrisJim Sep 2, 2024

colin-axner Sep 2, 2024

bznein Sep 2, 2024

colin-axner Sep 2, 2024

colin-axner Sep 2, 2024

colin-axner Sep 2, 2024

colin-axner left a comment

chatton left a comment

DimitrisJim left a comment

sonarqubecloud bot commented Sep 2, 2024

		@@ -193,32 +193,24 @@ func (k Keeper) ConnectionOpenConfirm(goCtx context.Context, msg connectiontyp
		func (k Keeper) ChannelOpenInit(goCtx context.Context, msg channeltypes.MsgChannelOpenInit) (*channeltypes.MsgChannelOpenInitResponse, error) {

chore (api)!: remove capabilities from channel handshakes #7232

chore (api)!: remove capabilities from channel handshakes #7232

Conversation

bznein commented Aug 30, 2024 • edited by crodriguezvega Loading

Description

bznein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bznein commented Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

chatton left a comment

Choose a reason for hiding this comment

DimitrisJim left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 2, 2024

Quality Gate passed for 'ibc-go'

bznein commented Aug 30, 2024 •

edited by crodriguezvega

Loading

bznein commented Aug 30, 2024 •

edited

Loading