From c0358c34e5789cc90e2fac38366ff94187939fc3 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 23 May 2023 21:26:56 +0200 Subject: [PATCH] Handle ordered packets in UnreceivedPackets query. (backport #3346) (#3629) * fix: handle ordered packets in `UnreceivedPackets` query Co-authored-by: Cian Hatton (cherry picked from commit c77f80f812940fe3b93980d13a5cdd6980e907cc) * fix conflicts --------- Co-authored-by: Jim Fasarakis-Hilliard Co-authored-by: Carlos Rodriguez --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- CHANGELOG.md | 2 + docs/ibc/integration.md | 2 +- modules/core/04-channel/keeper/grpc_query.go | 53 ++++++++-- .../core/04-channel/keeper/grpc_query_test.go | 100 +++++++++++++++++- 5 files changed, 146 insertions(+), 13 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index c517591a095..c1a3ba9d84b 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -43,7 +43,7 @@ write a little note why. - [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting)). - [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. -- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules/10-structure.md). +- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules/11-structure.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing). - [ ] Updated relevant documentation (`docs/`) or specification (`x//spec/`). - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). diff --git a/CHANGELOG.md b/CHANGELOG.md index a343e34aa38..b79ede85162 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes +* [\#3346](https://github.com/cosmos/ibc-go/pull/3346) Properly handle ordered channels in `UnreceivedPackets` query. + ## [v4.1.1](https://github.com/cosmos/ibc-go/releases/tag/v4.1.1) - 2022-10-27 ### Dependencies diff --git a/docs/ibc/integration.md b/docs/ibc/integration.md index d985986f3fd..64c7ead5d89 100644 --- a/docs/ibc/integration.md +++ b/docs/ibc/integration.md @@ -139,7 +139,7 @@ func NewApp(...args) *App { ### Module Managers -In order to use IBC, we need to add the new modules to the module `Manager` and to the `SimulationManager` in case your application supports [simulations](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules/13-simulator.md). +In order to use IBC, we need to add the new modules to the module `Manager` and to the `SimulationManager` in case your application supports [simulations](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules/14-simulator.md). ```go // app.go diff --git a/modules/core/04-channel/keeper/grpc_query.go b/modules/core/04-channel/keeper/grpc_query.go index 6936fec0596..5a3412c6e48 100644 --- a/modules/core/04-channel/keeper/grpc_query.go +++ b/modules/core/04-channel/keeper/grpc_query.go @@ -397,18 +397,55 @@ func (q Keeper) UnreceivedPackets(c context.Context, req *types.QueryUnreceivedP ctx := sdk.UnwrapSDKContext(c) - unreceivedSequences := []uint64{} + channel, found := q.GetChannel(sdk.UnwrapSDKContext(c), req.PortId, req.ChannelId) + if !found { + return nil, status.Error( + codes.NotFound, + sdkerrors.Wrapf(types.ErrChannelNotFound, "port-id: %s, channel-id %s", req.PortId, req.ChannelId).Error(), + ) + } - for i, seq := range req.PacketCommitmentSequences { - if seq == 0 { - return nil, status.Errorf(codes.InvalidArgument, "packet sequence %d cannot be 0", i) - } + var unreceivedSequences []uint64 + switch channel.Ordering { + case types.UNORDERED: + for i, seq := range req.PacketCommitmentSequences { + // filter for invalid sequences to ensure they are not included in the response value. + if seq == 0 { + return nil, status.Errorf(codes.InvalidArgument, "packet sequence %d cannot be 0", i) + } - // if packet receipt exists on the receiving chain, then packet has already been received - if _, found := q.GetPacketReceipt(ctx, req.PortId, req.ChannelId, seq); !found { - unreceivedSequences = append(unreceivedSequences, seq) + // if the packet receipt does not exist, then it is unreceived + if _, found := q.GetPacketReceipt(ctx, req.PortId, req.ChannelId, seq); !found { + unreceivedSequences = append(unreceivedSequences, seq) + } + } + case types.ORDERED: + nextSequenceRecv, found := q.GetNextSequenceRecv(ctx, req.PortId, req.ChannelId) + if !found { + return nil, status.Error( + codes.NotFound, + sdkerrors.Wrapf( + types.ErrSequenceReceiveNotFound, + "destination port: %s, destination channel: %s", req.PortId, req.ChannelId, + ).Error(), + ) } + for i, seq := range req.PacketCommitmentSequences { + // filter for invalid sequences to ensure they are not included in the response value. + if seq == 0 { + return nil, status.Errorf(codes.InvalidArgument, "packet sequence %d cannot be 0", i) + } + + // Any sequence greater than or equal to the next sequence to be received is not received. + if seq >= nextSequenceRecv { + unreceivedSequences = append(unreceivedSequences, seq) + } + } + default: + return nil, status.Error( + codes.InvalidArgument, + sdkerrors.Wrapf(types.ErrInvalidChannelOrdering, "channel order %s is not supported", channel.Ordering.String()).Error()) } selfHeight := clienttypes.GetSelfHeight(ctx) diff --git a/modules/core/04-channel/keeper/grpc_query_test.go b/modules/core/04-channel/keeper/grpc_query_test.go index 8e7ac77a57a..b8ed0d40a64 100644 --- a/modules/core/04-channel/keeper/grpc_query_test.go +++ b/modules/core/04-channel/keeper/grpc_query_test.go @@ -1110,7 +1110,7 @@ func (suite *KeeperTestSuite) TestQueryPacketAcknowledgements() { func (suite *KeeperTestSuite) TestQueryUnreceivedPackets() { var ( req *types.QueryUnreceivedPacketsRequest - expSeq = []uint64{} + expSeq = []uint64(nil) ) testCases := []struct { @@ -1156,6 +1156,46 @@ func (suite *KeeperTestSuite) TestQueryUnreceivedPackets() { }, false, }, + { + "invalid seq, ordered channel", + func() { + path := ibctesting.NewPath(suite.chainA, suite.chainB) + path.SetChannelOrdered() + suite.coordinator.Setup(path) + + req = &types.QueryUnreceivedPacketsRequest{ + PortId: path.EndpointA.ChannelConfig.PortID, + ChannelId: path.EndpointA.ChannelID, + PacketCommitmentSequences: []uint64{0}, + } + }, + false, + }, + { + "channel not found", + func() { + req = &types.QueryUnreceivedPacketsRequest{ + PortId: "invalid-port-id", + ChannelId: "invalid-channel-id", + } + }, + false, + }, + { + "basic success empty packet commitments", + func() { + path := ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.Setup(path) + + expSeq = []uint64(nil) + req = &types.QueryUnreceivedPacketsRequest{ + PortId: path.EndpointA.ChannelConfig.PortID, + ChannelId: path.EndpointA.ChannelID, + PacketCommitmentSequences: []uint64{}, + } + }, + true, + }, { "basic success unreceived packet commitments", func() { @@ -1181,7 +1221,7 @@ func (suite *KeeperTestSuite) TestQueryUnreceivedPackets() { suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetPacketReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, 1) - expSeq = []uint64{} + expSeq = []uint64(nil) req = &types.QueryUnreceivedPacketsRequest{ PortId: path.EndpointA.ChannelConfig.PortID, ChannelId: path.EndpointA.ChannelID, @@ -1195,7 +1235,7 @@ func (suite *KeeperTestSuite) TestQueryUnreceivedPackets() { func() { path := ibctesting.NewPath(suite.chainA, suite.chainB) suite.coordinator.Setup(path) - expSeq = []uint64{} // reset + expSeq = []uint64(nil) // reset packetCommitments := []uint64{} // set packet receipt for every other sequence @@ -1217,6 +1257,60 @@ func (suite *KeeperTestSuite) TestQueryUnreceivedPackets() { }, true, }, + { + "basic success empty packet commitments, ordered channel", + func() { + path := ibctesting.NewPath(suite.chainA, suite.chainB) + path.SetChannelOrdered() + suite.coordinator.Setup(path) + + expSeq = []uint64(nil) + req = &types.QueryUnreceivedPacketsRequest{ + PortId: path.EndpointA.ChannelConfig.PortID, + ChannelId: path.EndpointA.ChannelID, + PacketCommitmentSequences: []uint64{}, + } + }, + true, + }, + { + "basic success unreceived packet commitments, ordered channel", + func() { + path := ibctesting.NewPath(suite.chainA, suite.chainB) + path.SetChannelOrdered() + suite.coordinator.Setup(path) + + // Note: NextSequenceRecv is set to 1 on channel creation. + expSeq = []uint64{1} + req = &types.QueryUnreceivedPacketsRequest{ + PortId: path.EndpointA.ChannelConfig.PortID, + ChannelId: path.EndpointA.ChannelID, + PacketCommitmentSequences: []uint64{1}, + } + }, + true, + }, + { + "basic success multiple unreceived packet commitments, ordered channel", + func() { + path := ibctesting.NewPath(suite.chainA, suite.chainB) + path.SetChannelOrdered() + suite.coordinator.Setup(path) + + // Exercise scenario from issue #1532. NextSequenceRecv is 5, packet commitments provided are 2, 7, 9, 10. + // Packet sequence 2 is already received so only sequences 7, 9, 10 should be considered unreceived. + expSeq = []uint64{7, 9, 10} + packetCommitments := []uint64{2, 7, 9, 10} + suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetNextSequenceRecv(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, 5) + + req = &types.QueryUnreceivedPacketsRequest{ + PortId: path.EndpointA.ChannelConfig.PortID, + ChannelId: path.EndpointA.ChannelID, + PacketCommitmentSequences: packetCommitments, + } + }, + true, + }, } for _, tc := range testCases {