refactor: remove unnecessary decrement when fast forwarding upgrade sequence #5231
Conversation
| // NOTE: OnChanUpgradeInit will not be executed by the application | ||
| upgrade, err = k.ChanUpgradeInit(ctx, portID, channelID, proposedUpgradeFields) | ||
| if err != nil { | ||
| return types.Upgrade{}, errorsmod.Wrap(err, "failed to initialize upgrade") | ||
| } | ||
|
|
||
| // if the counterparty sequence is greater than the current sequence, we fast-forward to the counterparty sequence. | ||
| if counterpartyUpgradeSequence > channel.UpgradeSequence { | ||
| k.SetChannel(ctx, portID, channelID, channel) |
There was a problem hiding this comment.
i believe we still need to fastforward to the counterparty sequence, in case of the sequence being very much out of sync (timeouts).
There was a problem hiding this comment.
What do you think about this @chatton ?
There was a problem hiding this comment.
@charleenfei is right and I think you need to fast forward to the counterparty sequence before SetChannel. I think the reason the tests are passing at the moment is because we are missing a check to make sure that the upgrade sequences of both channel ends are in sync after ChannelUpgradeTry succeeds. If you add after this line a check like this suite.Require().Equal(path.EndpointA.GetChannel().UpgradeSequence, path.EndpointB.GetChannel().UpgradeSequence) you will see that this test will fail.
But I am thinking that probably we still need to do channel.UpgradeSequence = counterpartyUpgradeSequence - 1 anyway, because channel.UpgradeSequence is actually incremented in WriteUpgradeInitChannel below... Unless we move this code after WriteUpgradeInitChannel, I think.
There was a problem hiding this comment.
I re-added channel.UpgradeSequence = counterpartyUpgradeSequence - 1
There was a problem hiding this comment.
Thank you, @ThanhNhann. To be honest, I don't know what would be best now: either leave the code where it was before or move it after WriteUpgradeInitChannel and then we probably don't need to do channel.UpgradeSequence = counterpartyUpgradeSequence - 1 and we can do just channel.UpgradeSequence = counterpartyUpgradeSequence. @damiannolan any thoughts?
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## 04-channel-upgrades #5231 +/- ##
====================================================
Coverage 80.39% 80.39%
====================================================
Files 197 197
Lines 14984 14985 +1
====================================================
+ Hits 12046 12047 +1
Misses 2474 2474
Partials 464 464
|
ChanUpgradeTry function
Description
closes: #5218
closes: #5299
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.