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

Jump to bottom

chore: ctrl port connection id validation #454

Merged
merged 27 commits into from
Oct 7, 2021
Merged

chore: ctrl port connection id validation #454

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
755ce69
adding pipe char | to identifier regex
damiannolan Sep 29, 2021
a8128ce
updating GeneratePortID to use pipe delimiter in favour of dash, Pars…
damiannolan Sep 29, 2021
bce3407
adding CounterpartyHops method to expected channel keeper interface
damiannolan Sep 29, 2021
cd8ac79
updating tests to satisy delimiter updates
damiannolan Sep 29, 2021
45d8f8d
adding connection seq validation of ctrl port id and updating tests
damiannolan Sep 29, 2021
1ec19f1
cleanup
damiannolan Sep 29, 2021
5d01500
adding defensive check for ParseAddressFromVersion
damiannolan Sep 30, 2021
0c56d06
adding conn sequence parsing funcs to pkg types
damiannolan Oct 1, 2021
d3039bb
moving conn sequence validation to reusable func
damiannolan Oct 1, 2021
6761d2d
updating error msgs, adding tests for conn seq parsers
damiannolan Oct 1, 2021
e22f3ea
adding expected sequence to error msgs
damiannolan Oct 1, 2021
09f741a
updating ParseCtrlConnSequence to ParseControllerConnSequence
damiannolan Oct 1, 2021
6982141
fixing counterparty port error
damiannolan Oct 1, 2021
88bc3c8
Update modules/apps/27-interchain-accounts/keeper/handshake.go
damiannolan Oct 1, 2021
31536af
Update modules/apps/27-interchain-accounts/keeper/handshake.go
damiannolan Oct 1, 2021
d09703e
Update modules/apps/27-interchain-accounts/types/account.go
damiannolan Oct 1, 2021
ac87f9d
removing pipe from valid identifier regex
damiannolan Oct 1, 2021
b6925ab
adding error returns to parsing funcs, updating tests, error messages
damiannolan Oct 1, 2021
c21c153
separting imports in keys.go
damiannolan Oct 1, 2021
a8bab3a
Merge branch 'interchain-accounts' into damian/444-ctrl-port-connecti…
damiannolan Oct 1, 2021
6cedcf1
updating handshake tests
damiannolan Oct 1, 2021
59340ac
Update modules/apps/27-interchain-accounts/types/keys.go
damiannolan Oct 4, 2021
c84996f
renaming validation func, removing parenthesis in error msgs
damiannolan Oct 5, 2021
495c864
renaming func validateControllerPort -> validateControllerPortParams
damiannolan Oct 6, 2021
f68d7ec
Merge branch 'interchain-accounts' into damian/444-ctrl-port-connecti…
damiannolan Oct 6, 2021
b4d1191
Merge branch 'interchain-accounts' into damian/444-ctrl-port-connecti…
damiannolan Oct 7, 2021
9d4dfd7
Merge branch 'interchain-accounts' into damian/444-ctrl-port-connecti…
damiannolan Oct 7, 2021
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
86 changes: 81 additions & 5 deletions modules/apps/27-interchain-accounts/keeper/handshake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"

"github.com/cosmos/ibc-go/v2/modules/apps/27-interchain-accounts/types"
connectiontypes "github.com/cosmos/ibc-go/v2/modules/core/03-connection/types"
channeltypes "github.com/cosmos/ibc-go/v2/modules/core/04-channel/types"
porttypes "github.com/cosmos/ibc-go/v2/modules/core/05-port/types"
host "github.com/cosmos/ibc-go/v2/modules/core/24-host"
Expand All @@ -31,10 +32,25 @@ func (k Keeper) OnChanOpenInit(
version string,
) error {
if order != channeltypes.ORDERED {
return sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "invalid channel ordering: %s, expected %s", order.String(), channeltypes.ORDERED.String())
return sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s", channeltypes.ORDERED, order)
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
}

connSequence, err := types.ParseControllerConnSequence(portID)
if err != nil {
return sdkerrors.Wrapf(err, "expected format <app-version%[1]sconn-seq%[1]scounterparty-conn-seq%[1]sowner>, got %s", types.Delimiter, portID)
}

