Handle NextSequenceReceive for unordered channels.

[DimitrisJim]

Description

closes: #1783

Commit Message / Changelog Entry

fix: handle unordered channels in `NextSequenceReceive` query

see the guidelines for commit messages. (view raw markdown for examples)

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md).
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards and Go style guide.
• Wrote unit and integration tests.
• Updated relevant documentation (docs/) or specification (x/<module>/spec/).
• Added relevant godoc comments.
• Provide a commit message to be used for the changelog entry in the PR description for review.
• Re-reviewed Files changed in the Github PR explorer.
• Review Codecov Report in the comment section below once CI passes.

[charleenfei]

nice work! lgtm

[AdityaSripal]

LGTM!

[DimitrisJim]

Thanks for the reviews! 🙌

[colin-axner]

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)

[DimitrisJim]

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).

[colin-axner]

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

[DimitrisJim]

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