Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

feat: add empty address check on authority field #4713

Merged
merged 3 commits into from
Sep 19, 2023
Merged

feat: add empty address check on authority field #4713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ca96134
feat: add empty address check on authority field
colin-axner Sep 19, 2023
1a9d1bc
Merge branch 'main' into colin/4667-empty-authority
colin-axner Sep 19, 2023
0cb0532
Merge branch 'main' into colin/4667-empty-authority
colin-axner Sep 19, 2023
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
4 changes: 4 additions & 0 deletions modules/apps/27-interchain-accounts/controller/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ func NewKeeper(
legacySubspace = legacySubspace.WithKeyTable(types.ParamKeyTable())
}

if strings.TrimSpace(authority) == "" {
panic(fmt.Errorf("authority must be non-empty"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Was it really needed to use Errorf, since the string does not require formatting?

}

return Keeper{
storeKey: key,
cdc: cdc,
Expand Down
53 changes: 53 additions & 0 deletions modules/apps/27-interchain-accounts/controller/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"

"github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/keeper"
"github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types"
genesistypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/genesis/types"
icatypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/types"
Expand Down Expand Up @@ -107,6 +108,58 @@ func TestKeeperTestSuite(t *testing.T) {
testifysuite.Run(t, new(KeeperTestSuite))
}

func (suite *KeeperTestSuite) TestNewKeeper() {
testCases := []struct {
name string
instantiateFn func()
expPass bool
}{
{"success", func() {
keeper.NewKeeper(
suite.chainA.GetSimApp().AppCodec(),
suite.chainA.GetSimApp().GetKey(types.StoreKey),
suite.chainA.GetSimApp().GetSubspace(types.SubModuleName),
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
&suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
suite.chainA.GetSimApp().ScopedICAControllerKeeper,
suite.chainA.GetSimApp().MsgServiceRouter(),
suite.chainA.GetSimApp().ICAControllerKeeper.GetAuthority(),
)
}, true},
{"failure: empty authority", func() {
keeper.NewKeeper(
suite.chainA.GetSimApp().AppCodec(),
suite.chainA.GetSimApp().GetKey(types.StoreKey),
suite.chainA.GetSimApp().GetSubspace(types.SubModuleName),
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
&suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
suite.chainA.GetSimApp().ScopedICAControllerKeeper,
suite.chainA.GetSimApp().MsgServiceRouter(),
"", // authority
)
}, false},
}

for _, tc := range testCases {
tc := tc
suite.SetupTest()

suite.Run(tc.name, func() {
if tc.expPass {
suite.Require().NotPanics(
tc.instantiateFn,
)
} else {
suite.Require().Panics(
tc.instantiateFn,
)
}
})
}
}

func (suite *KeeperTestSuite) TestGetAllPorts() {
suite.SetupTest()

Expand Down
4 changes: 4 additions & 0 deletions modules/apps/27-interchain-accounts/host/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ func NewKeeper(
legacySubspace = legacySubspace.WithKeyTable(types.ParamKeyTable())
}

if strings.TrimSpace(authority) == "" {
panic(fmt.Errorf("authority must be non-empty"))
}

return Keeper{
storeKey: key,
cdc: cdc,
Expand Down
71 changes: 71 additions & 0 deletions modules/apps/27-interchain-accounts/host/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ import (

testifysuite "github.com/stretchr/testify/suite"

authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper"

genesistypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/genesis/types"
"github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/keeper"
"github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/types"
icatypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/types"
ibcfeekeeper "github.com/cosmos/ibc-go/v8/modules/apps/29-fee/keeper"
Expand Down Expand Up @@ -127,6 +130,74 @@ func TestKeeperTestSuite(t *testing.T) {
testifysuite.Run(t, new(KeeperTestSuite))
}

func (suite *KeeperTestSuite) TestNewKeeper() {
testCases := []struct {
name string
instantiateFn func()
expPass bool
}{
{"success", func() {
keeper.NewKeeper(
suite.chainA.GetSimApp().AppCodec(),
suite.chainA.GetSimApp().GetKey(types.StoreKey),
suite.chainA.GetSimApp().GetSubspace(types.SubModuleName),
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
&suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
suite.chainA.GetSimApp().AccountKeeper,
suite.chainA.GetSimApp().ScopedICAHostKeeper,
suite.chainA.GetSimApp().MsgServiceRouter(),
suite.chainA.GetSimApp().ICAHostKeeper.GetAuthority(),
)
}, true},
{"failure: interchain accounts module account does not exist", func() {
keeper.NewKeeper(
suite.chainA.GetSimApp().AppCodec(),
suite.chainA.GetSimApp().GetKey(types.StoreKey),
suite.chainA.GetSimApp().GetSubspace(types.SubModuleName),
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
&suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
authkeeper.AccountKeeper{}, // empty account keeper
suite.chainA.GetSimApp().ScopedICAHostKeeper,
suite.chainA.GetSimApp().MsgServiceRouter(),
suite.chainA.GetSimApp().ICAHostKeeper.GetAuthority(),
)
}, false},
{"failure: empty mock staking keeper", func() {
keeper.NewKeeper(
suite.chainA.GetSimApp().AppCodec(),
suite.chainA.GetSimApp().GetKey(types.StoreKey),
suite.chainA.GetSimApp().GetSubspace(types.SubModuleName),
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
&suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
suite.chainA.GetSimApp().AccountKeeper,
suite.chainA.GetSimApp().ScopedICAHostKeeper,
suite.chainA.GetSimApp().MsgServiceRouter(),
"", // authority
)
}, false},
}

for _, tc := range testCases {
tc := tc
suite.SetupTest()

suite.Run(tc.name, func() {
if tc.expPass {
suite.Require().NotPanics(
tc.instantiateFn,
)
} else {
suite.Require().Panics(
tc.instantiateFn,
)
}
})
}
}

func (suite *KeeperTestSuite) TestGetInterchainAccountAddress() {
suite.SetupTest()

Expand Down
4 changes: 4 additions & 0 deletions modules/apps/transfer/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ func NewKeeper(
legacySubspace = legacySubspace.WithKeyTable(types.ParamKeyTable())
}

if strings.TrimSpace(authority) == "" {
panic(fmt.Errorf("authority must be non-empty"))
}

return Keeper{
cdc: cdc,
storeKey: key,
Expand Down
70 changes: 70 additions & 0 deletions modules/apps/transfer/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ import (
"github.com/cosmos/cosmos-sdk/baseapp"
"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper"

"github.com/cosmos/ibc-go/v8/modules/apps/transfer/keeper"
"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
channelkeeper "github.com/cosmos/ibc-go/v8/modules/core/04-channel/keeper"
ibctesting "github.com/cosmos/ibc-go/v8/testing"
Expand Down Expand Up @@ -43,6 +45,74 @@ func TestKeeperTestSuite(t *testing.T) {
testifysuite.Run(t, new(KeeperTestSuite))
}

func (suite *KeeperTestSuite) TestNewKeeper() {
testCases := []struct {
name string
instantiateFn func()
expPass bool
}{
{"success", func() {
keeper.NewKeeper(
suite.chainA.GetSimApp().AppCodec(),
suite.chainA.GetSimApp().GetKey(types.StoreKey),
suite.chainA.GetSimApp().GetSubspace(types.ModuleName),
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
&suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
suite.chainA.GetSimApp().AccountKeeper,
suite.chainA.GetSimApp().BankKeeper,
suite.chainA.GetSimApp().ScopedTransferKeeper,
suite.chainA.GetSimApp().ICAControllerKeeper.GetAuthority(),
)
}, true},
{"failure: transfer module account does not exist", func() {
keeper.NewKeeper(
suite.chainA.GetSimApp().AppCodec(),
suite.chainA.GetSimApp().GetKey(types.StoreKey),
suite.chainA.GetSimApp().GetSubspace(types.ModuleName),
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
&suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
authkeeper.AccountKeeper{}, // empty account keeper
suite.chainA.GetSimApp().BankKeeper,
suite.chainA.GetSimApp().ScopedTransferKeeper,
suite.chainA.GetSimApp().ICAControllerKeeper.GetAuthority(),
)
}, false},
{"failure: empty authority", func() {
keeper.NewKeeper(
suite.chainA.GetSimApp().AppCodec(),
suite.chainA.GetSimApp().GetKey(types.StoreKey),
suite.chainA.GetSimApp().GetSubspace(types.ModuleName),
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
suite.chainA.GetSimApp().IBCKeeper.ChannelKeeper,
&suite.chainA.GetSimApp().IBCKeeper.PortKeeper,
suite.chainA.GetSimApp().AccountKeeper,
suite.chainA.GetSimApp().BankKeeper,
suite.chainA.GetSimApp().ScopedTransferKeeper,
"", // authority
)
}, false},
}

for _, tc := range testCases {
tc := tc
suite.SetupTest()

suite.Run(tc.name, func() {
if tc.expPass {
suite.Require().NotPanics(
tc.instantiateFn,
)
} else {
suite.Require().Panics(
tc.instantiateFn,
)
}
})
}
}

func (suite *KeeperTestSuite) TestSetGetTotalEscrowForDenom() {
const denom = "atom"
var expAmount sdkmath.Int
Expand Down
5 changes: 5 additions & 0 deletions modules/core/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package keeper
import (
"fmt"
"reflect"
"strings"

storetypes "cosmossdk.io/store/types"

Expand Down Expand Up @@ -64,6 +65,10 @@ func NewKeeper(
panic(fmt.Errorf("cannot initialize IBC keeper: empty scoped keeper"))
}

if strings.TrimSpace(authority) == "" {
panic(fmt.Errorf("authority must be non-empty"))
}

clientKeeper := clientkeeper.NewKeeper(cdc, key, paramSpace, stakingKeeper, upgradeKeeper)
connectionKeeper := connectionkeeper.NewKeeper(cdc, key, paramSpace, clientKeeper)
portKeeper := portkeeper.NewKeeper(scopedKeeper)
Expand Down
38 changes: 27 additions & 11 deletions modules/core/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,7 @@ func (suite *KeeperTestSuite) TestNewKeeper() {
stakingKeeper clienttypes.StakingKeeper
upgradeKeeper clienttypes.UpgradeKeeper
scopedKeeper capabilitykeeper.ScopedKeeper
newIBCKeeperFn = func() {
ibckeeper.NewKeeper(
suite.chainA.GetSimApp().AppCodec(),
suite.chainA.GetSimApp().GetKey(ibcexported.StoreKey),
suite.chainA.GetSimApp().GetSubspace(ibcexported.ModuleName),
stakingKeeper,
upgradeKeeper,
scopedKeeper,
suite.chainA.App.GetIBCKeeper().GetAuthority(),
)
}
newIBCKeeperFn func()
)

testCases := []struct {
Expand Down Expand Up @@ -113,6 +103,19 @@ func (suite *KeeperTestSuite) TestNewKeeper() {

scopedKeeper = emptyScopedKeeper
}, false},
{"failure: empty authority", func() {
newIBCKeeperFn = func() {
ibckeeper.NewKeeper(
suite.chainA.GetSimApp().AppCodec(),
suite.chainA.GetSimApp().GetKey(ibcexported.StoreKey),
suite.chainA.GetSimApp().GetSubspace(ibcexported.ModuleName),
stakingKeeper,
upgradeKeeper,
scopedKeeper,
"", // authority
)
}
}, false},
{"success: replace stakingKeeper with non-empty MockStakingKeeper", func() {
// use a different implementation of clienttypes.StakingKeeper
mockStakingKeeper := MockStakingKeeper{"not empty"}
Expand All @@ -126,6 +129,19 @@ func (suite *KeeperTestSuite) TestNewKeeper() {
suite.SetupTest()

suite.Run(tc.name, func() {
// set default behaviour
newIBCKeeperFn = func() {
ibckeeper.NewKeeper(
suite.chainA.GetSimApp().AppCodec(),
suite.chainA.GetSimApp().GetKey(ibcexported.StoreKey),
suite.chainA.GetSimApp().GetSubspace(ibcexported.ModuleName),
stakingKeeper,
upgradeKeeper,
scopedKeeper,
suite.chainA.App.GetIBCKeeper().GetAuthority(),
)
}

stakingKeeper = suite.chainA.GetSimApp().StakingKeeper
upgradeKeeper = suite.chainA.GetSimApp().UpgradeKeeper
scopedKeeper = suite.chainA.GetSimApp().ScopedIBCKeeper
Expand Down
Loading