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.

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

Implement MsgChannelUpgradeCancel message server handler #3848

Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
5cd004e
wip: adding restoreChannel and writeErrorReceipt functions
chatton Jun 1, 2023
cbc8e32
merge 04-channel-upgrades
chatton Jun 6, 2023
d01355f
emit error receipt events on abort
chatton Jun 6, 2023
6f7940b
remove unused function
chatton Jun 6, 2023
6d50848
added happy path test for abort
chatton Jun 6, 2023
795cd05
fleshed out TestAbortHandshake
chatton Jun 7, 2023
dfbc5d4
corrected docstring and removed unrequired checks in test
chatton Jun 7, 2023
b2a99ca
merge feature branch
chatton Jun 7, 2023
d429514
chore: added implementation of WriteUpgradeCancelChannel
chatton Jun 7, 2023
4330c51
Merge branch '04-channel-upgrades' into cian/issue#3649-implement-res…
chatton Jun 7, 2023
c1e05b8
fix linting
chatton Jun 7, 2023
619381b
Merge branch 'cian/issue#3649-implement-restorechannel' of https://gi…
chatton Jun 7, 2023
91605c1
addressing PR feedback
chatton Jun 7, 2023
d050ee8
merge with restore branch
chatton Jun 7, 2023
eaa2b91
addressing PR feedback
chatton Jun 7, 2023
d6bd3bf
merge 04-channel-upgrades
chatton Jun 7, 2023
cfe59ae
Merge branch 'cian/issue#3649-implement-restorechannel' into cian/iss…
chatton Jun 7, 2023
7d2c3d4
chore: adding implementation of ChanUpgradeCancel
chatton Jun 7, 2023
099b147
Merge branch '04-channel-upgrades' into cian/issue#3649-implement-res…
chatton Jun 7, 2023
759474f
added scaffolding for TestChanUpgradeCancel
chatton Jun 7, 2023
3916332
addressing PR feedback
chatton Jun 12, 2023
700acba
merge 04-channel-upgrades
chatton Jun 12, 2023
6b2ce2e
Merge branch 'cian/issue#3649-implement-restorechannel' into cian/iss…
chatton Jun 12, 2023
811c418
addressing PR feedback
chatton Jun 12, 2023
0f0305f
Merge branch '04-channel-upgrades' into cian/issue#3752-implement-wri…
chatton Jun 12, 2023
d6bd69c
merge 04-channel-upgrades
chatton Jun 14, 2023
51e9070
fix merge conflicts
chatton Jun 14, 2023
3e0bf6e
ran linter
chatton Jun 14, 2023
4e12bc0
merge 04-channel-upgrades
chatton Jun 14, 2023
df123ba
merge 04-channel-upgrades
chatton Jun 14, 2023
098d0a3
happy path test
chatton Jun 14, 2023
853eae2
adding tests for ChanUpgradeCancel
chatton Jun 14, 2023
c2ae441
add ChannelCancelUpgrade message server implementation
chatton Jun 14, 2023
85afe0f
chore: add msg server tests for ChannelUpgradeCancel
chatton Jun 14, 2023
8ced59b
chore: merge 04-channel-upgrades
chatton Jun 14, 2023
bf10856
addresing PR feedback
chatton Jun 16, 2023
0e34c52
Merge branch '04-channel-upgrades' into cian/issue#3753-implement-cha…
chatton Jun 16, 2023
8405547
Merge branch 'cian/issue#3753-implement-chanupgradecancel-handler-for…
chatton Jun 16, 2023
fb3897c
Merge branch 'cian/issue#3753-implement-chanupgradecancel-handler-for…
chatton Jun 16, 2023
2f540ce
corrected assertion arguments
chatton Jun 16, 2023
f470ffd
Merge branch 'cian/issue#3753-implement-chanupgradecancel-handler-for…
chatton Jun 16, 2023
78fb32b
address PR feedback
chatton Jun 16, 2023
1550e6f
Merge branch '04-channel-upgrades' into cian/issue#3754-implement-msg…
chatton Jun 16, 2023
ffc1b6f
addressing PR feedback
chatton Jun 16, 2023
415e66a
fix linter
chatton Jun 16, 2023
cc8cb48
re-added commented out test
chatton Jun 16, 2023
8ffceb8
Merge branch '04-channel-upgrades' into cian/issue#3754-implement-msg…
chatton Jun 19, 2023
269d9e0
pass the new upgrade sequence as an argument to restoreChannel
chatton Jun 19, 2023
2717582
rename variable to be more clear
chatton Jun 19, 2023
51335bd
Merge branch 'cian/issue#3754-implement-msgchannelupgradecancel-handl…
chatton Jun 19, 2023
d023ac0
use error receipt Sequence instead of sequence + 1
chatton Jun 19, 2023
04ac212
Merge branch '04-channel-upgrades' into cian/issue#3754-implement-msg…
chatton Jun 19, 2023
a3bbf80
addressing PR feedback
chatton Jun 19, 2023
8c58850
merge 04-channel-upgrades
chatton Jun 19, 2023
6f9f680
Merge branch '04-channel-upgrades' into cian/issue#3754-implement-msg…
chatton Jun 20, 2023
c36673a
addressing PR feedback
chatton Jun 20, 2023
0748052
Merge branch 'cian/issue#3754-implement-msgchannelupgradecancel-handl…
chatton Jun 20, 2023
cc45fe5
handle merge conflicts
chatton Jun 20, 2023
f2782b3
appease linter
chatton Jun 20, 2023
95c97db
Merge branch '04-channel-upgrades' into cian/issue#3754-implement-msg…
chatton Jun 20, 2023
0f3bc84
added test for capability not found
chatton Jun 21, 2023
fa56a67
use msg.ErrorReceipt.Sequence instead of returning from keeper fn
chatton Jun 21, 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
2 changes: 1 addition & 1 deletion .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,4 @@ linters-settings:
revive:
rules:
- name: if-return
disabled: true
disabled: true
21 changes: 21 additions & 0 deletions modules/core/04-channel/keeper/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -382,3 +382,24 @@ func emitErrorReceiptEvent(ctx sdk.Context, portID string, channelID string, cur
),
})
}

