refactor: include transaction response in ics27 channel acknowledgement

docs/app-modules/interchain-accounts/auth-modules.md

@@ -210,6 +210,123 @@ seq, err = keeper.icaControllerKeeper.SendTx(ctx, chanCap, portID, packetData, t
 The data within an `InterchainAccountPacketData` must be serialized using a format supported by the host chain. 
 If the host chain is using the ibc-go host chain submodule, `SerializeCosmosTx` should be used. If the `InterchainAccountPacketData.Data` is serialized using a format not support by the host chain, the packet will not be successfully received.  
 
+## `OnAcknowledgementPacket`
+
+Controller chains will be able to access the acknowledgement written into the host chain state once a relayer relays the acknowledgement. 
+The acknowledgement bytes will be passed to the auth module via the `OnAcknowledgementPacket` callback. 
+Auth modules are expected to know how to decode the acknowledgement. 
+
+If the controller chain is connected to a host chain using the host module on ibc-go, it may interpret the acknowledgement bytes as follows:
+
+Begin by unmarshaling the acknowledgement into sdk.TxMsgData:
+```go
+txMsgData := &sdk.TxMsgData{}
+if err := proto.Unmarshal(ack.Acknowledgement(), txMsgData); err != nil {
+    return err
+}
+```
+
+If the txMsgData.Data field is non nil, the host chain is using SDK version <= v0.45. 
+The auth module should interpret the txMsgData.Data as follows:
+
+```go
+switch len(txMsgData.Data) {
+case 0:
+    for _, msgData := range txMsgData.Data {
+        if err := handler(msgData); err != nil {
+            return err
+        }
+    }
+...
+}            
+```
+
+A handler will be needed to interpret what actions to perform based on the message type sent.
+A router could be used, or more simply a switch statement.
+
+```go
+func handler(msgData sdk.MsgData) error {
+switch msgData.TypeURL {
+case banktypes.MsgSend:
+    msgResponse := &banktypes.MsgSendResponse{}
+    if err := proto.Unmarshal(msgData.Data, msgResponse}; err != nil {
+        return err
+    }
+
+    handleBankSendMsg(msgResponse)
+
+case stakingtypes.MsgDelegate:
+    msgResponse := &stakingtypes.MsgDelegateResponse{}
+    if err := proto.Unmarshal(msgData.Data, msgResponse}; err != nil {
+        return err
+    }
+
+    handleStakingDelegateMsg(msgResponse)
+
+case transfertypes.MsgTransfer:
+    msgResponse := &transfertypes.MsgTransferResponse{}
+    if err := proto.Unmarshal(msgData.Data, msgResponse}; err != nil {
+        return err
+    }
+
+    handleIBCTransferMsg(msgResponse)
+
+default:
+    return
+}
+```
+
+If the txMsgData.Data is empty, the host chain is using SDK version > v0.45.
+The auth module should interpret the txMsgData.Responses as follows:
+
+```go
+...
+// switch statement from above continued
+default:
+    for _, any := range txMsgData.MsgResponses {
+        if err := handleAny(any); err != nil {
+            return err
+        }
+    }
+}
+``` 
+
+A handler will be needed to interpret what actions to perform based on the type url of the Any. 
+A router could be used, or more simply a switch statement. 
+It may be possible to deduplicate logic between `handler` and `handleAny`.
+
+```go
+func handleAny(any *codectypes.Any) error {
+switch any.TypeURL {
+case banktypes.MsgSend:
+    msgResponse, err := unpackBankMsgSendResponse(any)
+    if err != nil {
+        return err
+    }
+
+    handleBankSendMsg(msgResponse)
+
+case stakingtypes.MsgDelegate:
+    msgResponse, err := unpackStakingDelegateResponse(any)
+    if err != nil {
+        return err
+    }
+
+    handleStakingDelegateMsg(msgResponse)
+
+    case transfertypes.MsgTransfer:
+    msgResponse, err := unpackIBCTransferMsgResponse(any)
+    if err != nil {
+        return err
+    }
+
+    handleIBCTransferMsg(msgResponse)
+
+default:
+    return
+}
+```
+
 ### Integration into `app.go` file
 
 To integrate the authentication module into your chain, please follow the steps outlined above in [app.go integration](./integration.md#example-integration).

modules/apps/27-interchain-accounts/host/ibc_module.go

@@ -108,17 +108,16 @@ func (im IBCModule) OnRecvPacket(
 		return channeltypes.NewErrorAcknowledgement(types.ErrHostSubModuleDisabled.Error())
 	}
 
-	ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
-
-	if err := im.keeper.OnRecvPacket(ctx, packet); err != nil {
-		ack = types.NewErrorAcknowledgement(err)
-
+	txResponse, err := im.keeper.OnRecvPacket(ctx, packet)
+	if err != nil {
 		// Emit an event including the error msg
 		keeper.EmitWriteErrorAcknowledgementEvent(ctx, packet, err)
+
+		return types.NewErrorAcknowledgement(err)
 	}
 
 	// NOTE: acknowledgement will be written synchronously during IBC handler execution.
-	return ack
+	return channeltypes.NewResultAcknowledgement(txResponse)
 }
 
 // OnAcknowledgementPacket implements the IBCModule interface

modules/apps/27-interchain-accounts/host/ibc_module_test.go

@@ -7,8 +7,12 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
 	capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
+	"github.com/gogo/protobuf/proto"
 	"github.com/stretchr/testify/suite"
+	abcitypes "github.com/tendermint/tendermint/abci/types"
 	"github.com/tendermint/tendermint/crypto"
+	tmprotostate "github.com/tendermint/tendermint/proto/tendermint/state"
+	tmstate "github.com/tendermint/tendermint/state"
 
 	"github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/host/types"
 	icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types"
@@ -460,6 +464,22 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() {
 			}
 			packetData = icaPacketData.GetBytes()
 
+			// build expected ack
+			msgResponseBz, err := proto.Marshal(&banktypes.MsgSendResponse{})
+			suite.Require().NoError(err)
+
+			msgData := &sdk.MsgData{
+				MsgType: sdk.MsgTypeURL(msg),
+				Data:    msgResponseBz,
+			}
+
+			expectedTxResponse, err := proto.Marshal(&sdk.TxMsgData{
+				Data: []*sdk.MsgData{msgData},
+			})
+			suite.Require().NoError(err)
+
+			expectedAck := channeltypes.NewResultAcknowledgement(expectedTxResponse)
+
 			params := types.NewParams(true, []string{sdk.MsgTypeURL(msg)})
 			suite.chainB.GetSimApp().ICAHostKeeper.SetParams(suite.chainB.GetContext(), params)
 
@@ -478,7 +498,12 @@ func (suite *InterchainAccountsTestSuite) TestOnRecvPacket() {
 			suite.Require().True(ok)
 
 			ack := cbs.OnRecvPacket(suite.chainB.GetContext(), packet, nil)
-			suite.Require().Equal(tc.expAckSuccess, ack.Success())
+			if tc.expAckSuccess {
+				suite.Require().True(ack.Success())
+				suite.Require().Equal(expectedAck, ack)
+			} else {
+				suite.Require().False(ack.Success())
+			}
 
 		})
 	}
