From 5cd004e342928658053434c50307587965c18b5a Mon Sep 17 00:00:00 2001 From: chatton Date: Thu, 1 Jun 2023 16:40:10 +0100 Subject: [PATCH 01/33] wip: adding restoreChannel and writeErrorReceipt functions --- modules/core/04-channel/keeper/keeper.go | 12 +++++++ modules/core/04-channel/keeper/upgrade.go | 44 +++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/modules/core/04-channel/keeper/keeper.go b/modules/core/04-channel/keeper/keeper.go index 137559e5d9f..0221dbfa4ac 100644 --- a/modules/core/04-channel/keeper/keeper.go +++ b/modules/core/04-channel/keeper/keeper.go @@ -533,6 +533,12 @@ func (k Keeper) SetCounterpartyLastPacketSequence(ctx sdk.Context, portID, chann store.Set(host.ChannelCounterpartyLastPacketSequenceKey(portID, channelID), bz) } +// deleteCounterpartyLastPacketSequence deletes the counterparty last packet sequence from the store. +func (k Keeper) deleteCounterpartyLastPacketSequence(ctx sdk.Context, portID, channelID string) { + store := ctx.KVStore(k.storeKey) + store.Delete(host.ChannelCounterpartyLastPacketSequenceKey(portID, channelID)) +} + // SetUpgrade sets the proposed upgrade using the provided port and channel identifiers. func (k Keeper) SetUpgrade(ctx sdk.Context, portID, channelID string, upgrade types.Upgrade) { store := ctx.KVStore(k.storeKey) @@ -540,6 +546,12 @@ func (k Keeper) SetUpgrade(ctx sdk.Context, portID, channelID string, upgrade ty store.Set(host.ChannelUpgradeKey(portID, channelID), bz) } +// deleteUpgrade deletes the upgrade for the provided port and channel identifiers. +func (k Keeper) deleteUpgrade(ctx sdk.Context, portID, channelID string) { + store := ctx.KVStore(k.storeKey) + store.Delete(host.ChannelUpgradeKey(portID, channelID)) +} + // ValidateUpgradeFields validates the proposed upgrade fields against the existing channel. // It returns an error if the following constraints are not met: // - there exists at least one valid proposed change to the existing channel fields diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index c101ab01c60..7923a54ba59 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -107,6 +107,50 @@ func (k Keeper) WriteUpgradeTryChannel( emitChannelUpgradeTryEvent(ctx, portID, channelID, channel, proposedUpgrade) } +// abortHandshake 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) abortHandshake(ctx sdk.Context, portID, channelID string, upgradeError types.UpgradeError) error { + if err := k.restoreChannel(ctx, portID, channelID); err != nil { + return err + } + + if err := k.writeErrorReceipt(ctx, portID, channelID, upgradeError); err != nil { + return err + } + + + // TODO: callback execution + // cbs.OnChanUpgradeRestore() + + return nil +} + +// restoreChannel will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted +// it write an error receipt to state so counterparty can restore as well. +func (k Keeper) restoreChannel(ctx sdk.Context, portID, channelID string) error { + channel, found := k.GetChannel(ctx, portID, channelID) + if !found { + return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) + } + + channel.State = types.OPEN + channel.FlushStatus = types.NOTINFLUSH + + k.SetChannel(ctx, portID, channelID, channel) + + // delete state associated with upgrade which is no longer required. + k.deleteUpgrade(ctx, portID, channelID) + k.deleteCounterpartyLastPacketSequence(ctx, portID, channelID) + + return nil +} + +func (k Keeper) writeErrorReceipt(ctx sdk.Context, portID, channelID string, upgradeError types.UpgradeError) error { + k.SetUpgradeErrorReceipt(ctx, portID, channelID, upgradeError.GetErrorReceipt()) + // TODO: emit events + return nil +} + // constructProposedUpgrade returns the proposed upgrade from the provided arguments. func (k Keeper) constructProposedUpgrade(ctx sdk.Context, portID, channelID string, fields types.UpgradeFields, upgradeTimeout types.Timeout) (types.Upgrade, error) { seq, found := k.GetNextSequenceSend(ctx, portID, channelID) From d01355f01f62b612eac965cc0b327447d6c7dd66 Mon Sep 17 00:00:00 2001 From: chatton Date: Tue, 6 Jun 2023 16:28:42 +0100 Subject: [PATCH 02/33] emit error receipt events on abort --- modules/core/04-channel/keeper/events.go | 2 -- modules/core/04-channel/keeper/upgrade.go | 20 +++++++++++++++----- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/modules/core/04-channel/keeper/events.go b/modules/core/04-channel/keeper/events.go index 7f1936d3230..f23fb1283d9 100644 --- a/modules/core/04-channel/keeper/events.go +++ b/modules/core/04-channel/keeper/events.go @@ -366,8 +366,6 @@ func emitChannelUpgradeOpenEvent(ctx sdk.Context, portID string, channelID strin } // emitErrorReceiptEvent emits an error receipt event -// -//lint:ignore U1000 Ignore unused function temporarily for debugging func emitErrorReceiptEvent(ctx sdk.Context, portID string, channelID string, currentChannel types.Channel, upgrade types.Upgrade, err error) { ctx.EventManager().EmitEvents(sdk.Events{ sdk.NewEvent( diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index b48c9232fe5..8a4f8dc8399 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -111,15 +111,19 @@ func (k Keeper) WriteUpgradeTryChannel( // abortHandshake 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) abortHandshake(ctx sdk.Context, portID, channelID string, upgradeError types.UpgradeError) error { +func (k Keeper) abortHandshake(ctx sdk.Context, portID, channelID string, upgradeError *types.UpgradeError) error { if err := k.restoreChannel(ctx, portID, channelID); err != nil { return err } - if err := k.writeErrorReceipt(ctx, portID, channelID, upgradeError); err != nil { - return err + upgrade, found := k.GetUpgrade(ctx, portID, channelID) + if !found { + return errorsmod.Wrapf(types.ErrUpgradeNotFound, "port ID (%s) channel ID (%s)", portID, channelID) } + if err := k.writeErrorReceipt(ctx, portID, channelID, upgrade, upgradeError); err != nil { + return err + } // TODO: callback execution // cbs.OnChanUpgradeRestore() @@ -148,9 +152,15 @@ func (k Keeper) restoreChannel(ctx sdk.Context, portID, channelID string) error } // writeErrorReceipt will write an error receipt from the provided UpgradeError. -func (k Keeper) writeErrorReceipt(ctx sdk.Context, portID, channelID string, upgradeError types.UpgradeError) error { +func (k Keeper) writeErrorReceipt(ctx sdk.Context, portID, channelID string, upgrade types.Upgrade, upgradeError *types.UpgradeError) error { + channel, found := k.GetChannel(ctx, portID, channelID) + if !found { + return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) + } + k.SetUpgradeErrorReceipt(ctx, portID, channelID, upgradeError.GetErrorReceipt()) - // TODO: emit events + emitErrorReceiptEvent(ctx, portID, channelID, channel, upgrade, upgradeError) + return nil } From 6f7940b68f4dbdc500612505f0420cccf87d1d3b Mon Sep 17 00:00:00 2001 From: chatton Date: Tue, 6 Jun 2023 16:31:15 +0100 Subject: [PATCH 03/33] remove unused function --- modules/core/04-channel/keeper/keeper.go | 31 ------------------------ 1 file changed, 31 deletions(-) diff --git a/modules/core/04-channel/keeper/keeper.go b/modules/core/04-channel/keeper/keeper.go index e603aa46946..e48db3b5d88 100644 --- a/modules/core/04-channel/keeper/keeper.go +++ b/modules/core/04-channel/keeper/keeper.go @@ -2,7 +2,6 @@ package keeper import ( "encoding/binary" - "reflect" "strconv" "strings" @@ -552,36 +551,6 @@ func (k Keeper) deleteUpgrade(ctx sdk.Context, portID, channelID string) { store.Delete(host.ChannelUpgradeKey(portID, channelID)) } -// ValidateUpgradeFields validates the proposed upgrade fields against the existing channel. -// It returns an error if the following constraints are not met: -// - there exists at least one valid proposed change to the existing channel fields -// - the proposed order is a subset of the existing order -// - the proposed connection hops do not exist -// - the proposed version is non-empty (checked in UpgradeFields.ValidateBasic()) -// - the proposed connection hops are not open -func (k Keeper) ValidateUpgradeFields(ctx sdk.Context, proposedUpgrade types.UpgradeFields, currentChannel types.Channel) error { - currentFields := extractUpgradeFields(currentChannel) - - if reflect.DeepEqual(proposedUpgrade, currentFields) { - return errorsmod.Wrap(types.ErrChannelExists, "existing channel end is identical to proposed upgrade channel end") - } - - connectionID := proposedUpgrade.ConnectionHops[0] - connection, err := k.GetConnection(ctx, connectionID) - if err != nil { - return errorsmod.Wrapf(connectiontypes.ErrConnectionNotFound, "failed to retrieve connection: %s", connectionID) - } - - if connection.GetState() != int32(connectiontypes.OPEN) { - return errorsmod.Wrapf( - connectiontypes.ErrInvalidConnectionState, - "connection state is not OPEN (got %s)", connectiontypes.State(connection.GetState()).String(), - ) - } - - return nil -} - // common functionality for IteratePacketCommitment and IteratePacketAcknowledgement func (k Keeper) iterateHashes(ctx sdk.Context, iterator db.Iterator, cb func(portID, channelID string, sequence uint64, hash []byte) bool) { defer sdk.LogDeferred(ctx.Logger(), func() error { return iterator.Close() }) From 6d50848a97d5fb14da1d3e13f799f0e1e04b6623 Mon Sep 17 00:00:00 2001 From: chatton Date: Tue, 6 Jun 2023 16:49:02 +0100 Subject: [PATCH 04/33] added happy path test for abort --- modules/core/04-channel/keeper/export_test.go | 5 ++ modules/core/04-channel/keeper/upgrade.go | 13 +++-- .../core/04-channel/keeper/upgrade_test.go | 49 +++++++++++++++++++ 3 files changed, 62 insertions(+), 5 deletions(-) diff --git a/modules/core/04-channel/keeper/export_test.go b/modules/core/04-channel/keeper/export_test.go index 7199dc2f89a..b5cd95f9ae3 100644 --- a/modules/core/04-channel/keeper/export_test.go +++ b/modules/core/04-channel/keeper/export_test.go @@ -14,3 +14,8 @@ import ( func (k Keeper) ValidateUpgradeFields(ctx sdk.Context, proposedUpgrade types.UpgradeFields, currentChannel types.Channel) error { return k.validateUpgradeFields(ctx, proposedUpgrade, currentChannel) } + +// ValidateUpgradeFields is a wrapper around validateUpgradeFields to allow the function to be directly called in tests. +func (k Keeper) AbortHandshake(ctx sdk.Context, portID, channelID string, upgradeError *types.UpgradeError) error { + return k.abortHandshake(ctx, portID, channelID, upgradeError) +} diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 8a4f8dc8399..4a429381b00 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -112,19 +112,22 @@ func (k Keeper) WriteUpgradeTryChannel( // abortHandshake 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) abortHandshake(ctx sdk.Context, portID, channelID string, upgradeError *types.UpgradeError) error { - if err := k.restoreChannel(ctx, portID, channelID); err != nil { - return err - } - upgrade, found := k.GetUpgrade(ctx, portID, channelID) if !found { return errorsmod.Wrapf(types.ErrUpgradeNotFound, "port ID (%s) channel ID (%s)", portID, channelID) } - if err := k.writeErrorReceipt(ctx, portID, channelID, upgrade, upgradeError); err != nil { + if err := k.restoreChannel(ctx, portID, channelID); err != nil { return err } + // TODO: could we want to abort but have upgrade error be non-nil? + if upgradeError != nil { + if err := k.writeErrorReceipt(ctx, portID, channelID, upgrade, upgradeError); err != nil { + return err + } + } + // TODO: callback execution // cbs.OnChanUpgradeRestore() diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index f879431a299..cb46624f141 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -203,3 +203,52 @@ func (suite *KeeperTestSuite) TestValidateProposedUpgradeFields() { }) } } + +func (suite *KeeperTestSuite) TestAbortHandshake() { + var ( + path *ibctesting.Path + upgradeError *types.UpgradeError + ) + + tests := []struct { + name string + malleate func() + expPass bool + }{ + { + name: "success", + malleate: func() {}, + expPass: true, + }, + } + + for _, tc := range tests { + tc := tc + suite.Run(tc.name, func() { + suite.SetupTest() + path = ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.Setup(path) + + channelKeeper := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper + + path.EndpointA.ChannelConfig.Version = mock.Version + "-v2" + + suite.Require().NoError(path.EndpointA.ChanUpgradeInit()) + + path.EndpointA.Chain.Coordinator.CommitBlock(path.EndpointA.Chain) + + suite.Require().NoError(path.EndpointA.UpdateClient()) + suite.Require().NoError(path.EndpointB.UpdateClient()) + + tc.malleate() + + err := channelKeeper.AbortHandshake(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, upgradeError) + + if tc.expPass { + suite.Require().NoError(err) + } else { + suite.Require().Error(err) + } + }) + } +} From 795cd05f3012fa876b5fb4c053fb95a98088166c Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 7 Jun 2023 09:58:09 +0100 Subject: [PATCH 05/33] fleshed out TestAbortHandshake --- .../core/04-channel/keeper/upgrade_test.go | 67 ++++++++++++++++--- 1 file changed, 59 insertions(+), 8 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index cb46624f141..f442f3c6fa7 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -204,21 +204,41 @@ func (suite *KeeperTestSuite) TestValidateProposedUpgradeFields() { } } +// TestAbortHandshake tests that when the channel handshake is aborted, the channel state +// is restored the previous state and that an error receipt is written, and upgrade state which +// is no longer required is deleted. func (suite *KeeperTestSuite) TestAbortHandshake() { var ( - path *ibctesting.Path - upgradeError *types.UpgradeError + path *ibctesting.Path ) tests := []struct { - name string - malleate func() - expPass bool + name string + malleate func() + expPass bool + channelPresent bool }{ { - name: "success", - malleate: func() {}, - expPass: true, + name: "success", + malleate: func() {}, + expPass: true, + channelPresent: true, + }, + { + name: "upgrade does not exist", + malleate: func() { + suite.chainA.DeleteKey(host.ChannelUpgradeKey(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) + }, + expPass: false, + channelPresent: true, + }, + { + name: "channel does not exist", + malleate: func() { + suite.chainA.DeleteKey(host.ChannelKey(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) + }, + expPass: false, + channelPresent: false, }, } @@ -240,14 +260,45 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { suite.Require().NoError(path.EndpointA.UpdateClient()) suite.Require().NoError(path.EndpointB.UpdateClient()) + upgradeError := types.NewUpgradeError(1, types.ErrInvalidChannel) + tc.malleate() err := channelKeeper.AbortHandshake(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, upgradeError) if tc.expPass { suite.Require().NoError(err) + + channel, found := channelKeeper.GetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + suite.Require().True(found, "channel should be found") + + suite.Require().Equal(types.OPEN, channel.State, "channel state should be OPEN") + suite.Require().Equal(types.NOTINFLUSH, channel.FlushStatus, "channel flush status should be NOTINFLUSH") + + _, found = channelKeeper.GetUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + suite.Require().False(found, "upgrade info should be deleted") + + 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") + suite.Require().Equal(upgradeError.GetErrorReceipt(), errorReceipt, "error receipt should be stored") + } + + _, found = channelKeeper.GetCounterpartyLastPacketSequence(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + suite.Require().False(found, "counterparty last packet sequence should be found") + } else { suite.Require().Error(err) + channel, found := channelKeeper.GetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + 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 OPEN") + } + + _, found = channelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + suite.Require().False(found, "error receipt should not be written") + + // TODO: assertion that GetCounterpartyLastPacketSequence is present and correct } }) } From dfbc5d4fe8ce683ea8d9503a2f616888053feee2 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 7 Jun 2023 10:02:08 +0100 Subject: [PATCH 06/33] corrected docstring and removed unrequired checks in test --- modules/core/04-channel/keeper/export_test.go | 2 +- modules/core/04-channel/keeper/upgrade_test.go | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/modules/core/04-channel/keeper/export_test.go b/modules/core/04-channel/keeper/export_test.go index b5cd95f9ae3..4bca0ddaab2 100644 --- a/modules/core/04-channel/keeper/export_test.go +++ b/modules/core/04-channel/keeper/export_test.go @@ -15,7 +15,7 @@ func (k Keeper) ValidateUpgradeFields(ctx sdk.Context, proposedUpgrade types.Upg return k.validateUpgradeFields(ctx, proposedUpgrade, currentChannel) } -// ValidateUpgradeFields is a wrapper around validateUpgradeFields to allow the function to be directly called in tests. +// AbortHandshake is a wrapper around abortHandshake to allow the function to be directly called in tests. func (k Keeper) AbortHandshake(ctx sdk.Context, portID, channelID string, upgradeError *types.UpgradeError) error { return k.abortHandshake(ctx, portID, channelID, upgradeError) } diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index f442f3c6fa7..f87e06d29b5 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -255,11 +255,6 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { suite.Require().NoError(path.EndpointA.ChanUpgradeInit()) - path.EndpointA.Chain.Coordinator.CommitBlock(path.EndpointA.Chain) - - suite.Require().NoError(path.EndpointA.UpdateClient()) - suite.Require().NoError(path.EndpointB.UpdateClient()) - upgradeError := types.NewUpgradeError(1, types.ErrInvalidChannel) tc.malleate() From d4295145b0e3c91100719fd095e6a8e6e174553d Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 7 Jun 2023 10:31:14 +0100 Subject: [PATCH 07/33] chore: added implementation of WriteUpgradeCancelChannel --- modules/core/04-channel/keeper/events.go | 21 +++++++++++ modules/core/04-channel/keeper/keeper.go | 12 +++++++ modules/core/04-channel/keeper/upgrade.go | 43 +++++++++++++++++++++++ modules/core/04-channel/types/errors.go | 1 + modules/core/04-channel/types/events.go | 1 + 5 files changed, 78 insertions(+) diff --git a/modules/core/04-channel/keeper/events.go b/modules/core/04-channel/keeper/events.go index 7f1936d3230..0240456cfa9 100644 --- a/modules/core/04-channel/keeper/events.go +++ b/modules/core/04-channel/keeper/events.go @@ -388,3 +388,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) { + ctx.EventManager().EmitEvents(sdk.Events{ + sdk.NewEvent( + types.EventTypeChannelUpgradeCancelled, + 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), + ), + }) +} diff --git a/modules/core/04-channel/keeper/keeper.go b/modules/core/04-channel/keeper/keeper.go index 220a9994fdf..e48db3b5d88 100644 --- a/modules/core/04-channel/keeper/keeper.go +++ b/modules/core/04-channel/keeper/keeper.go @@ -532,6 +532,12 @@ func (k Keeper) SetCounterpartyLastPacketSequence(ctx sdk.Context, portID, chann store.Set(host.ChannelCounterpartyLastPacketSequenceKey(portID, channelID), bz) } +// deleteCounterpartyLastPacketSequence deletes the counterparty last packet sequence from the store. +func (k Keeper) deleteCounterpartyLastPacketSequence(ctx sdk.Context, portID, channelID string) { + store := ctx.KVStore(k.storeKey) + store.Delete(host.ChannelCounterpartyLastPacketSequenceKey(portID, channelID)) +} + // SetUpgrade sets the proposed upgrade using the provided port and channel identifiers. func (k Keeper) SetUpgrade(ctx sdk.Context, portID, channelID string, upgrade types.Upgrade) { store := ctx.KVStore(k.storeKey) @@ -539,6 +545,12 @@ func (k Keeper) SetUpgrade(ctx sdk.Context, portID, channelID string, upgrade ty store.Set(host.ChannelUpgradeKey(portID, channelID), bz) } +// deleteUpgrade deletes the upgrade for the provided port and channel identifiers. +func (k Keeper) deleteUpgrade(ctx sdk.Context, portID, channelID string) { + store := ctx.KVStore(k.storeKey) + store.Delete(host.ChannelUpgradeKey(portID, channelID)) +} + // common functionality for IteratePacketCommitment and IteratePacketAcknowledgement func (k Keeper) iterateHashes(ctx sdk.Context, iterator db.Iterator, cb func(portID, channelID string, sequence uint64, hash []byte) bool) { defer sdk.LogDeferred(ctx.Logger(), func() error { return iterator.Close() }) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 825b3de00f5..d421ad29fde 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -110,6 +110,29 @@ func (k Keeper) WriteUpgradeTryChannel( emitChannelUpgradeTryEvent(ctx, portID, channelID, channel, proposedUpgrade) } +// 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) error { + defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-cancel") + + 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)) + } + + upgrade, found := k.GetUpgrade(ctx, portID, channelID) + if !found { + panic(fmt.Sprintf("could not find existing upgrade when cancelling channel upgrade, channelID: %s, portID: %s", channelID, portID)) + } + + 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)) + } + + emitUpgradedCancelledEvent(ctx, portID, channelID, channel, upgrade) + return nil +} + // startFlushUpgradeHandshake will verify the counterparty proposed upgrade and the current channel state. // Once the counterparty information has been verified, it will be validated against the self proposed upgrade. // If any of the proposed upgrade fields are incompatible, an upgrade error will be returned resulting in an @@ -265,3 +288,23 @@ func (k Keeper) constructProposedUpgrade(ctx sdk.Context, portID, channelID stri LatestSequenceSend: seq - 1, }, nil } + +// restoreChannel will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted +// it write an error receipt to state so counterparty can restore as well. +func (k Keeper) restoreChannel(ctx sdk.Context, portID, channelID string) error { + channel, found := k.GetChannel(ctx, portID, channelID) + if !found { + return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) + } + + channel.State = types.OPEN + channel.FlushStatus = types.NOTINFLUSH + + k.SetChannel(ctx, portID, channelID, channel) + + // delete state associated with upgrade which is no longer required. + k.deleteUpgrade(ctx, portID, channelID) + k.deleteCounterpartyLastPacketSequence(ctx, portID, channelID) + + return nil +} diff --git a/modules/core/04-channel/types/errors.go b/modules/core/04-channel/types/errors.go index 1f3ff7890f8..a4815ce1db3 100644 --- a/modules/core/04-channel/types/errors.go +++ b/modules/core/04-channel/types/errors.go @@ -45,4 +45,5 @@ var ( ErrInvalidUpgradeSequence = errorsmod.Register(SubModuleName, 29, "invalid upgrade sequence") ErrUpgradeNotFound = errorsmod.Register(SubModuleName, 30, "upgrade not found") ErrIncompatibleCounterpartyUpgrade = errorsmod.Register(SubModuleName, 31, "incompatible counterparty upgrade") + ErrUpgradeRestoreFailed = errorsmod.Register(SubModuleName, 32, "restore failed") ) diff --git a/modules/core/04-channel/types/events.go b/modules/core/04-channel/types/events.go index e47a8e4d2af..9c7ebf028af 100644 --- a/modules/core/04-channel/types/events.go +++ b/modules/core/04-channel/types/events.go @@ -62,6 +62,7 @@ var ( EventTypeChannelUpgradeTry = "channel_upgrade_try" EventTypeChannelUpgradeAck = "channel_upgrade_ack" EventTypeChannelUpgradeOpen = "channel_upgrade_open" + EventTypeChannelUpgradeCancelled = "channel_upgrade_cancelled" AttributeValueCategory = fmt.Sprintf("%s_%s", ibcexported.ModuleName, SubModuleName) ) From c1e05b822c628f044f7e979a78678df033ab3826 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 7 Jun 2023 10:39:55 +0100 Subject: [PATCH 08/33] fix linting --- modules/core/04-channel/keeper/upgrade.go | 1 - modules/core/04-channel/keeper/upgrade_test.go | 4 +--- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 7f0f53ed14f..475f6f84f22 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -265,7 +265,6 @@ func (k Keeper) constructProposedUpgrade(ctx sdk.Context, portID, channelID stri }, nil } - // abortHandshake 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) abortHandshake(ctx sdk.Context, portID, channelID string, upgradeError *types.UpgradeError) error { diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 9fddbf58358..d3a62c925cd 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -394,9 +394,7 @@ func (suite *KeeperTestSuite) assertUpgradeError(actualError, expError error) { // is restored the previous state and that an error receipt is written, and upgrade state which // is no longer required is deleted. func (suite *KeeperTestSuite) TestAbortHandshake() { - var ( - path *ibctesting.Path - ) + var path *ibctesting.Path tests := []struct { name string From 91605c134fb87fdb209e824a59e2e14b62031341 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 7 Jun 2023 10:56:04 +0100 Subject: [PATCH 09/33] addressing PR feedback --- modules/core/04-channel/keeper/upgrade.go | 11 ++++---- .../core/04-channel/keeper/upgrade_test.go | 28 +++++++++++++------ modules/core/04-channel/types/errors.go | 1 + 3 files changed, 26 insertions(+), 14 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 475f6f84f22..249579d6231 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -268,6 +268,10 @@ func (k Keeper) constructProposedUpgrade(ctx sdk.Context, portID, channelID stri // abortHandshake 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) abortHandshake(ctx sdk.Context, portID, channelID string, upgradeError *types.UpgradeError) error { + if upgradeError == nil { + return errorsmod.Wrap(types.ErrInvalidUpgradeError, "cannot abort upgrade handshake with nil error") + } + upgrade, found := k.GetUpgrade(ctx, portID, channelID) if !found { return errorsmod.Wrapf(types.ErrUpgradeNotFound, "port ID (%s) channel ID (%s)", portID, channelID) @@ -277,11 +281,8 @@ func (k Keeper) abortHandshake(ctx sdk.Context, portID, channelID string, upgrad return err } - // TODO: could we want to abort but have upgrade error be non-nil? - if upgradeError != nil { - if err := k.writeErrorReceipt(ctx, portID, channelID, upgrade, upgradeError); err != nil { - return err - } + if err := k.writeErrorReceipt(ctx, portID, channelID, upgrade, upgradeError); err != nil { + return err } // TODO: callback execution diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index d3a62c925cd..b21950ef5b6 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -394,7 +394,10 @@ func (suite *KeeperTestSuite) assertUpgradeError(actualError, expError error) { // is restored the previous state and that an error receipt is written, and upgrade state which // is no longer required is deleted. func (suite *KeeperTestSuite) TestAbortHandshake() { - var path *ibctesting.Path + var ( + path *ibctesting.Path + upgradeError *types.UpgradeError + ) tests := []struct { name string @@ -424,6 +427,14 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { expPass: false, channelPresent: false, }, + { + name: "fails with nil upgrade error", + malleate: func() { + upgradeError = nil + }, + expPass: false, + channelPresent: true, + }, } for _, tc := range tests { @@ -439,7 +450,7 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { suite.Require().NoError(path.EndpointA.ChanUpgradeInit()) - upgradeError := types.NewUpgradeError(1, types.ErrInvalidChannel) + upgradeError = types.NewUpgradeError(1, types.ErrInvalidChannel) tc.malleate() @@ -451,31 +462,30 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { channel, found := channelKeeper.GetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) suite.Require().True(found, "channel should be found") - suite.Require().Equal(types.OPEN, channel.State, "channel state should be OPEN") - suite.Require().Equal(types.NOTINFLUSH, channel.FlushStatus, "channel flush status should be NOTINFLUSH") - + suite.Require().Equal(types.OPEN, channel.State, "channel state should be %s", types.OPEN.String()) + suite.Require().Equal(types.NOTINFLUSH, channel.FlushStatus, "channel flush status should be %s", types.NOTINFLUSH.String()) _, found = channelKeeper.GetUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) suite.Require().False(found, "upgrade info should be deleted") 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") - suite.Require().Equal(upgradeError.GetErrorReceipt(), errorReceipt, "error receipt should be stored") + suite.Require().Equal(upgradeError.GetErrorReceipt(), errorReceipt, "error receipt does not match expected error receipt") } _, found = channelKeeper.GetCounterpartyLastPacketSequence(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) - suite.Require().False(found, "counterparty last packet sequence should be found") + suite.Require().False(found, "counterparty last packet sequence should not be found") } else { suite.Require().Error(err) channel, found := channelKeeper.GetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) 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 OPEN") + suite.Require().Equal(types.INITUPGRADE, channel.State, "channel state should not be restored to %s", types.INITUPGRADE.String()) } _, found = channelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) - suite.Require().False(found, "error receipt should not be written") + suite.Require().False(found, "error receipt should not be found") // TODO: assertion that GetCounterpartyLastPacketSequence is present and correct } diff --git a/modules/core/04-channel/types/errors.go b/modules/core/04-channel/types/errors.go index 1f3ff7890f8..f73bfd3f326 100644 --- a/modules/core/04-channel/types/errors.go +++ b/modules/core/04-channel/types/errors.go @@ -45,4 +45,5 @@ var ( ErrInvalidUpgradeSequence = errorsmod.Register(SubModuleName, 29, "invalid upgrade sequence") ErrUpgradeNotFound = errorsmod.Register(SubModuleName, 30, "upgrade not found") ErrIncompatibleCounterpartyUpgrade = errorsmod.Register(SubModuleName, 31, "incompatible counterparty upgrade") + ErrInvalidUpgradeError = errorsmod.Register(SubModuleName, 32, "invalid upgrade error") ) From eaa2b914d2920d83a492965e1ddb87d920373e4a Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 7 Jun 2023 13:33:59 +0100 Subject: [PATCH 10/33] addressing PR feedback --- modules/core/04-channel/keeper/upgrade.go | 2 +- .../core/04-channel/keeper/upgrade_test.go | 44 +++++++++---------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 249579d6231..f54ac9d12b4 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -292,7 +292,7 @@ func (k Keeper) abortHandshake(ctx sdk.Context, portID, channelID string, upgrad } // restoreChannel will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted -// it write an error receipt to state so counterparty can restore as well. +// It will write an error receipt to state so that the counterparty can restore as well. func (k Keeper) restoreChannel(ctx sdk.Context, portID, channelID string) error { channel, found := k.GetChannel(ctx, portID, channelID) if !found { diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index b21950ef5b6..b2f01d22027 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -400,40 +400,35 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { ) tests := []struct { - name string - malleate func() - expPass bool - channelPresent bool + name string + malleate func() + expPass bool }{ { - name: "success", - malleate: func() {}, - expPass: true, - channelPresent: true, + name: "success", + malleate: func() {}, + expPass: true, }, { name: "upgrade does not exist", malleate: func() { suite.chainA.DeleteKey(host.ChannelUpgradeKey(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) }, - expPass: false, - channelPresent: true, + expPass: false, }, { name: "channel does not exist", malleate: func() { suite.chainA.DeleteKey(host.ChannelKey(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) }, - expPass: false, - channelPresent: false, + expPass: false, }, { name: "fails with nil upgrade error", malleate: func() { upgradeError = nil }, - expPass: false, - channelPresent: true, + expPass: false, }, } @@ -446,10 +441,13 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { channelKeeper := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper - path.EndpointA.ChannelConfig.Version = mock.Version + "-v2" - + path.EndpointA.ChannelConfig.Version = mock.UpgradeVersion suite.Require().NoError(path.EndpointA.ChanUpgradeInit()) + // fetch the upgrade before abort for assertions later on. + actualUpgrade, ok := channelKeeper.GetUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + suite.Require().True(ok, "upgrade should be found") + upgradeError = types.NewUpgradeError(1, types.ErrInvalidChannel) tc.malleate() @@ -468,10 +466,8 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { suite.Require().False(found, "upgrade info should be deleted") 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") - suite.Require().Equal(upgradeError.GetErrorReceipt(), errorReceipt, "error receipt does not match expected error receipt") - } + suite.Require().True(found, "error receipt should be found") + suite.Require().Equal(upgradeError.GetErrorReceipt(), errorReceipt, "error receipt does not match expected error receipt") _, found = channelKeeper.GetCounterpartyLastPacketSequence(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) suite.Require().False(found, "counterparty last packet sequence should not be found") @@ -479,14 +475,18 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { } else { suite.Require().Error(err) channel, found := channelKeeper.GetChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) - if tc.channelPresent { - suite.Require().True(found, "channel should be found") + 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()) } _, found = channelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) suite.Require().False(found, "error receipt should not be found") + upgrade, found := channelKeeper.GetUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + if found { // this should be all test cases except for when the upgrade is explicitly deleted. + suite.Require().Equal(actualUpgrade, upgrade, "upgrade info should not be deleted") + } + // TODO: assertion that GetCounterpartyLastPacketSequence is present and correct } }) From 7d2c3d41da36ae2a3d4a2a724db9da2d3ef2bbdf Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 7 Jun 2023 15:06:06 +0100 Subject: [PATCH 11/33] chore: adding implementation of ChanUpgradeCancel --- modules/core/04-channel/keeper/upgrade.go | 44 +++++++++++++++++++ modules/core/04-channel/types/errors.go | 1 + .../core/04-channel/types/expected_keepers.go | 10 +++++ 3 files changed, 55 insertions(+) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 9c82e0713c3..da9f33c424d 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -159,6 +159,50 @@ func (k Keeper) WriteUpgradeTryChannel( emitChannelUpgradeTryEvent(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, connection, proofHeight, errorReceiptProof, portID, channelID, errorReceipt); err != nil { + return errorsmod.Wrap(err, "failed to verify counterparty error receipt") + } + + if isEmptyReceipt(errorReceipt) { + return errorsmod.Wrap(types.ErrInvalidUpgradeErrorReceipt, "empty error receipt") + } + + // If counterparty sequence is less than the current sequence, abort transaction since this error receipt is from a previous upgrade + // Otherwise, set the sequence to counterparty's error sequence+1 so that both sides start with a fresh sequence + currentSequence := channel.UpgradeSequence + if errorReceipt.Sequence >= 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 +} + +// isEmptyReceipt returns true if the receipt is empty. +func isEmptyReceipt(receipt types.ErrorReceipt) bool { + return receipt == types.ErrorReceipt{} +} + // 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) error { diff --git a/modules/core/04-channel/types/errors.go b/modules/core/04-channel/types/errors.go index f169c8a2a1f..0c5869811e9 100644 --- a/modules/core/04-channel/types/errors.go +++ b/modules/core/04-channel/types/errors.go @@ -47,4 +47,5 @@ var ( ErrIncompatibleCounterpartyUpgrade = errorsmod.Register(SubModuleName, 31, "incompatible counterparty upgrade") ErrInvalidUpgradeError = errorsmod.Register(SubModuleName, 32, "invalid upgrade error") ErrUpgradeRestoreFailed = errorsmod.Register(SubModuleName, 33, "restore failed") + ErrInvalidUpgradeErrorReceipt = errorsmod.Register(SubModuleName, 34, "invalid error receipt") ) diff --git a/modules/core/04-channel/types/expected_keepers.go b/modules/core/04-channel/types/expected_keepers.go index 85a9019e353..a06c9d027d9 100644 --- a/modules/core/04-channel/types/expected_keepers.go +++ b/modules/core/04-channel/types/expected_keepers.go @@ -5,6 +5,7 @@ import ( capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" connectiontypes "github.com/cosmos/ibc-go/v7/modules/core/03-connection/types" + channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" "github.com/cosmos/ibc-go/v7/modules/core/exported" ) @@ -80,6 +81,15 @@ type ConnectionKeeper interface { channelID string, upgrade Upgrade, ) error + VerifyChannelUpgradeError( + ctx sdk.Context, + connection exported.ConnectionI, + height exported.Height, + proof []byte, + portID, + channelID string, + errorReceipt channeltypes.ErrorReceipt, + ) error } // PortKeeper expected account IBC port keeper From 759474f47abc44951f392b8bcb0fbf17d94bf8d4 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 7 Jun 2023 15:36:29 +0100 Subject: [PATCH 12/33] added scaffolding for TestChanUpgradeCancel --- .../core/04-channel/keeper/upgrade_test.go | 51 +++++++++++++++++++ .../core/04-channel/types/expected_keepers.go | 3 +- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index d7daa25d886..ea65294d864 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -732,3 +732,54 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { }) } } + +func (suite *KeeperTestSuite) TestChanUpgradeCancel() { + var ( + path *ibctesting.Path + ) + tests := []struct { + name string + malleate func() + expPass bool + }{ + { + name: "success", + malleate: func() {}, + expPass: true, + }, + } + + 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()) + + err = path.EndpointB.ChanUpgradeTry() + suite.Require().NoError(err) + + suite.Require().NoError(path.EndpointA.UpdateClient()) + + channelAKeeper := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper + + // TODO: populate these correctly + errorReceipt := types.ErrorReceipt{} + errReceiptProof := []byte{} + proofHeight := clienttypes.Height{} + + err = channelAKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errReceiptProof, proofHeight) + suite.Require().NoError(err) + + }) + } +} diff --git a/modules/core/04-channel/types/expected_keepers.go b/modules/core/04-channel/types/expected_keepers.go index a06c9d027d9..0baa27e8f03 100644 --- a/modules/core/04-channel/types/expected_keepers.go +++ b/modules/core/04-channel/types/expected_keepers.go @@ -5,7 +5,6 @@ import ( capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" connectiontypes "github.com/cosmos/ibc-go/v7/modules/core/03-connection/types" - channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" "github.com/cosmos/ibc-go/v7/modules/core/exported" ) @@ -88,7 +87,7 @@ type ConnectionKeeper interface { proof []byte, portID, channelID string, - errorReceipt channeltypes.ErrorReceipt, + errorReceipt ErrorReceipt, ) error } From 3916332cc3b0e14867cf92bf46ca998fe9c99146 Mon Sep 17 00:00:00 2001 From: chatton Date: Mon, 12 Jun 2023 12:03:07 +0100 Subject: [PATCH 13/33] addressing PR feedback --- modules/core/04-channel/keeper/export_test.go | 6 +++--- modules/core/04-channel/keeper/upgrade.go | 18 +++++++++++++++--- modules/core/04-channel/keeper/upgrade_test.go | 18 +++++++++++++++--- 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/modules/core/04-channel/keeper/export_test.go b/modules/core/04-channel/keeper/export_test.go index baa6bd95a91..0ff36d4355b 100644 --- a/modules/core/04-channel/keeper/export_test.go +++ b/modules/core/04-channel/keeper/export_test.go @@ -31,7 +31,7 @@ func (k Keeper) ValidateUpgradeFields(ctx sdk.Context, proposedUpgrade types.Upg return k.validateUpgradeFields(ctx, proposedUpgrade, currentChannel) } -// AbortHandshake is a wrapper around abortHandshake to allow the function to be directly called in tests. -func (k Keeper) AbortHandshake(ctx sdk.Context, portID, channelID string, upgradeError *types.UpgradeError) error { - return k.abortHandshake(ctx, portID, channelID, upgradeError) +// AbortUpgrade is a wrapper around abortUpgrade to allow the function to be directly called in tests. +func (k Keeper) AbortUpgrade(ctx sdk.Context, portID, channelID string, err error) error { + return k.abortUpgrade(ctx, portID, channelID, err) } diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 7e1fd4f122f..37fdab1af26 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -326,10 +326,10 @@ func (k Keeper) constructProposedUpgrade(ctx sdk.Context, portID, channelID stri }, nil } -// abortHandshake will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted. +// 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) abortHandshake(ctx sdk.Context, portID, channelID string, upgradeError *types.UpgradeError) error { - if upgradeError == nil { +func (k Keeper) abortUpgrade(ctx sdk.Context, portID, channelID string, err error) error { + if err == nil { return errorsmod.Wrap(types.ErrInvalidUpgradeError, "cannot abort upgrade handshake with nil error") } @@ -342,6 +342,18 @@ func (k Keeper) abortHandshake(ctx sdk.Context, portID, channelID string, upgrad return err } + // in the case of application callbacks, the error may not be an upgrade error. + // in this case we need to construct one in order to write the error receipt. + upgradeError, ok := err.(*types.UpgradeError) + if !ok { + channel, found := k.GetChannel(ctx, portID, channelID) + if !found { + return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) + } + + upgradeError = types.NewUpgradeError(channel.UpgradeSequence, err) + } + if err := k.writeErrorReceipt(ctx, portID, channelID, upgrade, upgradeError); err != nil { return err } diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index b795bafb7f3..4f202889940 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -614,7 +614,7 @@ func (suite *KeeperTestSuite) assertUpgradeError(actualError, expError error) { func (suite *KeeperTestSuite) TestAbortHandshake() { var ( path *ibctesting.Path - upgradeError *types.UpgradeError + upgradeError error ) tests := []struct { @@ -627,6 +627,15 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { malleate: func() {}, expPass: true, }, + { + name: "regular error", + malleate: func() { + // in app callbacks error receipts should still be written if a regular error is returned. + // i.e. not an instance of `types.UpgradeError` + upgradeError = types.ErrInvalidUpgrade + }, + expPass: true, + }, { name: "upgrade does not exist", malleate: func() { @@ -670,7 +679,7 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { tc.malleate() - err := channelKeeper.AbortHandshake(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, upgradeError) + err := channelKeeper.AbortUpgrade(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, upgradeError) if tc.expPass { suite.Require().NoError(err) @@ -685,7 +694,10 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { errorReceipt, found := channelKeeper.GetUpgradeErrorReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) suite.Require().True(found, "error receipt should be found") - suite.Require().Equal(upgradeError.GetErrorReceipt(), errorReceipt, "error receipt does not match expected error receipt") + + if ue, ok := upgradeError.(*types.UpgradeError); ok { + suite.Require().Equal(ue.GetErrorReceipt(), errorReceipt, "error receipt does not match expected error receipt") + } _, found = channelKeeper.GetCounterpartyLastPacketSequence(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) suite.Require().False(found, "counterparty last packet sequence should not be found") From 811c4189a7855365e2ec54dda9c4bde4ee0edf2e Mon Sep 17 00:00:00 2001 From: chatton Date: Mon, 12 Jun 2023 13:00:42 +0100 Subject: [PATCH 14/33] addressing PR feedback --- modules/core/04-channel/keeper/events.go | 6 +++--- modules/core/04-channel/keeper/upgrade.go | 4 ++-- modules/core/04-channel/types/events.go | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/modules/core/04-channel/keeper/events.go b/modules/core/04-channel/keeper/events.go index 353a8670264..8390adfd88b 100644 --- a/modules/core/04-channel/keeper/events.go +++ b/modules/core/04-channel/keeper/events.go @@ -387,11 +387,11 @@ 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) { +// 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.EventTypeChannelUpgradeCancelled, + types.EventTypeChannelUpgradeCancel, sdk.NewAttribute(types.AttributeKeyPortID, portID), sdk.NewAttribute(types.AttributeKeyChannelID, channelID), sdk.NewAttribute(types.AttributeCounterpartyPortID, currentChannel.Counterparty.PortId), diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 2584602201c..cd0dcc7704e 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -160,7 +160,7 @@ func (k Keeper) WriteUpgradeCancelChannel(ctx sdk.Context, portID, channelID str 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)) + panic(fmt.Sprintf("could not find existing channel, channelID: %s, portID: %s", channelID, portID)) } upgrade, found := k.GetUpgrade(ctx, portID, channelID) @@ -172,7 +172,7 @@ func (k Keeper) WriteUpgradeCancelChannel(ctx sdk.Context, portID, channelID str return errorsmod.Wrap(types.ErrUpgradeRestoreFailed, fmt.Sprintf("failed to restore channel, channelID: %s, portID: %s", channelID, portID)) } - emitUpgradedCancelledEvent(ctx, portID, channelID, channel, upgrade) + emitChannelUpgradeCancelEvent(ctx, portID, channelID, channel, upgrade) return nil } diff --git a/modules/core/04-channel/types/events.go b/modules/core/04-channel/types/events.go index c951d7876cd..5141a27e4f3 100644 --- a/modules/core/04-channel/types/events.go +++ b/modules/core/04-channel/types/events.go @@ -61,8 +61,8 @@ var ( EventTypeChannelUpgradeInit = "channel_upgrade_init" EventTypeChannelUpgradeTry = "channel_upgrade_try" EventTypeChannelUpgradeAck = "channel_upgrade_ack" - EventTypeChannelUpgradeOpen = "channel_upgrade_open" - EventTypeChannelUpgradeCancelled = "channel_upgrade_cancelled" + EventTypeChannelUpgradeOpen = "channel_upgrade_open" + EventTypeChannelUpgradeCancel = "channel_upgrade_cancelled" AttributeValueCategory = fmt.Sprintf("%s_%s", ibcexported.ModuleName, SubModuleName) ) From 51e907065f1c2a47b0e88cf52b954a1ad9ccaa56 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 14 Jun 2023 10:58:55 +0100 Subject: [PATCH 15/33] fix merge conflicts --- modules/core/04-channel/keeper/export_test.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/modules/core/04-channel/keeper/export_test.go b/modules/core/04-channel/keeper/export_test.go index 4a7cd48a989..d97fd5e5a67 100644 --- a/modules/core/04-channel/keeper/export_test.go +++ b/modules/core/04-channel/keeper/export_test.go @@ -31,13 +31,7 @@ func (k Keeper) ValidateUpgradeFields(ctx sdk.Context, proposedUpgrade types.Upg return k.validateUpgradeFields(ctx, proposedUpgrade, currentChannel) } -<<<<<<< HEAD -// AbortUpgrade is a wrapper around abortUpgrade to allow the function to be directly called in tests. -func (k Keeper) AbortUpgrade(ctx sdk.Context, portID, channelID string, err error) error { - return k.abortUpgrade(ctx, portID, channelID, err) -======= // WriteUpgradeOpenChannel is a wrapper around writeUpgradeOpenChannel to allow the function to be directly called in tests. func (k Keeper) WriteUpgradeOpenChannel(ctx sdk.Context, portID, channelID string) { k.writeUpgradeOpenChannel(ctx, portID, channelID) ->>>>>>> 04-channel-upgrades } From 3e0bf6ebcf8285fb721a55ea241f128805fb6b90 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 14 Jun 2023 11:05:48 +0100 Subject: [PATCH 16/33] ran linter --- modules/core/04-channel/types/events.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/modules/core/04-channel/types/events.go b/modules/core/04-channel/types/events.go index 5141a27e4f3..d82703c40f3 100644 --- a/modules/core/04-channel/types/events.go +++ b/modules/core/04-channel/types/events.go @@ -51,16 +51,16 @@ 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" + 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" From 098d0a31fc0449cd70d9bf96f254c02811b59c8e Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 14 Jun 2023 11:49:14 +0100 Subject: [PATCH 17/33] happy path test --- modules/core/04-channel/keeper/upgrade.go | 3 ++- .../core/04-channel/keeper/upgrade_test.go | 20 +++++++++++++------ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index c69ec892328..df65fc0d6ad 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -259,7 +259,8 @@ func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, err // If counterparty sequence is less than the current sequence, abort transaction since this error receipt is from a previous upgrade // Otherwise, set the sequence to counterparty's error sequence+1 so that both sides start with a fresh sequence currentSequence := channel.UpgradeSequence - if errorReceipt.Sequence >= currentSequence { + counterpartySequence := errorReceipt.Sequence + if counterpartySequence < currentSequence { return errorsmod.Wrap(types.ErrInvalidUpgradeSequence, "error sequence must be less than current sequence") } diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 6f9dd60e22e..016170b4f32 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -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" @@ -909,6 +910,13 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { 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) @@ -916,14 +924,14 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { channelAKeeper := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper - // TODO: populate these correctly - errorReceipt := types.ErrorReceipt{} - errReceiptProof := []byte{} - proofHeight := clienttypes.Height{} + upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + errorReceiptProof, proofHeight := suite.chainB.QueryProof(upgradeErrorReceiptKey) - err = channelAKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errReceiptProof, proofHeight) - suite.Require().NoError(err) + errorReceipt, ok := suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + suite.Require().True(ok) + err = channelAKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errorReceiptProof, proofHeight) + suite.Require().NoError(err) }) } } From 853eae266f906bc02a691eba7804e828db98755c Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 14 Jun 2023 14:28:31 +0100 Subject: [PATCH 18/33] adding tests for ChanUpgradeCancel --- modules/core/04-channel/keeper/upgrade.go | 16 ++-- .../core/04-channel/keeper/upgrade_test.go | 75 ++++++++++++++++--- 2 files changed, 71 insertions(+), 20 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index df65fc0d6ad..595ddc78d57 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -252,12 +252,8 @@ func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, err return errorsmod.Wrap(err, "failed to verify counterparty error receipt") } - if isEmptyReceipt(errorReceipt) { - return errorsmod.Wrap(types.ErrInvalidUpgradeErrorReceipt, "empty error receipt") - } - - // If counterparty sequence is less than the current sequence, abort transaction since this error receipt is from a previous upgrade - // Otherwise, set the sequence to counterparty's error sequence+1 so that both sides start with a fresh sequence + // 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 { @@ -270,11 +266,6 @@ func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, err return nil } -// isEmptyReceipt returns true if the receipt is empty. -func isEmptyReceipt(receipt types.ErrorReceipt) bool { - return receipt == types.ErrorReceipt{} -} - // 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) { @@ -290,8 +281,11 @@ func (k Keeper) WriteUpgradeCancelChannel(ctx sdk.Context, portID, channelID str 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) } diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 016170b4f32..bc20e0fc2f0 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -880,17 +880,65 @@ func (suite *KeeperTestSuite) TestAbortHandshake() { func (suite *KeeperTestSuite) TestChanUpgradeCancel() { var ( - path *ibctesting.Path + path *ibctesting.Path + errorReceipt types.ErrorReceipt + errorReceiptProof []byte + proofHeight clienttypes.Height ) tests := []struct { name string malleate func() - expPass bool + expError error }{ { name: "success", malleate: func() {}, - expPass: true, + 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, }, } @@ -922,16 +970,25 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { suite.Require().NoError(path.EndpointA.UpdateClient()) - channelAKeeper := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper - upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) - errorReceiptProof, proofHeight := suite.chainB.QueryProof(upgradeErrorReceiptKey) + errorReceiptProof, proofHeight = suite.chainB.QueryProof(upgradeErrorReceiptKey) - errorReceipt, ok := suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + 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) - err = channelAKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errorReceiptProof, proofHeight) - suite.Require().NoError(err) + 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) + } }) } } From c2ae4418042d2a353eeee09eedf3cd23b64dc10f Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 14 Jun 2023 14:56:26 +0100 Subject: [PATCH 19/33] add ChannelCancelUpgrade message server implementation --- modules/core/keeper/msg_server.go | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index 33df77fb5b9..75c06385e18 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -820,5 +820,31 @@ 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) + 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 + } + + if err := cbs.OnChanUpgradeRestore(ctx, msg.PortId, msg.ChannelId); err != nil { + ctx.Logger().Error("channel upgrade cancel failed", "port-id", msg.PortId, "error", err.Error()) + return nil, err + } + + ctx.Logger().Info("channel upgrade cancel succeeded", "port-id", msg.PortId, "error", err.Error()) + + return &channeltypes.MsgChannelUpgradeCancelResponse{}, nil } From 85afe0fea55d142845af438a0363651ea5fdc81f Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 14 Jun 2023 16:00:52 +0100 Subject: [PATCH 20/33] chore: add msg server tests for ChannelUpgradeCancel --- .golangci.yml | 2 +- modules/core/keeper/msg_server.go | 4 +- modules/core/keeper/msg_server_test.go | 113 +++++++++++++++++++++++++ 3 files changed, 117 insertions(+), 2 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 56f4fbcf3c9..4e07742d050 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -63,4 +63,4 @@ linters-settings: revive: rules: - name: if-return - disabled: true \ No newline at end of file + disabled: true diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index 75c06385e18..8a3e1abc9fb 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -844,7 +844,9 @@ func (k Keeper) ChannelUpgradeCancel(goCtx context.Context, msg *channeltypes.Ms return nil, err } - ctx.Logger().Info("channel upgrade cancel succeeded", "port-id", msg.PortId, "error", err.Error()) + 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 } diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go index d2e23861d7c..734f19d5542 100644 --- a/modules/core/keeper/msg_server_test.go +++ b/modules/core/keeper/msg_server_test.go @@ -9,6 +9,7 @@ import ( clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" + porttypes "github.com/cosmos/ibc-go/v7/modules/core/05-port/types" commitmenttypes "github.com/cosmos/ibc-go/v7/modules/core/23-commitment/types" host "github.com/cosmos/ibc-go/v7/modules/core/24-host" "github.com/cosmos/ibc-go/v7/modules/core/exported" @@ -924,3 +925,115 @@ func (suite *KeeperTestSuite) TestChannelUpgradeTry() { }) } } + +func (suite *KeeperTestSuite) TestChannelUpgradeCancel() { + var ( + path *ibctesting.Path + msg *channeltypes.MsgChannelUpgradeCancel + ) + + cases := []struct { + name string + malleate func() + expErr error + }{ + { + name: "success", + malleate: func() {}, + expErr: nil, + }, + { + name: "invalid proof", + malleate: func() { + msg.ProofErrorReceipt = []byte("invalid proof") + }, + expErr: commitmenttypes.ErrInvalidProof, + }, + { + name: "invalid error receipt sequence", + malleate: func() { + msg.ErrorReceipt.Sequence = 0 + }, + expErr: channeltypes.ErrInvalidUpgradeSequence, + }, + { + name: "invalid channel id", + malleate: func() { + msg.ChannelId = ibctesting.InvalidID + }, + expErr: channeltypes.ErrInvalidChannel, + }, + { + name: "invalid port id", + malleate: func() { + msg.PortId = ibctesting.InvalidID + }, + expErr: porttypes.ErrInvalidPort, + }, + } + + for _, tc := range cases { + tc := tc + suite.Run(tc.name, func() { + suite.SetupTest() + + path = ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.Setup(path) + + preUpgradeVersion := path.EndpointA.ChannelConfig.Version + // configure the channel upgrade version on testing endpoints + path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion + path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = ibcmock.UpgradeVersion + + err := path.EndpointA.ChanUpgradeInit() + suite.Require().NoError(err) + + err = path.EndpointB.UpdateClient() + suite.Require().NoError(err) + + // 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 channeltypes.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) + + errorReceipt, ok := suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + suite.Require().True(ok) + + msg = &channeltypes.MsgChannelUpgradeCancel{ + PortId: path.EndpointA.ChannelConfig.PortID, + ChannelId: path.EndpointA.ChannelID, + ErrorReceipt: errorReceipt, + ProofErrorReceipt: errorReceiptProof, + ProofHeight: proofHeight, + Signer: suite.chainA.SenderAccount.GetAddress().String(), + } + + tc.malleate() + + res, err := suite.chainA.GetSimApp().GetIBCKeeper().ChannelUpgradeCancel(suite.chainA.GetContext(), msg) + + expPass := tc.expErr == nil + if expPass { + suite.Require().NoError(err) + channel := path.EndpointA.GetChannel() + suite.Require().Equal(preUpgradeVersion, channel.Version, "channel version should be reverted") + suite.Require().Equalf(channeltypes.OPEN, channel.State, "channel state should be %s", channeltypes.OPEN.String()) + suite.Require().Equalf(channeltypes.NOTINFLUSH, channel.FlushStatus, "channel flush status should be %s", channeltypes.NOTINFLUSH.String()) + suite.Require().Equal(errorReceipt.Sequence+1, channel.UpgradeSequence, "channel upgrade sequence should be incremented") + } else { + suite.Require().Error(err) + suite.Require().Nil(res) + } + }) + } +} From bf108561c807b7b5d13bfd7e3436524d0b13bb41 Mon Sep 17 00:00:00 2001 From: chatton Date: Fri, 16 Jun 2023 09:32:38 +0100 Subject: [PATCH 21/33] addresing PR feedback --- modules/core/04-channel/keeper/upgrade.go | 9 ++++++++- modules/core/04-channel/keeper/upgrade_test.go | 10 ++++------ modules/core/04-channel/types/errors.go | 1 - 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 595ddc78d57..a92ef651627 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -248,12 +248,19 @@ func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, err return errorsmod.Wrap(err, "failed to retrieve connection using the channel connection hops") } + if connection.GetState() != int32(connectiontypes.OPEN) { + return errorsmod.Wrapf( + connectiontypes.ErrInvalidConnectionState, + "connection state is not OPEN (got %s)", connectiontypes.State(connection.GetState()).String(), + ) + } + 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. + // Otherwise, set our upgrade sequence to the counterparty's error sequence + 1 so that both sides start with a fresh sequence. currentSequence := channel.UpgradeSequence counterpartySequence := errorReceipt.Sequence if counterpartySequence < currentSequence { diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index bc20e0fc2f0..0c76c55d3b5 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -921,7 +921,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { expError: connectiontypes.ErrConnectionNotFound, }, { - name: "counter party upgrade sequence less than current sequence", + name: "counter partyupgrade 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) @@ -953,8 +953,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { 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.EndpointA.ChanUpgradeInit()) suite.Require().NoError(path.EndpointB.UpdateClient()) @@ -965,8 +964,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { return "", fmt.Errorf("mock app callback failed") } - err = path.EndpointB.ChanUpgradeTry() - suite.Require().NoError(err) + suite.Require().NoError(path.EndpointB.ChanUpgradeTry()) suite.Require().NoError(path.EndpointA.UpdateClient()) @@ -979,7 +977,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { tc.malleate() - err = suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errorReceiptProof, proofHeight) + 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 { diff --git a/modules/core/04-channel/types/errors.go b/modules/core/04-channel/types/errors.go index 8a3e628bba0..3b11df9b162 100644 --- a/modules/core/04-channel/types/errors.go +++ b/modules/core/04-channel/types/errors.go @@ -47,5 +47,4 @@ 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") ) From 2f540ce06ad495701831efd23a312598bda2f5dc Mon Sep 17 00:00:00 2001 From: chatton Date: Fri, 16 Jun 2023 13:04:25 +0100 Subject: [PATCH 22/33] corrected assertion arguments --- modules/core/04-channel/keeper/upgrade_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 0c76c55d3b5..44d32871742 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -985,7 +985,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { channel := path.EndpointA.GetChannel() suite.Require().Equal(errorReceipt.Sequence+1, channel.UpgradeSequence, "upgrade sequence should be incremented") } else { - suite.Require().ErrorIs(tc.expError, err) + suite.Require().ErrorIs(err, tc.expError) } }) } From 78fb32b9b16afd9efb4af318283e66cad58e7156 Mon Sep 17 00:00:00 2001 From: chatton Date: Fri, 16 Jun 2023 13:44:03 +0100 Subject: [PATCH 23/33] address PR feedback --- modules/core/04-channel/types/events.go | 2 +- modules/core/keeper/msg_server.go | 6 ++-- modules/core/keeper/msg_server_test.go | 43 +++++++++++++++++-------- 3 files changed, 34 insertions(+), 17 deletions(-) diff --git a/modules/core/04-channel/types/events.go b/modules/core/04-channel/types/events.go index d82703c40f3..8a7f9a3e9b4 100644 --- a/modules/core/04-channel/types/events.go +++ b/modules/core/04-channel/types/events.go @@ -62,7 +62,7 @@ var ( EventTypeChannelUpgradeTry = "channel_upgrade_try" EventTypeChannelUpgradeAck = "channel_upgrade_ack" EventTypeChannelUpgradeOpen = "channel_upgrade_open" - EventTypeChannelUpgradeCancel = "channel_upgrade_cancelled" + EventTypeChannelUpgradeCancel = "channel_upgrade_cancel" AttributeValueCategory = fmt.Sprintf("%s_%s", ibcexported.ModuleName, SubModuleName) ) diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index 8a3e1abc9fb..08cd48de71e 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -836,12 +836,12 @@ func (k Keeper) ChannelUpgradeCancel(goCtx context.Context, msg *channeltypes.Ms 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 + return nil, errorsmod.Wrap(err, "channel upgrade cancel failed") } if err := cbs.OnChanUpgradeRestore(ctx, msg.PortId, msg.ChannelId); err != nil { - ctx.Logger().Error("channel upgrade cancel failed", "port-id", msg.PortId, "error", err.Error()) - return nil, err + 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, 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) diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go index 734f19d5542..a6b5b90fd40 100644 --- a/modules/core/keeper/msg_server_test.go +++ b/modules/core/keeper/msg_server_test.go @@ -9,7 +9,6 @@ import ( clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" - porttypes "github.com/cosmos/ibc-go/v7/modules/core/05-port/types" commitmenttypes "github.com/cosmos/ibc-go/v7/modules/core/23-commitment/types" host "github.com/cosmos/ibc-go/v7/modules/core/24-host" "github.com/cosmos/ibc-go/v7/modules/core/exported" @@ -952,23 +951,41 @@ func (suite *KeeperTestSuite) TestChannelUpgradeCancel() { { name: "invalid error receipt sequence", malleate: func() { - msg.ErrorReceipt.Sequence = 0 + const invalidSequence = 0 + + errorReceipt, ok := suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + suite.Require().True(ok) + + errorReceipt.Sequence = invalidSequence + + // overwrite the error receipt with an invalid sequence. + suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, errorReceipt) + + // ensure that the error receipt is committed to state. + suite.coordinator.CommitBlock(suite.chainB) + suite.Require().NoError(path.EndpointA.UpdateClient()) + + // retrieve the error receipt proof and proof height. + upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) + errorReceiptProof, proofHeight := suite.chainB.QueryProof(upgradeErrorReceiptKey) + + // provide a valid proof of the error receipt with an invalid sequence. + msg.ErrorReceipt.Sequence = invalidSequence + msg.ProofErrorReceipt = errorReceiptProof + msg.ProofHeight = proofHeight }, expErr: channeltypes.ErrInvalidUpgradeSequence, }, { - name: "invalid channel id", + name: "application callback fails", malleate: func() { - msg.ChannelId = ibctesting.InvalidID - }, - expErr: channeltypes.ErrInvalidChannel, - }, - { - name: "invalid port id", - malleate: func() { - msg.PortId = ibctesting.InvalidID + suite.chainA.GetSimApp().IBCMockModule.IBCApp.OnChanUpgradeRestore = func(ctx sdk.Context, portID, channelID string) error { + // return an error type that is not returned in the regular flow. + return upgradetypes.ErrIntOverflowUpgrade + } }, - expErr: porttypes.ErrInvalidPort, + // error should be what the application callback returned. + expErr: upgradetypes.ErrIntOverflowUpgrade, }, } @@ -1031,8 +1048,8 @@ func (suite *KeeperTestSuite) TestChannelUpgradeCancel() { suite.Require().Equalf(channeltypes.NOTINFLUSH, channel.FlushStatus, "channel flush status should be %s", channeltypes.NOTINFLUSH.String()) suite.Require().Equal(errorReceipt.Sequence+1, channel.UpgradeSequence, "channel upgrade sequence should be incremented") } else { - suite.Require().Error(err) suite.Require().Nil(res) + suite.Require().ErrorIs(err, tc.expErr) } }) } From 415e66a7454a82f65f8dce8277ac7310b71368bc Mon Sep 17 00:00:00 2001 From: chatton Date: Fri, 16 Jun 2023 15:37:00 +0100 Subject: [PATCH 24/33] fix linter --- modules/core/keeper/msg_server_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go index ca79046c53b..d0195a22c48 100644 --- a/modules/core/keeper/msg_server_test.go +++ b/modules/core/keeper/msg_server_test.go @@ -1056,7 +1056,7 @@ func (suite *KeeperTestSuite) TestChannelUpgradeCancel() { suite.Require().Equalf(prevChannel.State, channel.State, "channel state should be %s", prevChannel.State.String()) suite.Require().Equalf(prevChannel.FlushStatus, channel.FlushStatus, "channel flush status should be %s", prevChannel.FlushStatus.String()) // TODO - //suite.Require().Equal(prevChannel.UpgradeSequence, channel.UpgradeSequence, "channel upgrade sequence should not incremented") + } }) } From cc8cb484628409c2fa2f35455369c245eabbd531 Mon Sep 17 00:00:00 2001 From: chatton Date: Fri, 16 Jun 2023 15:38:38 +0100 Subject: [PATCH 25/33] re-added commented out test --- modules/core/keeper/msg_server_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go index d0195a22c48..fe08b6d3aa4 100644 --- a/modules/core/keeper/msg_server_test.go +++ b/modules/core/keeper/msg_server_test.go @@ -1055,8 +1055,7 @@ func (suite *KeeperTestSuite) TestChannelUpgradeCancel() { suite.Require().Equal(prevChannel.Version, channel.Version, "channel version should not be changed") suite.Require().Equalf(prevChannel.State, channel.State, "channel state should be %s", prevChannel.State.String()) suite.Require().Equalf(prevChannel.FlushStatus, channel.FlushStatus, "channel flush status should be %s", prevChannel.FlushStatus.String()) - // TODO - + // TODO suite.Require().Equal(prevChannel.UpgradeSequence, channel.UpgradeSequence, "channel upgrade sequence should not incremented") } }) } From 269d9e05d073a7460887606954f0bc19c68a5015 Mon Sep 17 00:00:00 2001 From: chatton Date: Mon, 19 Jun 2023 09:57:22 +0100 Subject: [PATCH 26/33] pass the new upgrade sequence as an argument to restoreChannel --- modules/core/04-channel/keeper/upgrade.go | 28 +++++++++---------- .../core/04-channel/keeper/upgrade_test.go | 6 ++-- modules/core/keeper/msg_server.go | 5 ++-- modules/core/keeper/msg_server_test.go | 2 +- 4 files changed, 20 insertions(+), 21 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 572e3f406e7..a4e17a4455e 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -231,32 +231,32 @@ func (k Keeper) WriteUpgradeAckChannel( } // 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 { +func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, errorReceipt types.ErrorReceipt, errorReceiptProof []byte, proofHeight clienttypes.Height) (uint64, error) { channel, found := k.GetChannel(ctx, portID, channelID) if !found { - return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) + return 0, 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) + return 0, 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") + return 0, errorsmod.Wrap(err, "failed to retrieve connection using the channel connection hops") } if connection.GetState() != int32(connectiontypes.OPEN) { - return errorsmod.Wrapf( + return 0, errorsmod.Wrapf( connectiontypes.ErrInvalidConnectionState, "connection state is not OPEN (got %s)", connectiontypes.State(connection.GetState()).String(), ) } if err := k.connectionKeeper.VerifyChannelUpgradeError(ctx, portID, channelID, connection, errorReceipt, errorReceiptProof, proofHeight); err != nil { - return errorsmod.Wrap(err, "failed to verify counterparty error receipt") + return 0, 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. @@ -264,18 +264,15 @@ func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, err currentSequence := channel.UpgradeSequence counterpartySequence := errorReceipt.Sequence if counterpartySequence < currentSequence { - return errorsmod.Wrap(types.ErrInvalidUpgradeSequence, "error sequence must be less than current sequence") + return 0, 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 + return errorReceipt.Sequence + 1, 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) { +func (k Keeper) WriteUpgradeCancelChannel(ctx sdk.Context, portID, channelID string, sequence uint64) { defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-cancel") upgrade, found := k.GetUpgrade(ctx, portID, channelID) @@ -290,7 +287,7 @@ func (k Keeper) WriteUpgradeCancelChannel(ctx sdk.Context, portID, channelID str previousState := channel.State - k.restoreChannel(ctx, portID, channelID, channel) + k.restoreChannel(ctx, portID, channelID, sequence, 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) @@ -608,7 +605,7 @@ func (k Keeper) AbortUpgrade(ctx sdk.Context, portID, channelID string, err erro return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) } - k.restoreChannel(ctx, portID, channelID, channel) + k.restoreChannel(ctx, portID, channelID, channel.UpgradeSequence, channel) // in the case of application callbacks, the error may not be an upgrade error. // in this case we need to construct one in order to write the error receipt. @@ -628,9 +625,10 @@ func (k Keeper) AbortUpgrade(ctx sdk.Context, portID, channelID string, err erro } // restoreChannel will restore the channel state and flush status to their pre-upgrade state so that upgrade is aborted. -func (k Keeper) restoreChannel(ctx sdk.Context, portID, channelID string, currentChannel types.Channel) { +func (k Keeper) restoreChannel(ctx sdk.Context, portID, channelID string, upgradeSequence uint64, currentChannel types.Channel) { currentChannel.State = types.OPEN currentChannel.FlushStatus = types.NOTINFLUSH + currentChannel.UpgradeSequence = upgradeSequence k.SetChannel(ctx, portID, channelID, currentChannel) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 44d32871742..9f019ad1e59 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -977,15 +977,15 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { tc.malleate() - err := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errorReceiptProof, proofHeight) + newUpgradeSequence, 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") + suite.Require().Equal(errorReceipt.Sequence+1, newUpgradeSequence, "upgrade sequence should be incremented") } else { suite.Require().ErrorIs(err, tc.expError) + suite.Require().Equal(uint64(0), newUpgradeSequence, "upgrade sequence should not be incremented in the case of an error") } }) } diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index 08cd48de71e..72251391cbe 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -834,7 +834,8 @@ func (k Keeper) ChannelUpgradeCancel(goCtx context.Context, msg *channeltypes.Ms 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 { + newUpgradeSequence, err := k.ChannelKeeper.ChanUpgradeCancel(ctx, msg.PortId, msg.ChannelId, msg.ErrorReceipt, msg.ProofErrorReceipt, msg.ProofHeight) + if err != nil { ctx.Logger().Error("channel upgrade cancel failed", "port-id", msg.PortId, "error", err.Error()) return nil, errorsmod.Wrap(err, "channel upgrade cancel failed") } @@ -844,7 +845,7 @@ func (k Keeper) ChannelUpgradeCancel(goCtx context.Context, msg *channeltypes.Ms 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) + k.ChannelKeeper.WriteUpgradeCancelChannel(ctx, msg.PortId, msg.ChannelId, newUpgradeSequence) ctx.Logger().Info("channel upgrade cancel succeeded", "port-id", msg.PortId, "channel-id", msg.ChannelId) diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go index fe08b6d3aa4..80cb5f1e6da 100644 --- a/modules/core/keeper/msg_server_test.go +++ b/modules/core/keeper/msg_server_test.go @@ -1055,7 +1055,7 @@ func (suite *KeeperTestSuite) TestChannelUpgradeCancel() { suite.Require().Equal(prevChannel.Version, channel.Version, "channel version should not be changed") suite.Require().Equalf(prevChannel.State, channel.State, "channel state should be %s", prevChannel.State.String()) suite.Require().Equalf(prevChannel.FlushStatus, channel.FlushStatus, "channel flush status should be %s", prevChannel.FlushStatus.String()) - // TODO suite.Require().Equal(prevChannel.UpgradeSequence, channel.UpgradeSequence, "channel upgrade sequence should not incremented") + suite.Require().Equal(prevChannel.UpgradeSequence, channel.UpgradeSequence, "channel upgrade sequence should not incremented") } }) } From 2717582c44e0d4b2b16fb4da1f7423684dece266 Mon Sep 17 00:00:00 2001 From: chatton Date: Mon, 19 Jun 2023 09:57:52 +0100 Subject: [PATCH 27/33] rename variable to be more clear --- modules/core/04-channel/keeper/upgrade.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index a4e17a4455e..161be8fcd48 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -272,7 +272,7 @@ func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, err // 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, sequence uint64) { +func (k Keeper) WriteUpgradeCancelChannel(ctx sdk.Context, portID, channelID string, newUpgradeSequence uint64) { defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-cancel") upgrade, found := k.GetUpgrade(ctx, portID, channelID) @@ -287,7 +287,7 @@ func (k Keeper) WriteUpgradeCancelChannel(ctx sdk.Context, portID, channelID str previousState := channel.State - k.restoreChannel(ctx, portID, channelID, sequence, channel) + k.restoreChannel(ctx, portID, channelID, newUpgradeSequence, 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) From d023ac002995639f2b73da777b9896066bd3115b Mon Sep 17 00:00:00 2001 From: chatton Date: Mon, 19 Jun 2023 11:10:55 +0100 Subject: [PATCH 28/33] use error receipt Sequence instead of sequence + 1 --- modules/core/04-channel/keeper/upgrade.go | 2 +- modules/core/04-channel/keeper/upgrade_test.go | 2 +- modules/core/keeper/msg_server_test.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 56d56a1585a..869693f044b 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -268,7 +268,7 @@ func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, err return 0, errorsmod.Wrap(types.ErrInvalidUpgradeSequence, "error sequence must be less than current sequence") } - return errorReceipt.Sequence + 1, nil + return errorReceipt.Sequence, nil } // WriteUpgradeCancelChannel writes a channel which has canceled the upgrade process.Auxiliary upgrade state is diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index a1b8386f958..0335eeabb9c 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -1173,7 +1173,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { expPass := tc.expError == nil if expPass { suite.Require().NoError(err) - suite.Require().Equal(errorReceipt.Sequence+1, newUpgradeSequence, "upgrade sequence should be incremented") + suite.Require().Equal(errorReceipt.Sequence, newUpgradeSequence, "upgrade sequence should be incremented") } else { suite.Require().ErrorIs(err, tc.expError) suite.Require().Equal(uint64(0), newUpgradeSequence, "upgrade sequence should not be incremented in the case of an error") diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go index 80cb5f1e6da..a3154a87d71 100644 --- a/modules/core/keeper/msg_server_test.go +++ b/modules/core/keeper/msg_server_test.go @@ -1045,7 +1045,7 @@ func (suite *KeeperTestSuite) TestChannelUpgradeCancel() { suite.Require().Equal(prevChannel.Version, channel.Version, "channel version should be reverted") suite.Require().Equalf(channeltypes.OPEN, channel.State, "channel state should be %s", channeltypes.OPEN.String()) suite.Require().Equalf(channeltypes.NOTINFLUSH, channel.FlushStatus, "channel flush status should be %s", channeltypes.NOTINFLUSH.String()) - suite.Require().Equal(errorReceipt.Sequence+1, channel.UpgradeSequence, "channel upgrade sequence should be incremented") + suite.Require().Equal(errorReceipt.Sequence, channel.UpgradeSequence, "channel upgrade sequence should be incremented") } else { suite.Require().Nil(res) suite.Require().ErrorIs(err, tc.expErr) From a3bbf808cb8cd658f4459e61bb9757cf2ed950a1 Mon Sep 17 00:00:00 2001 From: chatton Date: Mon, 19 Jun 2023 17:14:12 +0100 Subject: [PATCH 29/33] addressing PR feedback --- modules/core/04-channel/keeper/upgrade.go | 5 ++--- modules/core/04-channel/keeper/upgrade_test.go | 4 ++-- modules/core/keeper/msg_server_test.go | 11 ++++------- testing/mock/ibc_module.go | 7 +++++++ testing/mock/mock.go | 3 +++ 5 files changed, 18 insertions(+), 12 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 869693f044b..388a6f6b350 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -712,6 +712,8 @@ func (k Keeper) AbortUpgrade(ctx sdk.Context, portID, channelID string, err erro return errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) } + // the channel upgrade sequence has already been updated in ChannelUpgradeTry, so we can pass + // its updated value. k.restoreChannel(ctx, portID, channelID, channel.UpgradeSequence, channel) // in the case of application callbacks, the error may not be an upgrade error. @@ -725,9 +727,6 @@ func (k Keeper) AbortUpgrade(ctx sdk.Context, portID, channelID string, err erro return err } - // TODO: callback execution - // cbs.OnChanUpgradeRestore() - return nil } diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 0335eeabb9c..9ebbc1a1678 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -1173,10 +1173,10 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { expPass := tc.expError == nil if expPass { suite.Require().NoError(err) - suite.Require().Equal(errorReceipt.Sequence, newUpgradeSequence, "upgrade sequence should be incremented") + suite.Require().Equal(errorReceipt.Sequence, newUpgradeSequence, "upgrade sequence should be set to the error receipt sequence") } else { suite.Require().ErrorIs(err, tc.expError) - suite.Require().Equal(uint64(0), newUpgradeSequence, "upgrade sequence should not be incremented in the case of an error") + suite.Require().Equal(uint64(0), newUpgradeSequence) } }) } diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go index a3154a87d71..6ef8b47a288 100644 --- a/modules/core/keeper/msg_server_test.go +++ b/modules/core/keeper/msg_server_test.go @@ -966,8 +966,7 @@ func (suite *KeeperTestSuite) TestChannelUpgradeCancel() { suite.Require().NoError(path.EndpointA.UpdateClient()) // retrieve the error receipt proof and proof height. - upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) - errorReceiptProof, proofHeight := suite.chainB.QueryProof(upgradeErrorReceiptKey) + errorReceiptProof, proofHeight := path.EndpointB.QueryProof(host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)) // provide a valid proof of the error receipt with an invalid sequence. msg.ErrorReceipt.Sequence = invalidSequence @@ -981,11 +980,11 @@ func (suite *KeeperTestSuite) TestChannelUpgradeCancel() { malleate: func() { suite.chainA.GetSimApp().IBCMockModule.IBCApp.OnChanUpgradeRestore = func(ctx sdk.Context, portID, channelID string) error { // return an error type that is not returned in the regular flow. - return upgradetypes.ErrIntOverflowUpgrade + return ibcmock.MockApplicationCallbackError } }, // error should be what the application callback returned. - expErr: upgradetypes.ErrIntOverflowUpgrade, + expErr: ibcmock.MockApplicationCallbackError, }, } @@ -1006,8 +1005,6 @@ func (suite *KeeperTestSuite) TestChannelUpgradeCancel() { // fetch the previous channel when it is in the INITUPGRADE state. prevChannel := path.EndpointA.GetChannel() - 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 channeltypes.Order, connectionHops []string, counterpartyVersion string, @@ -1045,7 +1042,7 @@ func (suite *KeeperTestSuite) TestChannelUpgradeCancel() { suite.Require().Equal(prevChannel.Version, channel.Version, "channel version should be reverted") suite.Require().Equalf(channeltypes.OPEN, channel.State, "channel state should be %s", channeltypes.OPEN.String()) suite.Require().Equalf(channeltypes.NOTINFLUSH, channel.FlushStatus, "channel flush status should be %s", channeltypes.NOTINFLUSH.String()) - suite.Require().Equal(errorReceipt.Sequence, channel.UpgradeSequence, "channel upgrade sequence should be incremented") + suite.Require().Equal(errorReceipt.Sequence, channel.UpgradeSequence, "channel upgrade sequence should be set to error receipt sequence") } else { suite.Require().Nil(res) suite.Require().ErrorIs(err, tc.expErr) diff --git a/testing/mock/ibc_module.go b/testing/mock/ibc_module.go index f93b79c30ae..d355778df0a 100644 --- a/testing/mock/ibc_module.go +++ b/testing/mock/ibc_module.go @@ -14,6 +14,13 @@ import ( "github.com/cosmos/ibc-go/v7/modules/core/exported" ) +// applicationCallbackError is a custom error type that will be unique for testing purposes. +type applicationCallbackError struct{} + +func (e applicationCallbackError) Error() string { + return "mock application callback failed" +} + // IBCModule implements the ICS26 callbacks for testing/mock. type IBCModule struct { appModule *AppModule diff --git a/testing/mock/mock.go b/testing/mock/mock.go index e50585eb49d..050c62823ef 100644 --- a/testing/mock/mock.go +++ b/testing/mock/mock.go @@ -37,6 +37,9 @@ var ( MockAckCanaryCapabilityName = "mock acknowledgement canary capability name" MockTimeoutCanaryCapabilityName = "mock timeout canary capability name" UpgradeVersion = fmt.Sprintf("%s-v2", Version) + // MockApplicationCallbackError should be returned when an application callback should fail. It is possible to + // test that this error was returned using ErrorIs. + MockApplicationCallbackError error = &applicationCallbackError{} ) var _ porttypes.IBCModule = IBCModule{} From c36673a60bf2de0eed0aa8d90a446ee0befe762f Mon Sep 17 00:00:00 2001 From: chatton Date: Tue, 20 Jun 2023 10:21:55 +0100 Subject: [PATCH 30/33] addressing PR feedback --- modules/core/keeper/msg_server_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go index 6ef8b47a288..4b1948b417c 100644 --- a/modules/core/keeper/msg_server_test.go +++ b/modules/core/keeper/msg_server_test.go @@ -1017,7 +1017,7 @@ func (suite *KeeperTestSuite) TestChannelUpgradeCancel() { suite.Require().NoError(path.EndpointA.UpdateClient()) upgradeErrorReceiptKey := host.ChannelUpgradeErrorKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) - errorReceiptProof, proofHeight := suite.chainB.QueryProof(upgradeErrorReceiptKey) + errorReceiptProof, proofHeight := path.EndpointB.QueryProof(upgradeErrorReceiptKey) errorReceipt, ok := suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.GetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) suite.Require().True(ok) From f2782b3973e55dd809a1e3fd73212cc6f0d23d9a Mon Sep 17 00:00:00 2001 From: chatton Date: Tue, 20 Jun 2023 16:01:18 +0100 Subject: [PATCH 31/33] appease linter --- modules/core/04-channel/keeper/upgrade.go | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index e59c398de82..bb162f94e2a 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -356,6 +356,7 @@ func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, err return errorReceipt.Sequence, nil } + // ChanUpgradeTimeout times out an outstanding upgrade. // This should be used by the initialising chain when the counterparty chain has not responded to an upgrade proposal within the specified timeout period. func (k Keeper) ChanUpgradeTimeout( From 0f3bc849d7f8dfeb3dacd0fc87a9519b185f146f Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 21 Jun 2023 09:33:57 +0100 Subject: [PATCH 32/33] added test for capability not found --- modules/core/keeper/msg_server_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go index 0406c9187ff..7cdc0a36c10 100644 --- a/modules/core/keeper/msg_server_test.go +++ b/modules/core/keeper/msg_server_test.go @@ -975,6 +975,13 @@ func (suite *KeeperTestSuite) TestChannelUpgradeCancel() { }, expErr: channeltypes.ErrInvalidUpgradeSequence, }, + { + name: "capability not found", + malleate: func() { + msg.ChannelId = ibctesting.InvalidID + }, + expErr: capabilitytypes.ErrCapabilityNotFound, + }, } for _, tc := range cases { From fa56a6729bc6759e86420bc988188b9413a8cd60 Mon Sep 17 00:00:00 2001 From: chatton Date: Wed, 21 Jun 2023 09:46:30 +0100 Subject: [PATCH 33/33] use msg.ErrorReceipt.Sequence instead of returning from keeper fn --- modules/core/04-channel/keeper/upgrade.go | 16 ++++++++-------- modules/core/04-channel/keeper/upgrade_test.go | 4 +--- modules/core/keeper/msg_server.go | 5 ++--- 3 files changed, 11 insertions(+), 14 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index 4ef89d1c444..ee1e9380c68 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -318,32 +318,32 @@ func (k Keeper) WriteUpgradeAckChannel( } // 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) (uint64, error) { +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 0, errorsmod.Wrapf(types.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID) + 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 0, errorsmod.Wrapf(types.ErrInvalidChannelState, "expected one of [%s, %s], got %s", types.INITUPGRADE, types.TRYUPGRADE, channel.State) + 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 0, errorsmod.Wrap(err, "failed to retrieve connection using the channel connection hops") + return errorsmod.Wrap(err, "failed to retrieve connection using the channel connection hops") } if connection.GetState() != int32(connectiontypes.OPEN) { - return 0, errorsmod.Wrapf( + return errorsmod.Wrapf( connectiontypes.ErrInvalidConnectionState, "connection state is not OPEN (got %s)", connectiontypes.State(connection.GetState()).String(), ) } if err := k.connectionKeeper.VerifyChannelUpgradeError(ctx, portID, channelID, connection, errorReceipt, errorReceiptProof, proofHeight); err != nil { - return 0, errorsmod.Wrap(err, "failed to verify counterparty error receipt") + 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. @@ -351,10 +351,10 @@ func (k Keeper) ChanUpgradeCancel(ctx sdk.Context, portID, channelID string, err currentSequence := channel.UpgradeSequence counterpartySequence := errorReceipt.Sequence if counterpartySequence < currentSequence { - return 0, errorsmod.Wrapf(types.ErrInvalidUpgradeSequence, "error receipt sequence (%d) must be greater than or equal to current sequence (%d)", counterpartySequence, currentSequence) + return errorsmod.Wrapf(types.ErrInvalidUpgradeSequence, "error receipt sequence (%d) must be greater than or equal to current sequence (%d)", counterpartySequence, currentSequence) } - return errorReceipt.Sequence, nil + return nil } // ChanUpgradeTimeout times out an outstanding upgrade. diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index b370626dd51..0dc7c0c71c8 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -1178,15 +1178,13 @@ func (suite *KeeperTestSuite) TestChanUpgradeCancel() { tc.malleate() - newUpgradeSequence, err := suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper.ChanUpgradeCancel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, errorReceipt, errorReceiptProof, proofHeight) + 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) - suite.Require().Equal(errorReceipt.Sequence, newUpgradeSequence, "upgrade sequence should be set to the error receipt sequence") } else { suite.Require().ErrorIs(err, tc.expError) - suite.Require().Equal(uint64(0), newUpgradeSequence) } }) } diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index bce49fc5378..b8d78997e0b 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -834,15 +834,14 @@ func (k Keeper) ChannelUpgradeCancel(goCtx context.Context, msg *channeltypes.Ms return nil, errorsmod.Wrapf(porttypes.ErrInvalidRoute, "route not found to module: %s", module) } - newUpgradeSequence, err := k.ChannelKeeper.ChanUpgradeCancel(ctx, msg.PortId, msg.ChannelId, msg.ErrorReceipt, msg.ProofErrorReceipt, msg.ProofHeight) - if err != nil { + 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, errorsmod.Wrap(err, "channel upgrade cancel failed") } cbs.OnChanUpgradeRestore(ctx, msg.PortId, msg.ChannelId) - k.ChannelKeeper.WriteUpgradeCancelChannel(ctx, msg.PortId, msg.ChannelId, newUpgradeSequence) + k.ChannelKeeper.WriteUpgradeCancelChannel(ctx, msg.PortId, msg.ChannelId, msg.ErrorReceipt.Sequence) ctx.Logger().Info("channel upgrade cancel succeeded", "port-id", msg.PortId, "channel-id", msg.ChannelId)