Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

chore: add defensive check to ensure metadata does not change when reopening an active channel #847

Merged
merged 14 commits into from
Feb 8, 2022
Merged

chore: add defensive check to ensure metadata does not change when reopening an active channel #847

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a0386ec
add defensive check to ensure metadata does not change when reopening…
colin-axner Feb 3, 2022
e67068d
Merge branch 'main' into colin/795-reopen-active-check
colin-axner Feb 3, 2022
5521ee3
add previous metadata check to onchanopentry
colin-axner Feb 3, 2022
1d5bf68
readjust handshake checks to use helper function, remove tests becaus…
colin-axner Feb 3, 2022
21adb6c
add tests for IsPreviousMetadataEqual
colin-axner Feb 3, 2022
2f2f4dc
Merge branch 'colin/795-reopen-active-check' of github.com:cosmos/ibc…
colin-axner Feb 3, 2022
3fdc6de
fix bug
colin-axner Feb 3, 2022
3559f2d
use correct version in OpenInit handshake
colin-axner Feb 7, 2022
146c7fa
Apply Aditya's error improvement comments
colin-axner Feb 7, 2022
bebbfa7
Update modules/apps/27-interchain-accounts/types/metadata.go
colin-axner Feb 7, 2022
e7b48ca
Merge branch 'main' into colin/795-reopen-active-check
colin-axner Feb 7, 2022
0f422e2
test: add tests for previous metadata being different
colin-axner Feb 8, 2022
4c2c0f3
Merge branch 'colin/795-reopen-active-check' of github.com:cosmos/ibc…
colin-axner Feb 8, 2022
42557b3
Merge branch 'main' into colin/795-reopen-active-check
colin-axner Feb 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions modules/apps/27-interchain-accounts/controller/keeper/handshake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper

import (
"fmt"
"strings"

sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -49,9 +50,20 @@ func (k Keeper) OnChanOpenInit(
return err
}

activeChannelID, found := k.GetOpenActiveChannel(ctx, connectionHops[0], portID)
activeChannelID, found := k.GetActiveChannelID(ctx, connectionHops[0], portID)
if found {
return sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s", activeChannelID, portID)
channel, found := k.channelKeeper.GetChannel(ctx, portID, channelID)
if !found {
panic(fmt.Sprintf("active channel mapping set for %s but channel does not exist in channel store", activeChannelID))
}

if channel.State == channeltypes.OPEN {
return sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s", activeChannelID, portID)
}

if channel.Version != version {
return sdkerrors.Wrap(icatypes.ErrInvalidVersion, "previous active channel metadata does not match provided version")
}
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

return nil
Expand Down
45 changes: 45 additions & 0 deletions modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,24 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
},
true,
},
{
"success - previous active channel closed",
Copy link
Member

Choose a reason for hiding this comment

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

Is there an error testcase for when previous metadata is different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unreachable, but I might as well add one

func() {
suite.chainA.GetSimApp().ICAControllerKeeper.SetActiveChannelID(suite.chainA.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
channel := channeltypes.Channel{
State: channeltypes.CLOSED,
Ordering: channeltypes.ORDERED,
Counterparty: counterparty,
ConnectionHops: []string{path.EndpointA.ConnectionID},
Version: TestVersion,
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

path.EndpointA.SetChannel(channel)
},
true,
},
{
"invalid order - UNORDERED",
func() {
Expand Down Expand Up @@ -150,6 +168,33 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
},
false,
},
{
"metadata does not match previous active channel",
func() {
// construct mismatching metadata
metadata.Address = TestAccAddress.String()

versionBytes, err := icatypes.ModuleCdc.MarshalJSON(&metadata)
suite.Require().NoError(err)

channel.Version = string(versionBytes)

// set active channel to CLOSED
suite.chainA.GetSimApp().ICAControllerKeeper.SetActiveChannelID(suite.chainA.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
channel := channeltypes.Channel{
State: channeltypes.CLOSED,
Ordering: channeltypes.ORDERED,
Counterparty: counterparty,
ConnectionHops: []string{path.EndpointA.ConnectionID},
Version: TestVersion,
}

path.EndpointA.SetChannel(channel)
},
false,
},
}

for _, tc := range testCases {
Expand Down