From 18990b4e4f7c8423e2e65184824ae96f9f3656fa Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 15 Sep 2021 13:45:57 +0200 Subject: [PATCH 01/17] adding NegotiateAppVersion implementation and tests --- modules/apps/27-interchain-accounts/module.go | 18 ++++- .../27-interchain-accounts/module_test.go | 70 +++++++++++++++++++ 2 files changed, 86 insertions(+), 2 deletions(-) create mode 100644 modules/apps/27-interchain-accounts/module_test.go diff --git a/modules/apps/27-interchain-accounts/module.go b/modules/apps/27-interchain-accounts/module.go index 740eea4ad6b..f6f075a7f03 100644 --- a/modules/apps/27-interchain-accounts/module.go +++ b/modules/apps/27-interchain-accounts/module.go @@ -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, +) (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 } diff --git a/modules/apps/27-interchain-accounts/module_test.go b/modules/apps/27-interchain-accounts/module_test.go new file mode 100644 index 00000000000..db73022ba18 --- /dev/null +++ b/modules/apps/27-interchain-accounts/module_test.go @@ -0,0 +1,70 @@ +package interchain_accounts_test + +import ( + "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 = types.Version + + 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.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), types.PortID) + suite.Require().NoError(err) + + cbs, ok := suite.chainA.App.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) + suite.Require().NoError(err) + suite.Require().True(strings.Contains(version, types.Version)) +} From 49e290165313085fa5ad895cbc8f6d24c8292d16 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 15 Sep 2021 13:46:41 +0200 Subject: [PATCH 02/17] updating GenerateAddress to return sdk.AccAddress, fixing tests --- .../27-interchain-accounts/types/account.go | 6 +- .../types/account_test.go | 61 +++++++++++++------ 2 files changed, 47 insertions(+), 20 deletions(-) diff --git a/modules/apps/27-interchain-accounts/types/account.go b/modules/apps/27-interchain-accounts/types/account.go index 93f8b18e9a4..dc41ffd64fe 100644 --- a/modules/apps/27-interchain-accounts/types/account.go +++ b/modules/apps/27-interchain-accounts/types/account.go @@ -20,9 +20,9 @@ const ( ICAPrefix string = "ics-27" ) -// 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 a truncated SHA256 hash of the provided port identifier +func GenerateAddress(portID string) sdk.AccAddress { + return sdk.AccAddress(tmhash.SumTruncated([]byte(portID))) } // GeneratePortID generates the portID for a specific owner diff --git a/modules/apps/27-interchain-accounts/types/account_test.go b/modules/apps/27-interchain-accounts/types/account_test.go index 15bc23eb3e8..e72241fc1e0 100644 --- a/modules/apps/27-interchain-accounts/types/account_test.go +++ b/modules/apps/27-interchain-accounts/types/account_test.go @@ -1,6 +1,7 @@ package types_test import ( + "fmt" "testing" "github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types" @@ -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 } @@ -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", + 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 { @@ -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) From 1f73b16f75f46c6aa0aa8073e36c7c8b44d98324 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 15 Sep 2021 13:47:33 +0200 Subject: [PATCH 03/17] updating ica handshake flow to parse address from version string, fixing associated tests --- .../27-interchain-accounts/keeper/account.go | 19 +++++++----- .../keeper/handshake.go | 31 ++++++++++++++----- .../keeper/handshake_test.go | 4 ++- .../keeper/keeper_test.go | 3 +- 4 files changed, 41 insertions(+), 16 deletions(-) diff --git a/modules/apps/27-interchain-accounts/keeper/account.go b/modules/apps/27-interchain-accounts/keeper/account.go index 43e3bc4b143..176e6545c94 100644 --- a/modules/apps/27-interchain-accounts/keeper/account.go +++ b/modules/apps/27-interchain-accounts/keeper/account.go @@ -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 } interchainAccount := types.NewInterchainAccount( - authtypes.NewBaseAccountWithAddress(address), + authtypes.NewBaseAccountWithAddress(accAddr), portID, ) k.accountKeeper.NewAccount(ctx, interchainAccount) k.accountKeeper.SetAccount(ctx, interchainAccount) k.SetInterchainAccountAddress(ctx, portID, interchainAccount.Address) + + return nil } func (k Keeper) GetInterchainAccount(ctx sdk.Context, addr sdk.AccAddress) (types.InterchainAccount, error) { diff --git a/modules/apps/27-interchain-accounts/keeper/handshake.go b/modules/apps/27-interchain-accounts/keeper/handshake.go index e4677b72188..e209e7eb047 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake.go @@ -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" @@ -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) @@ -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) { return sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid counterparty version: %s, expected %s", counterpartyVersion, types.Version) } @@ -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 + } + 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)) + k.SetInterchainAccountAddress(ctx, portID, accAddr) + return nil } diff --git a/modules/apps/27-interchain-accounts/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/keeper/handshake_test.go index 35f09372f76..4ab8a2ee27d 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake_test.go @@ -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" @@ -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)) diff --git a/modules/apps/27-interchain-accounts/keeper/keeper_test.go b/modules/apps/27-interchain-accounts/keeper/keeper_test.go index 7064d6ebc07..a2440c75e18 100644 --- a/modules/apps/27-interchain-accounts/keeper/keeper_test.go +++ b/modules/apps/27-interchain-accounts/keeper/keeper_test.go @@ -1,6 +1,7 @@ package keeper_test import ( + "fmt" "testing" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" @@ -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")) return path } From b967f085e2b5e9d25b1de4ded90d7abd307e7910 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 15 Sep 2021 14:00:41 +0200 Subject: [PATCH 04/17] updating module_tests --- modules/apps/27-interchain-accounts/module_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/modules/apps/27-interchain-accounts/module_test.go b/modules/apps/27-interchain-accounts/module_test.go index db73022ba18..58bf50579fd 100644 --- a/modules/apps/27-interchain-accounts/module_test.go +++ b/modules/apps/27-interchain-accounts/module_test.go @@ -1,6 +1,7 @@ package interchain_accounts_test import ( + "fmt" "strings" "testing" @@ -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")) return path } @@ -50,10 +51,10 @@ func (suite *InterchainAccountsTestSuite) TestNegotiateAppVersion() { path := NewICAPath(suite.chainA, suite.chainB) suite.coordinator.SetupConnections(path) - module, _, err := suite.chainA.App.GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), types.PortID) + module, _, err := suite.chainA.GetSimApp().GetIBCKeeper().PortKeeper.LookupModuleByPort(suite.chainA.GetContext(), types.PortID) suite.Require().NoError(err) - cbs, ok := suite.chainA.App.GetIBCKeeper().Router.GetRoute(module) + cbs, ok := suite.chainA.GetSimApp().GetIBCKeeper().Router.GetRoute(module) suite.Require().True(ok) counterpartyPortID, err := types.GeneratePortID("testing", path.EndpointA.ConnectionID, path.EndpointB.ConnectionID) @@ -67,4 +68,5 @@ func (suite *InterchainAccountsTestSuite) TestNegotiateAppVersion() { version, err := cbs.NegotiateAppVersion(suite.chainA.GetContext(), channeltypes.ORDERED, path.EndpointA.ConnectionID, types.PortID, *counterparty, types.Version) suite.Require().NoError(err) suite.Require().True(strings.Contains(version, types.Version)) + suite.Require().True(strings.Contains(version, types.GenerateAddress(counterpartyPortID).String())) } From 52fe314a58002b4bf9e20547c9357598b354a8be Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 15 Sep 2021 14:11:12 +0200 Subject: [PATCH 05/17] derive ica addresses from module account addr --- .../27-interchain-accounts/types/account.go | 7 +++--- .../apps/27-interchain-accounts/types/keys.go | 24 ++++++++++++------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/modules/apps/27-interchain-accounts/types/account.go b/modules/apps/27-interchain-accounts/types/account.go index dc41ffd64fe..d7847b2d3f8 100644 --- a/modules/apps/27-interchain-accounts/types/account.go +++ b/modules/apps/27-interchain-accounts/types/account.go @@ -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" ) @@ -20,9 +20,10 @@ const ( ICAPrefix string = "ics-27" ) -// GenerateAddress returns an sdk.AccAddress using a truncated SHA256 hash of the provided port identifier +// GenerateAddress returns an sdk.AccAddress using the provided port identifier and derived from the IBC interchainaccounts module account func GenerateAddress(portID string) sdk.AccAddress { - return sdk.AccAddress(tmhash.SumTruncated([]byte(portID))) + moduleAcc := address.Module(ModuleName, ModuleAccountKey) + return sdk.AccAddress(address.Derive(moduleAcc, []byte(portID))) } // GeneratePortID generates the portID for a specific owner diff --git a/modules/apps/27-interchain-accounts/types/keys.go b/modules/apps/27-interchain-accounts/types/keys.go index 909813ec46d..07066ea0777 100644 --- a/modules/apps/27-interchain-accounts/types/keys.go +++ b/modules/apps/27-interchain-accounts/types/keys.go @@ -6,22 +6,34 @@ const ( // ModuleName defines the Interchain Account module name ModuleName = "interchainaccounts" - // Version defines the current version the IBC tranfer + // Version defines the current version the IBC interchainaccounts // module supports Version = "ics27-1" + // PortID is the default port id that the interchainaccounts module binds to PortID = "ibcaccount" - StoreKey = ModuleName + // StoreKey is the store key string for IBC interchainaccounts + StoreKey = ModuleName + + // RouterKey is the message route for IBC interchainaccounts RouterKey = ModuleName + // QuerierRoute is the querier route for IBC interchainaccounts + QuerierRoute = ModuleName +) + +var ( + // ModuleAccountKey defines the key used to construct to the IBC interchainaccounts module account address + ModuleAccountKey = []byte(ModuleName) + // Key to store portID in our store PortKey = "portID" - QuerierRoute = ModuleName - // MemStoreKey defines the in-memory store key MemStoreKey = "mem_capability" + + KeyPrefixRegisteredAccount = []byte("register") ) func KeyActiveChannel(portId string) []byte { @@ -32,10 +44,6 @@ func KeyOwnerAccount(portId string) []byte { return []byte(fmt.Sprintf("owner/%s", portId)) } -var ( - KeyPrefixRegisteredAccount = []byte("register") -) - func GetIdentifier(portID, channelID string) string { return fmt.Sprintf("%s/%s/", portID, channelID) } From bd66f33c8a0b185a97f8911445af5f5f7b2f6423 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 15 Sep 2021 18:30:04 +0200 Subject: [PATCH 06/17] removing unused keys --- modules/apps/27-interchain-accounts/types/keys.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/modules/apps/27-interchain-accounts/types/keys.go b/modules/apps/27-interchain-accounts/types/keys.go index 07066ea0777..3c1a721d9da 100644 --- a/modules/apps/27-interchain-accounts/types/keys.go +++ b/modules/apps/27-interchain-accounts/types/keys.go @@ -29,11 +29,6 @@ var ( // Key to store portID in our store PortKey = "portID" - - // MemStoreKey defines the in-memory store key - MemStoreKey = "mem_capability" - - KeyPrefixRegisteredAccount = []byte("register") ) func KeyActiveChannel(portId string) []byte { From 62e5c393e9bd05703f00f000e93a1a6785ac5f07 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 16 Sep 2021 15:30:40 +0200 Subject: [PATCH 07/17] adding improved version validation, updating tests --- .../27-interchain-accounts/keeper/account.go | 13 ++---- .../keeper/account_test.go | 4 +- .../keeper/handshake.go | 30 +++++++------- .../keeper/handshake_test.go | 21 +++++----- .../keeper/keeper_test.go | 11 ++++- .../keeper/relay_test.go | 2 +- modules/apps/27-interchain-accounts/module.go | 2 +- .../27-interchain-accounts/module_test.go | 15 ++++--- .../27-interchain-accounts/types/account.go | 25 +++++++----- .../types/account_test.go | 38 +++++++++--------- .../27-interchain-accounts/types/errors.go | 6 +-- .../apps/27-interchain-accounts/types/keys.go | 12 +++--- .../27-interchain-accounts/types/validate.go | 40 +++++++++++++++++++ 13 files changed, 137 insertions(+), 82 deletions(-) create mode 100644 modules/apps/27-interchain-accounts/types/validate.go diff --git a/modules/apps/27-interchain-accounts/keeper/account.go b/modules/apps/27-interchain-accounts/keeper/account.go index 176e6545c94..5f31c3df0c8 100644 --- a/modules/apps/27-interchain-accounts/keeper/account.go +++ b/modules/apps/27-interchain-accounts/keeper/account.go @@ -41,15 +41,10 @@ func (k Keeper) InitInterchainAccount(ctx sdk.Context, connectionID, counterpart } // 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 - } - +// 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 nil + return } interchainAccount := types.NewInterchainAccount( @@ -60,8 +55,6 @@ func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, address, portID strin k.accountKeeper.NewAccount(ctx, interchainAccount) k.accountKeeper.SetAccount(ctx, interchainAccount) k.SetInterchainAccountAddress(ctx, portID, interchainAccount.Address) - - return nil } func (k Keeper) GetInterchainAccount(ctx sdk.Context, addr sdk.AccAddress) (types.InterchainAccount, error) { diff --git a/modules/apps/27-interchain-accounts/keeper/account_test.go b/modules/apps/27-interchain-accounts/keeper/account_test.go index 3d9cd9bae49..69ac5a5dc87 100644 --- a/modules/apps/27-interchain-accounts/keeper/account_test.go +++ b/modules/apps/27-interchain-accounts/keeper/account_test.go @@ -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) diff --git a/modules/apps/27-interchain-accounts/keeper/handshake.go b/modules/apps/27-interchain-accounts/keeper/handshake.go index e209e7eb047..e6d0b32ca8d 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake.go @@ -1,7 +1,6 @@ package keeper import ( - "fmt" "strings" sdk "github.com/cosmos/cosmos-sdk/types" @@ -40,8 +39,8 @@ func (k Keeper) OnChanOpenInit( return sdkerrors.Wrapf(porttypes.ErrInvalidPort, "counterparty port-id must be '%s', (%s != %s)", types.PortID, counterparty.PortId, types.PortID) } - if !strings.Contains(version, types.Version) { - return sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid version: %s, expected %s", version, types.Version) + if err := types.ValidateVersion(version); err != nil { + return err } existingChannelID, found := k.GetActiveChannel(ctx, portID) @@ -76,12 +75,12 @@ func (k Keeper) OnChanOpenTry( return sdkerrors.Wrapf(channeltypes.ErrInvalidChannelOrdering, "invalid channel ordering: %s, expected %s", order.String(), channeltypes.ORDERED.String()) } - if !strings.Contains(version, types.Version) { - return sdkerrors.Wrapf(types.ErrInvalidVersion, "invalid version: %s, expected %s", version, types.Version) + if err := types.ValidateVersion(version); err != nil { + return err } - if !strings.Contains(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 } // On the host chain the capability may only be claimed during the OnChanOpenTry @@ -90,12 +89,15 @@ func (k Keeper) OnChanOpenTry( return err } - // Register interchain account if it does not already exist - accAddr := strings.TrimPrefix(version, fmt.Sprintf("%s-", types.Version)) - if err := k.RegisterInterchainAccount(ctx, accAddr, counterparty.PortId); err != nil { - return err + accAddr := types.GenerateAddress(counterparty.PortId) + parsedAddr := types.ParseAddressFromVersion(version) + if strings.Compare(parsedAddr, accAddr.String()) != 0 { + return sdkerrors.Wrapf(types.ErrInvalidAccountAddress, "invalid account address: %s, expected %s", parsedAddr, accAddr) } + // Register interchain account if it does not already exist + k.RegisterInterchainAccount(ctx, accAddr, counterparty.PortId) + return nil } @@ -109,13 +111,13 @@ func (k Keeper) OnChanOpenAck( channelID string, counterpartyVersion string, ) error { - if !strings.Contains(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 } k.SetActiveChannel(ctx, portID, channelID) - accAddr := strings.TrimPrefix(counterpartyVersion, fmt.Sprintf("%s-", types.Version)) + accAddr := types.ParseAddressFromVersion(counterpartyVersion) k.SetInterchainAccountAddress(ctx, portID, accAddr) return nil diff --git a/modules/apps/27-interchain-accounts/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/keeper/handshake_test.go index 4ab8a2ee27d..328423088b0 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake_test.go @@ -1,8 +1,6 @@ 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" @@ -64,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)) @@ -138,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 { @@ -146,7 +149,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { suite.Run(tc.name, func() { suite.SetupTest() // reset path = NewICAPath(suite.chainA, suite.chainB) - owner := "owner" + owner := TestOwnerAddress counterpartyVersion = types.Version suite.coordinator.SetupConnections(path) @@ -160,7 +163,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { Ordering: channeltypes.ORDERED, Counterparty: counterparty, ConnectionHops: []string{path.EndpointB.ConnectionID}, - Version: fmt.Sprintf("%s-%s", types.Version, types.GenerateAddress("ics-27-0-0-testing")), + Version: TestVersion, } chanCap, err = suite.chainB.App.GetScopedIBCKeeper().NewCapability(suite.chainB.GetContext(), host.ChannelCapabilityPath(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)) @@ -213,11 +216,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() @@ -266,10 +268,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() diff --git a/modules/apps/27-interchain-accounts/keeper/keeper_test.go b/modules/apps/27-interchain-accounts/keeper/keeper_test.go index a2440c75e18..c2104776db3 100644 --- a/modules/apps/27-interchain-accounts/keeper/keeper_test.go +++ b/modules/apps/27-interchain-accounts/keeper/keeper_test.go @@ -12,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)) +) + type KeeperTestSuite struct { suite.Suite @@ -37,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 = fmt.Sprintf("%s-%s", types.Version, types.GenerateAddress("ics-27-0-0-testing")) + path.EndpointB.ChannelConfig.Version = TestVersion return path } @@ -105,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 diff --git a/modules/apps/27-interchain-accounts/keeper/relay_test.go b/modules/apps/27-interchain-accounts/keeper/relay_test.go index 66c3b22d51a..ea836fb8c05 100644 --- a/modules/apps/27-interchain-accounts/keeper/relay_test.go +++ b/modules/apps/27-interchain-accounts/keeper/relay_test.go @@ -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) diff --git a/modules/apps/27-interchain-accounts/module.go b/modules/apps/27-interchain-accounts/module.go index f6f075a7f03..a97d733701f 100644 --- a/modules/apps/27-interchain-accounts/module.go +++ b/modules/apps/27-interchain-accounts/module.go @@ -270,5 +270,5 @@ func (am AppModule) NegotiateAppVersion( addr := types.GenerateAddress(counterparty.PortId) - return fmt.Sprintf("%s-%s", types.Version, addr), nil + return fmt.Sprint(types.Version, types.Delimiter, addr), nil } diff --git a/modules/apps/27-interchain-accounts/module_test.go b/modules/apps/27-interchain-accounts/module_test.go index 58bf50579fd..6662d414163 100644 --- a/modules/apps/27-interchain-accounts/module_test.go +++ b/modules/apps/27-interchain-accounts/module_test.go @@ -2,7 +2,6 @@ package interchain_accounts_test import ( "fmt" - "strings" "testing" "github.com/stretchr/testify/suite" @@ -12,6 +11,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)) +) + type InterchainAccountsTestSuite struct { suite.Suite @@ -37,7 +43,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 = fmt.Sprintf("%s-%s", types.Version, types.GenerateAddress("ics-27-0-0-testing")) + path.EndpointB.ChannelConfig.Version = TestVersion return path } @@ -57,7 +63,7 @@ func (suite *InterchainAccountsTestSuite) TestNegotiateAppVersion() { cbs, ok := suite.chainA.GetSimApp().GetIBCKeeper().Router.GetRoute(module) suite.Require().True(ok) - counterpartyPortID, err := types.GeneratePortID("testing", path.EndpointA.ConnectionID, path.EndpointB.ConnectionID) + counterpartyPortID, err := types.GeneratePortID(TestOwnerAddress, path.EndpointA.ConnectionID, path.EndpointB.ConnectionID) suite.Require().NoError(err) counterparty := &channeltypes.Counterparty{ @@ -67,6 +73,5 @@ func (suite *InterchainAccountsTestSuite) TestNegotiateAppVersion() { version, err := cbs.NegotiateAppVersion(suite.chainA.GetContext(), channeltypes.ORDERED, path.EndpointA.ConnectionID, types.PortID, *counterparty, types.Version) suite.Require().NoError(err) - suite.Require().True(strings.Contains(version, types.Version)) - suite.Require().True(strings.Contains(version, types.GenerateAddress(counterpartyPortID).String())) + suite.Require().NoError(types.ValidateVersion(version)) } diff --git a/modules/apps/27-interchain-accounts/types/account.go b/modules/apps/27-interchain-accounts/types/account.go index d7847b2d3f8..62ae112d2b3 100644 --- a/modules/apps/27-interchain-accounts/types/account.go +++ b/modules/apps/27-interchain-accounts/types/account.go @@ -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" - "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" + yaml "gopkg.in/yaml.v2" connectiontypes "github.com/cosmos/ibc-go/modules/core/03-connection/types" ) @@ -20,10 +19,14 @@ const ( ICAPrefix string = "ics-27" ) -// GenerateAddress returns an sdk.AccAddress using the provided port identifier and derived from the IBC interchainaccounts module account +// GenerateAddress returns an sdk.AccAddress using the provided port identifier func GenerateAddress(portID string) sdk.AccAddress { - moduleAcc := address.Module(ModuleName, ModuleAccountKey) - return sdk.AccAddress(address.Derive(moduleAcc, []byte(portID))) + 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)) } // GeneratePortID generates the portID for a specific owner @@ -33,21 +36,21 @@ func GenerateAddress(portID string) sdk.AccAddress { // 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 { diff --git a/modules/apps/27-interchain-accounts/types/account_test.go b/modules/apps/27-interchain-accounts/types/account_test.go index e72241fc1e0..8f3bbc1126b 100644 --- a/modules/apps/27-interchain-accounts/types/account_test.go +++ b/modules/apps/27-interchain-accounts/types/account_test.go @@ -4,10 +4,16 @@ import ( "fmt" "testing" + sdk "github.com/cosmos/cosmos-sdk/types" + "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" - "github.com/stretchr/testify/suite" +) + +var ( + // TestOwnerAddress defines a reusable bech32 address for testing purposes + TestOwnerAddress = "cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs" ) type TypesTestSuite struct { @@ -26,26 +32,22 @@ func (suite *TypesTestSuite) SetupTest() { suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1)) } -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 TestTypesTestSuite(t *testing.T) { suite.Run(t, new(TypesTestSuite)) } +func (suite *TypesTestSuite) TestGenerateAddress() { + addr := types.GenerateAddress("test-port-id") + accAddr, err := sdk.AccAddressFromBech32(addr.String()) + + suite.Require().NoError(err, "TestGenerateAddress failed") + suite.Require().NotEmpty(accAddr) +} + func (suite *TypesTestSuite) TestGeneratePortID() { var ( path *ibctesting.Path - owner = "testing" + owner = TestOwnerAddress ) testCases := []struct { @@ -57,7 +59,7 @@ func (suite *TypesTestSuite) TestGeneratePortID() { { "success", func() {}, - "ics-27-0-0-testing", + fmt.Sprintf("ics-27-0-0-%s", TestOwnerAddress), true, }, { @@ -65,7 +67,7 @@ func (suite *TypesTestSuite) TestGeneratePortID() { func() { path.EndpointA.ConnectionID = "connection-1" }, - "ics-27-1-0-testing", + fmt.Sprintf("ics-27-1-0-%s", TestOwnerAddress), true, }, { @@ -98,7 +100,7 @@ func (suite *TypesTestSuite) TestGeneratePortID() { tc := tc suite.Run(tc.name, func() { suite.SetupTest() // reset - path = NewICAPath(suite.chainA, suite.chainB) + path = ibctesting.NewPath(suite.chainA, suite.chainB) suite.coordinator.Setup(path) tc.malleate() diff --git a/modules/apps/27-interchain-accounts/types/errors.go b/modules/apps/27-interchain-accounts/types/errors.go index 32425a81071..5c5bf3fed44 100644 --- a/modules/apps/27-interchain-accounts/types/errors.go +++ b/modules/apps/27-interchain-accounts/types/errors.go @@ -11,9 +11,9 @@ var ( ErrUnsupportedChain = sdkerrors.Register(ModuleName, 5, "unsupported chain") ErrInvalidOutgoingData = sdkerrors.Register(ModuleName, 6, "invalid outgoing data") ErrInvalidRoute = sdkerrors.Register(ModuleName, 7, "invalid route") - ErrInterchainAccountNotFound = sdkerrors.Register(ModuleName, 8, "Interchain Account not found") - ErrInterchainAccountAlreadySet = sdkerrors.Register(ModuleName, 9, "Interchain Account is already set") + ErrInterchainAccountNotFound = sdkerrors.Register(ModuleName, 8, "interchain account not found") + ErrInterchainAccountAlreadySet = sdkerrors.Register(ModuleName, 9, "interchain account is already set") ErrActiveChannelNotFound = sdkerrors.Register(ModuleName, 10, "no active channel for this owner") ErrInvalidVersion = sdkerrors.Register(ModuleName, 11, "invalid interchain accounts version") - ErrInvalidOwnerAddress = sdkerrors.Register(ModuleName, 12, "invalid owner address") + ErrInvalidAccountAddress = sdkerrors.Register(ModuleName, 12, "invalid account address") ) diff --git a/modules/apps/27-interchain-accounts/types/keys.go b/modules/apps/27-interchain-accounts/types/keys.go index 3c1a721d9da..d5b7fdf4a38 100644 --- a/modules/apps/27-interchain-accounts/types/keys.go +++ b/modules/apps/27-interchain-accounts/types/keys.go @@ -1,6 +1,8 @@ package types -import "fmt" +import ( + "fmt" +) const ( // ModuleName defines the Interchain Account module name @@ -21,13 +23,13 @@ const ( // QuerierRoute is the querier route for IBC interchainaccounts QuerierRoute = ModuleName + + // Delimiter is the delimiter used when parsing the interchainaccounts version string + Delimiter = "|" ) var ( - // ModuleAccountKey defines the key used to construct to the IBC interchainaccounts module account address - ModuleAccountKey = []byte(ModuleName) - - // Key to store portID in our store + // PortKey defines the key to store the port ID in store PortKey = "portID" ) diff --git a/modules/apps/27-interchain-accounts/types/validate.go b/modules/apps/27-interchain-accounts/types/validate.go new file mode 100644 index 00000000000..fbbf2152fd0 --- /dev/null +++ b/modules/apps/27-interchain-accounts/types/validate.go @@ -0,0 +1,40 @@ +package types + +import ( + "regexp" + "strings" + + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" +) + +// DefaultMaxAddrLength defines the default maximum character length used in validation of addresses +var DefaultMaxAddrLength = 64 + +// DefaultMinAddrLength defines the default minimum character length used in validation of addresses +var DefaultMinAddrLength = 32 + +// IsValidAddr defines a regular expression to check if the provided string consists of +// strictly alphanumeric characters +var IsValidAddr = regexp.MustCompile("^[a-zA-Z0-9]*$").MatchString + +// ValidateVersion performs basic validation of the provided interchainaccounts version string +func ValidateVersion(version string) error { + s := strings.Split(version, Delimiter) + + if s[0] != Version { + return sdkerrors.Wrapf(ErrInvalidVersion, "invalid version: expected %s, got %s", Version, s[0]) + } + + if len(s) > 1 { + if !IsValidAddr(s[1]) && len(s[1]) <= DefaultMaxAddrLength && len(s[1]) >= DefaultMinAddrLength { + return sdkerrors.Wrapf( + ErrInvalidAccountAddress, + "address must contain strictly alphanumeric characters, %d-%d characters in length", + DefaultMinAddrLength, + DefaultMaxAddrLength, + ) + } + } + + return nil +} From 4ee74bfd8a6ab4837a0f89580a4ad8a991444c25 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 16 Sep 2021 15:36:23 +0200 Subject: [PATCH 08/17] removing redundant local var - owner --- modules/apps/27-interchain-accounts/keeper/handshake_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/modules/apps/27-interchain-accounts/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/keeper/handshake_test.go index 328423088b0..214314f9bb4 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake_test.go @@ -149,11 +149,10 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { suite.Run(tc.name, func() { suite.SetupTest() // reset path = NewICAPath(suite.chainA, suite.chainB) - owner := TestOwnerAddress counterpartyVersion = types.Version suite.coordinator.SetupConnections(path) - err := InitInterchainAccount(path.EndpointA, owner) + err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) suite.Require().NoError(err) // default values From b2f3e3b29223cbdb6870405c4c47afd191731d2d Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 16 Sep 2021 15:40:55 +0200 Subject: [PATCH 09/17] updating Delimiter godoc --- modules/apps/27-interchain-accounts/types/keys.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/27-interchain-accounts/types/keys.go b/modules/apps/27-interchain-accounts/types/keys.go index d5b7fdf4a38..c4c1c786d76 100644 --- a/modules/apps/27-interchain-accounts/types/keys.go +++ b/modules/apps/27-interchain-accounts/types/keys.go @@ -24,7 +24,7 @@ const ( // QuerierRoute is the querier route for IBC interchainaccounts QuerierRoute = ModuleName - // Delimiter is the delimiter used when parsing the interchainaccounts version string + // Delimiter is the delimiter used for the interchainaccounts version string Delimiter = "|" ) From c7d78041f11e99e3fe401e5d9b01e1b4bf013809 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 16 Sep 2021 18:21:49 +0200 Subject: [PATCH 10/17] updating validation logic --- modules/apps/27-interchain-accounts/module.go | 4 +--- modules/apps/27-interchain-accounts/types/validate.go | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/modules/apps/27-interchain-accounts/module.go b/modules/apps/27-interchain-accounts/module.go index a97d733701f..bcb4e9c17ba 100644 --- a/modules/apps/27-interchain-accounts/module.go +++ b/modules/apps/27-interchain-accounts/module.go @@ -268,7 +268,5 @@ func (am AppModule) NegotiateAppVersion( return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "failed to negotiate app version: expected %s, got %s", types.Version, proposedVersion) } - addr := types.GenerateAddress(counterparty.PortId) - - return fmt.Sprint(types.Version, types.Delimiter, addr), nil + return fmt.Sprint(types.Version, types.Delimiter, types.GenerateAddress(counterparty.PortId)), nil } diff --git a/modules/apps/27-interchain-accounts/types/validate.go b/modules/apps/27-interchain-accounts/types/validate.go index fbbf2152fd0..1c305bd3899 100644 --- a/modules/apps/27-interchain-accounts/types/validate.go +++ b/modules/apps/27-interchain-accounts/types/validate.go @@ -26,7 +26,7 @@ func ValidateVersion(version string) error { } if len(s) > 1 { - if !IsValidAddr(s[1]) && len(s[1]) <= DefaultMaxAddrLength && len(s[1]) >= DefaultMinAddrLength { + if !IsValidAddr(s[1]) || len(s[1]) > DefaultMaxAddrLength || len(s[1]) < DefaultMinAddrLength { return sdkerrors.Wrapf( ErrInvalidAccountAddress, "address must contain strictly alphanumeric characters, %d-%d characters in length", From 001313ce2864605fcf7a737958b7dddfd5b023ed Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Thu, 16 Sep 2021 18:37:15 +0200 Subject: [PATCH 11/17] adding test cases for ValidateVersion --- .../types/validate_test.go | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 modules/apps/27-interchain-accounts/types/validate_test.go diff --git a/modules/apps/27-interchain-accounts/types/validate_test.go b/modules/apps/27-interchain-accounts/types/validate_test.go new file mode 100644 index 00000000000..656e629954f --- /dev/null +++ b/modules/apps/27-interchain-accounts/types/validate_test.go @@ -0,0 +1,53 @@ +package types_test + +import ( + "fmt" + + "github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types" +) + +func (suite *TypesTestSuite) TestValidateVersion() { + testCases := []struct { + name string + version string + expPass bool + }{ + { + "success", + fmt.Sprint(types.Version, types.Delimiter, TestOwnerAddress), + true, + }, + { + "invalid version", + "ics27-5|abc123", + false, + }, + { + "invalid account address - 31 chars", + "ics27-1|xtignpvthxbwxtmnzyfwhhywobaatlt", + false, + }, + { + "invalid account address - 65 chars", + "ics27-1|ofwafxhdmqcdbpzvrccxkidbunrwyyoboyctignpvthxbwxtmnzyfwhhywobaatlt", + false, + }, + { + "invalid account address - non alphanumeric characters", + "ics27-1|abc_123", + false, + }, + } + + for _, tc := range testCases { + suite.Run(tc.name, func() { + err := types.ValidateVersion(tc.version) + + if tc.expPass { + suite.Require().NoError(err, tc.name) + } else { + suite.Require().Error(err, tc.name) + } + }) + } +} From d6725e1794b81182e5ea3d398d1742bbc04c2426 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 17 Sep 2021 10:00:56 +0200 Subject: [PATCH 12/17] adding additional validation testcase, updating godocs --- .../apps/27-interchain-accounts/types/validate.go | 3 ++- .../27-interchain-accounts/types/validate_test.go | 13 +++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/modules/apps/27-interchain-accounts/types/validate.go b/modules/apps/27-interchain-accounts/types/validate.go index 1c305bd3899..9e1bac5851b 100644 --- a/modules/apps/27-interchain-accounts/types/validate.go +++ b/modules/apps/27-interchain-accounts/types/validate.go @@ -17,7 +17,8 @@ var DefaultMinAddrLength = 32 // strictly alphanumeric characters var IsValidAddr = regexp.MustCompile("^[a-zA-Z0-9]*$").MatchString -// ValidateVersion performs basic validation of the provided interchainaccounts version string +// ValidateVersion performs basic validation of the provided ics27 version string +// When no delimiter is present it compares against the expected version func ValidateVersion(version string) error { s := strings.Split(version, Delimiter) diff --git a/modules/apps/27-interchain-accounts/types/validate_test.go b/modules/apps/27-interchain-accounts/types/validate_test.go index 656e629954f..d85e7e2f271 100644 --- a/modules/apps/27-interchain-accounts/types/validate_test.go +++ b/modules/apps/27-interchain-accounts/types/validate_test.go @@ -17,24 +17,29 @@ func (suite *TypesTestSuite) TestValidateVersion() { fmt.Sprint(types.Version, types.Delimiter, TestOwnerAddress), true, }, + { + "success - version only", + fmt.Sprint(types.Version), + true, + }, { "invalid version", - "ics27-5|abc123", + fmt.Sprint("ics27-5", types.Delimiter, TestOwnerAddress), false, }, { "invalid account address - 31 chars", - "ics27-1|xtignpvthxbwxtmnzyfwhhywobaatlt", + fmt.Sprint(types.Version, types.Delimiter, "xtignpvthxbwxtmnzyfwhhywobaatlt"), false, }, { "invalid account address - 65 chars", - "ics27-1|ofwafxhdmqcdbpzvrccxkidbunrwyyoboyctignpvthxbwxtmnzyfwhhywobaatlt", + fmt.Sprint(types.Version, types.Delimiter, "ofwafxhdmqcdbpzvrccxkidbunrwyyoboyctignpvthxbwxtmnzyfwhhywobaatlt"), false, }, { "invalid account address - non alphanumeric characters", - "ics27-1|abc_123", + fmt.Sprint(types.Version, types.Delimiter, "-_-"), false, }, } From 32240ffa9e804683b8dbfefbdda7aaf30ca620c9 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 17 Sep 2021 16:26:12 +0200 Subject: [PATCH 13/17] updating Version -> VersionPrefix, error msgs, validation tests --- .../27-interchain-accounts/keeper/account.go | 2 +- .../keeper/handshake.go | 6 ++-- .../keeper/handshake_test.go | 4 +-- .../keeper/keeper_test.go | 6 ++-- modules/apps/27-interchain-accounts/module.go | 6 ++-- .../27-interchain-accounts/module_test.go | 9 ++++-- .../27-interchain-accounts/types/account.go | 2 +- .../apps/27-interchain-accounts/types/keys.go | 30 ++++++++++--------- .../27-interchain-accounts/types/validate.go | 18 +++++------ .../types/validate_test.go | 19 +++++++----- 10 files changed, 56 insertions(+), 46 deletions(-) diff --git a/modules/apps/27-interchain-accounts/keeper/account.go b/modules/apps/27-interchain-accounts/keeper/account.go index 5f31c3df0c8..814e96d6dad 100644 --- a/modules/apps/27-interchain-accounts/keeper/account.go +++ b/modules/apps/27-interchain-accounts/keeper/account.go @@ -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 diff --git a/modules/apps/27-interchain-accounts/keeper/handshake.go b/modules/apps/27-interchain-accounts/keeper/handshake.go index e6d0b32ca8d..19dd325ba66 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake.go @@ -1,8 +1,6 @@ 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" @@ -91,8 +89,8 @@ func (k Keeper) OnChanOpenTry( accAddr := types.GenerateAddress(counterparty.PortId) parsedAddr := types.ParseAddressFromVersion(version) - if strings.Compare(parsedAddr, accAddr.String()) != 0 { - return sdkerrors.Wrapf(types.ErrInvalidAccountAddress, "invalid account address: %s, expected %s", parsedAddr, accAddr) + 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 diff --git a/modules/apps/27-interchain-accounts/keeper/handshake_test.go b/modules/apps/27-interchain-accounts/keeper/handshake_test.go index 214314f9bb4..d3ba9b20a31 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake_test.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake_test.go @@ -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)) @@ -149,7 +149,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() { suite.Run(tc.name, func() { suite.SetupTest() // reset path = NewICAPath(suite.chainA, suite.chainB) - counterpartyVersion = types.Version + counterpartyVersion = types.VersionPrefix suite.coordinator.SetupConnections(path) err := InitInterchainAccount(path.EndpointA, TestOwnerAddress) diff --git a/modules/apps/27-interchain-accounts/keeper/keeper_test.go b/modules/apps/27-interchain-accounts/keeper/keeper_test.go index c2104776db3..d95f1fc6a57 100644 --- a/modules/apps/27-interchain-accounts/keeper/keeper_test.go +++ b/modules/apps/27-interchain-accounts/keeper/keeper_test.go @@ -15,8 +15,10 @@ import ( 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 = fmt.Sprintf("%s|%s", types.Version, types.GenerateAddress("ics-27-0-0-"+TestOwnerAddress)) + TestVersion = fmt.Sprintf("%s|%s", types.VersionPrefix, types.GenerateAddress(TestPortID)) ) type KeeperTestSuite struct { @@ -43,7 +45,7 @@ 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.EndpointA.ChannelConfig.Version = types.VersionPrefix path.EndpointB.ChannelConfig.Version = TestVersion return path diff --git a/modules/apps/27-interchain-accounts/module.go b/modules/apps/27-interchain-accounts/module.go index bcb4e9c17ba..db077fa1d96 100644 --- a/modules/apps/27-interchain-accounts/module.go +++ b/modules/apps/27-interchain-accounts/module.go @@ -264,9 +264,9 @@ func (am AppModule) NegotiateAppVersion( counterparty channeltypes.Counterparty, proposedVersion string, ) (string, error) { - if proposedVersion != types.Version { - return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "failed to negotiate app version: expected %s, got %s", types.Version, proposedVersion) + if proposedVersion != types.VersionPrefix { + return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "failed to negotiate app version: expected %s, got %s", types.VersionPrefix, proposedVersion) } - return fmt.Sprint(types.Version, types.Delimiter, types.GenerateAddress(counterparty.PortId)), nil + return types.NewAppVersion(types.GenerateAddress(counterparty.PortId).String()), nil } diff --git a/modules/apps/27-interchain-accounts/module_test.go b/modules/apps/27-interchain-accounts/module_test.go index 6662d414163..e4c7b077456 100644 --- a/modules/apps/27-interchain-accounts/module_test.go +++ b/modules/apps/27-interchain-accounts/module_test.go @@ -14,8 +14,10 @@ import ( 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 = fmt.Sprintf("%s|%s", types.Version, types.GenerateAddress("ics-27-0-0-"+TestOwnerAddress)) + TestVersion = fmt.Sprintf("%s|%s", types.VersionPrefix, types.GenerateAddress(TestPortID)) ) type InterchainAccountsTestSuite struct { @@ -42,7 +44,7 @@ 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.EndpointA.ChannelConfig.Version = types.VersionPrefix path.EndpointB.ChannelConfig.Version = TestVersion return path @@ -71,7 +73,8 @@ func (suite *InterchainAccountsTestSuite) TestNegotiateAppVersion() { ChannelId: path.EndpointB.ChannelID, } - version, err := cbs.NegotiateAppVersion(suite.chainA.GetContext(), channeltypes.ORDERED, path.EndpointA.ConnectionID, types.PortID, *counterparty, types.Version) + 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) } diff --git a/modules/apps/27-interchain-accounts/types/account.go b/modules/apps/27-interchain-accounts/types/account.go index 62ae112d2b3..7948e2e6afa 100644 --- a/modules/apps/27-interchain-accounts/types/account.go +++ b/modules/apps/27-interchain-accounts/types/account.go @@ -26,7 +26,7 @@ func GenerateAddress(portID string) sdk.AccAddress { // 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)) + return strings.TrimPrefix(version, fmt.Sprint(VersionPrefix, Delimiter)) } // GeneratePortID generates the portID for a specific owner diff --git a/modules/apps/27-interchain-accounts/types/keys.go b/modules/apps/27-interchain-accounts/types/keys.go index c4c1c786d76..011c5e68ec4 100644 --- a/modules/apps/27-interchain-accounts/types/keys.go +++ b/modules/apps/27-interchain-accounts/types/keys.go @@ -5,42 +5,44 @@ import ( ) const ( - // ModuleName defines the Interchain Account module name + // ModuleName defines the interchain accounts module name ModuleName = "interchainaccounts" - // Version defines the current version the IBC interchainaccounts - // module supports - Version = "ics27-1" + // VersionPrefix defines the current version for interchain accounts + VersionPrefix = "ics27-1" - // PortID is the default port id that the interchainaccounts module binds to + // PortID is the default port id that the interchain accounts module binds to PortID = "ibcaccount" - // StoreKey is the store key string for IBC interchainaccounts + // StoreKey is the store key string for interchain accounts StoreKey = ModuleName - // RouterKey is the message route for IBC interchainaccounts + // RouterKey is the message route for interchain accounts RouterKey = ModuleName - // QuerierRoute is the querier route for IBC interchainaccounts + // QuerierRoute is the querier route for interchain accounts QuerierRoute = ModuleName - // Delimiter is the delimiter used for the interchainaccounts version string + // Delimiter is the delimiter used for the interchain accounts version string Delimiter = "|" ) var ( // PortKey defines the key to store the port ID in store - PortKey = "portID" + PortKey = []byte{0x01} ) +// NewVersion returns a complete version string in the format: VersionPrefix + Delimter + AccAddress +func NewAppVersion(addr string) string { + return fmt.Sprint(VersionPrefix, Delimiter, addr) +} + +// KeyActiveChannel creates and returns a new key used for active channels store operations func KeyActiveChannel(portId string) []byte { return []byte(fmt.Sprintf("activeChannel/%s", portId)) } +// KeyOwnerAccount creates and returns a new key used for owner account store operations func KeyOwnerAccount(portId string) []byte { return []byte(fmt.Sprintf("owner/%s", portId)) } - -func GetIdentifier(portID, channelID string) string { - return fmt.Sprintf("%s/%s/", portID, channelID) -} diff --git a/modules/apps/27-interchain-accounts/types/validate.go b/modules/apps/27-interchain-accounts/types/validate.go index 9e1bac5851b..596d0c467ae 100644 --- a/modules/apps/27-interchain-accounts/types/validate.go +++ b/modules/apps/27-interchain-accounts/types/validate.go @@ -8,10 +8,7 @@ import ( ) // DefaultMaxAddrLength defines the default maximum character length used in validation of addresses -var DefaultMaxAddrLength = 64 - -// DefaultMinAddrLength defines the default minimum character length used in validation of addresses -var DefaultMinAddrLength = 32 +var DefaultMaxAddrLength = 128 // IsValidAddr defines a regular expression to check if the provided string consists of // strictly alphanumeric characters @@ -22,16 +19,19 @@ var IsValidAddr = regexp.MustCompile("^[a-zA-Z0-9]*$").MatchString func ValidateVersion(version string) error { s := strings.Split(version, Delimiter) - if s[0] != Version { - return sdkerrors.Wrapf(ErrInvalidVersion, "invalid version: expected %s, got %s", Version, s[0]) + if s[0] != VersionPrefix { + return sdkerrors.Wrapf(ErrInvalidVersion, "expected %s, got %s", VersionPrefix, s[0]) } if len(s) > 1 { - if !IsValidAddr(s[1]) || len(s[1]) > DefaultMaxAddrLength || len(s[1]) < DefaultMinAddrLength { + if len(s) != 2 { + return sdkerrors.Wrap(ErrInvalidAccountAddress, "unexpected address format") + } + + if !IsValidAddr(s[1]) || len(s[1]) == 0 || len(s[1]) > DefaultMaxAddrLength { return sdkerrors.Wrapf( ErrInvalidAccountAddress, - "address must contain strictly alphanumeric characters, %d-%d characters in length", - DefaultMinAddrLength, + "address must contain strictly alphanumeric characters, not exceeding %d characters in length", DefaultMaxAddrLength, ) } diff --git a/modules/apps/27-interchain-accounts/types/validate_test.go b/modules/apps/27-interchain-accounts/types/validate_test.go index d85e7e2f271..beaaf9443a6 100644 --- a/modules/apps/27-interchain-accounts/types/validate_test.go +++ b/modules/apps/27-interchain-accounts/types/validate_test.go @@ -14,12 +14,12 @@ func (suite *TypesTestSuite) TestValidateVersion() { }{ { "success", - fmt.Sprint(types.Version, types.Delimiter, TestOwnerAddress), + fmt.Sprint(types.VersionPrefix, types.Delimiter, TestOwnerAddress), true, }, { "success - version only", - fmt.Sprint(types.Version), + fmt.Sprint(types.VersionPrefix), true, }, { @@ -28,18 +28,23 @@ func (suite *TypesTestSuite) TestValidateVersion() { false, }, { - "invalid account address - 31 chars", - fmt.Sprint(types.Version, types.Delimiter, "xtignpvthxbwxtmnzyfwhhywobaatlt"), + "invalid account address - empty", + fmt.Sprint(types.VersionPrefix, types.Delimiter, ""), false, }, { - "invalid account address - 65 chars", - fmt.Sprint(types.Version, types.Delimiter, "ofwafxhdmqcdbpzvrccxkidbunrwyyoboyctignpvthxbwxtmnzyfwhhywobaatlt"), + "invalid account address - exceeded character length", + fmt.Sprint(types.VersionPrefix, types.Delimiter, "ofwafxhdmqcdbpzvrccxkidbunrwyyoboyctignpvthxbwxtmnzyfwhhywobaatltfwafxhdmqcdbpzvrccxkidbunrwyyoboyctignpvthxbwxtmnzyfwhhywobaatlt"), false, }, { "invalid account address - non alphanumeric characters", - fmt.Sprint(types.Version, types.Delimiter, "-_-"), + fmt.Sprint(types.VersionPrefix, types.Delimiter, "-_-"), + false, + }, + { + "invalid account address - address contains additional delimiter", + fmt.Sprint(types.VersionPrefix, types.Delimiter, "cosmos17dtl0mjt3t77kpu|hg2edqzjpszulwhgzuj9ljs"), false, }, } From e4af997e223b7353a9708fa65683cb0c1aad3f89 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Fri, 17 Sep 2021 16:51:44 +0200 Subject: [PATCH 14/17] updating NewAppVersion func sig and usage --- .../27-interchain-accounts/keeper/keeper_test.go | 2 +- modules/apps/27-interchain-accounts/module.go | 2 +- .../apps/27-interchain-accounts/module_test.go | 2 +- .../apps/27-interchain-accounts/types/keys.go | 4 ++-- .../types/validate_test.go | 16 +++++++--------- 5 files changed, 12 insertions(+), 14 deletions(-) diff --git a/modules/apps/27-interchain-accounts/keeper/keeper_test.go b/modules/apps/27-interchain-accounts/keeper/keeper_test.go index d95f1fc6a57..b7aa9093388 100644 --- a/modules/apps/27-interchain-accounts/keeper/keeper_test.go +++ b/modules/apps/27-interchain-accounts/keeper/keeper_test.go @@ -18,7 +18,7 @@ var ( // 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 = fmt.Sprintf("%s|%s", types.VersionPrefix, types.GenerateAddress(TestPortID)) + TestVersion = types.NewAppVersion(types.VersionPrefix, types.Delimiter, types.GenerateAddress(TestPortID).String()) ) type KeeperTestSuite struct { diff --git a/modules/apps/27-interchain-accounts/module.go b/modules/apps/27-interchain-accounts/module.go index db077fa1d96..3b9f8076f4a 100644 --- a/modules/apps/27-interchain-accounts/module.go +++ b/modules/apps/27-interchain-accounts/module.go @@ -268,5 +268,5 @@ func (am AppModule) NegotiateAppVersion( return "", sdkerrors.Wrapf(types.ErrInvalidVersion, "failed to negotiate app version: expected %s, got %s", types.VersionPrefix, proposedVersion) } - return types.NewAppVersion(types.GenerateAddress(counterparty.PortId).String()), nil + return types.NewAppVersion(types.VersionPrefix, types.Delimiter, types.GenerateAddress(counterparty.PortId).String()), nil } diff --git a/modules/apps/27-interchain-accounts/module_test.go b/modules/apps/27-interchain-accounts/module_test.go index e4c7b077456..87fe9353b31 100644 --- a/modules/apps/27-interchain-accounts/module_test.go +++ b/modules/apps/27-interchain-accounts/module_test.go @@ -17,7 +17,7 @@ var ( // 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 = fmt.Sprintf("%s|%s", types.VersionPrefix, types.GenerateAddress(TestPortID)) + TestVersion = types.NewAppVersion(types.VersionPrefix, types.Delimiter, types.GenerateAddress(TestPortID).String()) ) type InterchainAccountsTestSuite struct { diff --git a/modules/apps/27-interchain-accounts/types/keys.go b/modules/apps/27-interchain-accounts/types/keys.go index 011c5e68ec4..47df826dece 100644 --- a/modules/apps/27-interchain-accounts/types/keys.go +++ b/modules/apps/27-interchain-accounts/types/keys.go @@ -33,8 +33,8 @@ var ( ) // NewVersion returns a complete version string in the format: VersionPrefix + Delimter + AccAddress -func NewAppVersion(addr string) string { - return fmt.Sprint(VersionPrefix, Delimiter, addr) +func NewAppVersion(versionPrefix, delimiter, addr string) string { + return fmt.Sprint(versionPrefix, delimiter, addr) } // KeyActiveChannel creates and returns a new key used for active channels store operations diff --git a/modules/apps/27-interchain-accounts/types/validate_test.go b/modules/apps/27-interchain-accounts/types/validate_test.go index beaaf9443a6..3f7157ca696 100644 --- a/modules/apps/27-interchain-accounts/types/validate_test.go +++ b/modules/apps/27-interchain-accounts/types/validate_test.go @@ -1,8 +1,6 @@ package types_test import ( - "fmt" - "github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types" ) @@ -14,37 +12,37 @@ func (suite *TypesTestSuite) TestValidateVersion() { }{ { "success", - fmt.Sprint(types.VersionPrefix, types.Delimiter, TestOwnerAddress), + types.NewAppVersion(types.VersionPrefix, types.Delimiter, TestOwnerAddress), true, }, { "success - version only", - fmt.Sprint(types.VersionPrefix), + types.NewAppVersion(types.VersionPrefix, "", ""), true, }, { "invalid version", - fmt.Sprint("ics27-5", types.Delimiter, TestOwnerAddress), + types.NewAppVersion("ics27-5", types.Delimiter, TestOwnerAddress), false, }, { "invalid account address - empty", - fmt.Sprint(types.VersionPrefix, types.Delimiter, ""), + types.NewAppVersion(types.VersionPrefix, types.Delimiter, ""), false, }, { "invalid account address - exceeded character length", - fmt.Sprint(types.VersionPrefix, types.Delimiter, "ofwafxhdmqcdbpzvrccxkidbunrwyyoboyctignpvthxbwxtmnzyfwhhywobaatltfwafxhdmqcdbpzvrccxkidbunrwyyoboyctignpvthxbwxtmnzyfwhhywobaatlt"), + types.NewAppVersion(types.VersionPrefix, types.Delimiter, "ofwafxhdmqcdbpzvrccxkidbunrwyyoboyctignpvthxbwxtmnzyfwhhywobaatltfwafxhdmqcdbpzvrccxkidbunrwyyoboyctignpvthxbwxtmnzyfwhhywobaatlt"), false, }, { "invalid account address - non alphanumeric characters", - fmt.Sprint(types.VersionPrefix, types.Delimiter, "-_-"), + types.NewAppVersion(types.VersionPrefix, types.Delimiter, "-_-"), false, }, { "invalid account address - address contains additional delimiter", - fmt.Sprint(types.VersionPrefix, types.Delimiter, "cosmos17dtl0mjt3t77kpu|hg2edqzjpszulwhgzuj9ljs"), + types.NewAppVersion(types.VersionPrefix, types.Delimiter, "cosmos17dtl0mjt3t77kpu|hg2edqzjpszulwhgzuj9ljs"), false, }, } From 18e9601ff8c8eb8a526609cedb724cf60b207d85 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 20 Sep 2021 13:25:24 +0200 Subject: [PATCH 15/17] updating NewAppVersion args and returning more appropriate errors for handshake --- .../27-interchain-accounts/keeper/handshake.go | 8 ++++---- .../27-interchain-accounts/keeper/keeper_test.go | 2 +- modules/apps/27-interchain-accounts/module.go | 2 +- .../apps/27-interchain-accounts/module_test.go | 2 +- .../apps/27-interchain-accounts/types/keys.go | 4 ++-- .../types/validate_test.go | 16 ++++++++-------- 6 files changed, 17 insertions(+), 17 deletions(-) diff --git a/modules/apps/27-interchain-accounts/keeper/handshake.go b/modules/apps/27-interchain-accounts/keeper/handshake.go index 19dd325ba66..d3ea5de071a 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake.go @@ -38,7 +38,7 @@ func (k Keeper) OnChanOpenInit( } if err := types.ValidateVersion(version); err != nil { - return err + return sdkerrors.Wrap(err, "version validation failed") } existingChannelID, found := k.GetActiveChannel(ctx, portID) @@ -74,11 +74,11 @@ func (k Keeper) OnChanOpenTry( } if err := types.ValidateVersion(version); err != nil { - return err + return sdkerrors.Wrap(err, "version validation failed") } if err := types.ValidateVersion(counterpartyVersion); err != nil { - return err + return sdkerrors.Wrap(err, "counterparty version validation failed") } // On the host chain the capability may only be claimed during the OnChanOpenTry @@ -110,7 +110,7 @@ func (k Keeper) OnChanOpenAck( counterpartyVersion string, ) error { if err := types.ValidateVersion(counterpartyVersion); err != nil { - return err + return sdkerrors.Wrap(err, "counterparty version validation failed") } k.SetActiveChannel(ctx, portID, channelID) diff --git a/modules/apps/27-interchain-accounts/keeper/keeper_test.go b/modules/apps/27-interchain-accounts/keeper/keeper_test.go index b7aa9093388..0f7c12d8189 100644 --- a/modules/apps/27-interchain-accounts/keeper/keeper_test.go +++ b/modules/apps/27-interchain-accounts/keeper/keeper_test.go @@ -18,7 +18,7 @@ var ( // 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()) + TestVersion = types.NewAppVersion(types.VersionPrefix, types.GenerateAddress(TestPortID).String()) ) type KeeperTestSuite struct { diff --git a/modules/apps/27-interchain-accounts/module.go b/modules/apps/27-interchain-accounts/module.go index 3b9f8076f4a..efa808f8d01 100644 --- a/modules/apps/27-interchain-accounts/module.go +++ b/modules/apps/27-interchain-accounts/module.go @@ -268,5 +268,5 @@ func (am AppModule) NegotiateAppVersion( 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 + return types.NewAppVersion(types.VersionPrefix, types.GenerateAddress(counterparty.PortId).String()), nil } diff --git a/modules/apps/27-interchain-accounts/module_test.go b/modules/apps/27-interchain-accounts/module_test.go index 87fe9353b31..fbfea84a189 100644 --- a/modules/apps/27-interchain-accounts/module_test.go +++ b/modules/apps/27-interchain-accounts/module_test.go @@ -17,7 +17,7 @@ var ( // 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()) + TestVersion = types.NewAppVersion(types.VersionPrefix, types.GenerateAddress(TestPortID).String()) ) type InterchainAccountsTestSuite struct { diff --git a/modules/apps/27-interchain-accounts/types/keys.go b/modules/apps/27-interchain-accounts/types/keys.go index 47df826dece..51841d0de0b 100644 --- a/modules/apps/27-interchain-accounts/types/keys.go +++ b/modules/apps/27-interchain-accounts/types/keys.go @@ -33,8 +33,8 @@ var ( ) // NewVersion returns a complete version string in the format: VersionPrefix + Delimter + AccAddress -func NewAppVersion(versionPrefix, delimiter, addr string) string { - return fmt.Sprint(versionPrefix, delimiter, addr) +func NewAppVersion(versionPrefix, accAddr string) string { + return fmt.Sprint(versionPrefix, Delimiter, accAddr) } // KeyActiveChannel creates and returns a new key used for active channels store operations diff --git a/modules/apps/27-interchain-accounts/types/validate_test.go b/modules/apps/27-interchain-accounts/types/validate_test.go index 3f7157ca696..aa1bb33c8ae 100644 --- a/modules/apps/27-interchain-accounts/types/validate_test.go +++ b/modules/apps/27-interchain-accounts/types/validate_test.go @@ -12,37 +12,37 @@ func (suite *TypesTestSuite) TestValidateVersion() { }{ { "success", - types.NewAppVersion(types.VersionPrefix, types.Delimiter, TestOwnerAddress), + types.NewAppVersion(types.VersionPrefix, TestOwnerAddress), true, }, { - "success - version only", - types.NewAppVersion(types.VersionPrefix, "", ""), + "success - version prefix only", + types.VersionPrefix, true, }, { "invalid version", - types.NewAppVersion("ics27-5", types.Delimiter, TestOwnerAddress), + types.NewAppVersion("ics27-5", TestOwnerAddress), false, }, { "invalid account address - empty", - types.NewAppVersion(types.VersionPrefix, types.Delimiter, ""), + types.NewAppVersion(types.VersionPrefix, ""), false, }, { "invalid account address - exceeded character length", - types.NewAppVersion(types.VersionPrefix, types.Delimiter, "ofwafxhdmqcdbpzvrccxkidbunrwyyoboyctignpvthxbwxtmnzyfwhhywobaatltfwafxhdmqcdbpzvrccxkidbunrwyyoboyctignpvthxbwxtmnzyfwhhywobaatlt"), + types.NewAppVersion(types.VersionPrefix, "ofwafxhdmqcdbpzvrccxkidbunrwyyoboyctignpvthxbwxtmnzyfwhhywobaatltfwafxhdmqcdbpzvrccxkidbunrwyyoboyctignpvthxbwxtmnzyfwhhywobaatlt"), false, }, { "invalid account address - non alphanumeric characters", - types.NewAppVersion(types.VersionPrefix, types.Delimiter, "-_-"), + types.NewAppVersion(types.VersionPrefix, "-_-"), false, }, { "invalid account address - address contains additional delimiter", - types.NewAppVersion(types.VersionPrefix, types.Delimiter, "cosmos17dtl0mjt3t77kpu|hg2edqzjpszulwhgzuj9ljs"), + types.NewAppVersion(types.VersionPrefix, "cosmos17dtl0mjt3t77kpu|hg2edqzjpszulwhgzuj9ljs"), false, }, } From 3d5baabf6b8128f190b8cb38276c513b0ec4a867 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 20 Sep 2021 16:05:00 +0200 Subject: [PATCH 16/17] Update modules/apps/27-interchain-accounts/keeper/handshake.go Co-authored-by: Sean King --- modules/apps/27-interchain-accounts/keeper/handshake.go | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/apps/27-interchain-accounts/keeper/handshake.go b/modules/apps/27-interchain-accounts/keeper/handshake.go index d3ea5de071a..bccd1be1a16 100644 --- a/modules/apps/27-interchain-accounts/keeper/handshake.go +++ b/modules/apps/27-interchain-accounts/keeper/handshake.go @@ -87,6 +87,7 @@ func (k Keeper) OnChanOpenTry( return err } + // Check to ensure that the version string contains the expected address generated from the Counterparty portID accAddr := types.GenerateAddress(counterparty.PortId) parsedAddr := types.ParseAddressFromVersion(version) if parsedAddr != accAddr.String() { From f400bdc4298feda238eae083863685052cf8bbf5 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Mon, 20 Sep 2021 16:15:44 +0200 Subject: [PATCH 17/17] updating ValidateVersion godoc --- modules/apps/27-interchain-accounts/types/validate.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/modules/apps/27-interchain-accounts/types/validate.go b/modules/apps/27-interchain-accounts/types/validate.go index 596d0c467ae..691eb369e41 100644 --- a/modules/apps/27-interchain-accounts/types/validate.go +++ b/modules/apps/27-interchain-accounts/types/validate.go @@ -14,8 +14,11 @@ var DefaultMaxAddrLength = 128 // strictly alphanumeric characters var IsValidAddr = regexp.MustCompile("^[a-zA-Z0-9]*$").MatchString -// ValidateVersion performs basic validation of the provided ics27 version string -// When no delimiter is present it compares against the expected version +// ValidateVersion performs basic validation of the provided ics27 version string. +// An ics27 version string may include an optional account address as per [TODO: Add spec when available] +// ValidateVersion first attempts to split the version string using the standard delimiter, then asserts a supported +// version prefix is included, followed by additional checks which enforce constraints on the optional account address. +// When no delimiter is present, only the version prefix is validated func ValidateVersion(version string) error { s := strings.Split(version, Delimiter)