counterpartyConnSequence, err := types.ParseHostConnSequence(portID)
if err != nil {
return sdkerrors.Wrapf(err, "expected format <app-version%[1]sconn-seq%[1]scounterparty-conn-seq%[1]sowner>, got %s", types.Delimiter, portID)
}

if err := k.validateConnectionParams(ctx, channelID, portID, connSequence, counterpartyConnSequence); err != nil {
return sdkerrors.Wrapf(err, "failed to validate controller port (%s)", portID)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe the error message should say connection params? Otherwise maybe we should rename the function to validateControllerPort?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing! Any preference on this before I update?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think validateControllerPort makes the most sense to me. It will likely align closer with the ICS spec

Copy link
Member Author

@damiannolan damiannolan Oct 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Cool, I've updated to validateControllerPort. My only gripe with this, and perhaps its overly pedantic, is that we never actually pass the controller port to this function. We pass the self port ID to retrieve the channel which is obviously going to be different in the OnChainOpenInit and OnChanOpenTry callbacks.

We could... 🤔

  1. Call it validateControllerPortParams and leave the args the same.
  2. Move the conn sequence parsing into the func and pass the controller port as an arg

The only problem I see with opt 2 is that having two port ID args may be confusing(?), while opt 1 is a bit long winded...

Either way I'm happy to leave it as is now if you are 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

I vote for validateControllerPortParams and leave the args the same

}

if counterparty.PortId != types.PortID {
return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "counterparty port-id must be '%s', (%s != %s)", types.PortID, counterparty.PortId, types.PortID)
return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "expected %s, got %s", types.PortID, counterparty.PortId)
}

if err := types.ValidateVersion(version); err != nil {
Expand Down Expand Up @@ -70,7 +86,25 @@ func (k Keeper) OnChanOpenTry(
counterpartyVersion string,
) error {
if order != channeltypes.ORDERED {
return sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "invalid channel ordering: %s, expected %s", order.String(), channeltypes.ORDERED.String())
return sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "expected %s channel, got %s", channeltypes.ORDERED, order)
}

if portID != types.PortID {
return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "expected %s, got %s", types.PortID, portID)
}

connSequence, err := types.ParseHostConnSequence(counterparty.PortId)
if err != nil {
return sdkerrors.Wrapf(err, "expected format <app-version%[1]sconn-seq%[1]scounterparty-conn-seq%[1]sowner>, got %s", types.Delimiter, portID)
Copy link
Member Author

Choose a reason for hiding this comment

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

Format string here may look a little odd using %[1]s, see explicit argument indexing here. Avoids having to specify types.Delimiter multiple times in args.

Suggestions for a better error message are welcome, but I think including the expected format as @crodriguezvega mentioned would be valuable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd define the expected string as a const and then link the spec in the comment for the expected format. I'd also recommend using controller-conn-seq and host-conn-seq since counterparty means different things depending on context

}

counterpartyConnSequence, err := types.ParseControllerConnSequence(counterparty.PortId)
if err != nil {
return sdkerrors.Wrapf(err, "expected format <app-version%[1]sconn-seq%[1]scounterparty-conn-seq%[1]sowner>, got %s", types.Delimiter, portID)
}

if err := k.validateConnectionParams(ctx, channelID, portID, connSequence, counterpartyConnSequence); err != nil {
return sdkerrors.Wrapf(err, "failed to validate controller port (%s)", counterparty.PortId)
}

