From aa63a78d7f5b62d34c44c0c2740d496f5975f1f1 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 14 Jun 2023 13:08:04 +0200 Subject: [PATCH 1/3] updating tests to use expError in favour of expPass bool --- .../core/04-channel/keeper/upgrade_test.go | 57 ++++++++++--------- 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index 269b4f32c9c..bcda50f63f4 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -335,12 +335,12 @@ func (suite *KeeperTestSuite) TestChanUpgradeAck() { testCases := []struct { name string malleate func() - expPass bool + expError error }{ { "success", func() {}, - true, + nil, }, { "success with later upgrade sequence", @@ -358,7 +358,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeAck() { err := path.EndpointA.UpdateClient() suite.Require().NoError(err) }, - true, + nil, }, { "channel not found", @@ -366,21 +366,21 @@ func (suite *KeeperTestSuite) TestChanUpgradeAck() { path.EndpointA.ChannelID = ibctesting.InvalidID path.EndpointA.ChannelConfig.PortID = ibctesting.InvalidID }, - false, + types.ErrChannelNotFound, }, { "channel state is not in INITUPGRADE or TRYUPGRADE state", func() { suite.Require().NoError(path.EndpointA.SetChannelState(types.CLOSED)) }, - false, + types.ErrInvalidChannelState, }, { "counterparty flush status is not in FLUSHING or FLUSHCOMPLETE", func() { counterpartyFlushStatus = types.NOTINFLUSH }, - false, + types.ErrInvalidFlushStatus, }, { "connection not found", @@ -389,7 +389,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeAck() { channel.ConnectionHops = []string{"connection-100"} path.EndpointA.SetChannel(channel) }, - false, + connectiontypes.ErrConnectionNotFound, }, { "invalid connection state", @@ -398,7 +398,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeAck() { connectionEnd.State = connectiontypes.UNINITIALIZED path.EndpointA.SetConnection(connectionEnd) }, - false, + connectiontypes.ErrInvalidConnectionState, }, { "upgrade not found", @@ -406,38 +406,39 @@ func (suite *KeeperTestSuite) TestChanUpgradeAck() { store := suite.chainA.GetContext().KVStore(suite.chainA.GetSimApp().GetKey(exported.ModuleName)) store.Delete(host.ChannelUpgradeKey(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)) }, - false, + types.ErrUpgradeNotFound, }, { - "channel end version mismatch on crossing hellos", + "startFlushUpgradeHandshake fails due to proof verification failure, counterparty upgrade connection hops are tampered with", func() { - channel := path.EndpointA.GetChannel() - channel.State = types.TRYUPGRADE - - path.EndpointA.SetChannel(channel) - - upgrade := path.EndpointA.GetChannelUpgrade() - upgrade.Fields.Version = "invalid-version" - - path.EndpointA.SetChannelUpgrade(upgrade) + counterpartyUpgrade.Fields.ConnectionHops = []string{ibctesting.InvalidID} }, - false, + commitmenttypes.ErrInvalidProof, }, { - "startFlushUpgradeHandshake fails due to proof verification failure, counterparty upgrade connection hops are tampered with", + "startFlushUpgradeHandshake fails due to mismatch in upgrade ordering", func() { - counterpartyUpgrade.Fields.ConnectionHops = []string{ibctesting.InvalidID} + upgrade := path.EndpointA.GetChannelUpgrade() + upgrade.Fields.Ordering = types.NONE + + path.EndpointA.SetChannelUpgrade(upgrade) }, - false, + types.NewUpgradeError(1, types.ErrIncompatibleCounterpartyUpgrade), }, { - "startFlushUpgradeHandshake fails due to mismatch in upgrade sequences", + "channel end version mismatch on crossing hellos", func() { channel := path.EndpointA.GetChannel() - channel.UpgradeSequence = 5 + channel.State = types.TRYUPGRADE + path.EndpointA.SetChannel(channel) + + upgrade := path.EndpointA.GetChannelUpgrade() + upgrade.Fields.Version = "invalid-version" + + path.EndpointA.SetChannelUpgrade(upgrade) }, - false, + types.NewUpgradeError(1, types.ErrIncompatibleCounterpartyUpgrade), }, } @@ -475,10 +476,10 @@ func (suite *KeeperTestSuite) TestChanUpgradeAck() { proofChannel, proofUpgrade, proofHeight, ) - if tc.expPass { + if expPass := tc.expError == nil; expPass { suite.Require().NoError(err) } else { - suite.Require().Error(err) + suite.assertUpgradeError(err, tc.expError) } }) } From a1832c9be0d5ce98db795db25490a6a629bf0f28 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 14 Jun 2023 13:08:55 +0200 Subject: [PATCH 2/3] move write fn under chanUpgradeAck --- modules/core/04-channel/keeper/upgrade.go | 52 +++++++++++------------ 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/modules/core/04-channel/keeper/upgrade.go b/modules/core/04-channel/keeper/upgrade.go index b0e38453443..c0ee66eb85f 100644 --- a/modules/core/04-channel/keeper/upgrade.go +++ b/modules/core/04-channel/keeper/upgrade.go @@ -204,32 +204,6 @@ func (k Keeper) WriteUpgradeTryChannel(ctx sdk.Context, portID, channelID string return channel, upgrade } -// WriteUpgradeAckChannel writes a channel which has successfully passed the UpgradeAck handshake step as well as -// setting the upgrade for that channel. -// An event is emitted for the handshake step. -func (k Keeper) WriteUpgradeAckChannel( - ctx sdk.Context, - portID, channelID string, - proposedUpgrade types.Upgrade, -) { - defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-ack") - - channel, found := k.GetChannel(ctx, portID, channelID) - if !found { - panic(fmt.Sprintf("could not find existing channel when updating channel state in successful ChanUpgradeAck step, channelID: %s, portID: %s", channelID, portID)) - } - - previousState := channel.State - channel.State = types.ACKUPGRADE - channel.FlushStatus = types.FLUSHING - - k.SetChannel(ctx, portID, channelID, channel) - k.SetUpgrade(ctx, portID, channelID, proposedUpgrade) - - k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", previousState, "new-state", types.ACKUPGRADE.String()) - emitChannelUpgradeAckEvent(ctx, portID, channelID, channel, proposedUpgrade) -} - // ChanUpgradeAck is called by a module to accept the ACKUPGRADE handshake step of the channel upgrade protocol. // This method should only be called by the IBC core msg server. // This method will verify that the counterparty has entered TRYUPGRADE @@ -293,6 +267,32 @@ func (k Keeper) ChanUpgradeAck( return nil } +// WriteUpgradeAckChannel writes a channel which has successfully passed the UpgradeAck handshake step as well as +// setting the upgrade for that channel. +// An event is emitted for the handshake step. +func (k Keeper) WriteUpgradeAckChannel( + ctx sdk.Context, + portID, channelID string, + proposedUpgrade types.Upgrade, +) { + defer telemetry.IncrCounter(1, "ibc", "channel", "upgrade-ack") + + channel, found := k.GetChannel(ctx, portID, channelID) + if !found { + panic(fmt.Sprintf("could not find existing channel when updating channel state in successful ChanUpgradeAck step, channelID: %s, portID: %s", channelID, portID)) + } + + previousState := channel.State + channel.State = types.ACKUPGRADE + channel.FlushStatus = types.FLUSHING + + k.SetChannel(ctx, portID, channelID, channel) + k.SetUpgrade(ctx, portID, channelID, proposedUpgrade) + + k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", previousState, "new-state", types.ACKUPGRADE.String()) + emitChannelUpgradeAckEvent(ctx, portID, channelID, channel, proposedUpgrade) +} + // 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 From ee8377657525ca0e9fd72e74bb02e341de4926f8 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 14 Jun 2023 13:15:22 +0200 Subject: [PATCH 3/3] make expPass construction two lines instead of one --- modules/core/04-channel/keeper/upgrade_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/core/04-channel/keeper/upgrade_test.go b/modules/core/04-channel/keeper/upgrade_test.go index bcda50f63f4..96a873c813e 100644 --- a/modules/core/04-channel/keeper/upgrade_test.go +++ b/modules/core/04-channel/keeper/upgrade_test.go @@ -476,7 +476,8 @@ func (suite *KeeperTestSuite) TestChanUpgradeAck() { proofChannel, proofUpgrade, proofHeight, ) - if expPass := tc.expError == nil; expPass { + expPass := tc.expError == nil + if expPass { suite.Require().NoError(err) } else { suite.assertUpgradeError(err, tc.expError)