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 5 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
19 changes: 12 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,23 +40,28 @@ 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)
// 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 nil
func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, address, portID string) error {
accAddr, err := sdk.AccAddressFromBech32(address)
if err != nil {
return err
}

account := k.accountKeeper.GetAccount(ctx, address)
if account != nil {
return
if acc := k.accountKeeper.GetAccount(ctx, accAddr); acc != nil {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably return a more descriptive error here. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, wondering if this check to get an account needs to change at all given we are using the new module acc feature?

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 the point is to do a no-op if the account already exists since it could create an attack vector if an error is returned. Or at least that's what comes to mind. It'd be great to have an inline comment explaining the reasoning

Copy link
Member Author

Choose a reason for hiding this comment

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

Is the idea here that there may be an account that already exists and this would return nil (no-op) in the event that a new channel was being established between chains that had already registered an interchain account?

}

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

k.accountKeeper.NewAccount(ctx, interchainAccount)
k.accountKeeper.SetAccount(ctx, interchainAccount)
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
k.SetInterchainAccountAddress(ctx, portID, interchainAccount.Address)

return nil
}

func (k Keeper) GetInterchainAccount(ctx sdk.Context, addr sdk.AccAddress) (types.InterchainAccount, error) {
Expand Down
31 changes: 24 additions & 7 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,9 @@
package keeper

import (
"fmt"
"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 +39,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 !strings.Contains(version, types.Version) {
return sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid version: %s, expected %s", version, types.Version)
}

existingChannelID, found := k.GetActiveChannel(ctx, portID)
Expand Down Expand Up @@ -71,10 +75,12 @@ 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 !strings.Contains(version, types.Version) {
return sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid version: %s, expected %s", version, types.Version)
}
if counterpartyVersion != types.Version {

if !strings.Contains(counterpartyVersion, types.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.

With the inclusion of the account address in the version string we now have to deviate from the spec as we cannot explicitly check for equality.

I've updated these checks to use strings.Contains(), but perhaps there's some room for discussion on this topic

Copy link
Contributor

Choose a reason for hiding this comment

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

Contains may be okay. I'm wondering though should we be more strict about this and have some kind of validate version string helper function to ensure the format we expect is being passed in?

@colin-axner would like to hear your thoughts

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 think this would be a nicer option. Just basic assertion of ${types.Version}-${validateBech32addr}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I'd definitely like stronger validation. The two areas we do validation now is in 24-host and identifier parsing

I think the address validation should be light weight. I'm not sure it needs to be a valid bech 32 address since this can be address on a different chain. Maybe just a string which doesn't contain a -? It'd be nice if the interchain accounts mentioned this validation

I'm also okay doing some arbitrary address length validation and increasing the maximum allowed length if someone runs into an issue

Adding a validation function also increases the readability of the code

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated! See here

return sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: %s, expected %s", counterpartyVersion, types.Version)
}

Expand All @@ -85,22 +91,33 @@ func (k Keeper) OnChanOpenTry(
}

// Register interchain account if it does not already exist
k.RegisterInterchainAccount(ctx, counterparty.PortId)
accAddr := strings.TrimPrefix(version, fmt.Sprintf("%s-", types.Version))
if err := k.RegisterInterchainAccount(ctx, accAddr, counterparty.PortId); err != nil {
return err
}

colin-axner marked this conversation as resolved.
Show resolved Hide resolved
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 {
if !strings.Contains(counterpartyVersion, types.Version) {
return sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: %s, expected %s", counterpartyVersion, types.Version)
}

k.SetActiveChannel(ctx, portID, channelID)

accAddr := strings.TrimPrefix(counterpartyVersion, fmt.Sprintf("%s-", types.Version))
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
k.SetInterchainAccountAddress(ctx, portID, accAddr)

return nil
}

Expand Down
4 changes: 3 additions & 1 deletion modules/apps/27-interchain-accounts/keeper/handshake_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package keeper_test

import (
"fmt"

capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"

"github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types"
Expand Down Expand Up @@ -158,7 +160,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
Ordering: channeltypes.ORDERED,
Counterparty: counterparty,
ConnectionHops: []string{path.EndpointB.ConnectionID},
Version: types.Version,
Version: fmt.Sprintf("%s-%s", types.Version, types.GenerateAddress("ics-27-0-0-testing")),
}

chanCap, err = suite.chainB.App.GetScopedIBCKeeper().NewCapability(suite.chainB.GetContext(), host.ChannelCapabilityPath(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID))
Expand Down
3 changes: 2 additions & 1 deletion 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 Down Expand Up @@ -36,7 +37,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 = fmt.Sprintf("%s-%s", types.Version, types.GenerateAddress("ics-27-0-0-testing"))
damiannolan marked this conversation as resolved.
Show resolved Hide resolved

return path
}
Expand Down
18 changes: 16 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,20 @@ 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)
}

