diff --git a/docs/app-modules/interchain-accounts/auth-modules.md b/docs/app-modules/interchain-accounts/auth-modules.md index 4d8b126dd2f..b87265d4e28 100644 --- a/docs/app-modules/interchain-accounts/auth-modules.md +++ b/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). diff --git a/modules/apps/27-interchain-accounts/host/ibc_module.go b/modules/apps/27-interchain-accounts/host/ibc_module.go index cec21d258c3..be302e09151 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module.go +++ b/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 diff --git a/modules/apps/27-interchain-accounts/host/ibc_module_test.go b/modules/apps/27-interchain-accounts/host/ibc_module_test.go index 0b312d34dd6..56baa70e847 100644 --- a/modules/apps/27-interchain-accounts/host/ibc_module_test.go +++ b/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) +} diff --git a/modules/apps/27-interchain-accounts/host/keeper/relay.go b/modules/apps/27-interchain-accounts/host/keeper/relay.go index 803045794ad..8610a50f5e5 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/relay.go +++ b/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 } diff --git a/modules/apps/27-interchain-accounts/host/keeper/relay_test.go b/modules/apps/27-interchain-accounts/host/keeper/relay_test.go index f128804b415..fda31c34ef0 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/relay_test.go +++ b/modules/apps/27-interchain-accounts/host/keeper/relay_test.go @@ -424,12 +424,14 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() { 0, ) - err = suite.chainB.GetSimApp().ICAHostKeeper.OnRecvPacket(suite.chainB.GetContext(), packet) + txResponse, err := suite.chainB.GetSimApp().ICAHostKeeper.OnRecvPacket(suite.chainB.GetContext(), packet) if tc.expPass { suite.Require().NoError(err) + suite.Require().NotNil(txResponse) } else { suite.Require().Error(err) + suite.Require().Nil(txResponse) } }) }