refactor: include transaction response in ics27 channel acknowledgement (#811)

## Description

ref: #701

---

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.

- [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [x] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [x] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing)
- [x] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [x] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [x] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [x] Re-reviewed `Files changed` in the Github PR explorer
- [x] Review `Codecov Report` in the comment section below once CI passes

(cherry picked from commit 6c48f7e)

Author: colin-axner

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"
@@ -458,6 +462,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)
 
@@ -476,7 +496,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())
+			}
 
 		})
 	}
@@ -685,3 +710,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
 }