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

Make AbortUpgrade panic on failure #4011

Merged
merged 6 commits into from
Jul 6, 2023
Merged

Make AbortUpgrade panic on failure #4011

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b06ac7d
chore: update abort upgrade function to panic on error
chatton Jul 5, 2023
2f3e2b3
Merge branch '04-channel-upgrades' into cian/issue#3903-AbortUpgrade-…
chatton Jul 5, 2023
5b5d60b
Merge branch '04-channel-upgrades' into cian/issue#3903-AbortUpgrade-…
chatton Jul 5, 2023
488a050
Merge branch '04-channel-upgrades' into cian/issue#3903-AbortUpgrade-…
chatton Jul 5, 2023
d4022a4
apply review suggestions
Jul 6, 2023
cb7575c
Merge branch '04-channel-upgrades' into cian/issue#3903-AbortUpgrade-…
Jul 6, 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
13 changes: 11 additions & 2 deletions modules/core/04-channel/keeper/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -802,9 +802,18 @@ func (k Keeper) constructProposedUpgrade(ctx sdk.Context, portID, channelID stri
}, nil
}

// AbortUpgrade will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted.
// MustAbortUpgrade will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted.
// any unnecessary state is deleted. An error receipt is written, and the OnChanUpgradeRestore callback is called.
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
func (k Keeper) AbortUpgrade(ctx sdk.Context, portID, channelID string, err error) error {
// This function is expected to always succeed, a panic will occur if an error occurs.
func (k Keeper) MustAbortUpgrade(ctx sdk.Context, portID, channelID string, err error) {
if err := k.abortUpgrade(ctx, portID, channelID, err); err != nil {
panic(err)
}
}

// abortUpgrade will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted.
// any unnecessary state is deleted. An error receipt is written, and the OnChanUpgradeRestore callback is called.
func (k Keeper) abortUpgrade(ctx sdk.Context, portID, channelID string, err error) error {
Copy link
Contributor

@DimitrisJim DimitrisJim Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

do we see abortUpgrade ever being used w/o panicking, i.e point in keeping it separate?

Copy link
Contributor Author

@chatton chatton Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think so. I left it here as I typically like having panics happen in a single place. I find it easier to deal with regular errors as much as possible, and panic only when we need to.

if err == nil {
return errorsmod.Wrap(types.ErrInvalidUpgradeError, "cannot abort upgrade handshake with nil error")
}
Expand Down
12 changes: 8 additions & 4 deletions modules/core/04-channel/keeper/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1305,10 +1305,10 @@ func (suite *KeeperTestSuite) TestAbortHandshake() {

tc.malleate()

err := channelKeeper.AbortUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, upgradeError)
Copy link
Contributor

Choose a reason for hiding this comment

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

guess we could also define the function as a var and trim down on the NotPanics/Panics calls in a similar vein. Leaving to your discretion.


if tc.expPass {
suite.Require().NoError(err)
suite.Require().NotPanics(func() {
channelKeeper.MustAbortUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, upgradeError)
})

channel, found := channelKeeper.GetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
suite.Require().True(found, "channel should be found")
Expand All @@ -1329,7 +1329,11 @@ func (suite *KeeperTestSuite) TestAbortHandshake() {
suite.Require().False(found, "counterparty last packet sequence should not be found")

} else {
suite.Require().Error(err)

suite.Require().Panics(func() {
channelKeeper.MustAbortUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, upgradeError)
})

channel, found := channelKeeper.GetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
if found { // test cases uses a channel that exists
suite.Require().Equal(types.INITUPGRADE, channel.State, "channel state should not be restored to %s", types.INITUPGRADE.String())
Expand Down
16 changes: 4 additions & 12 deletions modules/core/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -769,9 +769,7 @@ func (k Keeper) ChannelUpgradeTry(goCtx context.Context, msg *channeltypes.MsgCh
if err != nil {
ctx.Logger().Error("channel upgrade try failed", "error", errorsmod.Wrap(err, "channel upgrade try failed"))
if upgradeErr, ok := err.(*channeltypes.UpgradeError); ok {
if err := k.ChannelKeeper.AbortUpgrade(ctx, msg.PortId, msg.ChannelId, upgradeErr); err != nil {
return nil, errorsmod.Wrap(err, "channel upgrade try (abort upgrade) failed")
}
k.ChannelKeeper.MustAbortUpgrade(ctx, msg.PortId, msg.ChannelId, upgradeErr)

// NOTE: a FAILURE result is returned to the client and an error receipt is written to state.
// This signals to the relayer to begin the cancel upgrade handshake subprotocol.
Expand All @@ -786,9 +784,7 @@ func (k Keeper) ChannelUpgradeTry(goCtx context.Context, msg *channeltypes.MsgCh
upgradeVersion, err := cbs.OnChanUpgradeTry(cacheCtx, msg.PortId, msg.ChannelId, upgrade.Fields.Ordering, upgrade.Fields.ConnectionHops, upgrade.Fields.Version)
if err != nil {
ctx.Logger().Error("channel upgrade try callback failed", "port-id", msg.PortId, "channel-id", msg.ChannelId, "error", err.Error())
if err := k.ChannelKeeper.AbortUpgrade(ctx, msg.PortId, msg.ChannelId, err); err != nil {
return nil, err
}
k.ChannelKeeper.MustAbortUpgrade(ctx, msg.PortId, msg.ChannelId, err)

return &channeltypes.MsgChannelUpgradeTryResponse{Result: channeltypes.FAILURE}, nil
}
Expand Down Expand Up @@ -826,9 +822,7 @@ func (k Keeper) ChannelUpgradeAck(goCtx context.Context, msg *channeltypes.MsgCh
if err != nil {
ctx.Logger().Error("channel upgrade ack failed", "error", errorsmod.Wrap(err, "channel upgrade ack failed"))
if upgradeErr, ok := err.(*channeltypes.UpgradeError); ok {
if err := k.ChannelKeeper.AbortUpgrade(ctx, msg.PortId, msg.ChannelId, upgradeErr); err != nil {
return nil, errorsmod.Wrap(err, "channel upgrade ack (abort upgrade) failed")
}
k.ChannelKeeper.MustAbortUpgrade(ctx, msg.PortId, msg.ChannelId, upgradeErr)

// NOTE: a FAILURE result is returned to the client and an error receipt is written to state.
// This signals to the relayer to begin the cancel upgrade handshake subprotocol.
Expand All @@ -843,9 +837,7 @@ func (k Keeper) ChannelUpgradeAck(goCtx context.Context, msg *channeltypes.MsgCh
err = cbs.OnChanUpgradeAck(cacheCtx, msg.PortId, msg.ChannelId, msg.CounterpartyUpgrade.Fields.Version)
if err != nil {
ctx.Logger().Error("channel upgrade ack callback failed", "port-id", msg.PortId, "channel-id", msg.ChannelId, "error", err.Error())
if err := k.ChannelKeeper.AbortUpgrade(ctx, msg.PortId, msg.ChannelId, err); err != nil {
return nil, errorsmod.Wrapf(err, "channel upgrade ack callback (abort upgrade) failed for port ID: %s, channel ID: %s", msg.PortId, msg.ChannelId)
}
k.ChannelKeeper.MustAbortUpgrade(ctx, msg.PortId, msg.ChannelId, err)

return &channeltypes.MsgChannelUpgradeAckResponse{Result: channeltypes.FAILURE}, nil
}
Expand Down