// emitChannelUpgradeCancelEvent emits an upgraded cancelled event.
func emitChannelUpgradeCancelEvent(ctx sdk.Context, portID string, channelID string, currentChannel types.Channel, upgrade types.Upgrade) {
ctx.EventManager().EmitEvents(sdk.Events{
sdk.NewEvent(
types.EventTypeChannelUpgradeCancel,
sdk.NewAttribute(types.AttributeKeyPortID, portID),
sdk.NewAttribute(types.AttributeKeyChannelID, channelID),
sdk.NewAttribute(types.AttributeCounterpartyPortID, currentChannel.Counterparty.PortId),
sdk.NewAttribute(types.AttributeCounterpartyChannelID, currentChannel.Counterparty.ChannelId),
sdk.NewAttribute(types.AttributeKeyUpgradeConnectionHops, upgrade.Fields.ConnectionHops[0]),
sdk.NewAttribute(types.AttributeKeyUpgradeVersion, upgrade.Fields.Version),
sdk.NewAttribute(types.AttributeKeyUpgradeOrdering, upgrade.Fields.Ordering.String()),
sdk.NewAttribute(types.AttributeKeyUpgradeSequence, fmt.Sprintf("%d", currentChannel.UpgradeSequence)),
),
sdk.NewEvent(
sdk.EventTypeMessage,
sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
),
})
}
59 changes: 59 additions & 0 deletions modules/core/04-channel/keeper/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,65 @@ func (k Keeper) WriteUpgradeAckChannel(
emitChannelUpgradeAckEvent(ctx, portID, channelID, channel, proposedUpgrade)
}

