Add implementation for WriteUpgradeCancelChannel #3773
Conversation
| if err := k.restoreChannel(ctx, portID, channelID); err != nil { | ||
| return errorsmod.Wrap(types.ErrUpgradeRestoreFailed, fmt.Sprintf("failed to restore channel, channelID: %s, portID: %s", channelID, portID)) | ||
| } |
There was a problem hiding this comment.
do we want to just panic here too? I think mixing errors/panics is worse than using one or the other. Is a regular state reversion an okay outcome if a restore fails?
The current implementation of restore is using an error return, but I guess this could be a panic as well
My personal preference would be to use errors everywhere, even if the code should be unreachable (i.e. channel not found in a later handshake step)
There was a problem hiding this comment.
i like the idea of returning an error, because as we discussed before then it is clear from the method signature that WriteUpgrade... could fail.
…thub.com/cosmos/ibc-go into cian/issue#3649-implement-restorechannel
| @@ -388,3 +386,24 @@ func emitErrorReceiptEvent(ctx sdk.Context, portID string, channelID string, cur | |||
| ), | |||
| }) | |||
| } | |||
|
|
|||
| // emitUpgradedCancelledEvent emits an upgraded cancelled event. | |||
| func emitUpgradedCancelledEvent(ctx sdk.Context, portID string, channelID string, currentChannel types.Channel, upgrade types.Upgrade) { | |||
There was a problem hiding this comment.
To make it more consistent with the naming of the other functions:
| func emitUpgradedCancelledEvent(ctx sdk.Context, portID string, channelID string, currentChannel types.Channel, upgrade types.Upgrade) { | |
| func emitChannelUpgradeCancelEvent(ctx sdk.Context, portID string, channelID string, currentChannel types.Channel, upgrade types.Upgrade) { |
| EventTypeChannelUpgradeTry = "channel_upgrade_try" | ||
| EventTypeChannelUpgradeAck = "channel_upgrade_ack" | ||
| EventTypeChannelUpgradeOpen = "channel_upgrade_open" | ||
| EventTypeChannelUpgradeCancelled = "channel_upgrade_cancelled" |
There was a problem hiding this comment.
Even though I agree that events should be named in simple past tense, this is not the convention in the codebase at the moment...
| EventTypeChannelUpgradeCancelled = "channel_upgrade_cancelled" | |
| EventTypeChannelUpgradeCancel = "channel_upgrade_cancelled" |
|
|
||
| errorReceipt, found := channelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) | ||
| if upgradeError != nil { | ||
| suite.Require().True(found, "error receipt should be found") |
There was a problem hiding this comment.
Can we remove the if upgradeError != nil check completely? In the success case, the error receipt should be present, so suite.Require().True(found... will fail if it's not.
| if tc.channelPresent { | ||
| suite.Require().True(found, "channel should be found") | ||
| suite.Require().Equal(types.INITUPGRADE, channel.State, "channel state should not be restored to %s", types.INITUPGRADE.String()) | ||
| } |
There was a problem hiding this comment.
Should there be here an else block to check suite.Require().False(found, "channel should not be found") in case the test case has channelPresent set to false?
|
|
||
| channel, found := k.GetChannel(ctx, portID, channelID) | ||
| if !found { | ||
| panic(fmt.Sprintf("could not find existing channel when cancelling channel upgrade, channelID: %s, portID: %s", channelID, portID)) |
There was a problem hiding this comment.
Is it really necessary to add when cancelling channel upgrade to the error message? Wouldn't there be any other information in the log (like a stack trace) to figure that out?
There was a problem hiding this comment.
yeah you're right it's not really required
…ue#3752-implement-writeupgradecancelchannel
…teupgradecancelchannel
Description
This PR uses some methods introduced in #3728 so should be reviewed after.closes: #3752
Commit Message / Changelog Entry
N/A Feature Branch
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.