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

Use new port router for onchanupgradetry #7067

Merged
merged 6 commits into from
Aug 7, 2024
Merged

Use new port router for onchanupgradetry #7067

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a5521ab
chore: add fee implementation for OnChanOpenTry
chatton Aug 7, 2024
fae6122
chore: fix wiring in app.gos
chatton Aug 7, 2024
61c4799
chore: updated callbacks tests to handle new OnChanOpenTry
chatton Aug 7, 2024
824d62f
chore: lint, correct error string
chatton Aug 7, 2024
abcdb32
chore: remove commented code
chatton Aug 7, 2024
50be8e1
chore: addressing PR feedback
chatton Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions modules/apps/27-interchain-accounts/host/ibc_module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,10 +203,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenTry() {
// ensure channel on chainB is set in state
suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, *channel)

module, _, err := suite.chainB.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID)
suite.Require().NoError(err)

cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainB.App.GetIBCKeeper().PortKeeper.AppRouter.HandshakeRoute(icatypes.HostPortID)
suite.Require().True(ok)

version, err := cbs.OnChanOpenTry(suite.chainB.GetContext(), channel.Ordering, channel.ConnectionHops,
Expand Down
6 changes: 6 additions & 0 deletions modules/apps/29-fee/fee_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,23 +76,29 @@ func RemoveFeeMiddleware(chain *ibctesting.TestChain) {
chain.GetSimApp().IBCKeeper.PortKeeper.Router = nil

newRouter := porttypes.NewRouter() // Create a new router
newAppRouter := porttypes.NewAppRouter()

// Remove Fee middleware from transfer module
chain.GetSimApp().TransferKeeper.WithICS4Wrapper(channelKeeper)
transferStack := transfer.NewIBCModule(chain.GetSimApp().TransferKeeper)
newRouter.AddRoute(transfertypes.ModuleName, transferStack)
newAppRouter.AddRoute(transfertypes.ModuleName, transferStack)

// Remove Fee middleware from icahost submodule
chain.GetSimApp().ICAHostKeeper.WithICS4Wrapper(channelKeeper)
icaHostStack := icahost.NewIBCModule(chain.GetSimApp().ICAHostKeeper)
newRouter.AddRoute(icahosttypes.SubModuleName, icaHostStack)
newAppRouter.AddRoute(icahosttypes.SubModuleName, icaHostStack)

// Remove Fee middleware from icacontroller submodule
chain.GetSimApp().ICAControllerKeeper.WithICS4Wrapper(channelKeeper)
icaControllerStack := icacontroller.NewIBCMiddleware(chain.GetSimApp().ICAControllerKeeper)
newRouter.AddRoute(icacontrollertypes.SubModuleName, icaControllerStack)
newAppRouter.AddRoute(icacontrollertypes.SubModuleName, icaControllerStack)

// Override and seal the router
chain.GetSimApp().IBCKeeper.SetRouter(newRouter)
chain.GetSimApp().IBCKeeper.SetAppRouter(newAppRouter)
}

// helper function
Expand Down
30 changes: 4 additions & 26 deletions modules/apps/29-fee/ibc_middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,33 +70,11 @@ func (im IBCMiddleware) OnChanOpenTry(
counterparty channeltypes.Counterparty,
counterpartyVersion string,
) (string, error) {
versionMetadata, err := types.MetadataFromVersion(counterpartyVersion)
if err != nil {
// Since it is valid for fee version to not be specified, the above middleware version may be for a middleware
// lower down in the stack. Thus, if it is not a fee version we pass the entire version string onto the underlying
// application.
return im.app.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, counterparty, counterpartyVersion)
if strings.TrimSpace(counterpartyVersion) != "" && counterpartyVersion != types.Version {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if strings.TrimSpace(counterpartyVersion) != "" && counterpartyVersion != types.Version {
if counterpartyVersion != types.Version {

Copy link
Contributor

Choose a reason for hiding this comment

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

I think on Try, the version must be set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense!

return "", errorsmod.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, counterpartyVersion)
}

if versionMetadata.FeeVersion != types.Version {
return "", errorsmod.Wrapf(types.ErrInvalidVersion, "expected %s, got %s", types.Version, versionMetadata.FeeVersion)
}

im.keeper.SetFeeEnabled(ctx, portID, channelID)

// call underlying app's OnChanOpenTry callback with the app versions
appVersion, err := im.app.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, counterparty, versionMetadata.AppVersion)
if err != nil {
return "", err
}

versionMetadata.AppVersion = appVersion
versionBytes, err := types.ModuleCdc.MarshalJSON(&versionMetadata)
if err != nil {
return "", err
}

return string(versionBytes), nil
return counterpartyVersion, nil
}

// OnChanOpenAck implements the IBCMiddleware interface
Expand Down Expand Up @@ -487,7 +465,7 @@ func (IBCMiddleware) WrapVersion(cbVersion, underlyingAppVersion string) string
return string(versionBytes)
}

