From bdadedbc0ea8d54c84e0b69fe5abe779937b7b31 Mon Sep 17 00:00:00 2001 From: MSalopek Date: Wed, 11 Dec 2024 11:37:20 +0100 Subject: [PATCH] feat!: enable unjail on pre-ccv chains (#2396) * feat!: enable unjail on pre-ccv chains * docs: update docstrings * feat!: enable unjail related functions * docs: update docstrings * fix: update handling (changeover completed checks) * fix comment * tests: add unjail tests * tests: improve unjail tests * tests: improve unjail tests * tests: appease linter * Update testing documentation --------- Co-authored-by: github-actions --- scripts/test_doc/test_documentation.md | 4 ++- tests/integration/democracy.go | 46 ++++++++++++++++++++++++ tests/integration/unbonding.go | 11 ++++++ testutil/integration/debug_test.go | 8 +++++ x/ccv/consumer/keeper/changeover.go | 2 +- x/ccv/consumer/keeper/validators.go | 35 ++++++++++++------ x/ccv/consumer/keeper/validators_test.go | 7 ++++ 7 files changed, 100 insertions(+), 13 deletions(-) diff --git a/scripts/test_doc/test_documentation.md b/scripts/test_doc/test_documentation.md index d8cf18e62e..ac1085ecd0 100644 --- a/scripts/test_doc/test_documentation.md +++ b/scripts/test_doc/test_documentation.md @@ -15,6 +15,7 @@ |----------|-------------------| [TestDemocracyRewardsDistribution](../../tests/integration/democracy.go#L77) | TestDemocracyRewardsDistribution checks that rewards to democracy representatives, community pool, and provider redistribution account are done correctly.
Details* Set up a democracy consumer chain.
* Create a new block.
* Check that rewards to democracy representatives, community pool, and provider redistribution account are distributed in the right proportions.
| [TestDemocracyMsgUpdateParams](../../tests/integration/democracy.go#L187) | TestDemocracyMsgUpdateParams checks that the consumer parameters can be updated through a governance proposal.
Details* Set up a democracy consumer chain.
* Submit a proposal containing changes to the consumer module parameters.
* Check that the proposal is executed, and the parameters are updated.
| + [TestDemocracyValidatorUnjail](../../tests/integration/democracy.go#L243) | TestDemocracyValidatorUnjail checks that the consumer validator can be unjailed when there is a standalone staking keeper available.
Details* Set up a democracy consumer chain.
* Jail a validator.
* Check that the validator is jailed.
* Unjail the validator.
* Check that the validator is unjailed.
| # [distribution.go](../../tests/integration/distribution.go) @@ -127,7 +128,8 @@ | Function | Short Description | |----------|-------------------| - [TestUndelegationCompletion](../../tests/integration/unbonding.go#L16) | TestUndelegationCompletion tests that undelegations complete after the unbonding period elapses on the provider, regardless of the consumer's state
Details* Set up CCV channel.
* Perform initial delegation of tokens followed by a partial undelegation (1/4 of the tokens).
* Verify that the staking unbonding operation is created as expected.
* Increment provider block height.
* Check that the unbonding operation has been completed.
* Verify that the token balances are correctly updated and the expected amount of tokens has been returned to the account.
| + [TestUndelegationCompletion](../../tests/integration/unbonding.go#L17) | TestUndelegationCompletion tests that undelegations complete after the unbonding period elapses on the provider, regardless of the consumer's state
Details* Set up CCV channel.
* Perform initial delegation of tokens followed by a partial undelegation (1/4 of the tokens).
* Verify that the staking unbonding operation is created as expected.
* Increment provider block height.
* Check that the unbonding operation has been completed.
* Verify that the token balances are correctly updated and the expected amount of tokens has been returned to the account.
| + [TestConsumerUnjailNoOp](../../tests/integration/unbonding.go#L50) | TestConsumerUnjailNoOp check that consumerKeeper can call .Unjail() without error. This operation must only be available in case the app also implements a "standalone" staking keeper. | # [valset_update.go](../../tests/integration/valset_update.go) diff --git a/tests/integration/democracy.go b/tests/integration/democracy.go index 645cdb7805..993a0d1bec 100644 --- a/tests/integration/democracy.go +++ b/tests/integration/democracy.go @@ -233,6 +233,52 @@ func (s *ConsumerDemocracyTestSuite) TestDemocracyMsgUpdateParams() { s.Assert().Equal(votersOldBalances, getAccountsBalances(s.consumerCtx(), bankKeeper, bondDenom, votingAccounts)) } +// TestDemocracyValidatorUnjail checks that the consumer validator can be unjailed when there is a standalone staking keeper available. +// @Long Description@ +// * Set up a democracy consumer chain. +// * Jail a validator. +// * Check that the validator is jailed. +// * Unjail the validator. +// * Check that the validator is unjailed. +func (s *ConsumerDemocracyTestSuite) TestDemocracyValidatorUnjail() { + stakingKeeper := s.consumerApp.GetTestStakingKeeper() + consumerKeeper := s.consumerApp.GetConsumerKeeper() + + validators, err := stakingKeeper.GetAllValidators(s.consumerCtx()) + s.Require().NoError(err) + + // setting up pre-conditions + // validator[0] is expected to be jailed + expectJailed := validators[0] + consAddr, err := expectJailed.GetConsAddr() + s.Require().NoError(err) + stakingKeeper.GetValidatorSet().Jail(s.consumerCtx(), consAddr) + + s.consumerChain.NextBlock() + + validators, err = stakingKeeper.GetAllValidators(s.consumerCtx()) + s.Require().NoError(err) + for _, validator := range validators { + if validator.OperatorAddress == expectJailed.OperatorAddress { + s.Require().True(validator.IsJailed()) + } else { + s.Require().False(validator.IsJailed()) + } + } + + // confirm unjail will not error and properly unjail + // in case of a consumer chain without standalone staking the call is a no-op + err = consumerKeeper.Unjail(s.consumerCtx(), consAddr) + s.Require().NoError(err) + s.consumerChain.NextBlock() + + validators, err = stakingKeeper.GetAllValidators(s.consumerCtx()) + s.Require().NoError(err) + for _, validator := range validators { + s.Require().False(validator.IsJailed()) + } +} + func submitProposalWithDepositAndVote(govKeeper govkeeper.Keeper, ctx sdk.Context, msgs []sdk.Msg, accounts []ibctesting.SenderAccount, proposer sdk.AccAddress, depositAmount sdk.Coins, ) error { diff --git a/tests/integration/unbonding.go b/tests/integration/unbonding.go index 3f13d7cd1f..7727240eaa 100644 --- a/tests/integration/unbonding.go +++ b/tests/integration/unbonding.go @@ -2,6 +2,7 @@ package integration import ( "cosmossdk.io/math" + sdk "github.com/cosmos/cosmos-sdk/types" ) // TestUndelegationCompletion tests that undelegations complete after @@ -43,3 +44,13 @@ func (s *CCVTestSuite) TestUndelegationCompletion() { "unexpected initial balance after unbonding; test: %s", ) } + +// TestConsumerUnjailNoOp check that consumerKeeper can call .Unjail() without error. +// This operation must only be available in case the app also implements a "standalone" staking keeper. +func (s *CCVTestSuite) TestConsumerUnjailNoOp() { + consumerKeeper := s.consumerApp.GetConsumerKeeper() + + // this is a no-op + err := consumerKeeper.Unjail(s.consumerCtx(), sdk.ConsAddress([]byte{0x01, 0x02, 0x03})) + s.Require().NoError(err) +} diff --git a/testutil/integration/debug_test.go b/testutil/integration/debug_test.go index 6092298a15..5c076a9f48 100644 --- a/testutil/integration/debug_test.go +++ b/testutil/integration/debug_test.go @@ -61,6 +61,10 @@ func TestDemocracyMsgUpdateParams(t *testing.T) { runConsumerDemocracyTestByName(t, "TestDemocracyMsgUpdateParams") } +func TestDemocracyUnjail(t *testing.T) { + runConsumerDemocracyTestByName(t, "TestDemocracyValidatorUnjail") +} + // // Distribution tests // @@ -193,6 +197,10 @@ func TestUndelegationCompletion(t *testing.T) { runCCVTestByName(t, "TestUndelegationCompletion") } +func TestConsumerUnjailNoOp(t *testing.T) { + runCCVTestByName(t, "TestConsumerUnjailNoOp") +} + // // Val set update tests // diff --git a/x/ccv/consumer/keeper/changeover.go b/x/ccv/consumer/keeper/changeover.go index 7f7ba4ad5b..044266e64c 100644 --- a/x/ccv/consumer/keeper/changeover.go +++ b/x/ccv/consumer/keeper/changeover.go @@ -9,7 +9,7 @@ import ( // ChangeoverIsComplete returns whether the standalone to consumer changeover process is complete. func (k Keeper) ChangeoverIsComplete(ctx sdk.Context) bool { if !k.IsPrevStandaloneChain(ctx) { - panic("ChangeoverIsComplete should only be called on previously standalone consumers") + return true } return ctx.BlockHeight() >= k.FirstConsumerHeight(ctx) } diff --git a/x/ccv/consumer/keeper/validators.go b/x/ccv/consumer/keeper/validators.go index 4386bb7b86..3fce5441a1 100644 --- a/x/ccv/consumer/keeper/validators.go +++ b/x/ccv/consumer/keeper/validators.go @@ -84,9 +84,14 @@ func (k Keeper) IterateValidators(context.Context, func(index int64, validator s return nil } -// Validator - unimplemented on CCV keeper -func (k Keeper) Validator(ctx context.Context, addr sdk.ValAddress) (stakingtypes.ValidatorI, error) { - panic("unimplemented on CCV keeper") +// Validator - unimplemented on CCV keeper but implemented on standalone keeper +func (k Keeper) Validator(sdkCtx context.Context, addr sdk.ValAddress) (stakingtypes.ValidatorI, error) { + ctx := sdk.UnwrapSDKContext(sdkCtx) + if k.ChangeoverIsComplete(ctx) && k.standaloneStakingKeeper != nil { + return k.standaloneStakingKeeper.Validator(ctx, addr) + } + + return stakingtypes.Validator{}, errors.New("unimplemented on CCV keeper") } // IsJailed returns the outstanding slashing flag for the given validator address @@ -174,16 +179,24 @@ func (k Keeper) SlashWithInfractionReason(goCtx context.Context, addr sdk.ConsAd // the provider validator set will soon be in effect, and jailing is n/a. func (k Keeper) Jail(context.Context, sdk.ConsAddress) error { return nil } -// Unjail - unimplemented on CCV keeper +// Unjail is enabled for previously standalone chains and chains implementing democracy staking. // -// This method should be a no-op even during a standalone to consumer changeover. -// Once the upgrade has happened as a part of the changeover, -// the provider validator set will soon be in effect, and jailing is n/a. -func (k Keeper) Unjail(context.Context, sdk.ConsAddress) error { return nil } +// This method should be a no-op for consumer chains that launched with the CCV module first. +func (k Keeper) Unjail(sdkCtx context.Context, addr sdk.ConsAddress) error { + ctx := sdk.UnwrapSDKContext(sdkCtx) + if k.ChangeoverIsComplete(ctx) && k.standaloneStakingKeeper != nil { + return k.standaloneStakingKeeper.Unjail(ctx, addr) + } + return nil +} -// Delegation - unimplemented on CCV keeper -func (k Keeper) Delegation(ctx context.Context, addr sdk.AccAddress, valAddr sdk.ValAddress) (stakingtypes.DelegationI, error) { - panic("unimplemented on CCV keeper") +// Delegation - unimplemented on CCV keeper but implemented on standalone keeper +func (k Keeper) Delegation(sdkCtx context.Context, addr sdk.AccAddress, valAddr sdk.ValAddress) (stakingtypes.DelegationI, error) { + ctx := sdk.UnwrapSDKContext(sdkCtx) + if k.ChangeoverIsComplete(ctx) && k.standaloneStakingKeeper != nil { + return k.standaloneStakingKeeper.Delegation(ctx, addr, valAddr) + } + return stakingtypes.Delegation{}, errors.New("unimplemented on CCV keeper") } // MaxValidators - unimplemented on CCV keeper diff --git a/x/ccv/consumer/keeper/validators_test.go b/x/ccv/consumer/keeper/validators_test.go index 349a3b2b40..b24ef44dde 100644 --- a/x/ccv/consumer/keeper/validators_test.go +++ b/x/ccv/consumer/keeper/validators_test.go @@ -151,6 +151,13 @@ func TestIsValidatorJailed(t *testing.T) { isJailed3, err := consumerKeeper.IsValidatorJailed(ctx, consAddr) require.NoError(t, err) require.True(t, isJailed3) + + // confirm that unjail returns no error and validator remains jailed + mocks.MockStakingKeeper.EXPECT().IsValidatorJailed(ctx, consAddr).Return(true, nil).Times(1) + require.NoError(t, consumerKeeper.Unjail(ctx, consAddr)) + isJailed3, err = consumerKeeper.IsValidatorJailed(ctx, consAddr) + require.NoError(t, err) + require.True(t, isJailed3) } func TestSlash(t *testing.T) {