Added optional packet metadata to the packet and message types (#2305)

* added optional packet metadata to the packet and message types

* added docs

* breaking the api (backports should add a utility function for this)

* adding nil metadata on all the calls

* added metadata to the cli

* added events

* breaking api for FungibleTokenPacketData

* hex encoding metadata

* added abstraction

* fixed bad merge

* added tests with metadata

* added missing metadata to packet for recv

* cleaning up metadata on every test

* reset metadata

* added metadata flag

* lint

* Update modules/apps/transfer/client/cli/tx.go

Co-authored-by: Damian Nolan <damiannolan@gmail.com>

* fixed bad call in tests

Co-authored-by: Damian Nolan <damiannolan@gmail.com>
(cherry picked from commit 82397d6)

# Conflicts:
#	docs/apps/transfer/messages.md
#	docs/ibc/proto-docs.md
#	go.mod
#	go.sum
#	modules/apps/29-fee/transfer_test.go
#	modules/apps/transfer/ibc_module.go
#	modules/apps/transfer/keeper/mbt_relay_test.go
#	modules/apps/transfer/keeper/relay_test.go
#	modules/apps/transfer/spec/05_events.md
#	modules/apps/transfer/types/packet.pb.go
#	modules/apps/transfer/types/tx.pb.go
#	proto/ibc/applications/interchain_accounts/controller/v1/tx.proto

Author: nicolaslara

docs/apps/transfer/messages.md

@@ -0,0 +1,37 @@
+<!--
+order: 4
+-->
+
+# Messages
+
+## `MsgTransfer`
+
+A fungible token cross chain transfer is achieved by using the `MsgTransfer`:
+
+```go
+type MsgTransfer struct {
+  SourcePort        string
+  SourceChannel     string
+  Token             sdk.Coin
+  Sender            string
+  Receiver          string
+  TimeoutHeight     ibcexported.Height
+  TimeoutTimestamp  uint64
+  Metadata          []byte
+}
+```
+
+This message is expected to fail if:
+
+- `SourcePort` is invalid (see [24-host naming requirements](https://github.com/cosmos/ibc/blob/master/spec/core/ics-024-host-requirements/README.md#paths-identifiers-separators).
+- `SourceChannel` is invalid (see [24-host naming requirements](https://github.com/cosmos/ibc/blob/master/spec/core/ics-024-host-requirements/README.md#paths-identifiers-separators)).
+- `Token` is invalid (denom is invalid or amount is negative)
+  - `Token.Amount` is not positive.
+  - `Token.Denom` is not a valid IBC denomination as per [ADR 001 - Coin Source Tracing](../../../docs/architecture/adr-001-coin-source-tracing.md).
+- `Sender` is empty.
+- `Receiver` is empty.
+- `TimeoutHeight` and `TimeoutTimestamp` are both zero.
+
+This message will send a fungible token to the counterparty chain represented by the counterparty Channel End connected to the Channel End with the identifiers `SourcePort` and `SourceChannel`.
+
+The denomination provided for transfer should correspond to the same denomination represented on this chain. The prefixes will be added as necessary upon by the receiving chain.

docs/ibc/proto-docs.md

@@ -1097,13 +1097,24 @@ identifier fields.
 
 | Field | Type | Label | Description |
 | ----- | ---- | ----- | ----------- |
+<<<<<<< HEAD
 | `state` | [State](#ibc.core.channel.v1.State) |  | current state of the channel end |
 | `ordering` | [Order](#ibc.core.channel.v1.Order) |  | whether the channel is ordered or unordered |
 | `counterparty` | [Counterparty](#ibc.core.channel.v1.Counterparty) |  | counterparty channel end |
 | `connection_hops` | [string](#string) | repeated | list of connection identifiers, in order, along which packets sent on this channel will travel |
 | `version` | [string](#string) |  | opaque channel version, which is agreed upon during the handshake |
 | `port_id` | [string](#string) |  | port identifier |
 | `channel_id` | [string](#string) |  | channel identifier |
+=======
+| `source_port` | [string](#string) |  | the port on which the packet will be sent |
+| `source_channel` | [string](#string) |  | the channel by which the packet will be sent |
+| `token` | [cosmos.base.v1beta1.Coin](#cosmos.base.v1beta1.Coin) |  | the tokens to be transferred |
+| `sender` | [string](#string) |  | the sender address |
+| `receiver` | [string](#string) |  | the recipient address on the destination chain |
+| `timeout_height` | [ibc.core.client.v1.Height](#ibc.core.client.v1.Height) |  | Timeout height relative to the current block height. The timeout is disabled when set to 0. |
+| `timeout_timestamp` | [uint64](#uint64) |  | Timeout timestamp in absolute nanoseconds since unix epoch. The timeout is disabled when set to 0. |
+| `metadata` | [bytes](#bytes) |  | optional metadata |
+>>>>>>> 82397d6 (Added optional packet metadata to the packet and message types (#2305))
 
 
 
@@ -1118,6 +1129,7 @@ Packet defines a type that carries data across different chains through IBC
 
 | Field | Type | Label | Description |
 | ----- | ---- | ----- | ----------- |
+<<<<<<< HEAD
 | `sequence` | [uint64](#uint64) |  | number corresponds to the order of sends and receives, where a Packet with an earlier sequence number must be sent and received before a Packet with a later sequence number. |
 | `source_port` | [string](#string) |  | identifies the port on the sending chain. |
 | `source_channel` | [string](#string) |  | identifies the channel end on the sending chain. |
@@ -1126,6 +1138,12 @@ Packet defines a type that carries data across different chains through IBC
 | `data` | [bytes](#bytes) |  | actual opaque bytes transferred directly to the application module |
 | `timeout_height` | [ibc.core.client.v1.Height](#ibc.core.client.v1.Height) |  | block height after which the packet times out |
 | `timeout_timestamp` | [uint64](#uint64) |  | block timestamp (in nanoseconds) after which the packet times out |
+=======
+| `sequence` | [uint64](#uint64) |  | sequence number of the transfer packet sent |
+
+
+
+>>>>>>> 82397d6 (Added optional packet metadata to the packet and message types (#2305))
 
 
 
@@ -1166,6 +1184,16 @@ Order defines if a channel is ORDERED or UNORDERED
 | ORDER_UNORDERED | 1 | packets can be delivered in any order, which may differ from the order in which they were sent. |
 | ORDER_ORDERED | 2 | packets are delivered exactly in the order which they were sent |
 
+<<<<<<< HEAD
+=======
+| Field | Type | Label | Description |
+| ----- | ---- | ----- | ----------- |
+| `denom` | [string](#string) |  | the token denomination to be transferred |
+| `amount` | [string](#string) |  | the token amount to be transferred |
+| `sender` | [string](#string) |  | the sender address |
+| `receiver` | [string](#string) |  | the recipient address on the destination chain |
+| `metadata` | [bytes](#bytes) |  | optional metadata |
+>>>>>>> 82397d6 (Added optional packet metadata to the packet and message types (#2305))
 
 
 <a name="ibc.core.channel.v1.State"></a>

go.mod

@@ -35,6 +35,10 @@ require (
 	github.com/beorn7/perks v1.0.1 // indirect
 	github.com/bgentry/speakeasy v0.1.0 // indirect
 	github.com/btcsuite/btcd v0.22.1 // indirect
+<<<<<<< HEAD
+=======
+	github.com/cenkalti/backoff/v4 v4.1.1 // indirect
+>>>>>>> 82397d6 (Added optional packet metadata to the packet and message types (#2305))
 	github.com/cespare/xxhash v1.1.0 // indirect
 	github.com/cespare/xxhash/v2 v2.1.2 // indirect
 	github.com/coinbase/rosetta-sdk-go v0.7.0 // indirect

go.sum

@@ -130,6 +130,11 @@ github.com/btcsuite/winsvc v1.0.0/go.mod h1:jsenWakMcC0zFBFurPLEAyrnc/teJEM1O46f
 github.com/casbin/casbin/v2 v2.1.2/go.mod h1:YcPU1XXisHhLzuxH9coDNf2FbKpjGlbCg3n9yuLkIJQ=
 github.com/cenkalti/backoff v2.2.1+incompatible h1:tNowT99t7UNflLxfYYSlKYsBpXdEet03Pg2g16Swow4=
 github.com/cenkalti/backoff v2.2.1+incompatible/go.mod h1:90ReRw6GdpyfrHakVjL/QHaoyV4aDUVVkXQJJJ3NXXM=
+<<<<<<< HEAD
+=======
+github.com/cenkalti/backoff/v4 v4.1.1 h1:G2HAfAmvm/GcKan2oOQpBXOd2tT2G57ZnZGWa1PxPBQ=
+github.com/cenkalti/backoff/v4 v4.1.1/go.mod h1:scbssz8iZGpm3xbr14ovlUdkxfGXNInqkPWOWmG2CLw=
+>>>>>>> 82397d6 (Added optional packet metadata to the packet and message types (#2305))
 github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU=
 github.com/cespare/cp v0.1.0/go.mod h1:SOGHArjBr4JWaSDEVpWpo/hNg6RoKrls6Oh40hiwW+s=
 github.com/cespare/xxhash v1.1.0 h1:a6HrQnmkObjyL+Gs60czilIUGqrzKutQD6XZog3p+ko=

modules/apps/29-fee/transfer_test.go

@@ -0,0 +1,70 @@
+package fee_test
+
+import (
+	sdk "github.com/cosmos/cosmos-sdk/types"
+
+	"github.com/cosmos/ibc-go/v6/modules/apps/29-fee/types"
+	transfertypes "github.com/cosmos/ibc-go/v6/modules/apps/transfer/types"
+	clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types"
+	ibctesting "github.com/cosmos/ibc-go/v6/testing"
+)
+
+// Integration test to ensure ics29 works with ics20
+func (suite *FeeTestSuite) TestFeeTransfer() {
+	path := ibctesting.NewPath(suite.chainA, suite.chainB)
+	feeTransferVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: transfertypes.Version}))
+	path.EndpointA.ChannelConfig.Version = feeTransferVersion
+	path.EndpointB.ChannelConfig.Version = feeTransferVersion
+	path.EndpointA.ChannelConfig.PortID = transfertypes.PortID
+	path.EndpointB.ChannelConfig.PortID = transfertypes.PortID
+
+	suite.coordinator.Setup(path)
+
+	// set up coin & ics20 packet
+	coin := ibctesting.TestCoin
+	fee := types.Fee{
+		RecvFee:    defaultRecvFee,
+		AckFee:     defaultAckFee,
+		TimeoutFee: defaultTimeoutFee,
+	}
+
+	msgs := []sdk.Msg{
+		types.NewMsgPayPacketFee(fee, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, suite.chainA.SenderAccount.GetAddress().String(), nil),
+		transfertypes.NewMsgTransfer(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, coin, suite.chainA.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String(), clienttypes.NewHeight(1, 100), 0, nil),
+	}
+	res, err := suite.chainA.SendMsgs(msgs...)
+	suite.Require().NoError(err) // message committed
+
+	// after incentivizing the packets
+	originalChainASenderAccountBalance := sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom))
+
+	packet, err := ibctesting.ParsePacketFromEvents(res.GetEvents())
+	suite.Require().NoError(err)
+
+	// register counterparty address on chainB
+	// relayerAddress is address of sender account on chainB, but we will use it on chainA
+	// to differentiate from the chainA.SenderAccount for checking successful relay payouts
+	relayerAddress := suite.chainB.SenderAccount.GetAddress()
+
+	msgRegister := types.NewMsgRegisterCounterpartyPayee(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, suite.chainB.SenderAccount.GetAddress().String(), relayerAddress.String())
+	_, err = suite.chainB.SendMsgs(msgRegister)
+	suite.Require().NoError(err) // message committed
+
+	// relay packet
+	err = path.RelayPacket(packet)
+	suite.Require().NoError(err) // relay committed
+
+	// ensure relayers got paid
+	// relayer for forward relay: chainB.SenderAccount
+	// relayer for reverse relay: chainA.SenderAccount
+
+	// check forward relay balance
+	suite.Require().Equal(
+		fee.RecvFee,
+		sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainB.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom)),
+	)
+
+	suite.Require().Equal(
+		fee.AckFee.Add(fee.TimeoutFee...), // ack fee paid, timeout fee refunded
+		sdk.NewCoins(suite.chainA.GetSimApp().BankKeeper.GetBalance(suite.chainA.GetContext(), suite.chainA.SenderAccount.GetAddress(), ibctesting.TestCoin.Denom)).Sub(originalChainASenderAccountBalance[0]))
+}

modules/apps/transfer/client/cli/tx.go

@@ -22,6 +22,7 @@ const (
 	flagPacketTimeoutHeight    = "packet-timeout-height"
 	flagPacketTimeoutTimestamp = "packet-timeout-timestamp"
 	flagAbsoluteTimeouts       = "absolute-timeouts"
+	flagMetadata               = "metadata"
 )
 
 // NewTransferTxCmd returns the command to create a NewMsgTransfer transaction
@@ -76,6 +77,11 @@ corresponding to the counterparty channel. Any timeout set to 0 is disabled.`),
 				return err
 			}
 
+			metadataStr, err := cmd.Flags().GetString(flagMetadata)
+			if err != nil {
+				return err
+			}
+
 			// if the timeouts are not absolute, retrieve latest block height and block timestamp
 			// for the consensus state connected to the destination port/channel
 			if !absoluteTimeouts {
@@ -111,7 +117,7 @@ corresponding to the counterparty channel. Any timeout set to 0 is disabled.`),
 			}
 
 			msg := types.NewMsgTransfer(
-				srcPort, srcChannel, coin, sender, receiver, timeoutHeight, timeoutTimestamp,
+				srcPort, srcChannel, coin, sender, receiver, timeoutHeight, timeoutTimestamp, []byte(metadataStr),
 			)
 			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
 		},
@@ -120,6 +126,7 @@ corresponding to the counterparty channel. Any timeout set to 0 is disabled.`),
 	cmd.Flags().String(flagPacketTimeoutHeight, types.DefaultRelativePacketTimeoutHeight, "Packet timeout block height. The timeout is disabled when set to 0-0.")
 	cmd.Flags().Uint64(flagPacketTimeoutTimestamp, types.DefaultRelativePacketTimeoutTimestamp, "Packet timeout timestamp in nanoseconds from now. Default is 10 minutes. The timeout is disabled when set to 0.")
 	cmd.Flags().Bool(flagAbsoluteTimeouts, false, "Timeout flags are used as absolute timeouts.")
+	cmd.Flags().String(flagMetadata, "", "Metadata to be sent along with the packet. The CLI accepts only strings here but you can construct a packet with arbitrary bytes via code.")
 	flags.AddTxFlagsToCmd(cmd)
 
 	return cmd

modules/apps/transfer/ibc_module.go

@@ -1,6 +1,7 @@
 package transfer
 
 import (
+	"encoding/hex"
 	"fmt"
 	"math"
 
@@ -186,6 +187,23 @@ func (im IBCModule) OnRecvPacket(
 		}
 	}
 
+<<<<<<< HEAD
+=======
+	eventAttributes := []sdk.Attribute{
+		sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
+		sdk.NewAttribute(sdk.AttributeKeySender, data.Sender),
+		sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
+		sdk.NewAttribute(types.AttributeKeyDenom, data.Denom),
+		sdk.NewAttribute(types.AttributeKeyAmount, data.Amount),
+		sdk.NewAttribute(types.AttributeKeyMetadata, hex.EncodeToString(data.Metadata)),
+		sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success())),
+	}
+
+	if ackErr != nil {
+		eventAttributes = append(eventAttributes, sdk.NewAttribute(types.AttributeKeyAckError, ackErr.Error()))
+	}
+
+>>>>>>> 82397d6 (Added optional packet metadata to the packet and message types (#2305))
 	ctx.EventManager().EmitEvent(
 		sdk.NewEvent(
 			types.EventTypePacket,
@@ -230,6 +248,7 @@ func (im IBCModule) OnAcknowledgementPacket(
 			sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
 			sdk.NewAttribute(types.AttributeKeyDenom, data.Denom),
 			sdk.NewAttribute(types.AttributeKeyAmount, data.Amount),
+			sdk.NewAttribute(types.AttributeKeyMetadata, hex.EncodeToString(data.Metadata)),
 			sdk.NewAttribute(types.AttributeKeyAck, ack.String()),
 		),
 	)
@@ -276,6 +295,7 @@ func (im IBCModule) OnTimeoutPacket(
 			sdk.NewAttribute(types.AttributeKeyRefundReceiver, data.Sender),
 			sdk.NewAttribute(types.AttributeKeyRefundDenom, data.Denom),
 			sdk.NewAttribute(types.AttributeKeyRefundAmount, data.Amount),
+			sdk.NewAttribute(types.AttributeKeyMetadata, hex.EncodeToString(data.Metadata)),
 		),
 	)
 

modules/apps/transfer/keeper/mbt_relay_test.go

@@ -145,7 +145,8 @@ func FungibleTokenPacketFromTla(packet TlaFungibleTokenPacket) FungibleTokenPack
 			DenomFromTla(packet.Data.Denom),
 			packet.Data.Amount,
 			AddressFromString(packet.Data.Sender),
-			AddressFromString(packet.Data.Receiver)),
+			AddressFromString(packet.Data.Receiver),
+			nil),
 	}
 }
 
@@ -344,8 +345,14 @@ func (suite *KeeperTestSuite) TestModelBasedRelay() {
 							sdk.NewCoin(denom, amount),
 							sender,
 							tc.packet.Data.Receiver,
+<<<<<<< HEAD
 							clienttypes.NewHeight(0, 110),
 							0)
+=======
+							clienttypes.NewHeight(1, 110),
+							0,
+							nil)
+>>>>>>> 82397d6 (Added optional packet metadata to the packet and message types (#2305))
 					}
 				case "OnRecvPacket":
 					err = suite.chainB.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainB.GetContext(), packet, tc.packet.Data)

modules/apps/transfer/keeper/msg_server.go

@@ -21,7 +21,7 @@ func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types.
 
 	sequence, err := k.sendTransfer(
 		ctx, msg.SourcePort, msg.SourceChannel, msg.Token, sender, msg.Receiver, msg.TimeoutHeight, msg.TimeoutTimestamp,
-	)
+		msg.Metadata)
 	if err != nil {
 		return nil, err
 	}

modules/apps/transfer/keeper/msg_server_test.go

@@ -54,6 +54,7 @@ func (suite *KeeperTestSuite) TestMsgTransfer() {
 			path.EndpointA.ChannelID,
 			coin, suite.chainA.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String(),
 			suite.chainB.GetTimeoutHeight(), 0, // only use timeout height
+			[]byte("custom metadata"),
 		)
 
 		tc.malleate()