// UnwrapVersionUnsafe attempts to unmarshal the version string into a ics29 version. An error is returned if unsuccessful.
// UnwrapVersionUnsafe attempts to unmarshal the version string into a ics29 version. An error is returned if unsuccessful.
func (IBCMiddleware) UnwrapVersionUnsafe(version string) (string, string, error) {
metadata, err := types.MetadataFromVersion(version)
if err != nil {
Expand Down
5 changes: 2 additions & 3 deletions modules/apps/29-fee/ibc_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,9 @@ func (suite *FeeTestSuite) TestOnChanOpenTry() {
Version: tc.cpVersion,
}

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.MockFeePort)
suite.Require().NoError(err)
// suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.AppRouter.HandshakeRoute(ibctesting.MockFeePort)
suite.Require().True(ok)

_, err = cbs.OnChanOpenTry(suite.chainA.GetContext(), channel.Ordering, channel.ConnectionHops,
Expand Down
8 changes: 4 additions & 4 deletions modules/apps/callbacks/ibc_middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ func (IBCMiddleware) processCallback(
return err
}

// OnChanOpenInit defers to the underlying application
// OnChanOpenInit is a no-op for the callbacks middleware.
func (IBCMiddleware) OnChanOpenInit(
ctx sdk.Context,
channelOrdering channeltypes.Order,
Expand All @@ -319,16 +319,16 @@ func (IBCMiddleware) OnChanOpenInit(
return "", nil
}

// OnChanOpenTry defers to the underlying application
func (im IBCMiddleware) OnChanOpenTry(
// OnChanOpenTry is a no-op for the callbacks middleware.
func (IBCMiddleware) OnChanOpenTry(
ctx sdk.Context,
channelOrdering channeltypes.Order,
connectionHops []string, portID,
channelID string,
counterparty channeltypes.Counterparty,
counterpartyVersion string,
) (string, error) {
return im.app.OnChanOpenTry(ctx, channelOrdering, connectionHops, portID, channelID, counterparty, counterpartyVersion)
return "", nil
}

// OnChanOpenAck defers to the underlying application
Expand Down
6 changes: 2 additions & 4 deletions modules/apps/callbacks/ibc_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,12 +166,10 @@ func (s *CallbacksTestSuite) TestSendPacket() {
s.Run(tc.name, func() {
s.SetupTransferTest()

cbs, ok := GetSimApp(s.chainA).IBCKeeper.PortKeeper.AppRoute(transfertypes.ModuleName)
cbs, ok := GetSimApp(s.chainA).IBCKeeper.PortKeeper.AppRouter.HandshakeRoute(transfertypes.ModuleName)
s.Require().True(ok)

s.Require().Len(cbs, 1, "expected 1 legacy module")

legacyModule, ok := cbs[0].(*porttypes.LegacyIBCModule)
legacyModule, ok := cbs.(*porttypes.LegacyIBCModule)
s.Require().True(ok, "expected there to be a single legacy ibc module")

legacyModuleCbs := legacyModule.GetCallbacks()
Expand Down
7 changes: 5 additions & 2 deletions modules/apps/callbacks/testing/simapp/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,21 +527,26 @@ func NewSimApp(
var icaControllerStack porttypes.ClassicIBCModule
icaControllerStack = ibcmock.NewIBCModule(&mockModule, ibcmock.NewIBCApp("", scopedICAMockKeeper))
ibcAppRouter.AddRoute(icacontrollertypes.SubModuleName, icaControllerStack)
ibcAppRouter.AddRoute(ibcmock.ModuleName+icacontrollertypes.SubModuleName, icaControllerStack)
app.ICAAuthModule, ok = icaControllerStack.(ibcmock.IBCModule)
if !ok {
panic(fmt.Errorf("cannot convert %T to %T", icaControllerStack, app.ICAAuthModule))
}
icaControllerStack = icacontroller.NewIBCMiddlewareWithAuth(icaControllerStack, app.ICAControllerKeeper)
ibcAppRouter.AddRoute(icacontrollertypes.SubModuleName, icaControllerStack)
ibcAppRouter.AddRoute(ibcmock.ModuleName+icacontrollertypes.SubModuleName, icaControllerStack)
icaControllerStack = ibccallbacks.NewIBCMiddleware(icaControllerStack, app.IBCFeeKeeper, app.MockContractKeeper, maxCallbackGas)
ibcAppRouter.AddRoute(icacontrollertypes.SubModuleName, icaControllerStack)
ibcAppRouter.AddRoute(ibcmock.ModuleName+icacontrollertypes.SubModuleName, icaControllerStack)

var icaICS4Wrapper porttypes.ICS4Wrapper
icaICS4Wrapper, ok = icaControllerStack.(porttypes.ICS4Wrapper)
if !ok {
panic(fmt.Errorf("cannot convert %T to %T", icaControllerStack, icaICS4Wrapper))
}
icaControllerStack = ibcfee.NewIBCMiddleware(icaControllerStack, app.IBCFeeKeeper)
ibcAppRouter.AddRoute(icacontrollertypes.SubModuleName, icaControllerStack)
ibcAppRouter.AddRoute(ibcmock.ModuleName+icacontrollertypes.SubModuleName, icaControllerStack)
// Since the callbacks middleware itself is an ics4wrapper, it needs to be passed to the ica controller keeper
app.ICAControllerKeeper.WithICS4Wrapper(icaICS4Wrapper)

Expand All @@ -563,8 +568,6 @@ func NewSimApp(
AddRoute(icahosttypes.SubModuleName, icaHostStack).
AddRoute(ibcmock.ModuleName+icacontrollertypes.SubModuleName, icaControllerStack) // ica with mock auth module stack route to ica (top level of middleware stack)

ibcAppRouter.AddRoute(ibcmock.ModuleName+icacontrollertypes.SubModuleName, icaControllerStack)

// Create Mock IBC Fee module stack for testing
// SendPacket, mock module cannot send packets

Expand Down
5 changes: 1 addition & 4 deletions modules/apps/transfer/ibc_module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,10 +180,7 @@ func (suite *TransferTestSuite) TestOnChanOpenTry() {
}
counterpartyVersion = types.V2

module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), ibctesting.TransferPort)
suite.Require().NoError(err)

cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.Route(module)
cbs, ok := suite.chainA.App.GetIBCKeeper().PortKeeper.AppRouter.HandshakeRoute(ibctesting.TransferPort)
suite.Require().True(ok)

tc.malleate() // explicitly change fields in channel and testChannel
Expand Down
57 changes: 44 additions & 13 deletions modules/core/05-port/types/ibc_legacy_module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,50 @@ func (im *LegacyIBCModule) OnChanOpenInit(
return reconstructVersion(im.cbs, negotiatedVersions)
}

// OnChanOpenTry implements the IBCModule interface.
func (im *LegacyIBCModule) OnChanOpenTry(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is almost an exact duplicate, I played around with trying to generalize the Init/Try functions, but it ended up looking even uglier, happy to revisit this idea, but I think it's fine as is for now unless someone has a suggestion for removing the duplication.

ctx sdk.Context,
order channeltypes.Order,
connectionHops []string,
portID,
channelID string,
counterparty channeltypes.Counterparty,
counterpartyVersion string,
) (string, error) {
negotiatedVersions := make([]string, len(im.cbs))

for i := len(im.cbs) - 1; i >= 0; i-- {
cbVersion := counterpartyVersion

// To maintain backwards compatibility, we must handle two cases:
// - relayer provides empty version (use default versions)
// - relayer provides version which chooses to not enable a middleware
//
// If an application is a VersionWrapper which means it modifies the version string
// and the version string is non-empty (don't use default), then the application must
// attempt to unmarshal the version using the UnwrapVersionUnsafe interface function.
// If it is unsuccessful, no callback will occur to this application as the version
// indicates it should be disabled.
if wrapper, ok := im.cbs[i].(VersionWrapper); ok && strings.TrimSpace(counterpartyVersion) != "" {
appVersion, underlyingAppVersion, err := wrapper.UnwrapVersionUnsafe(counterpartyVersion)
if err != nil {
// middleware disabled
negotiatedVersions[i] = ""
continue
}
cbVersion, counterpartyVersion = appVersion, underlyingAppVersion
}

negotiatedVersion, err := im.cbs[i].OnChanOpenTry(ctx, order, connectionHops, portID, channelID, counterparty, cbVersion)
if err != nil {
return "", errorsmod.Wrapf(err, "channel open try callback failed for port ID: %s, channel ID: %s", portID, channelID)
}
negotiatedVersions[i] = negotiatedVersion
}

return reconstructVersion(im.cbs, negotiatedVersions)
}

// reconstructVersion will generate the channel version by applying any version wrapping as necessary.
// Version wrapping will only occur if the negotiated version is non=empty and the application is a VersionWrapper.
func reconstructVersion(cbs []ClassicIBCModule, negotiatedVersions []string) (string, error) {
Expand All @@ -90,19 +134,6 @@ func reconstructVersion(cbs []ClassicIBCModule, negotiatedVersions []string) (st
return version, nil
}

// OnChanOpenTry implements the IBCModule interface.
func (LegacyIBCModule) OnChanOpenTry(
ctx sdk.Context,
order channeltypes.Order,
connectionHops []string,
portID,
channelID string,
counterparty channeltypes.Counterparty,
counterpartyVersion string,
) (string, error) {
return "", nil
}

// OnChanOpenAck implements the IBCModule interface
func (LegacyIBCModule) OnChanOpenAck(
ctx sdk.Context,
Expand Down
2 changes: 1 addition & 1 deletion modules/core/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ func (k *Keeper) ChannelOpenTry(goCtx context.Context, msg *channeltypes.MsgChan
ctx := sdk.UnwrapSDKContext(goCtx)

// Retrieve application callbacks from router
cbs, ok := k.PortKeeper.Route(msg.PortId)
cbs, ok := k.PortKeeper.AppRouter.HandshakeRoute(msg.PortId)
if !ok {
ctx.Logger().Error("channel open try failed", "port-id", msg.PortId, "error", errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", msg.PortId))
return nil, errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", msg.PortId)
Expand Down
9 changes: 5 additions & 4 deletions testing/simapp/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,22 +480,27 @@ func NewSimApp(
var icaControllerStack porttypes.ClassicIBCModule
icaControllerStack = ibcmock.NewIBCModule(&mockModule, ibcmock.NewIBCApp("", scopedICAMockKeeper))
ibcAppRouter.AddRoute(icacontrollertypes.SubModuleName, icaControllerStack)
ibcAppRouter.AddRoute(ibcmock.ModuleName+icacontrollertypes.SubModuleName, icaControllerStack)
var ok bool
app.ICAAuthModule, ok = icaControllerStack.(ibcmock.IBCModule)
if !ok {
panic(fmt.Errorf("cannot convert %T into %T", icaControllerStack, app.ICAAuthModule))
}
icaControllerStack = icacontroller.NewIBCMiddlewareWithAuth(icaControllerStack, app.ICAControllerKeeper)
ibcAppRouter.AddRoute(icacontrollertypes.SubModuleName, icaControllerStack)
ibcAppRouter.AddRoute(ibcmock.ModuleName+icacontrollertypes.SubModuleName, icaControllerStack)
icaControllerStack = ibcfee.NewIBCMiddleware(icaControllerStack, app.IBCFeeKeeper)
ibcAppRouter.AddRoute(icacontrollertypes.SubModuleName, icaControllerStack)
ibcAppRouter.AddRoute(ibcmock.ModuleName+icacontrollertypes.SubModuleName, icaControllerStack)

// RecvPacket, message that originates from core IBC and goes down to app, the flow is:
// channel.RecvPacket -> fee.OnRecvPacket -> icaHost.OnRecvPacket

var icaHostStack porttypes.ClassicIBCModule
icaHostStack = icahost.NewIBCModule(app.ICAHostKeeper)
ibcAppRouter.AddRoute(icahosttypes.SubModuleName, icaHostStack)
icaHostStack = ibcfee.NewIBCMiddleware(icaHostStack, app.IBCFeeKeeper)
ibcAppRouter.AddRoute(icahosttypes.SubModuleName, icaHostStack)

// Add host, controller & ica auth modules to IBC router
ibcRouter.
Expand All @@ -506,10 +511,6 @@ func NewSimApp(
AddRoute(icahosttypes.SubModuleName, icaHostStack).
AddRoute(ibcmock.ModuleName+icacontrollertypes.SubModuleName, icaControllerStack) // ica with mock auth module stack route to ica (top level of middleware stack)

ibcAppRouter.
AddRoute(icahosttypes.SubModuleName, icaHostStack).
AddRoute(ibcmock.ModuleName+icacontrollertypes.SubModuleName, icaControllerStack)

// Create Mock IBC Fee module stack for testing
// SendPacket, mock module cannot send packets

Expand Down
Loading