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

feat: ica app version negotiation #410

Merged
merged 18 commits into from
Sep 20, 2021
Merged

feat: ica app version negotiation #410

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
18990b4
adding NegotiateAppVersion implementation and tests
damiannolan Sep 15, 2021
49e2901
updating GenerateAddress to return sdk.AccAddress, fixing tests
damiannolan Sep 15, 2021
1f73b16
updating ica handshake flow to parse address from version string, fix…
damiannolan Sep 15, 2021
b967f08
updating module_tests
damiannolan Sep 15, 2021
52fe314
derive ica addresses from module account addr
damiannolan Sep 15, 2021
bd66f33
removing unused keys
damiannolan Sep 15, 2021
62e5c39
adding improved version validation, updating tests
damiannolan Sep 16, 2021
4ee74bf
removing redundant local var - owner
damiannolan Sep 16, 2021
b2f3e3b
updating Delimiter godoc
damiannolan Sep 16, 2021
c7d7804
updating validation logic
damiannolan Sep 16, 2021
001313c
adding test cases for ValidateVersion
damiannolan Sep 16, 2021
d6725e1
adding additional validation testcase, updating godocs
damiannolan Sep 17, 2021
32240ff
updating Version -> VersionPrefix, error msgs, validation tests
damiannolan Sep 17, 2021
e4af997
updating NewAppVersion func sig and usage
damiannolan Sep 17, 2021
18e9601
updating NewAppVersion args and returning more appropriate errors for…
damiannolan Sep 20, 2021
c691632
Merge branch 'interchain-accounts' into damian/378-ica-set-addr-on-ctrl
damiannolan Sep 20, 2021
3d5baab
Update modules/apps/27-interchain-accounts/keeper/handshake.go
damiannolan Sep 20, 2021
f400bdc
updating ValidateVersion godoc
damiannolan Sep 20, 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
12 changes: 5 additions & 7 deletions modules/apps/27-interchain-accounts/keeper/account.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
View file
Open in desktop
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
37 changes: 28 additions & 9 deletions modules/apps/27-interchain-accounts/keeper/handshake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package keeper

import (
"strings"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
Expand Down Expand Up @@ -36,8 +38,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 +74,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 +89,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 strings.Compare(parsedAddr, accAddr.String()) != 0 {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
return sdkerrors.Wrapf(types.ErrInvalidAccountAddress, "invalid account address: %s, expected %s", parsedAddr, accAddr)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

// 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
20 changes: 11 additions & 9 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 @@ -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 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
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
12 changes: 10 additions & 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,6 +1,7 @@
package keeper_test

import (
"fmt"
"testing"

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

var (
// TestOwnerAddress defines a reusable bech32 address for testing purposes
TestOwnerAddress = "cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs"
// TestVersion defines a resuable interchainaccounts version string for testing purposes
TestVersion = fmt.Sprintf("%s|%s", types.Version, types.GenerateAddress("ics-27-0-0-"+TestOwnerAddress))
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
)

type KeeperTestSuite struct {
suite.Suite

Expand All @@ -36,7 +44,7 @@ func NewICAPath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path {
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.EndpointB.ChannelConfig.Version = TestVersion

return path
}
Expand Down Expand Up @@ -104,7 +112,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
View file
Open in desktop
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
View file
Open in desktop
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.Version {
return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "failed to negotiate app version: expected %s, got %s", types.Version, proposedVersion)
}

return fmt.Sprint(types.Version, types.Delimiter, types.GenerateAddress(counterparty.PortId)), nil
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}
77 changes: 77 additions & 0 deletions modules/apps/27-interchain-accounts/module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
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"
// TestVersion defines a resuable interchainaccounts version string for testing purposes
TestVersion = fmt.Sprintf("%s|%s", types.Version, types.GenerateAddress("ics-27-0-0-"+TestOwnerAddress))
)

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.Version
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.Version)
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
suite.Require().NoError(err)
suite.Require().NoError(types.ValidateVersion(version))
}
24 changes: 14 additions & 10 deletions modules/apps/27-interchain-accounts/types/account.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,12 @@ import (
"fmt"
"strings"

yaml "gopkg.in/yaml.v2"

crypto "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
"github.com/tendermint/tendermint/crypto/tmhash"
yaml "gopkg.in/yaml.v2"

connectiontypes "github.com/cosmos/ibc-go/modules/core/03-connection/types"
)
Expand All @@ -20,9 +19,14 @@ const (
ICAPrefix string = "ics-27"
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
)

