Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle NextSequenceReceive for unordered channels. #3357

Merged
merged 2 commits into from
Mar 29, 2023
Merged

Handle NextSequenceReceive for unordered channels. #3357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
429a5f2
fix: handle NextSequenceReceive for unordered channels.
DimitrisJim Mar 26, 2023
96aec3b
Merge branch 'main' into jim/1783-confusion-regarding-next-sequence-recv
DimitrisJim Mar 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions modules/core/04-channel/keeper/grpc_query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,14 +480,25 @@ func (q Keeper) NextSequenceReceive(c context.Context, req *types.QueryNextSeque
}

ctx := sdk.UnwrapSDKContext(c)
sequence, found := q.GetNextSequenceRecv(ctx, req.PortId, req.ChannelId)
channel, found := q.GetChannel(ctx, req.PortId, req.ChannelId)
if !found {
return nil, status.Error(
codes.NotFound,
errorsmod.Wrapf(types.ErrSequenceReceiveNotFound, "port-id: %s, channel-id %s", req.PortId, req.ChannelId).Error(),
errorsmod.Wrapf(types.ErrChannelNotFound, "port-id: %s, channel-id %s", req.PortId, req.ChannelId).Error(),
)
}

// Return the next sequence received for ordered channels and 0 for unordered channels.
var sequence uint64
if channel.Ordering == types.ORDERED {
sequence, found = q.GetNextSequenceRecv(ctx, req.PortId, req.ChannelId)
Comment on lines +491 to +494
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @DimitrisJim! Code looks great!

Just want to note that if we add another channel ordering, like ORDERED_ALLOW_TIMEOUT we may need to adjust this handling. It's not incredibly clear whether ORDERED channels or UNORDERED channels are the exception here, but since we are referring to the next sequence receive, it probably makes sense to add an if statement for the channel types which do not use that value (UNORDERED). I see the referenced issue suggested this structure of code.

An alternative solution:

if channel.Ordering == types.UNORDERED {
    // unordered channels do not make use of the next sequence receive
    return 0, nil
}

sequence, found := q.GetNextSequenceRecv(ctx, req.PortId, req.ChannelId)

// etc

In this scenario, since we perform the query by default, ORDERED_ALLOW_TIMEOUT would function properly without additional changes. I believe we will need to modify this code with the addition of ordered allow timeout channels (not yet implemented)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, can totally see that point, wasn't aware of the possibility of another ordered variant. Does it make sense to fix it pronto? (Note that some grep-ing found one other case where we special case on ORDERED during packet acknowledgement but I'm unsure if the same semantics would apply).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, the new ordered variant is a bit of a new concept for us as well, which is why this issue wasn't so obvious. I only barely made the connection thinking about explicit return values (I prefer the code to be explicit with what it returns rather than being implicit by not executing some conditional)

I think it could make sense to fix since we know it will be an issue? Might save someone some time debugging later. I'm also perfectly happy having an issue opened.

(Note that some grep-ing found one other case where we special case on ORDERED during packet acknowledgement but I'm unsure if the same semantics would apply).

When implementing the new channel ordering, we will need to modify this code (already specified in the spec). We will likely need to do some grep-ing to look into all switches/conditionals on channel ordering. Core IBC isn't too well setup abstraction wise for new channel types. It's somewhat unclear how many different ordering types will ever exist

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix this up today. Better to keep things future proof as much as possible.

if !found {
return nil, status.Error(
codes.NotFound,
errorsmod.Wrapf(types.ErrSequenceReceiveNotFound, "port-id: %s, channel-id %s", req.PortId, req.ChannelId).Error(),
)
}
}
selfHeight := clienttypes.GetSelfHeight(ctx)
return types.NewQueryNextSequenceReceiveResponse(sequence, nil, selfHeight), nil
}
Expand Down
23 changes: 20 additions & 3 deletions modules/core/04-channel/keeper/grpc_query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1419,12 +1419,29 @@ func (suite *KeeperTestSuite) TestQueryNextSequenceReceive() {
false,
},
{
"success",
"basic success on unordered channel returns zero",
func() {
path := ibctesting.NewPath(suite.chainA, suite.chainB)
suite.coordinator.Setup(path)
expSeq = 1
suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetNextSequenceRecv(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, expSeq)

expSeq = 0
req = &types.QueryNextSequenceReceiveRequest{
PortId: path.EndpointA.ChannelConfig.PortID,
ChannelId: path.EndpointA.ChannelID,
}
},
true,
},
{
"basic success on ordered channel returns the set receive sequence",
func() {
path := ibctesting.NewPath(suite.chainA, suite.chainB)
path.SetChannelOrdered()
suite.coordinator.Setup(path)

expSeq = 3
seq := uint64(3)
suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetNextSequenceRecv(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, seq)

req = &types.QueryNextSequenceReceiveRequest{
PortId: path.EndpointA.ChannelConfig.PortID,
Expand Down