imp: add logic for setting NextSeqRecv, NextSeqAck in OnChanUpgradeOpen
#5438
Conversation
charleenfei
changed the title
OnChanUpgradeOpen
charleenfei
changed the title
OnChanUpgradeOpenNextSeqRecv, NextSeqAck in OnChanUpgradeOpen
…o charly/set_nextseqrecv
charleenfei
requested review from
colin-axner,
AdityaSripal and
damiannolan
as code owners
| @@ -351,30 +351,6 @@ func (suite *KeeperTestSuite) TestRecvPacket() { | |||
| channel := path.EndpointB.GetChannel() | |||
| channel.State = types.FLUSHING | |||
| path.EndpointB.SetChannel(channel) | |||
|
|
|||
There was a problem hiding this comment.
i removed these tests bc we aren't checking these sequences in RecvPacket anymore
charleenfei
requested review from
crodriguezvega,
chatton and
DimitrisJim
as code owners
…o charly/set_nextseqrecv
There was a problem hiding this comment.
Only a comment on even more assertion in test, but its implicitly checked. Shouldn't block merge if not written, with an optional isssue opened to add this if people find it important. I'm personally ambivalent, just saw that the test comment may be expecting that to be the case
| ctx := suite.chainA.GetContext() | ||
|
|
||
| // assert that sequence (reset in ChanUpgradeTry) is 1 because channel is UNORDERED |
There was a problem hiding this comment.
I don't understand the reset comment
There was a problem hiding this comment.
Should we also be assert the counterpartyNextSequenceSend is 2 here?
There was a problem hiding this comment.
i believe we set the sequences to 1 so that the upgrade proof can be verified without the exact last sequences
There was a problem hiding this comment.
Fantastic, @charleenfei! Thank you for adapting the code so quickly to the spec updates.
There was a problem hiding this comment.
Nice work! Agree with @DimitrisJim's testing comments for improvements :)
LGTM
…deOpen. (#5460) * Add key for storing counterparty next sequence send. * nit: function order localtion * SetNextSequenceSend -> SetCounterpartyNextSequenceSend * Add assertions to check for correct values set in tests. * Rename keys. * Apply suggestions from code review Co-authored-by: colin axnér <[email protected]> --------- Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: colin axnér <[email protected]>
|
i'm gonna merge this one first after checks pass before this one, as there will be some reworking of that logic |
Description
ref spec changes
note that the counterparty last send seq and other pruning related topics will be addressed in upcoming PRs.
this PR covers just the logic for setting
NextSeqRecvandNextSeqAckin the upgrade handlers.Commit Message / Changelog Entry
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.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.