From 4d62aa3fbd70199a965463b5e35e936016da9e68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Fri, 28 Jan 2022 12:12:46 +0100 Subject: [PATCH 1/5] fix build and relay.go tests --- .../27-interchain-accounts/host/ibc_module.go | 11 ++-- .../host/keeper/relay.go | 64 +++++++++++++------ .../host/keeper/relay_test.go | 4 +- 3 files changed, 52 insertions(+), 27 deletions(-) diff --git a/modules/apps/27-interchain-accounts/host/ibc_module.go b/modules/apps/27-interchain-accounts/host/ibc_module.go index 15606eb9e74..04a3f8c6b13 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 = channeltypes.NewErrorAcknowledgement(icatypes.AcknowledgementError) - + txResponse, err := im.keeper.OnRecvPacket(ctx, packet) + if err != nil { // Emit an event including the error msg keeper.EmitWriteErrorAcknowledgementEvent(ctx, packet, err) + + return channeltypes.NewErrorAcknowledgement(icatypes.AcknowledgementError) } // 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/keeper/relay.go b/modules/apps/27-interchain-accounts/host/keeper/relay.go index 80d434309f4..9d0c24c81fb 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/relay.go +++ b/modules/apps/27-interchain-accounts/host/keeper/relay.go @@ -3,61 +3,84 @@ 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) { if err := k.authenticateTx(ctx, msgs, 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 @@ -84,20 +107,21 @@ func (k Keeper) authenticateTx(ctx sdk.Context, msgs []sdk.Msg, portID string) e 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 37cefe34bba..2ccf9e0078e 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) } }) } From 800fa566c429ebb923924040a2dded6eab2dcfcc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 2 Feb 2022 12:08:06 +0100 Subject: [PATCH 2/5] chore: add tests OnRecvPacket test now checks for the expected ack format Added an indicator test to ensure the inclusion of abci.ResponseDeliverTx.Data in consensus --- .../host/ibc_module_test.go | 87 ++++++++++++++++++- 1 file changed, 86 insertions(+), 1 deletion(-) 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 37c81bfd955..cc083c18bd5 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" @@ -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()) + } }) } @@ -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) +} From 520af3757da757f3fa5f7e420d9a3d6efc37d03a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 2 Feb 2022 12:37:59 +0100 Subject: [PATCH 3/5] add developer docs --- .../interchain-accounts/auth-modules.md | 115 ++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/docs/app-modules/interchain-accounts/auth-modules.md b/docs/app-modules/interchain-accounts/auth-modules.md index 4d8b126dd2f..523de1697ff 100644 --- a/docs/app-modules/interchain-accounts/auth-modules.md +++ b/docs/app-modules/interchain-accounts/auth-modules.md @@ -210,6 +210,121 @@ 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). From d7673b5039dbd25627812bb8a6df33d396bdd327 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 2 Feb 2022 12:42:22 +0100 Subject: [PATCH 4/5] fix spacing --- docs/app-modules/interchain-accounts/auth-modules.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/app-modules/interchain-accounts/auth-modules.md b/docs/app-modules/interchain-accounts/auth-modules.md index 523de1697ff..c08243935d7 100644 --- a/docs/app-modules/interchain-accounts/auth-modules.md +++ b/docs/app-modules/interchain-accounts/auth-modules.md @@ -252,6 +252,7 @@ A router could be used, or more simply a switch statement. if err := proto.Unmarshal(msgData.Data, msgResponse}; err != nil { return err } + handleBankSendMsg(msgResponse) case stakingtypes.MsgDelegate: @@ -259,6 +260,7 @@ A router could be used, or more simply a switch statement. if err := proto.Unmarshal(msgData.Data, msgResponse}; err != nil { return err } + handleStakingDelegateMsg(msgResponse) case transfertypes.MsgTransfer: @@ -266,6 +268,7 @@ A router could be used, or more simply a switch statement. if err := proto.Unmarshal(msgData.Data, msgResponse}; err != nil { return err } + handleIBCTransferMsg(msgResponse) default: From 379a8a3776b45e2604f3cdc98852c226e1435ec7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 2 Feb 2022 12:45:56 +0100 Subject: [PATCH 5/5] align code left --- .../interchain-accounts/auth-modules.md | 129 +++++++++--------- 1 file changed, 64 insertions(+), 65 deletions(-) diff --git a/docs/app-modules/interchain-accounts/auth-modules.md b/docs/app-modules/interchain-accounts/auth-modules.md index c08243935d7..b87265d4e28 100644 --- a/docs/app-modules/interchain-accounts/auth-modules.md +++ b/docs/app-modules/interchain-accounts/auth-modules.md @@ -220,76 +220,75 @@ If the controller chain is connected to a host chain using the host module on ib Begin by unmarshaling the acknowledgement into sdk.TxMsgData: ```go - txMsgData := &sdk.TxMsgData{} - if err := proto.Unmarshal(ack.Acknowledgement(), txMsgData); err != nil { - return err - } +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 - } +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 - } +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) + handleBankSendMsg(msgResponse) - case stakingtypes.MsgDelegate: - msgResponse := &stakingtypes.MsgDelegateResponse{} - if err := proto.Unmarshal(msgData.Data, msgResponse}; err != nil { - return err - } +case stakingtypes.MsgDelegate: + msgResponse := &stakingtypes.MsgDelegateResponse{} + if err := proto.Unmarshal(msgData.Data, msgResponse}; err != nil { + return err + } - handleStakingDelegateMsg(msgResponse) + handleStakingDelegateMsg(msgResponse) - case transfertypes.MsgTransfer: - msgResponse := &transfertypes.MsgTransferResponse{} - if err := proto.Unmarshal(msgData.Data, msgResponse}; err != nil { - return err - } +case transfertypes.MsgTransfer: + msgResponse := &transfertypes.MsgTransferResponse{} + if err := proto.Unmarshal(msgData.Data, msgResponse}; err != nil { + return err + } - handleIBCTransferMsg(msgResponse) + handleIBCTransferMsg(msgResponse) - default: - return - } +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 - } +... +// 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. @@ -297,35 +296,35 @@ 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 - } +func handleAny(any *codectypes.Any) error { +switch any.TypeURL { +case banktypes.MsgSend: + msgResponse, err := unpackBankMsgSendResponse(any) + if err != nil { + return err + } - handleBankSendMsg(msgResponse) + handleBankSendMsg(msgResponse) - case stakingtypes.MsgDelegate: -msgResponse, err := unpackStakingDelegateResponse(any) - if err != nil { - return err - } +case stakingtypes.MsgDelegate: + msgResponse, err := unpackStakingDelegateResponse(any) + if err != nil { + return err + } - handleStakingDelegateMsg(msgResponse) + handleStakingDelegateMsg(msgResponse) case transfertypes.MsgTransfer: -msgResponse, err := unpackIBCTransferMsgResponse(any) - if err != nil { - return err - } + msgResponse, err := unpackIBCTransferMsgResponse(any) + if err != nil { + return err + } - handleIBCTransferMsg(msgResponse) + handleIBCTransferMsg(msgResponse) - default: - return - } +default: + return +} ``` ### Integration into `app.go` file