if err := types.ValidateVersion(version); err != nil {
Expand All @@ -89,7 +123,11 @@ func (k Keeper) OnChanOpenTry(

// Check to ensure that the version string contains the expected address generated from the Counterparty portID
accAddr := types.GenerateAddress(k.accountKeeper.GetModuleAddress(types.ModuleName), counterparty.PortId)
parsedAddr := types.ParseAddressFromVersion(version)
parsedAddr, err := types.ParseAddressFromVersion(version)
if err != nil {
return sdkerrors.Wrapf(err, "expected format <app-version%saccount-address>, got %s", types.Delimiter, version)
}

if parsedAddr != accAddr.String() {
return sdkerrors.Wrapf(types.ErrInvalidAccountAddress, "version contains invalid account address: expected %s, got %s", parsedAddr, accAddr)
}
Expand All @@ -116,7 +154,11 @@ func (k Keeper) OnChanOpenAck(

k.SetActiveChannel(ctx, portID, channelID)

accAddr := types.ParseAddressFromVersion(counterpartyVersion)
accAddr, err := types.ParseAddressFromVersion(counterpartyVersion)
if err != nil {
return sdkerrors.Wrapf(err, "expected format <app-version%saccount-address>, got %s", types.Delimiter, counterpartyVersion)
}

k.SetInterchainAccountAddress(ctx, portID, accAddr)

return nil
Expand All @@ -130,3 +172,37 @@ func (k Keeper) OnChanOpenConfirm(
) error {
return nil
}

// validateConnectionParams asserts the provided connection sequence and counterparty connection sequence
// match that of the associated connection stored in state
func (k Keeper) validateConnectionParams(ctx sdk.Context, channelID, portID string, connectionSeq, counterpartyConnectionSeq uint64) error {
channel, found := k.channelKeeper.GetChannel(ctx, portID, channelID)
if !found {
return sdkerrors.Wrapf(channeltypes.ErrChannelNotFound, "port ID (%s) channel ID (%s)", portID, channelID)
}

counterpartyHops, found := k.channelKeeper.CounterpartyHops(ctx, channel)
if !found {
return sdkerrors.Wrap(connectiontypes.ErrConnectionNotFound, channel.ConnectionHops[0])
}

connSeq, err := connectiontypes.ParseConnectionSequence(channel.ConnectionHops[0])
if err != nil {
return sdkerrors.Wrapf(err, "failed to parse connection sequence (%s)", channel.ConnectionHops[0])
}

counterpartyConnSeq, err := connectiontypes.ParseConnectionSequence(counterpartyHops[0])
if err != nil {
return sdkerrors.Wrapf(err, "failed to parse counterparty connection sequence (%s)", counterpartyHops[0])
}

if connSeq != connectionSeq {
return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "sequence mismatch, expected (%d), got (%d)", connSeq, connectionSeq)
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
}

if counterpartyConnSeq != counterpartyConnectionSeq {
return sdkerrors.Wrapf(connectiontypes.ErrInvalidConnection, "counterparty sequence mismatch, expected (%d), got (%d)", counterpartyConnSeq, counterpartyConnectionSeq)
}

return nil
}
182 changes: 157 additions & 25 deletions modules/apps/27-interchain-accounts/keeper/handshake_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,33 +23,92 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
}{

{
"success", func() {}, true,
"success",
func() {
path.EndpointA.SetChannel(*channel)
},
true,
},
{
"invalid order - UNORDERED", func() {
"invalid order - UNORDERED",
func() {
channel.Ordering = channeltypes.UNORDERED
}, false,
},
false,
},
{
"invalid counterparty port ID", func() {
channel.Counterparty.PortId = ibctesting.MockPort
}, false,
"invalid port ID",
func() {
path.EndpointA.ChannelConfig.PortID = "invalid-port-id"
},
false,
},
{
"invalid version", func() {
"invalid counterparty port ID",
func() {
channel.Counterparty.PortId = "invalid-port-id"
},
false,
},
{
"invalid version",
func() {
channel.Version = "version"
}, false,
},
false,
},
{
"channel is already active", func() {
"channel not found",
func() {
path.EndpointA.ChannelID = "invalid-channel-id"
},
false,
},
{
"connection not found",
func() {
channel.ConnectionHops = []string{"invalid-connnection-id"}
path.EndpointA.SetChannel(*channel)
},
false,
},
{
"invalid connection sequence",
func() {
portID, err := types.GeneratePortID(TestOwnerAddress, "connection-1", "connection-0")
suite.Require().NoError(err)

path.EndpointA.ChannelConfig.PortID = portID
path.EndpointA.SetChannel(*channel)
},
false,
},
{
"invalid counterparty connection sequence",
func() {
portID, err := types.GeneratePortID(TestOwnerAddress, "connection-0", "connection-1")
suite.Require().NoError(err)

path.EndpointA.ChannelConfig.PortID = portID
path.EndpointA.SetChannel(*channel)
},
false,
},
{
"channel is already active",
func() {
suite.chainA.GetSimApp().ICAKeeper.SetActiveChannel(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
}, false,
},
false,
},
{
"capability already claimed", func() {
"capability already claimed",
func() {
path.EndpointA.SetChannel(*channel)
err := suite.chainA.GetSimApp().ScopedICAKeeper.ClaimCapability(suite.chainA.GetContext(), chanCap, host.ChannelCapabilityPath(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID))
suite.Require().NoError(err)
}, false,
},
false,
},
}

Expand Down Expand Up @@ -97,7 +156,6 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
}
}

