ICA: Rename TxBody, Remove serialization logic from controller, intro…

…duce CosmosTx type (#474) * remove ICA TxBody type, use repeated Any in packet data * adjust SerializeCosmosTx, fix tests * apply self nits * add memo length validation
cosmos · Oct 12, 2021 · 3e9afcb · 3e9afcb
1 parent 0ae4a75
commit 3e9afcb
Show file tree

Hide file tree

Showing 8 changed files with 390 additions and 302 deletions.
diff --git a/docs/ibc/proto-docs.md b/docs/ibc/proto-docs.md
@@ -17,7 +17,7 @@
     - [Query](#ibc.applications.interchain_accounts.v1.Query)
 
 - [ibc/applications/interchain_accounts/v1/types.proto](#ibc/applications/interchain_accounts/v1/types.proto)
-    - [IBCTxBody](#ibc.applications.interchain_accounts.v1.IBCTxBody)
+    - [CosmosTx](#ibc.applications.interchain_accounts.v1.CosmosTx)
     - [InterchainAccountPacketData](#ibc.applications.interchain_accounts.v1.InterchainAccountPacketData)
 
     - [Type](#ibc.applications.interchain_accounts.v1.Type)
@@ -398,10 +398,10 @@ Query defines the gRPC querier service.
 
 
 
-<a name="ibc.applications.interchain_accounts.v1.IBCTxBody"></a>
+<a name="ibc.applications.interchain_accounts.v1.CosmosTx"></a>
 
-### IBCTxBody
-Body of a tx for an ics27 IBC packet
+### CosmosTx
+CosmosTx contains a list of sdk.Msg's. It should be used when sending transactions to an SDK host chain.
 
 
 | Field | Type | Label | Description |
@@ -416,7 +416,7 @@ Body of a tx for an ics27 IBC packet
 <a name="ibc.applications.interchain_accounts.v1.InterchainAccountPacketData"></a>
 
 ### InterchainAccountPacketData
-InterchainAccountPacketData is comprised of araw transaction,type of transaction and optional memo field.
+InterchainAccountPacketData is comprised of a raw transaction, type of transaction and optional memo field.
 
 
 | Field | Type | Label | Description |

diff --git a/modules/apps/27-interchain-accounts/keeper/keeper.go b/modules/apps/27-interchain-accounts/keeper/keeper.go
@@ -55,23 +55,24 @@ func NewKeeper(
 	}
 }
 
-// SerializeCosmosTx marshals data to bytes using the provided codec
-func (k Keeper) SerializeCosmosTx(cdc codec.BinaryCodec, msgs []sdk.Msg) ([]byte, error) {
+// SerializeCosmosTx serializes a slice of sdk.Msg's using the CosmosTx type. The sdk.Msg's are
+// packed into Any's and inserted into the Messages field of a CosmosTx. The proto marshaled CosmosTx
+// bytes are returned.
+func (k Keeper) SerializeCosmosTx(cdc codec.BinaryCodec, msgs []sdk.Msg) (bz []byte, err error) {
 	msgAnys := make([]*codectypes.Any, len(msgs))
 
 	for i, msg := range msgs {
-		var err error
 		msgAnys[i], err = codectypes.NewAnyWithValue(msg)
 		if err != nil {
 			return nil, err
 		}
 	}
 
-	txBody := &types.IBCTxBody{
+	cosmosTx := &types.CosmosTx{
 		Messages: msgAnys,
 	}
 
-	bz, err := cdc.Marshal(txBody)
+	bz, err = cdc.Marshal(cosmosTx)
 	if err != nil {
 		return nil, err
 	}

diff --git a/modules/apps/27-interchain-accounts/keeper/relay.go b/modules/apps/27-interchain-accounts/keeper/relay.go
@@ -12,7 +12,7 @@ import (
 
 // TODO: implement middleware functionality, this will allow us to use capabilities to
 // manage helper module access to owner addresses they do not have capabilities for
-func (k Keeper) TrySendTx(ctx sdk.Context, portID string, data interface{}, memo string) (uint64, error) {
+func (k Keeper) TrySendTx(ctx sdk.Context, portID string, icaPacketData types.InterchainAccountPacketData) (uint64, error) {
 	// Check for the active channel
 	activeChannelId, found := k.GetActiveChannel(ctx, portID)
 	if !found {
@@ -27,7 +27,7 @@ func (k Keeper) TrySendTx(ctx sdk.Context, portID string, data interface{}, memo
 	destinationPort := sourceChannelEnd.GetCounterparty().GetPortID()
 	destinationChannel := sourceChannelEnd.GetCounterparty().GetChannelID()
 
-	return k.createOutgoingPacket(ctx, portID, activeChannelId, destinationPort, destinationChannel, data, memo)
+	return k.createOutgoingPacket(ctx, portID, activeChannelId, destinationPort, destinationChannel, icaPacketData)
 }
 
 func (k Keeper) createOutgoingPacket(
@@ -36,27 +36,10 @@ func (k Keeper) createOutgoingPacket(
 	sourceChannel,
 	destinationPort,
 	destinationChannel string,
-	data interface{},
-	memo string,
+	icaPacketData types.InterchainAccountPacketData,
 ) (uint64, error) {
-	if data == nil {
-		return 0, types.ErrInvalidOutgoingData
-	}
-
-	var (
-		txBytes []byte
-		err     error
-	)
-
-	switch data := data.(type) {
-	case []sdk.Msg:
-		txBytes, err = k.SerializeCosmosTx(k.cdc, data)
-	default:
-		return 0, sdkerrors.Wrapf(types.ErrInvalidOutgoingData, "message type %T is not supported", data)
-	}
-
-	if err != nil {
-		return 0, sdkerrors.Wrap(err, "serialization of transaction data failed")
+	if err := icaPacketData.ValidateBasic(); err != nil {
+		return 0, sdkerrors.Wrap(err, "invalid interchain account packet data")
 	}
 
 	channelCap, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(sourcePort, sourceChannel))
@@ -70,18 +53,12 @@ func (k Keeper) createOutgoingPacket(
 		return 0, channeltypes.ErrSequenceSendNotFound
 	}
 
-	packetData := types.InterchainAccountPacketData{
-		Type: types.EXECUTE_TX,
-		Data: txBytes,
-		Memo: memo,
-	}
-
 	// timeoutTimestamp is set to be a max number here so that we never recieve a timeout
 	// ics-27-1 uses ordered channels which can close upon recieving a timeout, which is an undesired effect
 	const timeoutTimestamp = ^uint64(0) >> 1 // Shift the unsigned bit to satisfy hermes relayer timestamp conversion
 
 	packet := channeltypes.NewPacket(
-		packetData.GetBytes(),
+		icaPacketData.GetBytes(),
 		sequence,
 		sourcePort,
 		sourceChannel,
@@ -100,25 +77,26 @@ func (k Keeper) createOutgoingPacket(
 
 // DeserializeCosmosTx unmarshals and unpacks a slice of transaction bytes
 // into a slice of sdk.Msg's.
-func (k Keeper) DeserializeCosmosTx(_ sdk.Context, txBytes []byte) ([]sdk.Msg, error) {
-	var txBody types.IBCTxBody
-
-	if err := k.cdc.Unmarshal(txBytes, &txBody); err != nil {
+func (k Keeper) DeserializeCosmosTx(_ sdk.Context, data []byte) ([]sdk.Msg, error) {
+	var cosmosTx types.CosmosTx
+	if err := k.cdc.Unmarshal(data, &cosmosTx); err != nil {
 		return nil, err
 	}
 
-	anys := txBody.Messages
-	res := make([]sdk.Msg, len(anys))
-	for i, any := range anys {
+	msgs := make([]sdk.Msg, len(cosmosTx.Messages))
+
+	for i, any := range cosmosTx.Messages {
 		var msg sdk.Msg
+
 		err := k.cdc.UnpackAny(any, &msg)
 		if err != nil {
 			return nil, err
 		}
-		res[i] = msg
+
+		msgs[i] = msg
 	}
 
-	return res, nil
+	return msgs, nil
 }
 
 func (k Keeper) AuthenticateTx(ctx sdk.Context, msgs []sdk.Msg, portId string) error {

diff --git a/modules/apps/27-interchain-accounts/keeper/relay_test.go b/modules/apps/27-interchain-accounts/keeper/relay_test.go
@@ -15,9 +15,9 @@ import (
 
 func (suite *KeeperTestSuite) TestTrySendTx() {
 	var (
-		path   *ibctesting.Path
-		msg    interface{}
-		portID string
+		path          *ibctesting.Path
+		icaPacketData types.InterchainAccountPacketData
+		portID        string
 	)
 
 	testCases := []struct {
@@ -27,9 +27,6 @@ func (suite *KeeperTestSuite) TestTrySendTx() {
 	}{
 		{
 			"success", func() {
-				amount, _ := sdk.ParseCoinsNormalized("100stake")
-				interchainAccountAddr, _ := suite.chainB.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID)
-				msg = []sdk.Msg{&banktypes.MsgSend{FromAddress: interchainAccountAddr, ToAddress: suite.chainB.SenderAccount.GetAddress().String(), Amount: amount}}
 			}, true,
 		},
 		{
@@ -38,45 +35,30 @@ func (suite *KeeperTestSuite) TestTrySendTx() {
 				interchainAccountAddr, _ := suite.chainB.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID)
 				msg1 := &banktypes.MsgSend{FromAddress: interchainAccountAddr, ToAddress: suite.chainB.SenderAccount.GetAddress().String(), Amount: amount}
 				msg2 := &banktypes.MsgSend{FromAddress: interchainAccountAddr, ToAddress: suite.chainB.SenderAccount.GetAddress().String(), Amount: amount}
-				msg = []sdk.Msg{msg1, msg2}
+				data, err := suite.chainB.GetSimApp().ICAKeeper.SerializeCosmosTx(suite.chainB.GetSimApp().AppCodec(), []sdk.Msg{msg1, msg2})
+				suite.Require().NoError(err)
+				icaPacketData.Data = data
 			}, true,
 		},
 		{
-			"incorrect outgoing data", func() {
-				msg = []byte{}
-			}, false,
-		},
-		{
-			"incorrect outgoing data - []sdk.Msg is not used", func() {
-				amount, _ := sdk.ParseCoinsNormalized("100stake")
-				interchainAccountAddr, _ := suite.chainB.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID)
-				msg = &banktypes.MsgSend{FromAddress: interchainAccountAddr, ToAddress: suite.chainB.SenderAccount.GetAddress().String(), Amount: amount}
+			"data is nil", func() {
+				icaPacketData.Data = nil
 			}, false,
 		},
 		{
 			"active channel not found", func() {
-				amount, _ := sdk.ParseCoinsNormalized("100stake")
-				interchainAccountAddr, _ := suite.chainB.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID)
-				msg = &banktypes.MsgSend{FromAddress: interchainAccountAddr, ToAddress: suite.chainB.SenderAccount.GetAddress().String(), Amount: amount}
 				portID = "incorrect portID"
 			}, false,
 		},
 		{
 			"channel does not exist", func() {
-				amount, _ := sdk.ParseCoinsNormalized("100stake")
-				interchainAccountAddr, _ := suite.chainB.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID)
-				msg = &banktypes.MsgSend{FromAddress: interchainAccountAddr, ToAddress: suite.chainB.SenderAccount.GetAddress().String(), Amount: amount}
 				suite.chainA.GetSimApp().ICAKeeper.SetActiveChannel(suite.chainA.GetContext(), portID, "channel-100")
 			}, false,
 		},
 		{
-			"data is nil", func() {
-				msg = nil
-			}, false,
-		},
-		{
-			"data is not an SDK message", func() {
-				msg = "not an sdk message"
+			"SendPacket fails - channel closed", func() {
+				err := path.EndpointA.SetChannelClosed()
+				suite.Require().NoError(err)
 			}, false,
 		},
 	}
@@ -88,16 +70,28 @@ func (suite *KeeperTestSuite) TestTrySendTx() {
 			suite.SetupTest() // reset
 			path = NewICAPath(suite.chainA, suite.chainB)
 			suite.coordinator.SetupConnections(path)
-			memo := "memo"
 
 			err := suite.SetupICAPath(path, TestOwnerAddress)
 			suite.Require().NoError(err)
 
 			portID = path.EndpointA.ChannelConfig.PortID
 
+			amount, _ := sdk.ParseCoinsNormalized("100stake")
+			interchainAccountAddr, _ := suite.chainB.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID)
+			msg := &banktypes.MsgSend{FromAddress: interchainAccountAddr, ToAddress: suite.chainB.SenderAccount.GetAddress().String(), Amount: amount}
+			data, err := suite.chainB.GetSimApp().ICAKeeper.SerializeCosmosTx(suite.chainB.GetSimApp().AppCodec(), []sdk.Msg{msg})
+			suite.Require().NoError(err)
+
+			// default packet data, must be modified in malleate for test cases expected to fail
+			icaPacketData = types.InterchainAccountPacketData{
+				Type: types.EXECUTE_TX,
+				Data: data,
+				Memo: "memo",
+			}
+
 			tc.malleate()
 
-			_, err = suite.chainA.GetSimApp().ICAKeeper.TrySendTx(suite.chainA.GetContext(), portID, msg, memo)
+			_, err = suite.chainA.GetSimApp().ICAKeeper.TrySendTx(suite.chainA.GetContext(), portID, icaPacketData)
 
 			if tc.expPass {
 				suite.Require().NoError(err)
@@ -112,7 +106,6 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
 	var (
 		path       *ibctesting.Path
 		msg        sdk.Msg
-		txBytes    []byte
 		packetData []byte
 		sourcePort string
 	)
@@ -129,30 +122,40 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
 				interchainAccountAddr, _ := suite.chainB.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID)
 				msg = &banktypes.MsgSend{FromAddress: interchainAccountAddr, ToAddress: suite.chainB.SenderAccount.GetAddress().String(), Amount: amount}
 				// build packet data
-				txBytes, err := suite.chainA.GetSimApp().ICAKeeper.SerializeCosmosTx(suite.chainA.Codec, []sdk.Msg{msg})
+				data, err := suite.chainA.GetSimApp().ICAKeeper.SerializeCosmosTx(suite.chainA.Codec, []sdk.Msg{msg})
 				suite.Require().NoError(err)
 
-				data := types.InterchainAccountPacketData{Type: types.EXECUTE_TX,
-					Data: txBytes}
-				packetData = data.GetBytes()
+				icaPacketData := types.InterchainAccountPacketData{
+					Type: types.EXECUTE_TX,
+					Data: data,
+				}
+				packetData = icaPacketData.GetBytes()
 			}, true,
 		},
 		{
-			"Cannot deserialize txBytes", func() {
-				txBytes = []byte("invalid tx bytes")
-				data := types.InterchainAccountPacketData{Type: types.EXECUTE_TX,
-					Data: txBytes}
-				packetData = data.GetBytes()
+			"Cannot deserialize packet data messages", func() {
+				data := []byte("invalid packet data")
+
+				icaPacketData := types.InterchainAccountPacketData{
+					Type: types.EXECUTE_TX,
+					Data: data,
+				}
+				packetData = icaPacketData.GetBytes()
 			}, false,
 		},
 		{
 			"Invalid packet type", func() {
-				txBytes = []byte{}
+				// build packet data
+				data, err := suite.chainA.GetSimApp().ICAKeeper.SerializeCosmosTx(suite.chainA.Codec, []sdk.Msg{&banktypes.MsgSend{}})
+				suite.Require().NoError(err)
+
 				// Type here is an ENUM
 				// Valid type is types.EXECUTE_TX
-				data := types.InterchainAccountPacketData{Type: 100,
-					Data: txBytes}
-				packetData = data.GetBytes()
+				icaPacketData := types.InterchainAccountPacketData{
+					Type: 100,
+					Data: data,
+				}
+				packetData = icaPacketData.GetBytes()
 			}, false,
 		},
 		{
@@ -172,11 +175,13 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
 				// Incorrect FromAddress
 				msg = &banktypes.MsgSend{FromAddress: suite.chainB.SenderAccount.GetAddress().String(), ToAddress: suite.chainB.SenderAccount.GetAddress().String(), Amount: amount}
 				// build packet data
-				txBytes, err := suite.chainA.GetSimApp().ICAKeeper.SerializeCosmosTx(suite.chainA.Codec, []sdk.Msg{msg})
+				data, err := suite.chainA.GetSimApp().ICAKeeper.SerializeCosmosTx(suite.chainA.Codec, []sdk.Msg{msg})
 				suite.Require().NoError(err)
-				data := types.InterchainAccountPacketData{Type: types.EXECUTE_TX,
-					Data: txBytes}
-				packetData = data.GetBytes()
+				icaPacketData := types.InterchainAccountPacketData{
+					Type: types.EXECUTE_TX,
+					Data: data,
+				}
+				packetData = icaPacketData.GetBytes()
 			}, false,
 		},
 	}