addr := types.GenerateAddress(counterparty.PortId)

return fmt.Sprintf("%s-%s", types.Version, addr), nil
}
72 changes: 72 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,72 @@
package interchain_accounts_test

import (
"fmt"
"strings"
"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"
)

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 = fmt.Sprintf("%s-%s", types.Version, types.GenerateAddress("ics-27-0-0-testing"))

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("testing", 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().True(strings.Contains(version, types.Version))
suite.Require().True(strings.Contains(version, types.GenerateAddress(counterpartyPortID).String()))
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
}
9 changes: 5 additions & 4 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 @@ -9,9 +9,9 @@ import (

crypto "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/address"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
"github.com/tendermint/tendermint/crypto/tmhash"

connectiontypes "github.com/cosmos/ibc-go/modules/core/03-connection/types"
)
Expand All @@ -20,9 +20,10 @@ 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 and derived from the IBC interchainaccounts module account
Copy link
Member Author

Choose a reason for hiding this comment

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

@colin-axner I've included this as per #303 after checking out the comment you had linked! I'm assuming this is a new approach which has yet to be adopted across the sdk. This seems to me like the correct usage but I'm not 100% as other modules use a different pattern.

For example transfer is composed with an authtypes.AccountKeeper and accesses the module accounts via that interface which from what I can tell are populated through app.go maccPerms

Copy link
Member Author

Choose a reason for hiding this comment

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

Also updated this return sig from []byte to sdk.AccAddress as it's probably preferred to use that type for bech32 encoding

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Deriving the address looks correct, but we probably need to create the module account in the ICA keeper (in NewKeeper function) and create the sub module account when the address is being generated. I may be mistaken, but typically you have to create a new account in the account keeper since it needs to track account information like sequence

It might be worth doing this in a separate pr. Trying to use this account should test whether everything was setup correctly

Copy link
Member Author

@damiannolan damiannolan Sep 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sure I can do this in a separate PR 👍

In the NewKeeper func for transfer I see assertion that the module account exists:

// ensure ibc transfer module account is set
if addr := authKeeper.GetModuleAddress(types.ModuleName); addr == nil {
	panic("the IBC transfer module account has not been set")
}

From what I can tell the module accounts are being created here

func GenerateAddress(portID string) sdk.AccAddress {
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
moduleAcc := address.Module(ModuleName, ModuleAccountKey)
return sdk.AccAddress(address.Derive(moduleAcc, []byte(portID)))
}

// GeneratePortID generates the portID for a specific owner
Expand Down
61 changes: 44 additions & 17 deletions modules/apps/27-interchain-accounts/types/account_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package types_test

import (
"fmt"
"testing"

"github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types"
Expand Down Expand Up @@ -32,7 +33,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 = fmt.Sprintf("%s-%s", types.Version, types.GenerateAddress("ics-27-0-0-testing"))

return path
}
Expand All @@ -44,27 +45,53 @@ func TestTypesTestSuite(t *testing.T) {
func (suite *TypesTestSuite) TestGeneratePortID() {
var (
path *ibctesting.Path
owner string
owner = "testing"
)
var testCases = []struct {

testCases := []struct {
name string
malleate func()
expValue string
expPass bool
}{
{"success", func() {}, "ics-27-0-0-owner123", true},
{"success with non matching connection sequences", func() {
path.EndpointA.ConnectionID = "connection-1"
}, "ics-27-1-0-owner123", true},
{"invalid owner address", func() {
owner = " "
}, "", false},
{"invalid connectionID", func() {
path.EndpointA.ConnectionID = "connection"
}, "", false},
{"invalid counterparty connectionID", func() {
path.EndpointB.ConnectionID = "connection"
}, "", false},
{
"success",
func() {},
"ics-27-0-0-testing",
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
true,
},
{
"success with non matching connection sequences",
func() {
path.EndpointA.ConnectionID = "connection-1"
},
"ics-27-1-0-testing",
true,
},
{
"invalid owner address",
func() {
owner = " "
},
"",
false,
},
{
"invalid connectionID",
func() {
path.EndpointA.ConnectionID = "connection"
},
"",
false,
},
{
"invalid counterparty connectionID",
func() {
path.EndpointB.ConnectionID = "connection"
},
"",
false,
},
}

for _, tc := range testCases {
Expand All @@ -73,11 +100,11 @@ func (suite *TypesTestSuite) TestGeneratePortID() {
suite.SetupTest() // reset
path = NewICAPath(suite.chainA, suite.chainB)
suite.coordinator.Setup(path)
owner = "owner123" // must be explicitly changed

tc.malleate()

portID, err := types.GeneratePortID(owner, path.EndpointA.ConnectionID, path.EndpointB.ConnectionID)

if tc.expPass {
suite.Require().NoError(err, tc.name)
suite.Require().Equal(tc.expValue, portID)
Expand Down
Loading