refactor: WriteAcknowledgement API (backport #882) (#943)

* refactor: WriteAcknowledgement API (#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 (cherry picked from commit acbc9b6) # Conflicts: # CHANGELOG.md * fix changelog merge conflict * backport migration docs Co-authored-by: Sean King <seantking@users.noreply.github.com> Co-authored-by: Colin Axnér <25233464+colin-axner@users.noreply.github.com>
cosmos · Feb 17, 2022 · 5534418 · 5534418
1 parent f68514c
commit 5534418
Show file tree

Hide file tree

Showing 8 changed files with 59 additions and 14 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -54,6 +54,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (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 
 * (testing) [\#892](https://github.com/cosmos/ibc-go/pull/892) IBC Mock modules store the scoped keeper and portID within the IBCMockApp. They also maintain reference to the AppModule to update the AppModule's list of IBC applications it references. Allows for the mock module to be reused as a base application in middleware stacks.
+* (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
 

diff --git a/docs/migrations/v2-to-v3.md b/docs/migrations/v2-to-v3.md
@@ -18,6 +18,12 @@ No genesis or in-place migrations are required when upgrading from v1 or v2 of i
 
 ## 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`. 
+
+
 ### ICS20
 
 The `transferkeeper.NewKeeper(...)` now takes in an ICS4Wrapper. 

diff --git a/modules/core/04-channel/keeper/packet.go b/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
 }

diff --git a/modules/core/04-channel/keeper/packet_test.go b/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)

diff --git a/modules/core/05-port/types/module.go b/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
 }
 

diff --git a/modules/core/keeper/msg_server.go b/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
 		}
 	}

diff --git a/testing/endpoint.go b/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
 	}

diff --git a/testing/mock/ack.go b/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{}
+}