revert icacallbacks refactor (#855)

Co-authored-by: vish-stride <[email protected]>

CHANGELOG.md

@@ -43,6 +43,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 <!-- GH ACTIONS TEMPLATE - INSERT NEW VERSION HERE -->
 ### Changelog
 
+## [v12.0.0](https://github.com/Stride-Labs/stride/releases/tag/v12.0.0) - 2023-07-15
+
+### On-Chain changes
+1. Add code required for ICS-consumer-migration ([#811](https://github.com/Stride-Labs/stride/pull/811))
+
+### Off-Chain changes
+1. Add helper scripts and tester files for ICS-consumer-migration ([#811](https://github.com/Stride-Labs/stride/pull/811))
+
 ## [v11.0.0](https://github.com/Stride-Labs/stride/releases/tag/v11.0.0) - 2023-06-23
 
 ### On-Chain changes

app/app.go

@@ -576,6 +576,7 @@ func NewStrideApp(
 	)
 
 	stakeibcModule := stakeibcmodule.NewAppModule(appCodec, app.StakeibcKeeper, app.AccountKeeper, app.BankKeeper)
+	stakeibcIBCModule := stakeibcmodule.NewIBCModule(app.StakeibcKeeper)
 
 	app.AutopilotKeeper = *autopilotkeeper.NewKeeper(
 		appCodec,
@@ -618,13 +619,19 @@ func NewStrideApp(
 	epochsModule := epochsmodule.NewAppModule(appCodec, app.EpochsKeeper)
 
 	icacallbacksModule := icacallbacksmodule.NewAppModule(appCodec, app.IcacallbacksKeeper, app.AccountKeeper, app.BankKeeper)
-	icacallbacksIBCModule := icacallbacksmodule.NewIBCModule(app.IcacallbacksKeeper)
 
-	// Register IBC calllbacks
-	if err := app.IcacallbacksKeeper.SetICACallbacks(
-		app.StakeibcKeeper.Callbacks(),
-		app.RecordsKeeper.Callbacks(),
-	); err != nil {
+	// Register ICA calllbacks
+	// NOTE: The icacallbacks struct implemented below provides a mapping from ICA channel owner to ICACallback handler,
+	// where the callback handler stores and routes to the various callback functions for a particular module.
+	// However, as of ibc-go v6, the icacontroller module owns the ICA channel. A consequence of this is that there can
+	// be no more than one module that implements ICA callbacks. Should we add an new module with ICA support in the future,
+	// we'll need to refactor this
+	err = app.IcacallbacksKeeper.SetICACallbackHandler(icacontrollertypes.SubModuleName, app.StakeibcKeeper.ICACallbackHandler())
+	if err != nil {
+		return nil
+	}
+	err = app.IcacallbacksKeeper.SetICACallbackHandler(ibctransfertypes.ModuleName, app.RecordsKeeper.ICACallbackHandler())
+	if err != nil {
 		return nil
 	}
 
@@ -641,23 +648,20 @@ func NewStrideApp(
 		app.MsgServiceRouter(),
 	)
 	icaModule := ica.NewAppModule(&app.ICAControllerKeeper, &app.ICAHostKeeper)
-
 	// Create the middleware stacks
 	// Stack one (ICAHost Stack) contains:
 	// - IBC
 	// - ICAHost
 	// - base app
 	icaHostIBCModule := icahost.NewIBCModule(app.ICAHostKeeper)
 
-	// Stack two (ICACallbacks Stack) contains
+	// Stack two (Stakeibc Stack) contains
 	// - IBC
 	// - ICA
 	// - stakeibc
-	// - ICACallbacks
 	// - base app
-	var icacallbacksStack porttypes.IBCModule = icacallbacksIBCModule
-	icacallbacksStack = stakeibcmodule.NewIBCMiddleware(icacallbacksStack, app.StakeibcKeeper)
-	icacallbacksStack = icacontroller.NewIBCMiddleware(icacallbacksStack, app.ICAControllerKeeper)
+	var stakeibcStack porttypes.IBCModule = stakeibcIBCModule
+	stakeibcStack = icacontroller.NewIBCMiddleware(stakeibcStack, app.ICAControllerKeeper)
 
 	// Stack three contains
 	// - IBC
@@ -672,12 +676,20 @@ func NewStrideApp(
 	transferStack = autopilot.NewIBCModule(app.AutopilotKeeper, transferStack)
 
 	// Create static IBC router, add transfer route, then set and seal it
+	// Two routes are included for the ICAController because of the following procedure when registering an ICA
+	//     1. RegisterInterchainAccount binds the new portId to the icacontroller module and initiates a channel opening
+	//     2. MsgChanOpenInit is invoked from the IBC message server.  The message server identifies that the
+	//        icacontroller module owns the portID and routes to the stakeibc stack (the "icacontroller" route below)
+	//     3. The stakeibc stack works top-down, first in the ICAController's OnChanOpenInit, and then in stakeibc's OnChanOpenInit
+	//     4. In stakeibc's OnChanOpenInit, the stakeibc module steals the portId from the icacontroller module
+	//     5. Now in OnChanOpenAck and any other subsequent IBC callback, the message server will identify
+	//        the portID owner as stakeibc and route to the same stakeibcStack, this time using the "stakeibc" route instead
 	ibcRouter := porttypes.NewRouter()
 	ibcRouter.
 		// ICAHost Stack
 		AddRoute(icahosttypes.SubModuleName, icaHostIBCModule).
-		// ICACallbacks Stack
-		AddRoute(icacontrollertypes.SubModuleName, icacallbacksStack).
+		// Stakeibc Stack
+		AddRoute(icacontrollertypes.SubModuleName, stakeibcStack).
 		// Transfer stack
 		AddRoute(ibctransfertypes.ModuleName, transferStack).
 		// Consumer stack

dockernet/config.sh

@@ -157,7 +157,7 @@ STRIDE_MAIN_CMD="$STRIDE_BINARY --home $DOCKERNET_HOME/state/${STRIDE_NODE_PREFI
 # GAIA 
 GAIA_CHAIN_ID=GAIA
 GAIA_NODE_PREFIX=gaia
-GAIA_NUM_NODES=1
+GAIA_NUM_NODES=3
 GAIA_BINARY="$DOCKERNET_HOME/../build/gaiad"
 GAIA_VAL_PREFIX=gval
 GAIA_REV_ACCT=grev1

x/icacallbacks/keeper/keeper.go

@@ -26,7 +26,7 @@ type (
 		storeKey     storetypes.StoreKey
 		memKey       storetypes.StoreKey
 		paramstore   paramtypes.Subspace
-		icacallbacks map[string]types.ICACallback
+		icacallbacks map[string]types.ICACallbackHandler
 		IBCKeeper    ibckeeper.Keeper
 	}
 )
@@ -48,7 +48,7 @@ func NewKeeper(
 		storeKey:     storeKey,
 		memKey:       memKey,
 		paramstore:   ps,
-		icacallbacks: make(map[string]types.ICACallback),
+		icacallbacks: make(map[string]types.ICACallbackHandler),
 		IBCKeeper:    ibcKeeper,
 	}
 }
@@ -57,38 +57,77 @@ func (k Keeper) Logger(ctx sdk.Context) log.Logger {
 	return ctx.Logger().With("module", fmt.Sprintf("x/%s", types.ModuleName))
 }
 
-func (k Keeper) SetICACallbacks(moduleCallbacks ...types.ModuleCallbacks) error {
-	for _, callbacks := range moduleCallbacks {
-		for _, callback := range callbacks {
-			if _, found := k.icacallbacks[callback.CallbackId]; found {
-				return fmt.Errorf("callback for ID %s already registered", callback.CallbackId)
-			}
-			k.icacallbacks[callback.CallbackId] = callback
-		}
+// Should we add a `AddICACallback`
+func (k *Keeper) SetICACallbackHandler(module string, handler types.ICACallbackHandler) error {
+	_, found := k.icacallbacks[module]
+	if found {
+		return fmt.Errorf("callback handler already set for %s", module)
 	}
+	k.icacallbacks[module] = handler.RegisterICACallbacks()
 	return nil
 }
 
-func (k Keeper) CallRegisteredICACallback(ctx sdk.Context, packet channeltypes.Packet, ackResponse *types.AcknowledgementResponse) error {
-	// Get the callback key and associated callback data from the packet
-	callbackDataKey := types.PacketID(packet.GetSourcePort(), packet.GetSourceChannel(), packet.Sequence)
+func (k *Keeper) GetICACallbackHandler(module string) (types.ICACallbackHandler, error) {
+	callback, found := k.icacallbacks[module]
+	if !found {
+		return nil, fmt.Errorf("no callback handler found for %s", module)
+	}
+	return callback, nil
+}
+
+func (k Keeper) GetCallbackDataFromPacket(ctx sdk.Context, modulePacket channeltypes.Packet, callbackDataKey string) (cbd *types.CallbackData, found bool) {
+	// get the relevant module from the channel and port
+	portID := modulePacket.GetSourcePort()
+	channelID := modulePacket.GetSourceChannel()
+	// fetch the callback data
 	callbackData, found := k.GetCallbackData(ctx, callbackDataKey)
 	if !found {
-		k.Logger(ctx).Info(fmt.Sprintf("callback data not found for portID: %s, channelID: %s, sequence: %d",
-			packet.SourcePort, packet.SourceChannel, packet.Sequence))
-		return nil
+		k.Logger(ctx).Info(fmt.Sprintf("callback data not found for portID: %s, channelID: %s, sequence: %d", portID, channelID, modulePacket.Sequence))
+		return nil, false
+	} else {
+		k.Logger(ctx).Info(fmt.Sprintf("callback data found for portID: %s, channelID: %s, sequence: %d", portID, channelID, modulePacket.Sequence))
+	}
+	return &callbackData, true
+}
+
+func (k Keeper) GetICACallbackHandlerFromPacket(ctx sdk.Context, modulePacket channeltypes.Packet) (*types.ICACallbackHandler, error) {
+	module, _, err := k.IBCKeeper.ChannelKeeper.LookupModuleByChannel(ctx, modulePacket.GetSourcePort(), modulePacket.GetSourceChannel())
+	if err != nil {
+		k.Logger(ctx).Error(fmt.Sprintf("error LookupModuleByChannel for portID: %s, channelID: %s, sequence: %d", modulePacket.GetSourcePort(), modulePacket.GetSourceChannel(), modulePacket.Sequence))
+		return nil, err
 	}
+	// fetch the callback function
+	callbackHandler, err := k.GetICACallbackHandler(module)
+	if err != nil {
+		return nil, errorsmod.Wrapf(types.ErrCallbackHandlerNotFound, "Callback handler does not exist for module %s | err: %s", module, err.Error())
+	}
+	return &callbackHandler, nil
+}
 
-	// If there's an associated callback function, execute it
-	callback, found := k.icacallbacks[callbackData.CallbackId]
+func (k Keeper) CallRegisteredICACallback(ctx sdk.Context, modulePacket channeltypes.Packet, ackResponse *types.AcknowledgementResponse) error {
+	callbackDataKey := types.PacketID(modulePacket.GetSourcePort(), modulePacket.GetSourceChannel(), modulePacket.Sequence)
+	callbackData, found := k.GetCallbackDataFromPacket(ctx, modulePacket, callbackDataKey)
 	if !found {
-		k.Logger(ctx).Info(fmt.Sprintf("No associated callback with callback data %v", callbackData))
 		return nil
 	}
-	if err := callback.CallbackFunc(ctx, packet, ackResponse, callbackData.CallbackArgs); err != nil {
-		errMsg := fmt.Sprintf("Error occured while calling ICACallback (%s) | err: %s", callbackData.CallbackId, err.Error())
-		k.Logger(ctx).Error(errMsg)
-		return errorsmod.Wrapf(types.ErrCallbackFailed, errMsg)
+	callbackHandler, err := k.GetICACallbackHandlerFromPacket(ctx, modulePacket)
+	if err != nil {
+		k.Logger(ctx).Error(fmt.Sprintf("GetICACallbackHandlerFromPacket %s", err.Error()))
+		return err
+	}
+
+	// call the callback
+	if (*callbackHandler).HasICACallback(callbackData.CallbackId) {
+		k.Logger(ctx).Info(fmt.Sprintf("Calling callback for %s", callbackData.CallbackId))
+		// if acknowledgement is empty, then it is a timeout
+		err := (*callbackHandler).CallICACallback(ctx, callbackData.CallbackId, modulePacket, ackResponse, callbackData.CallbackArgs)
+		if err != nil {
+			errMsg := fmt.Sprintf("Error occured while calling ICACallback (%s) | err: %s", callbackData.CallbackId, err.Error())
+			k.Logger(ctx).Error(errMsg)
+			return errorsmod.Wrapf(types.ErrCallbackFailed, errMsg)
+		}
+	} else {
+		k.Logger(ctx).Error(fmt.Sprintf("Callback %v has no associated callback", callbackData))
 	}
 
 	// remove the callback data