Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move Get/SetInterchainAccount to keeper.go, add tests #355

Merged
merged 6 commits into from
Aug 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 1 addition & 19 deletions modules/apps/27-interchain-accounts/keeper/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,25 +59,7 @@ func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, portId string) {

k.accountKeeper.NewAccount(ctx, interchainAccount)
k.accountKeeper.SetAccount(ctx, interchainAccount)
_ = k.SetInterchainAccountAddress(ctx, portId, interchainAccount.Address)
}

func (k Keeper) SetInterchainAccountAddress(ctx sdk.Context, portId string, address string) string {
store := ctx.KVStore(k.storeKey)
key := types.KeyOwnerAccount(portId)
store.Set(key, []byte(address))
return address
}

func (k Keeper) GetInterchainAccountAddress(ctx sdk.Context, portId string) (string, error) {
Copy link
Member Author

@damiannolan damiannolan Aug 27, 2021

Choose a reason for hiding this comment

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

Is there an inconsistency with this return sig? From looking at many other modules in the SDK and ibc-go, it seems most store accessor funcs are returning (type, bool) as opposed to (type, error)

Copy link
Contributor

@colin-axner colin-axner Aug 27, 2021

Choose a reason for hiding this comment

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

yes it should be (type, bool), the Get function for the store will never return an error that isn't "type not found" so it is cleaner to use a boolean and let the caller decide if that is an error or not (sometimes it isn't). Nice catch

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed! 👍

I can push an update to this PR and make this change + omitting the return value of SetInterchainAccountAddress which @crodriguezvega flagged. Alternatively can include in a subsequent PR with other housekeeping tasks, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

lets do it in this pr since the changes are minor

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do! Thanks for the comments on the tests also, I expected there to be some tweeks needed. First time dipping my toes in the test suite, looks great btw!

Thanks for reviewing.

store := ctx.KVStore(k.storeKey)
key := types.KeyOwnerAccount(portId)
if !store.Has(key) {
return "", sdkerrors.Wrap(types.ErrInterchainAccountNotFound, portId)
}

interchainAccountAddr := string(store.Get(key))
return interchainAccountAddr, nil
k.SetInterchainAccountAddress(ctx, portId, interchainAccount.Address)
}

func (k Keeper) GetInterchainAccount(ctx sdk.Context, addr sdk.AccAddress) (types.InterchainAccount, error) {
Expand Down
9 changes: 4 additions & 5 deletions modules/apps/27-interchain-accounts/keeper/grpc_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"

"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
Expand All @@ -23,11 +24,9 @@ func (k Keeper) InterchainAccountAddress(ctx context.Context, req *types.QueryIn
return nil, status.Error(codes.InvalidArgument, "counterparty portID cannot be empty")
}

sdkCtx := sdk.UnwrapSDKContext(ctx)

interchainAccountAddress, err := k.GetInterchainAccountAddress(sdkCtx, req.CounterpartyPortId)
if err != nil {
return nil, err
interchainAccountAddress, found := k.GetInterchainAccountAddress(sdk.UnwrapSDKContext(ctx), req.CounterpartyPortId)
if !found {
return nil, status.Error(codes.NotFound, sdkerrors.Wrap(types.ErrInterchainAccountNotFound, req.CounterpartyPortId).Error())
}

return &types.QueryInterchainAccountAddressResponse{InterchainAccountAddress: interchainAccountAddress}, nil
Expand Down
1 change: 0 additions & 1 deletion modules/apps/27-interchain-accounts/keeper/handshake.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,6 @@ func (k Keeper) OnChanOpenConfirm(
portID,
channelID string,
) error {

return nil
}

Expand Down
18 changes: 18 additions & 0 deletions modules/apps/27-interchain-accounts/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,3 +149,21 @@ func (k Keeper) IsActiveChannel(ctx sdk.Context, portId string) bool {
func (k Keeper) AuthenticateCapability(ctx sdk.Context, cap *capabilitytypes.Capability, name string) bool {
return k.scopedKeeper.AuthenticateCapability(ctx, cap, name)
}

// GetInterchainAccountAddress retrieves the InterchainAccount address from the store keyed by the provided portID
func (k Keeper) GetInterchainAccountAddress(ctx sdk.Context, portID string) (string, bool) {
store := ctx.KVStore(k.storeKey)
key := types.KeyOwnerAccount(portID)

if !store.Has(key) {
return "", false
}

return string(store.Get(key)), true
}

// SetInterchainAccountAddress stores the InterchainAccount address, keyed by the associated portID
func (k Keeper) SetInterchainAccountAddress(ctx sdk.Context, portID string, address string) {
store := ctx.KVStore(k.storeKey)
store.Set(types.KeyOwnerAccount(portID), []byte(address))
}
51 changes: 51 additions & 0 deletions modules/apps/27-interchain-accounts/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package keeper_test
import (
"testing"

authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
"github.com/stretchr/testify/suite"

"github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types"
Expand Down Expand Up @@ -40,6 +41,27 @@ func NewICAPath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path {
return path
}

// SetupICAPath invokes the InterchainAccounts entrypoint and subsequent channel handshake handlers
func SetupICAPath(path *ibctesting.Path, owner string) error {
if err := InitInterchainAccount(path.EndpointA, owner); err != nil {
return err
}

if err := path.EndpointB.ChanOpenTry(); err != nil {
return err
}

if err := path.EndpointA.ChanOpenAck(); err != nil {
return err
}

if err := path.EndpointB.ChanOpenConfirm(); err != nil {
return err
}

return nil
}

// InitInterchainAccount is a helper function for starting the channel handshake
// TODO: parse identifiers from events
func InitInterchainAccount(endpoint *ibctesting.Endpoint, owner string) error {
Expand Down Expand Up @@ -76,3 +98,32 @@ func (suite *KeeperTestSuite) TestGetPort() {
port := suite.chainA.GetSimApp().ICAKeeper.GetPort(suite.chainA.GetContext())
suite.Require().Equal(types.PortID, port)
}

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

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

counterpartyPortID := path.EndpointA.ChannelConfig.PortID
expectedAddr := authtypes.NewBaseAccountWithAddress(types.GenerateAddress(counterpartyPortID)).GetAddress()

retrievedAddr, found := suite.chainB.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainB.GetContext(), counterpartyPortID)
suite.Require().True(found)
suite.Require().Equal(expectedAddr.String(), retrievedAddr)

retrievedAddr, found = suite.chainA.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainA.GetContext(), "invalid port")
suite.Require().False(found)
suite.Require().Empty(retrievedAddr)
}

func (suite *KeeperTestSuite) TestSetInterchainAccountAddress() {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
expectedAddr, portID := "address", "port"
suite.chainA.GetSimApp().ICAKeeper.SetInterchainAccountAddress(suite.chainA.GetContext(), portID, expectedAddr)

retrievedAddr, found := suite.chainA.GetSimApp().ICAKeeper.GetInterchainAccountAddress(suite.chainA.GetContext(), portID)
suite.Require().True(found)
suite.Require().Equal(expectedAddr, retrievedAddr)
}
4 changes: 2 additions & 2 deletions modules/apps/27-interchain-accounts/keeper/relay.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,8 @@ func (k Keeper) AuthenticateTx(ctx sdk.Context, msgs []sdk.Msg, portId string) e
}
}

interchainAccountAddr, err := k.GetInterchainAccountAddress(ctx, portId)
if err != nil {
interchainAccountAddr, found := k.GetInterchainAccountAddress(ctx, portId)
if !found {
return sdkerrors.ErrUnauthorized
}

Expand Down