refactor: WriteAcknowledgement API (cosmos#882)

* refactor: WriteAcknowledgement takes exported.Acknowledgement instead of bytes

* fix: adding check for empty byte string

* chore: update changelog

* fixing test case + adding migration docs

* testing: Adding MockEmptyAcknowledgement to testing library

* docs: fix version

* test: add check for ack is nil

CHANGELOG.md

@@ -53,7 +53,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (modules/core/02-client) [\#536](https://github.com/cosmos/ibc-go/pull/536) `GetSelfConsensusState` return type changed from bool to error.
 * (channel) [\#644](https://github.com/cosmos/ibc-go/pull/644) Removes `CounterpartyHops` function from the ChannelKeeper.
 * (testing) [\#776](https://github.com/cosmos/ibc-go/pull/776) Adding helper fn to generate capability name for testing callbacks 
-
+* (channel) [\#882](https://github.com/cosmos/ibc-go/pull/882) The `WriteAcknowledgement` API now takes `exported.Acknowledgement` instead of a byte array
 
 ### State Machine Breaking
 

docs/migrations/v3-to-v4.md

@@ -0,0 +1,26 @@
+# Migrating from ibc-go v2 to v3
+
+This document is intended to highlight significant changes which may require more information than presented in the CHANGELOG.
+Any changes that must be done by a user of ibc-go should be documented here.
+
+There are four sections based on the four potential user groups of this document:
+- Chains
+- IBC Apps
+- Relayers
+- IBC Light Clients
+
+**Note:** ibc-go supports golang semantic versioning and therefore all imports must be updated to bump the version number on major releases.
+```go
+github.com/cosmos/ibc-go/v3 -> github.com/cosmos/ibc-go/v4
+```
+
+No genesis or in-place migrations required when upgrading from v1 or v2 of ibc-go.
+
+## Chains
+
+### IS04 - Channel 
+
+The `WriteAcknowledgement` API now takes the `exported.Acknowledgement` type instead of passing in the acknowledgement byte array directly. 
+This is an API breaking change and as such IBC application developers will have to update any calls to `WriteAcknowledgement`. 
+
+

modules/core/04-channel/keeper/packet.go

@@ -312,7 +312,7 @@ func (k Keeper) WriteAcknowledgement(
 	ctx sdk.Context,
 	chanCap *capabilitytypes.Capability,
 	packet exported.PacketI,
-	acknowledgement []byte,
+	acknowledgement exported.Acknowledgement,
 ) error {
 	channel, found := k.GetChannel(ctx, packet.GetDestPort(), packet.GetDestChannel())
 	if !found {
@@ -342,14 +342,19 @@ func (k Keeper) WriteAcknowledgement(
 		return types.ErrAcknowledgementExists
 	}
 
-	if len(acknowledgement) == 0 {
+	if acknowledgement == nil {
+		return sdkerrors.Wrap(types.ErrInvalidAcknowledgement, "acknowledgement cannot be nil")
+	}
+
+	bz := acknowledgement.Acknowledgement()
+	if len(bz) == 0 {
 		return sdkerrors.Wrap(types.ErrInvalidAcknowledgement, "acknowledgement cannot be empty")
 	}
 
 	// set the acknowledgement so that it can be verified on the other side
 	k.SetPacketAcknowledgement(
 		ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(),
-		types.CommitAcknowledgement(acknowledgement),
+		types.CommitAcknowledgement(bz),
 	)
 
 	// log that a packet acknowledgement has been written
@@ -362,7 +367,7 @@ func (k Keeper) WriteAcknowledgement(
 		"dst_channel", packet.GetDestChannel(),
 	)
 
-	EmitWriteAcknowledgementEvent(ctx, packet, channel, acknowledgement)
+	EmitWriteAcknowledgementEvent(ctx, packet, channel, bz)
 
 	return nil
 }

modules/core/04-channel/keeper/packet_test.go

@@ -493,7 +493,7 @@ func (suite *KeeperTestSuite) TestRecvPacket() {
 func (suite *KeeperTestSuite) TestWriteAcknowledgement() {
 	var (
 		path       *ibctesting.Path
-		ack        []byte
+		ack        exported.Acknowledgement
 		packet     exported.PacketI
 		channelCap *capabilitytypes.Capability
 	)
@@ -504,7 +504,7 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgement() {
 			func() {
 				suite.coordinator.Setup(path)
 				packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
-				ack = ibctesting.MockAcknowledgement
+				ack = ibcmock.MockAcknowledgement
 				channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
 			},
 			true,
@@ -513,13 +513,13 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgement() {
 			// use wrong channel naming
 			suite.coordinator.Setup(path)
 			packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, ibctesting.InvalidID, ibctesting.InvalidID, timeoutHeight, disabledTimeoutTimestamp)
-			ack = ibctesting.MockAcknowledgement
+			ack = ibcmock.MockAcknowledgement
 			channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
 		}, false},
 		{"channel not open", func() {
 			suite.coordinator.Setup(path)
 			packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
-			ack = ibctesting.MockAcknowledgement
+			ack = ibcmock.MockAcknowledgement
 
 			err := path.EndpointB.SetChannelClosed()
 			suite.Require().NoError(err)
@@ -530,7 +530,7 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgement() {
 			func() {
 				suite.coordinator.Setup(path)
 				packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
-				ack = ibctesting.MockAcknowledgement
+				ack = ibcmock.MockAcknowledgement
 				channelCap = capabilitytypes.NewCapability(3)
 			},
 			false,
@@ -540,14 +540,24 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgement() {
 			func() {
 				suite.coordinator.Setup(path)
 				packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
-				ack = ibctesting.MockAcknowledgement
-				suite.chainB.App.GetIBCKeeper().ChannelKeeper.SetPacketAcknowledgement(suite.chainB.GetContext(), packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(), ack)
+				ack = ibcmock.MockAcknowledgement
+				suite.chainB.App.GetIBCKeeper().ChannelKeeper.SetPacketAcknowledgement(suite.chainB.GetContext(), packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(), ack.Acknowledgement())
 				channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
 			},
 			false,
 		},
 		{
 			"empty acknowledgement",
+			func() {
+				suite.coordinator.Setup(path)
+				packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
+				ack = ibcmock.NewMockEmptyAcknowledgement()
+				channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
+			},
+			false,
+		},
+		{
+			"acknowledgement is nil",
 			func() {
 				suite.coordinator.Setup(path)
 				packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)

modules/core/05-port/types/module.go

@@ -110,7 +110,7 @@ type ICS4Wrapper interface {
 		ctx sdk.Context,
 		chanCap *capabilitytypes.Capability,
 		packet exported.PacketI,
-		ack []byte,
+		ack exported.Acknowledgement,
 	) error
 }
 

modules/core/keeper/msg_server.go

@@ -417,7 +417,7 @@ func (k Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPacke
 	// NOTE: IBC applications modules may call the WriteAcknowledgement asynchronously if the
 	// acknowledgement is nil.
 	if ack != nil {
-		if err := k.ChannelKeeper.WriteAcknowledgement(ctx, cap, msg.Packet, ack.Acknowledgement()); err != nil {
+		if err := k.ChannelKeeper.WriteAcknowledgement(ctx, cap, msg.Packet, ack); err != nil {
 			return nil, err
 		}
 	}

testing/endpoint.go

@@ -411,7 +411,7 @@ func (endpoint *Endpoint) WriteAcknowledgement(ack exported.Acknowledgement, pac
 	channelCap := endpoint.Chain.GetChannelCapability(packet.GetDestPort(), packet.GetDestChannel())
 
 	// no need to send message, acting as a handler
-	err := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.WriteAcknowledgement(endpoint.Chain.GetContext(), channelCap, packet, ack.Acknowledgement())
+	err := endpoint.Chain.App.GetIBCKeeper().ChannelKeeper.WriteAcknowledgement(endpoint.Chain.GetContext(), channelCap, packet, ack)
 	if err != nil {
 		return err
 	}

testing/mock/ack.go

@@ -0,0 +1,23 @@
+package mock
+
+// MockEmptyAcknowledgement implements the exported.Acknowledgement interface and always returns an empty byte string as Response
+type MockEmptyAcknowledgement struct {
+	Response []byte
+}
+
+// NewMockEmptyAcknowledgement returns a new instance of MockEmptyAcknowledgement
+func NewMockEmptyAcknowledgement() MockEmptyAcknowledgement {
+	return MockEmptyAcknowledgement{
+		Response: []byte{},
+	}
+}
+
+// Success implements the Acknowledgement interface
+func (ack MockEmptyAcknowledgement) Success() bool {
+	return true
+}
+
+// Acknowledgement implements the Acknowledgement interface
+func (ack MockEmptyAcknowledgement) Acknowledgement() []byte {
+	return []byte{}
+}