// GenerateAddress returns a truncated SHA256 hash using the provided port string
func GenerateAddress(port string) []byte {
return tmhash.SumTruncated([]byte(port))
// GenerateAddress returns an sdk.AccAddress using the provided port identifier
func GenerateAddress(portID string) sdk.AccAddress {
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
return sdk.AccAddress(tmhash.SumTruncated([]byte(portID)))
}

// ParseAddressFromVersion trims the interchainaccounts version prefix and returns the associated account address
func ParseAddressFromVersion(version string) string {
return strings.TrimPrefix(version, fmt.Sprint(Version, Delimiter))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should change Version -> VersionPrefix? I mostly just find it confusing referring to both ics27-1 and ics27-1|accaddr as version

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I can change to VersionPrefix. But I agree, it's confusing. Wondering if perhaps a protobuf struct would've been a better option completely... idk something like below where a metadata field could be used for any arbitrary piece of data, in our case an account address string:

message VersionMetadata {
    string version = 1;
    string metadata = 2;
}

The real problem here is with breaking existing APIs though I believe!

Copy link
Contributor

Choose a reason for hiding this comment

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

We can do protobuf, we would just need to encode/decode when passing and interacting with core IBC. The version in the channel is intentionally a string to allow applications the flexibility of deciding what sort of version they want

They main downside of encoding is that the version might look odd when printing out a channel struct, but if we json encode maybe it'd be fine?

Copy link
Member Author

Choose a reason for hiding this comment

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

So with this approach, the version/counterpartyVersion args to the IBCModule callbacks would remain as strings but we would json encode the structure for ICA and handle encoding within the keeper?

Let's keep this as an option and we can discuss as part of the audit. If it's something we decide is valuable or a better approach I can implement it later.

}

// GeneratePortID generates the portID for a specific owner
Expand All @@ -32,21 +36,21 @@ func GenerateAddress(port string) []byte {
// https://github.com/seantking/ibc/tree/sean/ics-27-updates/spec/app/ics-027-interchain-accounts#registering--controlling-flows
// TODO: update link to spec
func GeneratePortID(owner, connectionID, counterpartyConnectionID string) (string, error) {
ownerID := strings.TrimSpace(owner)
if ownerID == "" {
return "", sdkerrors.Wrap(ErrInvalidOwnerAddress, "owner address cannot be empty")
if strings.TrimSpace(owner) == "" {
return "", sdkerrors.Wrap(ErrInvalidAccountAddress, "owner address cannot be empty")
}

connectionSeq, err := connectiontypes.ParseConnectionSequence(connectionID)
if err != nil {
return "", sdkerrors.Wrap(err, "invalid connection identifier")
}

counterpartyConnectionSeq, err := connectiontypes.ParseConnectionSequence(counterpartyConnectionID)
if err != nil {
return "", sdkerrors.Wrap(err, "invalid counterparty connection identifier")
}

portID := fmt.Sprintf("%s-%d-%d-%s", ICAPrefix, connectionSeq, counterpartyConnectionSeq, ownerID)
return portID, nil
return fmt.Sprintf("%s-%d-%d-%s", ICAPrefix, connectionSeq, counterpartyConnectionSeq, owner), nil
}

type InterchainAccountI interface {
Expand Down
Loading