@@ -687,3 +712,63 @@ func (suite *InterchainAccountsTestSuite) TestControlAccountAfterChannelClose()
 	hasBalance = suite.chainB.GetSimApp().BankKeeper.HasBalance(suite.chainB.GetContext(), icaAddr, sdk.Coin{Denom: sdk.DefaultBondDenom, Amount: sdk.NewInt(0)})
 	suite.Require().True(hasBalance)
 }
+
+// The safety of including SDK MsgResponses in the acknowledgement rests
+// on the inclusion of the abcitypes.ResponseDeliverTx.Data in the
+// abcitypes.ResposneDeliverTx hash. If the abcitypes.ResponseDeliverTx.Data
+// gets removed from consensus they must no longer be used in the packet
+// acknowledgement.
+//
+// This test acts as an indicator that the abcitypes.ResponseDeliverTx.Data
+// may no longer be deterministic.
+func (suite *InterchainAccountsTestSuite) TestABCICodeDeterminism() {
+	msgResponseBz, err := proto.Marshal(&channeltypes.MsgChannelOpenInitResponse{})
+	suite.Require().NoError(err)
+
+	msgData := &sdk.MsgData{
+		MsgType: sdk.MsgTypeURL(&channeltypes.MsgChannelOpenInit{}),
+		Data:    msgResponseBz,
+	}
+
+	txResponse, err := proto.Marshal(&sdk.TxMsgData{
+		Data: []*sdk.MsgData{msgData},
+	})
+	suite.Require().NoError(err)
+
+	deliverTx := abcitypes.ResponseDeliverTx{
+		Data: txResponse,
+	}
+	responses := tmprotostate.ABCIResponses{
+		DeliverTxs: []*abcitypes.ResponseDeliverTx{
+			&deliverTx,
+		},
+	}
+
+	differentMsgResponseBz, err := proto.Marshal(&channeltypes.MsgRecvPacketResponse{})
+	suite.Require().NoError(err)
+
+	differentMsgData := &sdk.MsgData{
+		MsgType: sdk.MsgTypeURL(&channeltypes.MsgRecvPacket{}),
+		Data:    differentMsgResponseBz,
+	}
+
+	differentTxResponse, err := proto.Marshal(&sdk.TxMsgData{
+		Data: []*sdk.MsgData{differentMsgData},
+	})
+	suite.Require().NoError(err)
+
+	differentDeliverTx := abcitypes.ResponseDeliverTx{
+		Data: differentTxResponse,
+	}
+
+	differentResponses := tmprotostate.ABCIResponses{
+		DeliverTxs: []*abcitypes.ResponseDeliverTx{
+			&differentDeliverTx,
+		},
+	}
+
+	hash := tmstate.ABCIResponsesResultsHash(&responses)
+	differentHash := tmstate.ABCIResponsesResultsHash(&differentResponses)
+
+	suite.Require().NotEqual(hash, differentHash)
+}

