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

chore: set up IBCTestStakingKeeper interface #2028

Merged
merged 13 commits into from
Aug 23, 2022
Merged

chore: set up IBCTestStakingKeeper interface #2028

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c278d65
initial interface definition
charleenfei Aug 17, 2022
4e374f5
wip
charleenfei Aug 18, 2022
62916e1
update simapp
charleenfei Aug 18, 2022
5c5aa52
Merge branch 'main' into charly/update_testing_stakingkeeper_interface
charleenfei Aug 18, 2022
3673054
Merge branch 'main' into charly/update_testing_stakingkeeper_interface
charleenfei Aug 19, 2022
46f0480
Merge branch 'main' into charly/update_testing_stakingkeeper_interface
charleenfei Aug 22, 2022
bd27422
Merge branch 'main' into charly/update_testing_stakingkeeper_interface
charleenfei Aug 22, 2022
d1d88da
updates re: comments
charleenfei Aug 22, 2022
92f44ea
Merge branch 'main' into charly/update_testing_stakingkeeper_interface
charleenfei Aug 22, 2022
b27f8e5
update godocs
charleenfei Aug 22, 2022
ae50357
Merge branch 'main' into charly/update_testing_stakingkeeper_interface
charleenfei Aug 22, 2022
eea436e
Merge branch 'main' into charly/update_testing_stakingkeeper_interface
charleenfei Aug 22, 2022
782e84d
Merge branch 'main' into charly/update_testing_stakingkeeper_interface
charleenfei Aug 23, 2022
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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 •
edited
Loading

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
View file
Open in desktop
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
View file
Open in desktop
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)
}