Add standard acknowledgement to channels and apply usage in ibc trans…

…fer (#7263)

* gen ack proto type

* remove transfer ack

* change ics20 ack to use standard type

* update commented tests

* small typo fix

* revert back to module cdc

* update docs

* fix lint

* nit

* update ack event emission

* Update x/ibc-transfer/spec/01_concepts.md

* add comment for onacknowledgement

* Update proto/ibc/channel/channel.proto

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-authored-by: Christopher Goes <cwgoes@pluranimity.org>

docs/ibc/custom.md

@@ -340,7 +340,9 @@ receive acknowledegments with the IBC modules as byte strings.
 
 Thus, modules must agree on how to encode/decode acknowledgements. The process of creating an
 acknowledgement struct along with encoding and decoding it, is very similar to the packet data
-example above.
+example above. [ICS 04](https://github.com/cosmos/ics/tree/master/spec/ics-004-channel-and-packet-semantics#acknowledgement-envelope)
+specifies a recommended format for acknowledgements. This acknowledgement type can be imported from
+[channel types](https://github.com/cosmos/cosmos-sdk/tree/master/x/ibc/04-channel/types).
 
 #### Acknowledging Packets
 

proto/ibc/channel/channel.proto

@@ -48,7 +48,7 @@ message MsgChannelOpenConfirm {
   bytes             signer       = 5 [(gogoproto.casttype) = "github.com/cosmos/cosmos-sdk/types.AccAddress"];
 }
 
-// MsgChannelOpenConfirm defines a msg sent by a Relayer to Chain A
+// MsgChannelCloseInit defines a msg sent by a Relayer to Chain A
 // to close a channel with Chain B.
 message MsgChannelCloseInit {
   string port_id    = 1 [(gogoproto.moretags) = "yaml:\"port_id\""];
@@ -114,8 +114,8 @@ message Channel {
   Order ordering = 2;
   // counterparty channel end
   Counterparty counterparty = 3 [(gogoproto.nullable) = false];
-  // list of connection identifiers, in order, along which packets sent on this
-  // channel will travel
+  // list of connection identifiers, in order, along which packets sent on
+  // this channel will travel
   repeated string connection_hops = 4 [(gogoproto.moretags) = "yaml:\"connection_hops\""];
   // opaque channel version, which is agreed upon during the handshake
   string version = 5;
@@ -132,8 +132,8 @@ message IdentifiedChannel {
   Order ordering = 2;
   // counterparty channel end
   Counterparty counterparty = 3 [(gogoproto.nullable) = false];
-  // list of connection identifiers, in order, along which packets sent on this
-  // channel will travel
+  // list of connection identifiers, in order, along which packets sent on
+  // this channel will travel
   repeated string connection_hops = 4 [(gogoproto.moretags) = "yaml:\"connection_hops\""];
   // opaque channel version, which is agreed upon during the handshake
   string version = 5;
@@ -189,9 +189,9 @@ message Counterparty {
 message Packet {
   option (gogoproto.goproto_getters) = false;
 
-  // 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.
+  // 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.
   uint64 sequence = 1;
   // identifies the port on the sending chain.
   string source_port = 2 [(gogoproto.moretags) = "yaml:\"source_port\""];
@@ -223,3 +223,18 @@ message PacketAckCommitment {
   // packet commitment hash.
   bytes hash = 4;
 }
+
+// Acknowledgement is the recommended acknowledgement format to be used by
+// app-specific protocols.
+// NOTE: The field numbers 21 and 22 were explicitly chosen to avoid accidental
+// conflicts with other protobuf message formats used for acknowledgements.
+// The first byte of any message with this format will be the non-ASCII values
+// `0xaa` (result) or `0xb2` (error). Implemented as defined by ICS:
+// https://github.com/cosmos/ics/tree/master/spec/ics-004-channel-and-packet-semantics#acknowledgement-envelope
+message Acknowledgement {
+  // response contains either a result or an error and must be non-empty
+  oneof response {
+    bytes  result = 21;
+    string error  = 22;
+  }
+}

proto/ibc/transfer/transfer.proto

@@ -43,31 +43,25 @@ message FungibleTokenPacketData {
   string receiver = 4;
 }
 
-// FungibleTokenPacketAcknowledgement contains a boolean success flag and an
-// optional error msg error msg is empty string on success See spec for
-// onAcknowledgePacket:
-// https://github.com/cosmos/ics/tree/master/spec/ics-020-fungible-token-transfer#packet-relay
-message FungibleTokenPacketAcknowledgement {
-  bool   success = 1;
-  string error   = 2;
-}
-
-// DenomTrace contains the base denomination for ICS20 fungible tokens and the source tracing
-// information path.
+// DenomTrace contains the base denomination for ICS20 fungible tokens and the
+// source tracing information path.
 message DenomTrace {
-  // path defines the chain of port/channel identifiers used for tracing the source of the fungible
-  // token.
+  // path defines the chain of port/channel identifiers used for tracing the
+  // source of the fungible token.
   string path = 1;
   // base denomination of the relayed fungible token.
   string base_denom = 2;
 }
 
 // Params defines the set of IBC transfer parameters.
-// NOTE: To prevent a single token from being transferred, set the TransfersEnabled parameter to
-// true and then set the bank module's SendEnabled parameter for the denomination to false.
+// NOTE: To prevent a single token from being transferred, set the
+// TransfersEnabled parameter to true and then set the bank module's SendEnabled
+// parameter for the denomination to false.
 message Params {
-  // send_enabled enables or disables all cross-chain token transfers from this chain.
+  // send_enabled enables or disables all cross-chain token transfers from this
+  // chain.
   bool send_enabled = 1 [(gogoproto.moretags) = "yaml:\"send_enabled\""];
-  // receive_enabled enables or disables all cross-chain token transfers to this chain.
+  // receive_enabled enables or disables all cross-chain token transfers to this
+  // chain.
   bool receive_enabled = 2 [(gogoproto.moretags) = "yaml:\"receive_enabled\""];
 }

x/ibc-transfer/handler_test.go

@@ -48,7 +48,7 @@ func (suite *TransferTestSuite) TestHandleMsgTransfer() {
 	// relay send
 	fungibleTokenPacket := types.NewFungibleTokenPacketData(coinToSendToB.Denom, coinToSendToB.Amount.Uint64(), suite.chainA.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String())
 	packet := channeltypes.NewPacket(fungibleTokenPacket.GetBytes(), 1, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, timeoutHeight, 0)
-	ack := types.FungibleTokenPacketAcknowledgement{Success: true}
+	ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
 	err = suite.coordinator.RelayPacket(suite.chainA, suite.chainB, clientA, clientB, packet, ack.GetBytes())
 	suite.Require().NoError(err) // relay committed
 

x/ibc-transfer/keeper/relay.go

@@ -232,11 +232,15 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t
 // acknowledgement written on the receiving chain. If the acknowledgement
 // was a success then nothing occurs. If the acknowledgement failed, then
 // the sender is refunded their tokens using the refundPacketToken function.
-func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, data types.FungibleTokenPacketData, ack types.FungibleTokenPacketAcknowledgement) error {
-	if !ack.Success {
+func (k Keeper) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, data types.FungibleTokenPacketData, ack channeltypes.Acknowledgement) error {
+	switch ack.Response.(type) {
+	case *channeltypes.Acknowledgement_Error:
 		return k.refundPacketToken(ctx, packet, data)
+	default:
+		// the acknowledgement succeeded on the receiving chain so nothing
+		// needs to be executed and no error needs to be returned
+		return nil
 	}
-	return nil
 }
 
 // OnTimeoutPacket refunds the sender since the original packet sent was

x/ibc-transfer/keeper/relay_test.go

@@ -192,7 +192,7 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
 				// relay send packet
 				fungibleTokenPacket := types.NewFungibleTokenPacketData(coinFromBToA, suite.chainB.SenderAccount.GetAddress().String(), suite.chainA.SenderAccount.GetAddress().String())
 				packet := channeltypes.NewPacket(fungibleTokenPacket.GetBytes(), 1, channelB.PortID, channelB.ID, channelA.PortID, channelA.ID, 110, 0)
-				ack := types.FungibleTokenPacketAcknowledgement{Success: true}
+				ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
 				err = suite.coordinator.RelayPacket(suite.chainB, suite.chainA, clientB, clientA, packet, ack.GetBytes())
 				suite.Require().NoError(err) // relay committed
 
@@ -229,21 +229,16 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {
 // to chainB.
 func (suite *KeeperTestSuite) TestOnAcknowledgementPacket() {
 	var (
-		successAck = types.FungibleTokenPacketAcknowledgement{
-			Success: true,
-		}
-		failedAck = types.FungibleTokenPacketAcknowledgement{
-			Success: false,
-			Error:   "failed packet transfer",
-		}
+		successAck = channeltypes.NewResultAcknowledgement([]byte{byte(1)})
+		failedAck = channeltypes.NewErrorAcknowledgement("failed packet transfer")
 
 		channelA, channelB ibctesting.TestChannel
 		coins              sdk.Coins
 	)
 
 	testCases := []struct {
 		msg      string
-		ack      types.FungibleTokenPacketAcknowledgement
+		ack      channeltypes.Acknowledgement
 		malleate func()
 		source   bool
 		success  bool // success of ack

x/ibc-transfer/module.go

@@ -305,16 +305,11 @@ func (am AppModule) OnRecvPacket(
 		return nil, nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet data: %s", err.Error())
 	}
 
-	acknowledgement := types.FungibleTokenPacketAcknowledgement{
-		Success: true,
-		Error:   "",
-	}
+	acknowledgement := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
 
-	if err := am.keeper.OnRecvPacket(ctx, packet, data); err != nil {
-		acknowledgement = types.FungibleTokenPacketAcknowledgement{
-			Success: false,
-			Error:   err.Error(),
-		}
+	err := am.keeper.OnRecvPacket(ctx, packet, data)
+	if err != nil {
+		acknowledgement = channeltypes.NewErrorAcknowledgement(err.Error())
 	}
 
 	ctx.EventManager().EmitEvent(
@@ -324,6 +319,7 @@ func (am AppModule) OnRecvPacket(
 			sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
 			sdk.NewAttribute(types.AttributeKeyDenom, data.Denom),
 			sdk.NewAttribute(types.AttributeKeyAmount, fmt.Sprintf("%d", data.Amount)),
+			sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", err != nil)),
 		),
 	)
 
@@ -338,7 +334,7 @@ func (am AppModule) OnAcknowledgementPacket(
 	packet channeltypes.Packet,
 	acknowledgement []byte,
 ) (*sdk.Result, error) {
-	var ack types.FungibleTokenPacketAcknowledgement
+	var ack channeltypes.Acknowledgement
 	if err := types.ModuleCdc.UnmarshalJSON(acknowledgement, &ack); err != nil {
 		return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "cannot unmarshal ICS-20 transfer packet acknowledgement: %v", err)
 	}
@@ -358,15 +354,23 @@ func (am AppModule) OnAcknowledgementPacket(
 			sdk.NewAttribute(types.AttributeKeyReceiver, data.Receiver),
 			sdk.NewAttribute(types.AttributeKeyDenom, data.Denom),
 			sdk.NewAttribute(types.AttributeKeyAmount, fmt.Sprintf("%d", data.Amount)),
-			sdk.NewAttribute(types.AttributeKeyAckSuccess, fmt.Sprintf("%t", ack.Success)),
+			sdk.NewAttribute(types.AttributeKeyAck, fmt.Sprintf("%v", ack)),
 		),
 	)
 
-	if !ack.Success {
+	switch resp := ack.Response.(type) {
+	case *channeltypes.Acknowledgement_Result:
+		ctx.EventManager().EmitEvent(
+			sdk.NewEvent(
+				types.EventTypePacket,
+				sdk.NewAttribute(types.AttributeKeyAckSuccess, string(resp.Result)),
+			),
+		)
+	case *channeltypes.Acknowledgement_Error:
 		ctx.EventManager().EmitEvent(
 			sdk.NewEvent(
 				types.EventTypePacket,
-				sdk.NewAttribute(types.AttributeKeyAckError, ack.Error),
+				sdk.NewAttribute(types.AttributeKeyAckError, resp.Error),
 			),
 		)
 	}

x/ibc-transfer/spec/01_concepts.md

@@ -3,3 +3,13 @@ order: 1
 -->
 
 # Concepts
+
+## Acknowledgements
+
+ICS20 uses the recommended acknowledgement format as specified by [ICS 04](https://github.com/cosmos/ics/tree/master/spec/ics-004-channel-and-packet-semantics#acknowledgement-envelope).
+
+A successful receive of a transfer packet will result in a Result Acknowledgement being written
+with the value `[]byte(byte(1))` in the `Response` field. 
+
+An unsuccessful receive of a transfer packet will result in an Error Acknowledgement being written
+with the error message in the `Response` field. 

x/ibc-transfer/spec/05_events.md

@@ -21,17 +21,18 @@ order: 5
 | fungible_token_packet | receiver      | {receiver}      |
 | fungible_token_packet | denom         | {denom}         |
 | fungible_token_packet | amount        | {amount}        |
+| fungible_token_packet | success       | {ackSuccess}    |
 | denomination_trace    | trace_hash    | {hex_hash}      |
 
 ## OnAcknowledgePacket callback
 
-| Type                  | Attribute Key | Attribute Value |
-|-----------------------|---------------|-----------------|
-| fungible_token_packet | module        | transfer        |
-| fungible_token_packet | receiver      | {receiver}      |
-| fungible_token_packet | denom         | {denom}         |
-| fungible_token_packet | amount        | {amount}        |
-| fungible_token_packet | success       | {ackSuccess}    |
+| Type                  | Attribute Key   | Attribute Value   |
+|-----------------------|-----------------|-------------------|
+| fungible_token_packet | module          | transfer          |
+| fungible_token_packet | receiver        | {receiver}        |
+| fungible_token_packet | denom           | {denom}           |
+| fungible_token_packet | amount          | {amount}          |
+| fungible_token_packet | success | error | {ack.Response}    |
 
 ## OnTimeoutPacket callback
 

x/ibc-transfer/types/events.go

@@ -15,6 +15,7 @@ const (
 	AttributeKeyRefundDenom    = "refund_denom"
 	AttributeKeyRefundAmount   = "refund_amount"
 	AttributeKeyAckSuccess     = "success"
+	AttributeKeyAck            = "acknowledgement"
 	AttributeKeyAckError       = "error"
 	AttributeKeyTraceHash      = "trace_hash"
 )

x/ibc-transfer/types/packet.go

@@ -52,8 +52,3 @@ func (ftpd FungibleTokenPacketData) ValidateBasic() error {
 func (ftpd FungibleTokenPacketData) GetBytes() []byte {
 	return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&ftpd))
 }
-
-// GetBytes is a helper for serialising
-func (ack FungibleTokenPacketAcknowledgement) GetBytes() []byte {
-	return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&ack))
-}