// ChanUpgradeCancel is called by a module to cancel a channel upgrade that is in progress.
func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, errorReceipt types.ErrorReceipt, errorReceiptProof []byte, proofHeight clienttypes.Height) error {
channel, found := k.GetChannel(ctx, portID, channelID)
if !found {
return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID)
}

// the channel state must be in INITUPGRADE or TRYUPGRADE
if !collections.Contains(channel.State, []types.State{types.INITUPGRADE, types.TRYUPGRADE}) {
return errorsmod.Wrapf(types.ErrInvalidChannelState, "expected one of [%s, %s], got %s", types.INITUPGRADE, types.TRYUPGRADE, channel.State)
}

// get underlying connection for proof verification
connection, err := k.GetConnection(ctx, channel.ConnectionHops[0])
if err != nil {
return errorsmod.Wrap(err, "failed to retrieve connection using the channel connection hops")
}

if err := k.connectionKeeper.VerifyChannelUpgradeError(ctx, portID, channelID, connection, errorReceipt, errorReceiptProof, proofHeight); err != nil {
return errorsmod.Wrap(err, "failed to verify counterparty error receipt")
}

// If counterparty sequence is less than the current sequence, abort the transaction since this error receipt is from a previous upgrade.
// Otherwise, increment the counterparty's error sequence by 1 so that both sides start with a fresh sequence.
currentSequence := channel.UpgradeSequence
counterpartySequence := errorReceipt.Sequence
if counterpartySequence < currentSequence {
return errorsmod.Wrap(types.ErrInvalidUpgradeSequence, "error sequence must be less than current sequence")
}

channel.UpgradeSequence = errorReceipt.Sequence + 1
k.SetChannel(ctx, portID, channelID, channel)

return nil
}

// WriteUpgradeCancelChannel writes a channel which has canceled the upgrade process.Auxiliary upgrade state is
// also deleted.
func (k Keeper) WriteUpgradeCancelChannel(ctx sdk.Context, portID, channelID string) {
defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-cancel")

upgrade, found := k.GetUpgrade(ctx, portID, channelID)
if !found {
panic(fmt.Sprintf("could not find upgrade when updating channel state, channelID: %s, portID: %s", channelID, portID))
}

channel, found := k.GetChannel(ctx, portID, channelID)
if !found {
panic(fmt.Sprintf("could not find existing channel when updating channel state, channelID: %s, portID: %s", channelID, portID))
}

previousState := channel.State

k.restoreChannel(ctx, portID, channelID, channel)

k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", previousState, "new-state", types.OPEN.String())
emitChannelUpgradeCancelEvent(ctx, portID, channelID, channel, upgrade)
}

// ChanUpgradeAck is called by a module to accept the ACKUPGRADE handshake step of the channel upgrade protocol.
// This method should only be called by the IBC core msg server.
// This method will verify that the counterparty has entered TRYUPGRADE
Expand Down
116 changes: 116 additions & 0 deletions modules/core/04-channel/keeper/upgrade_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"

errorsmod "cosmossdk.io/errors"
sdk "github.com/cosmos/cosmos-sdk/types"

clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
connectiontypes "github.com/cosmos/ibc-go/v7/modules/core/03-connection/types"
Expand Down Expand Up @@ -876,3 +877,118 @@ func (suite *KeeperTestSuite) TestAbortHandshake() {
})
}
}

