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

feat: ica app version negotiation #410

Merged
merged 18 commits into from
Sep 20, 2021
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
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
14 changes: 6 additions & 8 deletions modules/apps/27-interchain-accounts/keeper/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ func (k Keeper) InitInterchainAccount(ctx sdk.Context, connectionID, counterpart
return sdkerrors.Wrap(err, "unable to bind to newly generated portID")
}

msg := channeltypes.NewMsgChannelOpenInit(portID, types.Version, channeltypes.ORDERED, []string{connectionID}, types.PortID, types.ModuleName)
msg := channeltypes.NewMsgChannelOpenInit(portID, types.VersionPrefix, channeltypes.ORDERED, []string{connectionID}, types.PortID, types.ModuleName)
handler := k.msgRouter.Handler(msg)
if _, err := handler(ctx, msg); err != nil {
return err
Expand All @@ -40,17 +40,15 @@ func (k Keeper) InitInterchainAccount(ctx sdk.Context, connectionID, counterpart
return nil
}

// Register interchain account if it has not already been created
func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, portID string) {
address := types.GenerateAddress(portID)

account := k.accountKeeper.GetAccount(ctx, address)
if account != nil {
// RegisterInterchainAccount attempts to create a new account using the provided address and stores it in state keyed by the provided port identifier
// If an account for the provided address already exists this function returns early (no-op)
func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, accAddr sdk.AccAddress, portID string) {
if acc := k.accountKeeper.GetAccount(ctx, accAddr); acc != nil {
return
}

interchainAccount := types.NewInterchainAccount(
authtypes.NewBaseAccountWithAddress(address),
authtypes.NewBaseAccountWithAddress(accAddr),
portID,
)

Expand Down
4 changes: 2 additions & 2 deletions modules/apps/27-interchain-accounts/keeper/account_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ func (suite *KeeperTestSuite) TestInitInterchainAccount() {
tc := tc

suite.Run(tc.name, func() {
suite.SetupTest() // reset
owner = "owner" // must be explicitly changed
suite.SetupTest() // reset
owner = TestOwnerAddress // must be explicitly changed
path = NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)

Expand Down
35 changes: 26 additions & 9 deletions modules/apps/27-interchain-accounts/keeper/handshake.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,9 @@ func (k Keeper) OnChanOpenInit(
if counterparty.PortId != types.PortID {
return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "counterparty port-id must be '%s', (%s != %s)", types.PortID, counterparty.PortId, types.PortID)
}
if version != types.Version {
return sdkerrors.Wrapf(channeltypes.ErrInvalidChannelVersion, "channel version must be '%s' (%s != %s)", types.Version, version, types.Version)

if err := types.ValidateVersion(version); err != nil {
return err
}

existingChannelID, found := k.GetActiveChannel(ctx, portID)
Expand Down Expand Up @@ -71,11 +72,13 @@ func (k Keeper) OnChanOpenTry(
if order != channeltypes.ORDERED {
return sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "invalid channel ordering: %s, expected %s", order.String(), channeltypes.ORDERED.String())
}
if version != types.Version {
return sdkerrors.Wrapf(types.ErrInvalidVersion, "got: %s, expected %s", version, types.Version)

if err := types.ValidateVersion(version); err != nil {
return err
}
if counterpartyVersion != types.Version {
return sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: %s, expected %s", counterpartyVersion, types.Version)

if err := types.ValidateVersion(counterpartyVersion); err != nil {
return err
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

// On the host chain the capability may only be claimed during the OnChanOpenTry
Expand All @@ -84,23 +87,37 @@ func (k Keeper) OnChanOpenTry(
return err
}

accAddr := types.GenerateAddress(counterparty.PortId)
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
parsedAddr := types.ParseAddressFromVersion(version)
if parsedAddr != accAddr.String() {
return sdkerrors.Wrapf(types.ErrInvalidAccountAddress, "version contains invalid account address: expected %s, got %s", parsedAddr, accAddr)
}

// Register interchain account if it does not already exist
k.RegisterInterchainAccount(ctx, counterparty.PortId)
k.RegisterInterchainAccount(ctx, accAddr, counterparty.PortId)

return nil
}

// OnChanOpenAck sets the active channel for the interchain account/owner pair
// and stores the associated interchain account address in state keyed by it's corresponding port identifier
//
// Controller Chain
func (k Keeper) OnChanOpenAck(
ctx sdk.Context,
portID,
channelID string,
counterpartyVersion string,
) error {
if counterpartyVersion != types.Version {
return sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: %s, expected %s", counterpartyVersion, types.Version)
if err := types.ValidateVersion(counterpartyVersion); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

@colin-axner I'm just wondering is there any way that a malicious relayer could pass in an address here to the ack step that isn't the address we validated on the Try step? I think we already discussed this but just refreshing my brain after the weekend.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, core IBC verifies a proof of the channel state set on the counterparty

If a relayer changed the version, the proof would fail

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks 🙏

return err
}

k.SetActiveChannel(ctx, portID, channelID)

accAddr := types.ParseAddressFromVersion(counterpartyVersion)
k.SetInterchainAccountAddress(ctx, portID, accAddr)

return nil
}

Expand Down
24 changes: 13 additions & 11 deletions modules/apps/27-interchain-accounts/keeper/handshake_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
suite.coordinator.SetupConnections(path)

// mock init interchain account
portID, err := types.GeneratePortID("owner", path.EndpointA.ConnectionID, path.EndpointB.ConnectionID)
portID, err := types.GeneratePortID(TestOwnerAddress, path.EndpointA.ConnectionID, path.EndpointB.ConnectionID)
suite.Require().NoError(err)
portCap := suite.chainA.GetSimApp().IBCKeeper.PortKeeper.BindPort(suite.chainA.GetContext(), portID)
suite.chainA.GetSimApp().ICAKeeper.ClaimCapability(suite.chainA.GetContext(), portCap, host.PortPath(portID))
Expand All @@ -75,7 +75,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
Ordering: channeltypes.ORDERED,
Counterparty: counterparty,
ConnectionHops: []string{path.EndpointA.ConnectionID},
Version: types.Version,
Version: types.VersionPrefix,
}

chanCap, err = suite.chainA.App.GetScopedIBCKeeper().NewCapability(suite.chainA.GetContext(), host.ChannelCapabilityPath(portID, path.EndpointA.ChannelID))
Expand Down Expand Up @@ -136,6 +136,11 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
suite.Require().NoError(err)
}, false,
},
{
"invalid account address", func() {
channel.Counterparty.PortId = "invalid-port-id"
}, false,
},
}

for _, tc := range testCases {
Expand All @@ -144,11 +149,10 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
suite.Run(tc.name, func() {
suite.SetupTest() // reset
path = NewICAPath(suite.chainA, suite.chainB)
owner := "owner"
counterpartyVersion = types.Version
counterpartyVersion = types.VersionPrefix
suite.coordinator.SetupConnections(path)

err := InitInterchainAccount(path.EndpointA, owner)
err := InitInterchainAccount(path.EndpointA, TestOwnerAddress)
suite.Require().NoError(err)

// default values
Expand All @@ -158,7 +162,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
Ordering: channeltypes.ORDERED,
Counterparty: counterparty,
ConnectionHops: []string{path.EndpointB.ConnectionID},
Version: types.Version,
Version: TestVersion,
}

chanCap, err = suite.chainB.App.GetScopedIBCKeeper().NewCapability(suite.chainB.GetContext(), host.ChannelCapabilityPath(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID))
Expand Down Expand Up @@ -211,11 +215,10 @@ func (suite *KeeperTestSuite) TestOnChanOpenAck() {
suite.Run(tc.name, func() {
suite.SetupTest() // reset
path = NewICAPath(suite.chainA, suite.chainB)
owner := "owner"
counterpartyVersion = types.Version
counterpartyVersion = TestVersion
suite.coordinator.SetupConnections(path)

err := InitInterchainAccount(path.EndpointA, owner)
err := InitInterchainAccount(path.EndpointA, TestOwnerAddress)
suite.Require().NoError(err)

err = path.EndpointB.ChanOpenTry()
Expand Down Expand Up @@ -264,10 +267,9 @@ func (suite *KeeperTestSuite) TestOnChanOpenConfirm() {
suite.Run(tc.name, func() {
suite.SetupTest() // reset
path = NewICAPath(suite.chainA, suite.chainB)
owner := "owner"
suite.coordinator.SetupConnections(path)

err := InitInterchainAccount(path.EndpointA, owner)
err := InitInterchainAccount(path.EndpointA, TestOwnerAddress)
suite.Require().NoError(err)

err = path.EndpointB.ChanOpenTry()
Expand Down
16 changes: 13 additions & 3 deletions modules/apps/27-interchain-accounts/keeper/keeper_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper_test

import (
"fmt"
"testing"

authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
Expand All @@ -11,6 +12,15 @@ import (
ibctesting "github.com/cosmos/ibc-go/testing"
)

var (
// TestOwnerAddress defines a reusable bech32 address for testing purposes
TestOwnerAddress = "cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs"
// TestPortID defines a resuable port identifier for testing purposes
TestPortID = fmt.Sprintf("ics-27-0-0-%s", TestOwnerAddress)
// TestVersion defines a resuable interchainaccounts version string for testing purposes
TestVersion = types.NewAppVersion(types.VersionPrefix, types.Delimiter, types.GenerateAddress(TestPortID).String())
)

type KeeperTestSuite struct {
suite.Suite

Expand All @@ -35,8 +45,8 @@ func NewICAPath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path {
path.EndpointB.ChannelConfig.PortID = types.PortID
path.EndpointA.ChannelConfig.Order = channeltypes.ORDERED
path.EndpointB.ChannelConfig.Order = channeltypes.ORDERED
path.EndpointA.ChannelConfig.Version = types.Version
path.EndpointB.ChannelConfig.Version = types.Version
path.EndpointA.ChannelConfig.Version = types.VersionPrefix
path.EndpointB.ChannelConfig.Version = TestVersion

return path
}
Expand Down Expand Up @@ -104,7 +114,7 @@ func (suite *KeeperTestSuite) TestGetInterchainAccountAddress() {
path := NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)

err := SetupICAPath(path, "testing")
err := SetupICAPath(path, TestOwnerAddress)
suite.Require().NoError(err)

counterpartyPortID := path.EndpointA.ChannelConfig.PortID
Expand Down
2 changes: 1 addition & 1 deletion modules/apps/27-interchain-accounts/keeper/relay_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func (suite *KeeperTestSuite) TestTrySendTx() {
suite.Run(tc.name, func() {
suite.SetupTest() // reset
path = NewICAPath(suite.chainA, suite.chainB)
owner := "owner"
owner := TestOwnerAddress
suite.coordinator.SetupConnections(path)

err := suite.SetupICAPath(path, owner)
Expand Down
16 changes: 14 additions & 2 deletions modules/apps/27-interchain-accounts/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,18 @@ func (am AppModule) OnTimeoutPacket(
return nil
}

func (am AppModule) NegotiateAppVersion(ctx sdk.Context, order channeltypes.Order, connectionID, portID string, counterparty channeltypes.Counterparty, proposedVersion string) (string, error) {
return "", nil
// NegotiateAppVersion implements the IBCModule interface
func (am AppModule) NegotiateAppVersion(
ctx sdk.Context,
order channeltypes.Order,
connectionID string,
portID string,
counterparty channeltypes.Counterparty,
proposedVersion string,
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
) (string, error) {
if proposedVersion != types.VersionPrefix {
return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "failed to negotiate app version: expected %s, got %s", types.VersionPrefix, proposedVersion)
}

return types.NewAppVersion(types.VersionPrefix, types.Delimiter, types.GenerateAddress(counterparty.PortId).String()), nil
}
80 changes: 80 additions & 0 deletions modules/apps/27-interchain-accounts/module_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
package interchain_accounts_test

import (
"fmt"
"testing"

"github.com/stretchr/testify/suite"

"github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types"
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"
ibctesting "github.com/cosmos/ibc-go/testing"
)

var (
// TestOwnerAddress defines a reusable bech32 address for testing purposes
TestOwnerAddress = "cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs"
// TestPortID defines a resuable port identifier for testing purposes
TestPortID = fmt.Sprintf("ics-27-0-0-%s", TestOwnerAddress)
// TestVersion defines a resuable interchainaccounts version string for testing purposes
TestVersion = types.NewAppVersion(types.VersionPrefix, types.Delimiter, types.GenerateAddress(TestPortID).String())
)

type InterchainAccountsTestSuite struct {
suite.Suite

coordinator *ibctesting.Coordinator

// testing chains used for convenience and readability
chainA *ibctesting.TestChain
chainB *ibctesting.TestChain
chainC *ibctesting.TestChain
}

func (suite *InterchainAccountsTestSuite) SetupTest() {
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 3)
suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(0))
suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1))
suite.chainC = suite.coordinator.GetChain(ibctesting.GetChainID(2))
}

func NewICAPath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path {
path := ibctesting.NewPath(chainA, chainB)
path.EndpointA.ChannelConfig.PortID = types.PortID
path.EndpointB.ChannelConfig.PortID = types.PortID
path.EndpointA.ChannelConfig.Order = channeltypes.ORDERED
path.EndpointB.ChannelConfig.Order = channeltypes.ORDERED
path.EndpointA.ChannelConfig.Version = types.VersionPrefix
path.EndpointB.ChannelConfig.Version = TestVersion

return path
}

func TestICATestSuite(t *testing.T) {
suite.Run(t, new(InterchainAccountsTestSuite))
}

func (suite *InterchainAccountsTestSuite) TestNegotiateAppVersion() {
suite.SetupTest()
path := NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)

module, _, err := suite.chainA.GetSimApp().GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), types.PortID)
suite.Require().NoError(err)

cbs, ok := suite.chainA.GetSimApp().GetIBCKeeper().Router.GetRoute(module)
suite.Require().True(ok)

counterpartyPortID, err := types.GeneratePortID(TestOwnerAddress, path.EndpointA.ConnectionID, path.EndpointB.ConnectionID)
suite.Require().NoError(err)

counterparty := &channeltypes.Counterparty{
PortId: counterpartyPortID,
ChannelId: path.EndpointB.ChannelID,
}

version, err := cbs.NegotiateAppVersion(suite.chainA.GetContext(), channeltypes.ORDERED, path.EndpointA.ConnectionID, types.PortID, *counterparty, types.VersionPrefix)
suite.Require().NoError(err)
suite.Require().NoError(types.ValidateVersion(version))
suite.Require().Equal(TestVersion, version)
}
Loading