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

chore: set up IBCTestStakingKeeper interface #2028

Merged
merged 13 commits into from
Aug 23, 2022
Merged
Show file tree
Hide file tree
Changes from 6 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
2 changes: 1 addition & 1 deletion testing/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ type TestingApp interface {

// ibc-go additions
GetBaseApp() *baseapp.BaseApp
GetStakingKeeper() stakingkeeper.Keeper
GetIBCTestStakingKeeper() ibctestingtypes.StakingKeeper
Copy link
Contributor

Choose a reason for hiding this comment

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

I honestly liked the name GetStakingKeeper, since the name of the type already indicates that this is for testing, so I consider that is redundant to add Test in the name of the function... But no problem if we decide to rename it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i renamed on request from @ValarDragon in the issue, i don't have a particularly strong opinion either way tho. I can see the value add in being explicit, but it is true that the type is also quite informative.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think now that the interface returns the ibctestingtypes.StakingKeeper it can be redundant. I do agree GetStakingKeeper is cleaner, but I understand this function will be implemented in app.go for a production chain.

@ValarDragon do you still prefer GetIBCTestStakingKeeper given the new return value

GetIBCKeeper() *keeper.Keeper
GetScopedIBCKeeper() capabilitykeeper.ScopedKeeper
GetTxConfig() client.TxConfig
Expand Down
4 changes: 2 additions & 2 deletions testing/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper"
stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
"github.com/stretchr/testify/require"
abci "github.com/tendermint/tendermint/abci/types"
Expand All @@ -27,6 +26,7 @@ import (

"github.com/cosmos/ibc-go/v5/modules/core/keeper"
"github.com/cosmos/ibc-go/v5/testing/simapp"
ibctestingtypes "github.com/cosmos/ibc-go/v5/testing/types"
)

var DefaultTestingAppInit = SetupTestingApp
Expand All @@ -36,7 +36,7 @@ type TestingApp interface {

// ibc-go additions
GetBaseApp() *baseapp.BaseApp
GetStakingKeeper() stakingkeeper.Keeper
GetIBCTestStakingKeeper() ibctestingtypes.StakingKeeper
GetIBCKeeper() *keeper.Keeper
GetScopedIBCKeeper() capabilitykeeper.ScopedKeeper
GetTxConfig() client.TxConfig
Expand Down
2 changes: 1 addition & 1 deletion testing/chain.go
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ func (chain *TestChain) GetConsensusState(clientID string, height exported.Heigh
// GetValsAtHeight will return the validator set of the chain at a given height. It will return
// a success boolean depending on if the validator set exists or not at that height.
func (chain *TestChain) GetValsAtHeight(height int64) (*tmtypes.ValidatorSet, bool) {
histInfo, ok := chain.App.GetStakingKeeper().GetHistoricalInfo(chain.GetContext(), height)
histInfo, ok := chain.App.GetIBCTestStakingKeeper().GetHistoricalInfo(chain.GetContext(), height)
if !ok {
return nil, false
}
Expand Down
6 changes: 3 additions & 3 deletions testing/chain_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@ func TestChangeValSet(t *testing.T) {
amount2, ok := sdk.NewIntFromString("30000000000000000000")
require.True(t, ok)

val := chainA.App.GetStakingKeeper().GetValidators(chainA.GetContext(), 4)
val := chainA.GetSimApp().StakingKeeper.GetValidators(chainA.GetContext(), 4)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it not possible to keep using App.GetIBCTestStakingKeeper() and add GetValidators and Delegate to the StakingKeeper interface?

Copy link
Contributor Author

@charleenfei charleenfei Aug 19, 2022

Choose a reason for hiding this comment

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

i think the idea is that in our simApp, we would continue using the staking module's staking keeper so we don't actually need to add these two methods as they are already there.

it's just that in the testing chain and testing app setup, we would like to enable osmosis to swap out their customised interfluid staking keeper so they can use our testing setup. this is why we only need to add that one method which is used in the testing/chain.go file to the new interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually requested those functions to not be added. I think we should keep our interfaces as small as possible (ideally we wouldn't even need the staking keeper interface)


chainA.App.GetStakingKeeper().Delegate(chainA.GetContext(), chainA.SenderAccounts[1].SenderAccount.GetAddress(),
chainA.GetSimApp().StakingKeeper.Delegate(chainA.GetContext(), chainA.SenderAccounts[1].SenderAccount.GetAddress(),
amount, types.Unbonded, val[1], true)
chainA.App.GetStakingKeeper().Delegate(chainA.GetContext(), chainA.SenderAccounts[3].SenderAccount.GetAddress(),
chainA.GetSimApp().StakingKeeper.Delegate(chainA.GetContext(), chainA.SenderAccounts[3].SenderAccount.GetAddress(),
amount2, types.Unbonded, val[3], true)

coord.CommitBlock(chainA)
Expand Down
3 changes: 2 additions & 1 deletion testing/simapp/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ import (
ibckeeper "github.com/cosmos/ibc-go/v5/modules/core/keeper"
ibcmock "github.com/cosmos/ibc-go/v5/testing/mock"
simappparams "github.com/cosmos/ibc-go/v5/testing/simapp/params"
ibctestingtypes "github.com/cosmos/ibc-go/v5/testing/types"
)

const appName = "SimApp"
Expand Down Expand Up @@ -748,7 +749,7 @@ func (app *SimApp) GetBaseApp() *baseapp.BaseApp {
}

// GetStakingKeeper implements the TestingApp interface.
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
func (app *SimApp) GetStakingKeeper() stakingkeeper.Keeper {
func (app *SimApp) GetIBCTestStakingKeeper() ibctestingtypes.StakingKeeper {
return app.StakingKeeper
}

Expand Down
12 changes: 12 additions & 0 deletions testing/types/expected_keepers.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package types
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

import (
sdk "github.com/cosmos/cosmos-sdk/types"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
)

// IBCTestingStakingKeeper defines the expected staking keeper interface used in the
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
// IBC testing package
type StakingKeeper interface {
GetHistoricalInfo(ctx sdk.Context, height int64) (stakingtypes.HistoricalInfo, bool)
}