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

test: account/keeper tests for ICA #420

Merged
merged 9 commits into from
Sep 24, 2021
13 changes: 0 additions & 13 deletions modules/apps/27-interchain-accounts/keeper/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,16 +58,3 @@ func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, portID string) {
k.accountKeeper.SetAccount(ctx, interchainAccount)
k.SetInterchainAccountAddress(ctx, portID, interchainAccount.Address)
}

func (k Keeper) GetInterchainAccount(ctx sdk.Context, addr sdk.AccAddress) (types.InterchainAccount, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't used anywhere. Some leftover code from the previous implementation I think.

acc := k.accountKeeper.GetAccount(ctx, addr)
if acc == nil {
return types.InterchainAccount{}, sdkerrors.Wrap(types.ErrInterchainAccountNotFound, "there is no account")
}

interchainAccount, ok := acc.(*types.InterchainAccount)
if !ok {
return types.InterchainAccount{}, sdkerrors.Wrap(types.ErrInterchainAccountNotFound, "account is not an interchain account")
}
return *interchainAccount, nil
}
6 changes: 5 additions & 1 deletion modules/apps/27-interchain-accounts/keeper/account_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ func (suite *KeeperTestSuite) TestInitInterchainAccount() {
malleate func()
expPass bool
}{

{
"success", func() {}, true,
},
Expand All @@ -31,6 +30,11 @@ func (suite *KeeperTestSuite) TestInitInterchainAccount() {
}, false,
},
*/
{
"fails to generate port-id", func() {
owner = ""
}, false,
},
{
"MsgChanOpenInit fails - channel is already active", func() {
portID, err := types.GeneratePortID(owner, path.EndpointA.ConnectionID, path.EndpointB.ConnectionID)
Expand Down
20 changes: 0 additions & 20 deletions modules/apps/27-interchain-accounts/keeper/handshake.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,23 +112,3 @@ func (k Keeper) OnChanOpenConfirm(
) error {
return nil
}

// May want to use these for re-opening a channel when it is closed
//// OnChanCloseInit implements the IBCModule interface
//func (am AppModule) OnChanCloseInit(
// ctx sdk.Context,
// portID,
// channelID string,
//) error {
// // Disallow user-initiated channel closing for transfer channels
// return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "user cannot close channel")
//}

//// OnChanCloseConfirm implements the IBCModule interface
//func (am AppModule) OnChanCloseConfirm(
// ctx sdk.Context,
// portID,
// channelID string,
//) error {
// return nil
//}
14 changes: 14 additions & 0 deletions modules/apps/27-interchain-accounts/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,20 @@ func (suite *KeeperTestSuite) TestGetInterchainAccountAddress() {
suite.Require().Empty(retrievedAddr)
}

func (suite *KeeperTestSuite) TestIsActiveChannel() {
suite.SetupTest() // reset
path := NewICAPath(suite.chainA, suite.chainB)
owner := "owner"
suite.coordinator.SetupConnections(path)

err := suite.SetupICAPath(path, owner)
suite.Require().NoError(err)
portID := path.EndpointA.ChannelConfig.PortID

isActive := suite.chainA.GetSimApp().ICAKeeper.IsActiveChannel(suite.chainA.GetContext(), portID)
suite.Require().Equal(isActive, true)
}

func (suite *KeeperTestSuite) TestSetInterchainAccountAddress() {
expectedAddr, portID := "address", "port"
suite.chainA.GetSimApp().ICAKeeper.SetInterchainAccountAddress(suite.chainA.GetContext(), portID, expectedAddr)
Expand Down
19 changes: 11 additions & 8 deletions modules/apps/27-interchain-accounts/types/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,12 @@ import (
"fmt"
"strings"

yaml "gopkg.in/yaml.v2"

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

connectiontypes "github.com/cosmos/ibc-go/modules/core/03-connection/types"
)
Expand Down Expand Up @@ -66,20 +65,24 @@ func NewInterchainAccount(ba *authtypes.BaseAccount, accountOwner string) *Inter
}

// SetPubKey - Implements AccountI
func (InterchainAccount) SetPubKey(pubKey crypto.PubKey) error {
func (ia InterchainAccount) SetPubKey(pubKey crypto.PubKey) error {
return fmt.Errorf("not supported for interchain accounts")
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto, is this what the SDK does for accounts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. This code was grandfathered in from the previous work on ICA. I assumed up to now that the override here for SetPubKey & SetSequence was necessary as an interchain account is controlled by another chains on-chain logic rather than a private key. Perhaps this isn't necessary though? Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets revisit during the internal audit. I'm wondering how this will interact with module accounts (if we even need this type)

}

// SetSequence - Implements AccountI
func (InterchainAccount) SetSequence(seq uint64) error {
func (ia InterchainAccount) SetSequence(seq uint64) error {
return fmt.Errorf("not supported for interchain accounts")
}

func (ia InterchainAccount) Validate() error {
if strings.TrimSpace(ia.AccountOwner) == "" {
return fmt.Errorf("AccountOwner cannot be empty")
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

return ia.BaseAccount.Validate()
}

type ibcAccountPretty struct {
type interchainAccountPretty struct {
Address sdk.AccAddress `json:"address" yaml:"address"`
Copy link
Member

Choose a reason for hiding this comment

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

This field Address has the same json tag as AccountOwner below, maybe we should update the json tag for AccountOwner as they're both using json:"address"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Updated.

PubKey string `json:"public_key" yaml:"public_key"`
AccountNumber uint64 `json:"account_number" yaml:"account_number"`
Expand All @@ -99,7 +102,7 @@ func (ia InterchainAccount) MarshalYAML() (interface{}, error) {
return nil, err
}

bs, err := yaml.Marshal(ibcAccountPretty{
bs, err := yaml.Marshal(interchainAccountPretty{
Address: accAddr,
PubKey: "",
AccountNumber: ia.AccountNumber,
Expand All @@ -121,7 +124,7 @@ func (ia InterchainAccount) MarshalJSON() ([]byte, error) {
return nil, err
}

return json.Marshal(ibcAccountPretty{
return json.Marshal(interchainAccountPretty{
Address: accAddr,
PubKey: "",
AccountNumber: ia.AccountNumber,
Expand All @@ -132,7 +135,7 @@ func (ia InterchainAccount) MarshalJSON() ([]byte, error) {

// UnmarshalJSON unmarshals raw JSON bytes into a ModuleAccount.
func (ia *InterchainAccount) UnmarshalJSON(bz []byte) error {
var alias ibcAccountPretty
var alias interchainAccountPretty
if err := json.Unmarshal(bz, &alias); err != nil {
return err
}
Expand Down
89 changes: 88 additions & 1 deletion modules/apps/27-interchain-accounts/types/account_test.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,18 @@
package types_test

import (
"encoding/json"
"testing"

"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
sdk "github.com/cosmos/cosmos-sdk/types"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
"github.com/stretchr/testify/suite"
"gopkg.in/yaml.v2"

"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"
)

type TypesTestSuite struct {
Expand Down Expand Up @@ -88,3 +94,84 @@ func (suite *TypesTestSuite) TestGeneratePortID() {
})
}
}

func (suite *TypesTestSuite) TestInterchainAccount() {
pubkey := secp256k1.GenPrivKey().PubKey()
addr := sdk.AccAddress(pubkey.Address())
baseAcc := authtypes.NewBaseAccountWithAddress(addr)
interchainAcc := types.NewInterchainAccount(baseAcc, "account-owner-id")
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

// should fail when trying to set the public key or sequence of an interchain account
err := interchainAcc.SetPubKey(pubkey)
suite.Require().Error(err)
err = interchainAcc.SetSequence(1)
suite.Require().Error(err)
}

func (suite *TypesTestSuite) TestGenesisAccountValidate() {
pubkey := secp256k1.GenPrivKey().PubKey()
addr := sdk.AccAddress(pubkey.Address())
baseAcc := authtypes.NewBaseAccountWithAddress(addr)
pubkey = secp256k1.GenPrivKey().PubKey()
ownerAddr := sdk.AccAddress(pubkey.Address())

testCases := []struct {
name string
acc authtypes.GenesisAccount
expPass bool
}{
{
"interchain account with empty AccountOwner field",
types.NewInterchainAccount(baseAcc, ""),
false,
},
{
"success",
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
types.NewInterchainAccount(baseAcc, ownerAddr.String()),
true,
},
}

for _, tc := range testCases {
err := tc.acc.Validate()

if tc.expPass {
suite.Require().NoError(err)
} else {
suite.Require().Error(err)
}
}
}

func (suite *TypesTestSuite) TestInterchainAccountMarshalYAML() {
suite.SetupTest() // reset
addr, err := sdk.AccAddressFromHex("0000000000000000000000000000000000000000")
suite.Require().NoError(err)
ba := authtypes.NewBaseAccountWithAddress(addr)

interchainAcc := types.NewInterchainAccount(ba, "accountOwner")

bs, err := yaml.Marshal(interchainAcc)
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to test the MarshalYAML() func on InterchainAccount like you do for MarshalJSON below

I think the function signature of MarshalYAML() should also be updated.

From:

func (ia InterchainAccount) MarshalYAML() (interface{}, error)

To:

func (ia InterchainAccount) MarshalYAML() ([]byte, error)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this. For some reason, I needed to use InterchainAccountPretty struct for the YAML test but not for the json.

suite.Require().NoError(err)

want := "|\n address: cosmos1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqnrql8a\n public_key: \"\"\n account_number: 0\n sequence: 0\n account_owner: accountOwner\n"
suite.Require().Equal(want, string(bs))
}

func (suite *TypesTestSuite) TestInterchainAccountJSON() {
pubkey := secp256k1.GenPrivKey().PubKey()
addr := sdk.AccAddress(pubkey.Address())
baseAcc := authtypes.NewBaseAccountWithAddress(addr)

interchainAcc := types.NewInterchainAccount(baseAcc, "owner-address")

bz, err := json.Marshal(interchainAcc)
suite.Require().NoError(err)

bz1, err := interchainAcc.MarshalJSON()
suite.Require().NoError(err)
suite.Require().Equal(string(bz), string(bz1))

var a types.InterchainAccount
suite.Require().NoError(json.Unmarshal(bz, &a))
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}