func (suite *KeeperTestSuite) TestChanUpgradeCancel() {
var (
path *ibctesting.Path
errorReceipt types.ErrorReceipt
errorReceiptProof []byte
proofHeight clienttypes.Height
)
tests := []struct {
name string
malleate func()
expError error
}{
{
name: "success",
malleate: func() {},
expError: nil,
},
{
name: "invalid channel state",
malleate: func() {
channel := path.EndpointA.GetChannel()
channel.State = types.INIT
path.EndpointA.SetChannel(channel)
},
expError: types.ErrInvalidChannelState,
},
{
name: "channel not found",
malleate: func() {
path.EndpointA.Chain.DeleteKey(host.ChannelKey(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID))
},
expError: types.ErrChannelNotFound,
},
{
name: "connection not found",
malleate: func() {
channel := path.EndpointA.GetChannel()
channel.ConnectionHops = []string{"connection-100"}
path.EndpointA.SetChannel(channel)
},
expError: connectiontypes.ErrConnectionNotFound,
},
{
name: "counter party upgrade sequence less than current sequence",
malleate: func() {
var ok bool
errorReceipt, ok = suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
suite.Require().True(ok)

// the channel sequence will be 1
errorReceipt.Sequence = 0

suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, errorReceipt)

suite.coordinator.CommitBlock(suite.chainB)
suite.Require().NoError(path.EndpointA.UpdateClient())

upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
errorReceiptProof, proofHeight = suite.chainB.QueryProof(upgradeErrorReceiptKey)
},
expError: types.ErrInvalidUpgradeSequence,
},
}

for _, tc := range tests {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest()

path = ibctesting.NewPath(suite.chainA, suite.chainB)
suite.coordinator.Setup(path)

path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion

err := path.EndpointA.ChanUpgradeInit()
suite.Require().NoError(err)

suite.Require().NoError(path.EndpointB.UpdateClient())

// cause the upgrade to fail on chain b so an error receipt is written.
suite.chainB.GetSimApp().IBCMockModule.IBCApp.OnChanUpgradeTry = func(
ctx sdk.Context, portID, channelID string, order types.Order, connectionHops []string, counterpartyVersion string,
) (string, error) {
return "", fmt.Errorf("mock app callback failed")
}

err = path.EndpointB.ChanUpgradeTry()
suite.Require().NoError(err)

suite.Require().NoError(path.EndpointA.UpdateClient())

upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
errorReceiptProof, proofHeight = suite.chainB.QueryProof(upgradeErrorReceiptKey)

var ok bool
errorReceipt, ok = suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
suite.Require().True(ok)

tc.malleate()

err = suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errorReceiptProof, proofHeight)

