Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: refactor chanUpgradeAck tests to use expected errors #3843

Merged
merged 8 commits into from
Jun 20, 2023
52 changes: 26 additions & 26 deletions modules/core/04-channel/keeper/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Member Author

Choose a reason for hiding this comment

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

moved this to below ChanUpgradeAck

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)
}

// 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) {
Expand Down Expand Up @@ -313,6 +287,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
Expand Down
58 changes: 30 additions & 28 deletions modules/core/04-channel/keeper/upgrade_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -358,29 +358,29 @@ func (suite *KeeperTestSuite) TestChanUpgradeAck() {
err := path.EndpointA.UpdateClient()
suite.Require().NoError(err)
},
true,
nil,
},
{
"channel not found",
func() {
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",
Expand All @@ -389,7 +389,7 @@ func (suite *KeeperTestSuite) TestChanUpgradeAck() {
channel.ConnectionHops = []string{"connection-100"}
path.EndpointA.SetChannel(channel)
},
false,
connectiontypes.ErrConnectionNotFound,
},
{
"invalid connection state",
Expand All @@ -398,46 +398,47 @@ func (suite *KeeperTestSuite) TestChanUpgradeAck() {
connectionEnd.State = connectiontypes.UNINITIALIZED
path.EndpointA.SetConnection(connectionEnd)
},
false,
connectiontypes.ErrInvalidConnectionState,
},
{
"upgrade not found",
func() {
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",
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to bottom as this is last in the call stack

"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",
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed this testcase from: "startFlushUpgradeHandshake fails due to mismatch in upgrade sequences" to this one which fails with ordering mismatch.

This was the one testcase which was actually failing on proof verification before we got to the checks afterwards.
We can't force mismatch on upgrade sequences in this test as the counterparty channel upgrade sequence is built using our channel upgrade sequence, so mutating one or the other causes proof verification to fail. Decided to test for mismatch in upgrade ordering instead

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),
},
}

Expand Down Expand Up @@ -475,10 +476,11 @@ func (suite *KeeperTestSuite) TestChanUpgradeAck() {
proofChannel, proofUpgrade, proofHeight,
)

if tc.expPass {
expPass := tc.expError == nil
if expPass {
suite.Require().NoError(err)
} else {
suite.Require().Error(err)
suite.assertUpgradeError(err, tc.expError)
}
})
}
Expand Down