modules/apps/27-interchain-accounts/host/keeper/relay.go

@@ -3,66 +3,89 @@ package keeper
 import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
+	"github.com/gogo/protobuf/proto"
 
 	"github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/host/types"
 	icatypes "github.com/cosmos/ibc-go/v3/modules/apps/27-interchain-accounts/types"
 	channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
 )
 
-// OnRecvPacket handles a given interchain accounts packet on a destination host chain
-func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet) error {
+// OnRecvPacket handles a given interchain accounts packet on a destination host chain.
+// If the transaction is successfully executed, the transaction response bytes will be returned.
+func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet) ([]byte, error) {
 	var data icatypes.InterchainAccountPacketData
 
 	if err := icatypes.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil {
 		// UnmarshalJSON errors are indeterminate and therefore are not wrapped and included in failed acks
-		return sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain account packet data")
+		return nil, sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain account packet data")
 	}
 
 	switch data.Type {
 	case icatypes.EXECUTE_TX:
 		msgs, err := icatypes.DeserializeCosmosTx(k.cdc, data.Data)
 		if err != nil {
-			return err
+			return nil, err
 		}
 
-		if err = k.executeTx(ctx, packet.SourcePort, packet.DestinationPort, packet.DestinationChannel, msgs); err != nil {
-			return err
+		txResponse, err := k.executeTx(ctx, packet.SourcePort, packet.DestinationPort, packet.DestinationChannel, msgs)
+		if err != nil {
+			return nil, err
 		}
 
-		return nil
+		return txResponse, nil
 	default:
-		return icatypes.ErrUnknownDataType
+		return nil, icatypes.ErrUnknownDataType
 	}
 }
 
-func (k Keeper) executeTx(ctx sdk.Context, sourcePort, destPort, destChannel string, msgs []sdk.Msg) error {
+// executeTx attempts to execute the provided transaction. It begins by authenticating the transaction signer.
+// If authentication succeeds, it does basic validation of the messages before attempting to deliver each message
+// into state. The state changes will only be committed if all messages in the transaction succeed. Thus the
+// execution of the transaction is atomic, all state changes are reverted if a single message fails.
+func (k Keeper) executeTx(ctx sdk.Context, sourcePort, destPort, destChannel string, msgs []sdk.Msg) ([]byte, error) {
 	channel, found := k.channelKeeper.GetChannel(ctx, destPort, destChannel)
 	if !found {
-		return channeltypes.ErrChannelNotFound
+		return nil, channeltypes.ErrChannelNotFound
 	}
 
 	if err := k.authenticateTx(ctx, msgs, channel.ConnectionHops[0], sourcePort); err != nil {
-		return err
+		return nil, err
+	}
+
+	txMsgData := &sdk.TxMsgData{
+		Data: make([]*sdk.MsgData, len(msgs)),
 	}
 
 	// CacheContext returns a new context with the multi-store branched into a cached storage object
 	// writeCache is called only if all msgs succeed, performing state transitions atomically
 	cacheCtx, writeCache := ctx.CacheContext()
-	for _, msg := range msgs {
+	for i, msg := range msgs {
 		if err := msg.ValidateBasic(); err != nil {
-			return err
+			return nil, err
+		}
+
+		msgResponse, err := k.executeMsg(cacheCtx, msg)
+		if err != nil {
+			return nil, err
 		}
 
-		if err := k.executeMsg(cacheCtx, msg); err != nil {
-			return err
+		txMsgData.Data[i] = &sdk.MsgData{
+			MsgType: sdk.MsgTypeURL(msg),
+			Data:    msgResponse,
 		}
+
 	}
 
 	// NOTE: The context returned by CacheContext() creates a new EventManager, so events must be correctly propagated back to the current context
 	ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events())
 	writeCache()
 
-	return nil
+	txResponse, err := proto.Marshal(txMsgData)
+	if err != nil {
+		return nil, sdkerrors.Wrap(err, "failed to marshal tx data")
+	}
+
+	return txResponse, nil
 }
 
 // authenticateTx ensures the provided msgs contain the correct interchain account signer address retrieved
@@ -89,20 +112,21 @@ func (k Keeper) authenticateTx(ctx sdk.Context, msgs []sdk.Msg, connectionID, po
 	return nil
 }
 
-// Attempts to get the message handler from the router and if found will then execute the message
-func (k Keeper) executeMsg(ctx sdk.Context, msg sdk.Msg) error {
+// Attempts to get the message handler from the router and if found will then execute the message.
+// If the message execution is successful, the proto marshaled message response will be returned.
+func (k Keeper) executeMsg(ctx sdk.Context, msg sdk.Msg) ([]byte, error) {
 	handler := k.msgRouter.Handler(msg)
 	if handler == nil {
-		return icatypes.ErrInvalidRoute
+		return nil, icatypes.ErrInvalidRoute
 	}
 
 	res, err := handler(ctx, msg)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	// NOTE: The sdk msg handler creates a new EventManager, so events must be correctly propagated back to the current context
 	ctx.EventManager().EmitEvents(res.GetEvents())
 
-	return nil
+	return res.Data, nil
 }