From 95b7e8e29c44d50462d9722ffb22d9b6314ff0d9 Mon Sep 17 00:00:00 2001 From: Sean King Date: Mon, 7 Feb 2022 17:43:42 +0100 Subject: [PATCH 1/7] refactor: WriteAcknowledgement takes exported.Acknowledgement instead of bytes --- modules/core/04-channel/keeper/packet.go | 10 ++++---- modules/core/04-channel/keeper/packet_test.go | 24 ++++++------------- modules/core/05-port/types/module.go | 2 +- modules/core/keeper/msg_server.go | 2 +- testing/endpoint.go | 2 +- 5 files changed, 14 insertions(+), 26 deletions(-) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index fd1322dcbbc..d7fdcba0672 100644 --- 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,12 @@ func (k Keeper) WriteAcknowledgement( return types.ErrAcknowledgementExists } - if len(acknowledgement) == 0 { - return sdkerrors.Wrap(types.ErrInvalidAcknowledgement, "acknowledgement cannot be empty") - } + bz := acknowledgement.Acknowledgement() // 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 +360,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 index 69587080cf9..992c38042ec 100644 --- 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,18 +540,8 @@ 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) - 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 = nil + 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, diff --git a/modules/core/05-port/types/module.go b/modules/core/05-port/types/module.go index 9c7442a9d76..dea418b7250 100644 --- 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 index 9342c0ff069..b19041f636d 100644 --- 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 index 39fe2b4408c..962ecf5ef35 100644 --- 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 } From 9f44d36c30e0360a0014a8c6cc2dbdb9073cc336 Mon Sep 17 00:00:00 2001 From: Sean King Date: Tue, 8 Feb 2022 12:14:34 +0100 Subject: [PATCH 2/7] fix: adding check for empty byte string --- modules/core/04-channel/keeper/packet.go | 3 +++ modules/core/04-channel/keeper/packet_test.go | 11 +++++++++++ 2 files changed, 14 insertions(+) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index d7fdcba0672..cdb3692e375 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -343,6 +343,9 @@ func (k Keeper) WriteAcknowledgement( } 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( diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index 992c38042ec..d103755b6f8 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -546,6 +546,17 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgement() { }, 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 = types.NewResultAcknowledgement([]byte{}) + 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, + }, } for i, tc := range testCases { tc := tc From b16f85e3b3dd27dace07f9b74dc4f32b3efcce39 Mon Sep 17 00:00:00 2001 From: Sean King Date: Tue, 8 Feb 2022 12:23:12 +0100 Subject: [PATCH 3/7] chore: update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index adb0e0243ea..3c65af32a02 100644 --- a/CHANGELOG.md +++ b/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 From 37ba44f24cc9d32d4c54f48148d493fd06cf8544 Mon Sep 17 00:00:00 2001 From: Sean King Date: Tue, 8 Feb 2022 12:49:19 +0100 Subject: [PATCH 4/7] fixing test case + adding migration docs --- docs/migrations/v3-to-v4.md | 26 +++++++++++++++++++ modules/core/04-channel/keeper/packet_test.go | 3 +-- 2 files changed, 27 insertions(+), 2 deletions(-) create mode 100644 docs/migrations/v3-to-v4.md diff --git a/docs/migrations/v3-to-v4.md b/docs/migrations/v3-to-v4.md new file mode 100644 index 00000000000..b7cda1af9dd --- /dev/null +++ b/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/v2 -> github.com/cosmos/ibc-go/v3 +``` + +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`. + + diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index d103755b6f8..7f17144be0a 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -551,8 +551,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 = types.NewResultAcknowledgement([]byte{}) - suite.chainB.App.GetIBCKeeper().ChannelKeeper.SetPacketAcknowledgement(suite.chainB.GetContext(), packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(), ack.Acknowledgement()) + ack = types.NewResultAcknowledgement(nil) channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) }, false, From 8d5c66da501a0162f2f66749ae2d77afa1ef88fe Mon Sep 17 00:00:00 2001 From: Sean King Date: Tue, 8 Feb 2022 18:14:49 +0100 Subject: [PATCH 5/7] testing: Adding MockEmptyAcknowledgement to testing library --- modules/core/04-channel/keeper/packet_test.go | 2 +- testing/mock/ack.go | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 testing/mock/ack.go diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index 7f17144be0a..3746bd2352b 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -551,7 +551,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 = types.NewResultAcknowledgement(nil) + ack = ibcmock.NewMockEmptyAcknowledgement() channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) }, false, diff --git a/testing/mock/ack.go b/testing/mock/ack.go new file mode 100644 index 00000000000..c25176a02da --- /dev/null +++ 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{} +} From fea63219c2d311f7f24de627b9900c7b7f527041 Mon Sep 17 00:00:00 2001 From: Sean King Date: Wed, 9 Feb 2022 12:58:24 +0100 Subject: [PATCH 6/7] docs: fix version --- docs/migrations/v3-to-v4.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/migrations/v3-to-v4.md b/docs/migrations/v3-to-v4.md index b7cda1af9dd..90e9af256d2 100644 --- a/docs/migrations/v3-to-v4.md +++ b/docs/migrations/v3-to-v4.md @@ -11,7 +11,7 @@ There are four sections based on the four potential user groups of this document **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/v2 -> github.com/cosmos/ibc-go/v3 +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. From d3b762511b705438f0c5e86623379c36642cf98a Mon Sep 17 00:00:00 2001 From: Sean King Date: Wed, 9 Feb 2022 13:07:36 +0100 Subject: [PATCH 7/7] test: add check for ack is nil --- modules/core/04-channel/keeper/packet.go | 4 ++++ modules/core/04-channel/keeper/packet_test.go | 10 ++++++++++ 2 files changed, 14 insertions(+) diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index cdb3692e375..5879c9ecb08 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -342,6 +342,10 @@ func (k Keeper) WriteAcknowledgement( return types.ErrAcknowledgementExists } + 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") diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go index 3746bd2352b..db6cce545c9 100644 --- a/modules/core/04-channel/keeper/packet_test.go +++ b/modules/core/04-channel/keeper/packet_test.go @@ -556,6 +556,16 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgement() { }, 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) + ack = nil + channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + }, + false, + }, } for i, tc := range testCases { tc := tc