// ChainA is controller, ChainB is host chain
func (suite *KeeperTestSuite) TestOnChanOpenTry() {
var (
channel *channeltypes.Channel
Expand All @@ -113,33 +171,103 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
}{

{
"success", func() {}, true,
"success",
func() {
path.EndpointB.SetChannel(*channel)
},
true,
},
{
"invalid order - UNORDERED", func() {
"invalid order - UNORDERED",
func() {
channel.Ordering = channeltypes.UNORDERED
}, false,
},
false,
},
{
"invalid port",
func() {
path.EndpointB.ChannelConfig.PortID = "invalid-port-id"
},
false,
},
{
"invalid counterparty port",
func() {
channel.Counterparty.PortId = "invalid-port-id"
},
false,
},
{
"invalid version", func() {
"channel not found",
func() {
path.EndpointB.ChannelID = "invalid-channel-id"
},
false,
},
{
"connection not found",
func() {
channel.ConnectionHops = []string{"invalid-connnection-id"}
path.EndpointB.SetChannel(*channel)
},
false,
},
{
"invalid connection sequence",
func() {
portID, err := types.GeneratePortID(TestOwnerAddress, "connection-0", "connection-1")
suite.Require().NoError(err)

channel.Counterparty.PortId = portID
path.EndpointB.SetChannel(*channel)
},
false,
},
{
"invalid counterparty connection sequence",
func() {
portID, err := types.GeneratePortID(TestOwnerAddress, "connection-1", "connection-0")
suite.Require().NoError(err)

channel.Counterparty.PortId = portID
path.EndpointB.SetChannel(*channel)
},
false,
},
{
"invalid version",
func() {
channel.Version = "version"
}, false,
},
false,
},
{
"invalid counterparty version", func() {
"invalid counterparty version",
func() {
counterpartyVersion = "version"
}, false,
},
false,
},
{
"capability already claimed", func() {
"capability already claimed",
func() {
path.EndpointB.SetChannel(*channel)
err := suite.chainB.GetSimApp().ScopedICAKeeper.ClaimCapability(suite.chainB.GetContext(), chanCap, host.ChannelCapabilityPath(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID))
suite.Require().NoError(err)
}, false,
},
false,
},
{
"invalid account address", func() {
channel.Counterparty.PortId = "invalid-port-id"
}, false,
"invalid account address",
func() {
portID, err := types.GeneratePortID("invalid-owner-addr", "connection-0", "connection-0")
suite.Require().NoError(err)

channel.Counterparty.PortId = portID
path.EndpointB.SetChannel(*channel)
},
false,
},
}

Expand All @@ -155,6 +283,10 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
err := InitInterchainAccount(path.EndpointA, TestOwnerAddress)
suite.Require().NoError(err)

// set the channel id on host
channelSequence := path.EndpointB.Chain.App.GetIBCKeeper().ChannelKeeper.GetNextChannelSequence(path.EndpointB.Chain.GetContext())
path.EndpointB.ChannelID = channeltypes.FormatChannelIdentifier(channelSequence)

// default values
counterparty := channeltypes.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
channel = &channeltypes.Channel{
Expand Down
3 changes: 1 addition & 2 deletions modules/apps/27-interchain-accounts/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package keeper_test

import (
"fmt"
"testing"

sdk "github.com/cosmos/cosmos-sdk/types"
Expand All @@ -21,7 +20,7 @@ var (
// TestOwnerAddress defines a reusable bech32 address for testing purposes
TestOwnerAddress = "cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs"
// TestPortID defines a resuable port identifier for testing purposes
TestPortID = fmt.Sprintf("%s-0-0-%s", types.VersionPrefix, TestOwnerAddress)
TestPortID, _ = types.GeneratePortID(TestOwnerAddress, "connection-0", "connection-0")
// TestVersion defines a resuable interchainaccounts version string for testing purposes
TestVersion = types.NewAppVersion(types.VersionPrefix, TestAccAddress.String())
)
Expand Down
Loading