expPass := tc.expError == nil
if expPass {
suite.Require().NoError(err)
channel := path.EndpointA.GetChannel()
suite.Require().Equal(errorReceipt.Sequence+1, channel.UpgradeSequence, "upgrade sequence should be incremented")
} else {
suite.Require().ErrorIs(tc.expError, err)
}
})
}
}
1 change: 1 addition & 0 deletions modules/core/04-channel/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,5 @@ var (
ErrIncompatibleCounterpartyUpgrade = errorsmod.Register(SubModuleName, 31, "incompatible counterparty upgrade")
ErrInvalidUpgradeError = errorsmod.Register(SubModuleName, 32, "invalid upgrade error")
ErrInvalidFlushStatus = errorsmod.Register(SubModuleName, 33, "invalid flush status")
ErrInvalidUpgradeErrorReceipt = errorsmod.Register(SubModuleName, 34, "invalid error receipt")
)
23 changes: 12 additions & 11 deletions modules/core/04-channel/types/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,18 @@ const (

// IBC channel events vars
var (
EventTypeChannelOpenInit = "channel_open_init"
EventTypeChannelOpenTry = "channel_open_try"
EventTypeChannelOpenAck = "channel_open_ack"
EventTypeChannelOpenConfirm = "channel_open_confirm"
EventTypeChannelCloseInit = "channel_close_init"
EventTypeChannelCloseConfirm = "channel_close_confirm"
EventTypeChannelClosed = "channel_close"
EventTypeChannelUpgradeInit = "channel_upgrade_init"
EventTypeChannelUpgradeTry = "channel_upgrade_try"
EventTypeChannelUpgradeAck = "channel_upgrade_ack"
EventTypeChannelUpgradeOpen = "channel_upgrade_open"
EventTypeChannelOpenInit = "channel_open_init"
EventTypeChannelOpenTry = "channel_open_try"
EventTypeChannelOpenAck = "channel_open_ack"
EventTypeChannelOpenConfirm = "channel_open_confirm"
EventTypeChannelCloseInit = "channel_close_init"
EventTypeChannelCloseConfirm = "channel_close_confirm"
EventTypeChannelClosed = "channel_close"
EventTypeChannelUpgradeInit = "channel_upgrade_init"
EventTypeChannelUpgradeTry = "channel_upgrade_try"
EventTypeChannelUpgradeAck = "channel_upgrade_ack"
EventTypeChannelUpgradeOpen = "channel_upgrade_open"
EventTypeChannelUpgradeCancel = "channel_upgrade_cancelled"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
EventTypeChannelUpgradeCancel = "channel_upgrade_cancelled"
EventTypeChannelUpgradeCancel = "channel_upgrade_cancel"


AttributeValueCategory = fmt.Sprintf("%s_%s", ibcexported.ModuleName, SubModuleName)
)
30 changes: 29 additions & 1 deletion modules/core/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -820,5 +820,33 @@ func (k Keeper) ChannelUpgradeTimeout(goCtx context.Context, msg *channeltypes.M

// ChannelUpgradeCancel defines a rpc handler method for MsgChannelUpgradeCancel.
func (k Keeper) ChannelUpgradeCancel(goCtx context.Context, msg *channeltypes.MsgChannelUpgradeCancel) (*channeltypes.MsgChannelUpgradeCancelResponse, error) {
return nil, nil
ctx := sdk.UnwrapSDKContext(goCtx)

module, _, err := k.ChannelKeeper.LookupModuleByChannel(ctx, msg.PortId, msg.ChannelId)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should add a testcase for module not found here I think!

if err != nil {
ctx.Logger().Error("channel upgrade cancel failed", "port-id", msg.PortId, "error", errorsmod.Wrap(err, "could not retrieve module from port-id"))
return nil, errorsmod.Wrap(err, "could not retrieve module from port-id")
}

cbs, ok := k.Router.GetRoute(module)
if !ok {
ctx.Logger().Error("channel upgrade cancel failed", "port-id", msg.PortId, "error", errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module))
return nil, errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module)
}

if err := k.ChannelKeeper.ChanUpgradeCancel(ctx, msg.PortId, msg.ChannelId, msg.ErrorReceipt, msg.ProofErrorReceipt, msg.ProofHeight); err != nil {
ctx.Logger().Error("channel upgrade cancel failed", "port-id", msg.PortId, "error", err.Error())
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return nil, err
return nil, errorsmod.Wrap(err, "channel upgrade cancel failed")

}

if err := cbs.OnChanUpgradeRestore(ctx, msg.PortId, msg.ChannelId); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if I missed the discussing about the naming, but what about onChanUpgradeCancel instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think it makes more sense for the applications to call a restore function, as they are just going to be handling any restoring to previous state, and not cancelling anything like a handshake on the IBC core level.

ctx.Logger().Error("channel upgrade cancel failed", "port-id", msg.PortId, "error", err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

Might not be consistent in the other upgrade handshake handlers, but this would make it more consistent with the open handshake handlers:

Suggested change
ctx.Logger().Error("channel upgrade cancel failed", "port-id", msg.PortId, "error", err.Error())
ctx.Logger().Error("channel upgrade cancel failed", "port-id", msg.PortId, "channel-id", msg.ChannelId, "error", errorsmod.Wrap(err, "channel upgrade restore callback failed"))

return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

Also for consistency:

Suggested change
return nil, err
return nil, errorsmod.Wrapf(err, "channel upgrade restore callback failed for port ID: %s, channel ID: %s", msg.PortId, msg.ChannelId)

}

k.ChannelKeeper.WriteUpgradeCancelChannel(ctx, msg.PortId, msg.ChannelId)

ctx.Logger().Info("channel upgrade cancel succeeded", "port-id", msg.PortId, "channel-id", msg.ChannelId)

return &channeltypes.MsgChannelUpgradeCancelResponse{}, nil
}
Loading