From fbed89d709bf6da8323e851f9565ee12830e3295 Mon Sep 17 00:00:00 2001 From: stana-ethernal Date: Fri, 23 Sep 2022 08:47:15 +0200 Subject: [PATCH 1/4] Democracy governance whitelisting #284 --- .../ante/forbidden_proposals_ante.go | 32 +++++ .../ante/forbidden_proposals_ante_test.go | 97 ++++++++++++++ app/consumer-democracy/ante_handler.go | 2 + app/consumer-democracy/app.go | 9 +- .../proposals_whitelisting.go | 68 ++++++++++ tests/e2e/democracy_test.go | 123 ++++++++++++++++++ x/ccv/democracy/governance/doc.go | 11 ++ x/ccv/democracy/governance/module.go | 69 ++++++++++ 8 files changed, 407 insertions(+), 4 deletions(-) create mode 100644 app/consumer-democracy/ante/forbidden_proposals_ante.go create mode 100644 app/consumer-democracy/ante/forbidden_proposals_ante_test.go create mode 100644 app/consumer-democracy/proposals_whitelisting.go create mode 100644 x/ccv/democracy/governance/doc.go create mode 100644 x/ccv/democracy/governance/module.go diff --git a/app/consumer-democracy/ante/forbidden_proposals_ante.go b/app/consumer-democracy/ante/forbidden_proposals_ante.go new file mode 100644 index 0000000000..1f5c5671fd --- /dev/null +++ b/app/consumer-democracy/ante/forbidden_proposals_ante.go @@ -0,0 +1,32 @@ +package ante + +import ( + "fmt" + + sdk "github.com/cosmos/cosmos-sdk/types" + govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" +) + +type ForbiddenProposalsDecorator struct { + IsProposalWhitelisted func(govtypes.Content) bool +} + +func NewForbiddenProposalsDecorator(whiteListFn func(govtypes.Content) bool) ForbiddenProposalsDecorator { + return ForbiddenProposalsDecorator{IsProposalWhitelisted: whiteListFn} +} + +func (decorator ForbiddenProposalsDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) { + currHeight := ctx.BlockHeight() + + for _, msg := range tx.GetMsgs() { + submitProposalMgs, ok := msg.(*govtypes.MsgSubmitProposal) + //if the message is MsgSubmitProposal, check if proposal is whitelisted + if ok { + if !decorator.IsProposalWhitelisted(submitProposalMgs.GetContent()) { + return ctx, fmt.Errorf("tx contains unsupported proposal message types at height %d", currHeight) + } + } + } + + return next(ctx, tx, simulate) +} diff --git a/app/consumer-democracy/ante/forbidden_proposals_ante_test.go b/app/consumer-democracy/ante/forbidden_proposals_ante_test.go new file mode 100644 index 0000000000..2912a4aa5d --- /dev/null +++ b/app/consumer-democracy/ante/forbidden_proposals_ante_test.go @@ -0,0 +1,97 @@ +package ante_test + +import ( + "testing" + + sdk "github.com/cosmos/cosmos-sdk/types" + authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" + banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" + govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" + "github.com/cosmos/cosmos-sdk/x/params/types/proposal" + app "github.com/cosmos/interchain-security/app/consumer-democracy" + "github.com/cosmos/interchain-security/app/consumer-democracy/ante" + "github.com/stretchr/testify/require" + "github.com/tendermint/spm/cosmoscmd" +) + +func TestForbiddenProposalsDecorator(t *testing.T) { + txCfg := cosmoscmd.MakeEncodingConfig(app.ModuleBasics).TxConfig + + testCases := []struct { + name string + ctx sdk.Context + msgs []sdk.Msg + expectErr bool + }{ + { + name: "Allowed param change", + ctx: sdk.Context{}, + msgs: []sdk.Msg{ + newParamChangeProposalMsg([]proposal.ParamChange{ + //only subspace and key are relevant for testing + {Subspace: banktypes.ModuleName, Key: "SendEnabled", Value: ""}, + }), + }, + expectErr: false, + }, + { + name: "Forbidden param change", + ctx: sdk.Context{}, + msgs: []sdk.Msg{ + newParamChangeProposalMsg([]proposal.ParamChange{ + {Subspace: authtypes.ModuleName, Key: "MaxMemoCharacters", Value: ""}, + }), + }, + expectErr: true, + }, + { + name: "Allowed and forbidden param changes in the same msg", + ctx: sdk.Context{}, + msgs: []sdk.Msg{ + newParamChangeProposalMsg([]proposal.ParamChange{ + {Subspace: banktypes.ModuleName, Key: "SendEnabled", Value: ""}, + {Subspace: authtypes.ModuleName, Key: "MaxMemoCharacters", Value: ""}, + }), + }, + expectErr: true, + }, + { + name: "Allowed and forbidden param changes in different msg", + ctx: sdk.Context{}, + msgs: []sdk.Msg{ + newParamChangeProposalMsg([]proposal.ParamChange{ + {Subspace: banktypes.ModuleName, Key: "SendEnabled", Value: ""}, + }), + newParamChangeProposalMsg([]proposal.ParamChange{ + {Subspace: authtypes.ModuleName, Key: "MaxMemoCharacters", Value: ""}, + }), + }, + expectErr: true, + }, + } + + for _, tc := range testCases { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + handler := ante.NewForbiddenProposalsDecorator(app.IsProposalWhitelisted) + + txBuilder := txCfg.NewTxBuilder() + require.NoError(t, txBuilder.SetMsgs(tc.msgs...)) + + _, err := handler.AnteHandle(tc.ctx, txBuilder.GetTx(), false, + func(ctx sdk.Context, _ sdk.Tx, _ bool) (sdk.Context, error) { return ctx, nil }) + if tc.expectErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} + +func newParamChangeProposalMsg(changes []proposal.ParamChange) *govtypes.MsgSubmitProposal { + paramChange := proposal.ParameterChangeProposal{Changes: changes} + msg, _ := govtypes.NewMsgSubmitProposal(¶mChange, sdk.NewCoins(), sdk.AccAddress{}) + return msg +} diff --git a/app/consumer-democracy/ante_handler.go b/app/consumer-democracy/ante_handler.go index 5e282ae06a..efe984faf0 100644 --- a/app/consumer-democracy/ante_handler.go +++ b/app/consumer-democracy/ante_handler.go @@ -6,6 +6,7 @@ import ( "github.com/cosmos/cosmos-sdk/x/auth/ante" ibcante "github.com/cosmos/ibc-go/v3/modules/core/ante" ibckeeper "github.com/cosmos/ibc-go/v3/modules/core/keeper" + democracyante "github.com/cosmos/interchain-security/app/consumer-democracy/ante" consumerante "github.com/cosmos/interchain-security/app/consumer/ante" ibcconsumerkeeper "github.com/cosmos/interchain-security/x/ccv/consumer/keeper" ) @@ -40,6 +41,7 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) { ante.NewRejectExtensionOptionsDecorator(), consumerante.NewMsgFilterDecorator(options.ConsumerKeeper), consumerante.NewDisabledModulesDecorator("/cosmos.evidence", "/cosmos.slashing"), + democracyante.NewForbiddenProposalsDecorator(IsProposalWhitelisted), ante.NewMempoolFeeDecorator(), ante.NewValidateBasicDecorator(), ante.NewTxTimeoutHeightDecorator(), diff --git a/app/consumer-democracy/app.go b/app/consumer-democracy/app.go index 1492244ce0..515d325d2d 100644 --- a/app/consumer-democracy/app.go +++ b/app/consumer-democracy/app.go @@ -90,9 +90,10 @@ import ( ccvstakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" ccvstaking "github.com/cosmos/interchain-security/x/ccv/democracy/staking" - ccvgov "github.com/cosmos/cosmos-sdk/x/gov" + gov "github.com/cosmos/cosmos-sdk/x/gov" ccvgovkeeper "github.com/cosmos/cosmos-sdk/x/gov/keeper" ccvgovtypes "github.com/cosmos/cosmos-sdk/x/gov/types" + ccvgov "github.com/cosmos/interchain-security/x/ccv/democracy/governance" // add mint ccvmint "github.com/cosmos/cosmos-sdk/x/mint" @@ -129,7 +130,7 @@ var ( ccvstaking.AppModuleBasic{}, ccvmint.AppModuleBasic{}, ccvdistr.AppModuleBasic{}, - ccvgov.NewAppModuleBasic( + gov.NewAppModuleBasic( // TODO: eventually remove upgrade proposal handler and cancel proposal handler paramsclient.ProposalHandler, ccvdistrclient.ProposalHandler, upgradeclient.ProposalHandler, upgradeclient.CancelProposalHandler, ), @@ -477,7 +478,7 @@ func New( capability.NewAppModule(appCodec, *app.CapabilityKeeper), crisis.NewAppModule(&app.CrisisKeeper, skipGenesisInvariants), feegrantmodule.NewAppModule(appCodec, app.AccountKeeper, app.BankKeeper, app.FeeGrantKeeper, app.interfaceRegistry), - ccvgov.NewAppModule(appCodec, app.GovKeeper, app.AccountKeeper, app.BankKeeper), + ccvgov.NewAppModule(appCodec, app.GovKeeper, app.AccountKeeper, app.BankKeeper, IsProposalWhitelisted), ccvmint.NewAppModule(appCodec, app.MintKeeper, app.AccountKeeper), slashing.NewAppModule(appCodec, app.SlashingKeeper, app.AccountKeeper, app.BankKeeper, app.ConsumerKeeper), ccvdistr.NewAppModule(appCodec, app.DistrKeeper, app.AccountKeeper, app.BankKeeper, app.StakingKeeper, authtypes.FeeCollectorName), @@ -580,7 +581,7 @@ func New( bank.NewAppModule(appCodec, app.BankKeeper, app.AccountKeeper), capability.NewAppModule(appCodec, *app.CapabilityKeeper), feegrantmodule.NewAppModule(appCodec, app.AccountKeeper, app.BankKeeper, app.FeeGrantKeeper, app.interfaceRegistry), - ccvgov.NewAppModule(appCodec, app.GovKeeper, app.AccountKeeper, app.BankKeeper), + ccvgov.NewAppModule(appCodec, app.GovKeeper, app.AccountKeeper, app.BankKeeper, IsProposalWhitelisted), ccvmint.NewAppModule(appCodec, app.MintKeeper, app.AccountKeeper), ccvstaking.NewAppModule(appCodec, app.StakingKeeper, app.AccountKeeper, app.BankKeeper), ccvdistr.NewAppModule(appCodec, app.DistrKeeper, app.AccountKeeper, app.BankKeeper, app.StakingKeeper, authtypes.FeeCollectorName), diff --git a/app/consumer-democracy/proposals_whitelisting.go b/app/consumer-democracy/proposals_whitelisting.go new file mode 100644 index 0000000000..718631515b --- /dev/null +++ b/app/consumer-democracy/proposals_whitelisting.go @@ -0,0 +1,68 @@ +package app + +import ( + banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" + distrtypes "github.com/cosmos/cosmos-sdk/x/distribution/types" + govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" + minttypes "github.com/cosmos/cosmos-sdk/x/mint/types" + "github.com/cosmos/cosmos-sdk/x/params/types/proposal" + stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" + ibctransfertypes "github.com/cosmos/ibc-go/v3/modules/apps/transfer/types" +) + +func IsProposalWhitelisted(content govtypes.Content) bool { + + switch c := content.(type) { + case *proposal.ParameterChangeProposal: + return isParamProposalWhitelisted(c.Changes) + + default: + return false + } + +} + +func isParamProposalWhitelisted(paramChanges []proposal.ParamChange) bool { + for _, paramChange := range paramChanges { + _, found := WhitelistedParams[paramChangeKey{Subspace: paramChange.Subspace, Key: paramChange.Key}] + if !found { + return false + } + } + return true +} + +type paramChangeKey struct { + Subspace, Key string +} + +var WhitelistedParams = map[paramChangeKey]struct{}{ + //bank + {Subspace: banktypes.ModuleName, Key: "SendEnabled"}: {}, + //governance + {Subspace: govtypes.ModuleName, Key: "depositparams"}: {}, //min_deposit, max_deposit_period + {Subspace: govtypes.ModuleName, Key: "votingparams"}: {}, //voting_period + {Subspace: govtypes.ModuleName, Key: "tallyparams"}: {}, //quorum,threshold,veto_threshold + //staking + {Subspace: stakingtypes.ModuleName, Key: "UnbondingTime"}: {}, + {Subspace: stakingtypes.ModuleName, Key: "MaxValidators"}: {}, + {Subspace: stakingtypes.ModuleName, Key: "MaxEntries"}: {}, + {Subspace: stakingtypes.ModuleName, Key: "HistoricalEntries"}: {}, + {Subspace: stakingtypes.ModuleName, Key: "BondDenom"}: {}, + //distribution + {Subspace: distrtypes.ModuleName, Key: "communitytax"}: {}, + {Subspace: distrtypes.ModuleName, Key: "baseproposerreward"}: {}, + {Subspace: distrtypes.ModuleName, Key: "bonusproposerreward"}: {}, + {Subspace: distrtypes.ModuleName, Key: "withdrawaddrenabled"}: {}, + //mint + {Subspace: minttypes.ModuleName, Key: "MintDenom"}: {}, + {Subspace: minttypes.ModuleName, Key: "InflationRateChange"}: {}, + {Subspace: minttypes.ModuleName, Key: "InflationMax"}: {}, + {Subspace: minttypes.ModuleName, Key: "InflationMin"}: {}, + {Subspace: minttypes.ModuleName, Key: "GoalBonded"}: {}, + {Subspace: minttypes.ModuleName, Key: "BlocksPerYear"}: {}, + //ibc transfer + {Subspace: ibctransfertypes.ModuleName, Key: "SendEnabled"}: {}, + {Subspace: ibctransfertypes.ModuleName, Key: "ReceiveEnabled"}: {}, + //TODO: interchain accounts are listed in spec but app does not include the module - check if something should be done +} diff --git a/tests/e2e/democracy_test.go b/tests/e2e/democracy_test.go index f399e04627..da48cf13ff 100644 --- a/tests/e2e/democracy_test.go +++ b/tests/e2e/democracy_test.go @@ -2,7 +2,10 @@ package e2e_test import ( "bytes" + "fmt" + "strconv" "testing" + "time" sdk "github.com/cosmos/cosmos-sdk/types" transfertypes "github.com/cosmos/ibc-go/v3/modules/apps/transfer/types" @@ -11,6 +14,13 @@ import ( channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" ibctesting "github.com/cosmos/ibc-go/v3/testing" + authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" + bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper" + govkeeper "github.com/cosmos/cosmos-sdk/x/gov/keeper" + govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" + minttypes "github.com/cosmos/cosmos-sdk/x/mint/types" + "github.com/cosmos/cosmos-sdk/x/params/types/proposal" + proposaltypes "github.com/cosmos/cosmos-sdk/x/params/types/proposal" appConsumer "github.com/cosmos/interchain-security/app/consumer-democracy" appProvider "github.com/cosmos/interchain-security/app/provider" "github.com/cosmos/interchain-security/testutil/simapp" @@ -253,6 +263,119 @@ func (s *ConsumerDemocracyTestSuite) TestDemocracyRewarsDistribution() { } } +func (s *ConsumerDemocracyTestSuite) TestDemocracyGovernanceWhitelisting() { + govKeeper := s.consumerChain.App.(*appConsumer.App).GovKeeper + stakingKeeper := s.consumerChain.App.(*appConsumer.App).StakingKeeper + bankKeeper := s.consumerChain.App.(*appConsumer.App).BankKeeper + authKeeper := s.consumerChain.App.(*appConsumer.App).AccountKeeper + mintKeeper := s.consumerChain.App.(*appConsumer.App).MintKeeper + newAuthParamValue := uint64(128) + newMintParamValue := sdk.NewDecWithPrec(1, 1) // "0.100000000000000000" + allowedChange := proposal.ParamChange{Subspace: minttypes.ModuleName, Key: "InflationMax", Value: fmt.Sprintf("\"%s\"", newMintParamValue)} + forbiddenChange := proposal.ParamChange{Subspace: authtypes.ModuleName, Key: "MaxMemoCharacters", Value: fmt.Sprintf("\"%s\"", strconv.FormatUint(newAuthParamValue, 10))} + votingAccounts := s.consumerChain.SenderAccounts + bondDenom := stakingKeeper.BondDenom(s.consumerCtx()) + depositAmount := govKeeper.GetDepositParams(s.consumerCtx()).MinDeposit + votingParams := govKeeper.GetVotingParams(s.consumerCtx()) + votingParams.VotingPeriod = 3 * time.Second + govKeeper.SetVotingParams(s.consumerCtx(), votingParams) + s.consumerChain.NextBlock() + votersOldBalances := getAccountsBalances(s.consumerCtx(), bankKeeper, bondDenom, votingAccounts) + + //submit proposal with forbidden and allowed changes + paramChange := proposaltypes.ParameterChangeProposal{Changes: []proposaltypes.ParamChange{allowedChange, forbiddenChange}} + err := submitProposalWithDepositAndVote(govKeeper, s.consumerCtx(), paramChange, votingAccounts, depositAmount) + s.Assert().NoError(err) + //set current header time to be equal or later than voting end time in order to process proposal from active queue, + //once the proposal is added to the chain + s.consumerChain.CurrentHeader.Time = s.consumerChain.CurrentHeader.Time.Add(votingParams.VotingPeriod) + s.consumerChain.NextBlock() + //at this moment, proposal is added, but not yet executed. we are saving old param values for comparison + oldAuthParamValue := authKeeper.GetParams(s.consumerCtx()).MaxMemoCharacters + oldMintParamValue := mintKeeper.GetParams(s.consumerCtx()).InflationMax + s.consumerChain.NextBlock() + //at this moment, proposal is executed or deleted if forbidden + currentAuthParamValue := authKeeper.GetParams(s.consumerCtx()).MaxMemoCharacters + currentMintParamValue := mintKeeper.GetParams(s.consumerCtx()).InflationMax + //check that parameters are not changed, since the proposal contained both forbidden and allowed changes + s.Assert().Equal(oldAuthParamValue, currentAuthParamValue) + s.Assert().NotEqual(newAuthParamValue, currentAuthParamValue) + s.Assert().Equal(oldMintParamValue, currentMintParamValue) + s.Assert().NotEqual(newMintParamValue, currentMintParamValue) + //deposit is refunded + s.Assert().Equal(votersOldBalances, getAccountsBalances(s.consumerCtx(), bankKeeper, bondDenom, votingAccounts)) + + //submit proposal with allowed changes + paramChange = proposaltypes.ParameterChangeProposal{Changes: []proposaltypes.ParamChange{allowedChange}} + err = submitProposalWithDepositAndVote(govKeeper, s.consumerCtx(), paramChange, votingAccounts, depositAmount) + s.Assert().NoError(err) + s.consumerChain.CurrentHeader.Time = s.consumerChain.CurrentHeader.Time.Add(votingParams.VotingPeriod) + s.consumerChain.NextBlock() + oldMintParamValue = mintKeeper.GetParams(s.consumerCtx()).InflationMax + s.consumerChain.NextBlock() + currentMintParamValue = mintKeeper.GetParams(s.consumerCtx()).InflationMax + //check that parameters are changed, since the proposal contained only allowed changes + s.Assert().Equal(newMintParamValue, currentMintParamValue) + s.Assert().NotEqual(oldMintParamValue, currentMintParamValue) + //deposit is refunded + s.Assert().Equal(votersOldBalances, getAccountsBalances(s.consumerCtx(), bankKeeper, bondDenom, votingAccounts)) + + //submit proposal with forbidden changes + paramChange = proposaltypes.ParameterChangeProposal{Changes: []proposaltypes.ParamChange{forbiddenChange}} + err = submitProposalWithDepositAndVote(govKeeper, s.consumerCtx(), paramChange, votingAccounts, depositAmount) + s.Assert().NoError(err) + s.consumerChain.CurrentHeader.Time = s.consumerChain.CurrentHeader.Time.Add(votingParams.VotingPeriod) + s.consumerChain.NextBlock() + oldAuthParamValue = authKeeper.GetParams(s.consumerCtx()).MaxMemoCharacters + s.consumerChain.NextBlock() + currentAuthParamValue = authKeeper.GetParams(s.consumerCtx()).MaxMemoCharacters + //check that parameters are not changed, since the proposal contained forbidden changes + s.Assert().Equal(oldAuthParamValue, currentAuthParamValue) + s.Assert().NotEqual(newAuthParamValue, currentAuthParamValue) + //deposit is refunded + s.Assert().Equal(votersOldBalances, getAccountsBalances(s.consumerCtx(), bankKeeper, bondDenom, votingAccounts)) +} + +func (s *ConsumerDemocracyTestSuite) TestDemocracyGovernanceWhitelistingKeys() { + paramKeeper := s.consumerChain.App.(*appConsumer.App).ParamsKeeper + for paramKey := range appConsumer.WhitelistedParams { + ss, ok := paramKeeper.GetSubspace(paramKey.Subspace) + s.Assert().True(ok) + hasKey := ss.Has(s.consumerCtx(), []byte(paramKey.Key)) + s.Assert().True(hasKey, "Invalid key %s for subspace %s", paramKey.Key, paramKey.Subspace) + } +} + +func submitProposalWithDepositAndVote(govKeeper govkeeper.Keeper, ctx sdk.Context, paramChange proposaltypes.ParameterChangeProposal, + accounts []ibctesting.SenderAccount, depositAmount sdk.Coins) error { + proposal, err := govKeeper.SubmitProposal(ctx, ¶mChange) + if err != nil { + return err + } + _, err = govKeeper.AddDeposit(ctx, proposal.ProposalId, accounts[0].SenderAccount.GetAddress(), depositAmount) //proposal becomes active + if err != nil { + return err + } + + for _, account := range accounts { + err = govKeeper.AddVote(ctx, proposal.ProposalId, account.SenderAccount.GetAddress(), govtypes.NewNonSplitVoteOption(govtypes.OptionYes)) + if err != nil { + return err + } + } + return nil +} + +func getAccountsBalances(ctx sdk.Context, bankKeeper bankkeeper.Keeper, bondDenom string, accounts []ibctesting.SenderAccount) map[string]sdk.Int { + accountsBalances := map[string]sdk.Int{} + for _, acc := range accounts { + accountsBalances[string(acc.SenderAccount.GetAddress())] = + bankKeeper.GetBalance(ctx, acc.SenderAccount.GetAddress(), bondDenom).Amount + } + + return accountsBalances +} + func (s *ConsumerDemocracyTestSuite) providerCtx() sdk.Context { return s.providerChain.GetContext() } diff --git a/x/ccv/democracy/governance/doc.go b/x/ccv/democracy/governance/doc.go new file mode 100644 index 0000000000..3b920e95e6 --- /dev/null +++ b/x/ccv/democracy/governance/doc.go @@ -0,0 +1,11 @@ +/* +Package governance defines a "wrapper" module around the Cosmos SDK's native +x/governance module. In other words, it provides the exact same functionality as +the native module in that it simply embeds the native module. However, it +overrides `EndBlock` core method to remove forbidden governance proposals from the +storage(s) before the original `EndBlock` method from the native module is called. + +The consumer chain should utilize the x/ccv/democracy/governance module to perform democratic +actions such as participating and voting within the chain's governance system. +*/ +package governance diff --git a/x/ccv/democracy/governance/module.go b/x/ccv/democracy/governance/module.go new file mode 100644 index 0000000000..253faa5a91 --- /dev/null +++ b/x/ccv/democracy/governance/module.go @@ -0,0 +1,69 @@ +package governance + +import ( + "github.com/cosmos/cosmos-sdk/codec" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/types/module" + + gov "github.com/cosmos/cosmos-sdk/x/gov" + "github.com/cosmos/cosmos-sdk/x/gov/keeper" + govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" + abci "github.com/tendermint/tendermint/abci/types" +) + +var ( + _ module.AppModule = AppModule{} + _ module.AppModuleSimulation = AppModule{} +) + +// AppModule embeds the Cosmos SDK's x/governance AppModule +type AppModule struct { + // embed the Cosmos SDK's x/governance AppModule + gov.AppModule + + keeper keeper.Keeper + isProposalWhitelisted func(govtypes.Content) bool +} + +// NewAppModule creates a new AppModule object using the native x/governance module AppModule constructor. +func NewAppModule(cdc codec.Codec, keeper keeper.Keeper, ak govtypes.AccountKeeper, bk govtypes.BankKeeper, isProposalWhitelisted func(govtypes.Content) bool) AppModule { + govAppModule := gov.NewAppModule(cdc, keeper, ak, bk) + return AppModule{ + AppModule: govAppModule, + keeper: keeper, + isProposalWhitelisted: isProposalWhitelisted, + } +} + +func (am AppModule) EndBlock(ctx sdk.Context, request abci.RequestEndBlock) []abci.ValidatorUpdate { + + am.keeper.IterateInactiveProposalsQueue(ctx, ctx.BlockHeader().Time, func(proposal govtypes.Proposal) bool { + //if there are forbidden proposals in inactive proposals queue, refund deposit and delete proposal from all storages + deleteProposalAndRefundDeposit(ctx, am, proposal, false) + return false + }) + + am.keeper.IterateActiveProposalsQueue(ctx, ctx.BlockHeader().Time, func(proposal govtypes.Proposal) bool { + //if there are forbidden proposals in active proposals queue, refund deposit, delete votes for that proposal + //and delete proposal from all storages + deleteProposalAndRefundDeposit(ctx, am, proposal, true) + return false + }) + + return am.AppModule.EndBlock(ctx, request) +} + +func deleteProposalAndRefundDeposit(ctx sdk.Context, am AppModule, proposal govtypes.Proposal, isActive bool) { + if !am.isProposalWhitelisted(proposal.GetContent()) { + //if the proposal is active, delete the votes related to it + if isActive { + //Tally's return result won't be used in decision if the tokens will be burned or refunded (they are always refunded), but + //this function needs to be called to delete the votes related to the given proposal, since the deleteVote function is + // private and cannot be called directly from the overridden app module + + am.keeper.Tally(ctx, proposal) + } + am.keeper.DeleteProposal(ctx, proposal.ProposalId) + am.keeper.RefundDeposits(ctx, proposal.ProposalId) + } +} From afc88487a519273ec3898f6978a3044c3d4f80b9 Mon Sep 17 00:00:00 2001 From: stana-ethernal Date: Mon, 26 Sep 2022 12:33:22 +0200 Subject: [PATCH 2/4] Democracy gov whitelist - added logs and event --- x/ccv/democracy/governance/module.go | 52 ++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 14 deletions(-) diff --git a/x/ccv/democracy/governance/module.go b/x/ccv/democracy/governance/module.go index 253faa5a91..2f1a098574 100644 --- a/x/ccv/democracy/governance/module.go +++ b/x/ccv/democracy/governance/module.go @@ -1,6 +1,8 @@ package governance import ( + "fmt" + "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/module" @@ -11,6 +13,10 @@ import ( abci "github.com/tendermint/tendermint/abci/types" ) +const ( + AttributeValueProposalForbidden = "proposal_forbidden" +) + var ( _ module.AppModule = AppModule{} _ module.AppModuleSimulation = AppModule{} @@ -39,31 +45,49 @@ func (am AppModule) EndBlock(ctx sdk.Context, request abci.RequestEndBlock) []ab am.keeper.IterateInactiveProposalsQueue(ctx, ctx.BlockHeader().Time, func(proposal govtypes.Proposal) bool { //if there are forbidden proposals in inactive proposals queue, refund deposit and delete proposal from all storages - deleteProposalAndRefundDeposit(ctx, am, proposal, false) + deleteForbiddenProposal(ctx, am, proposal, false) return false }) am.keeper.IterateActiveProposalsQueue(ctx, ctx.BlockHeader().Time, func(proposal govtypes.Proposal) bool { //if there are forbidden proposals in active proposals queue, refund deposit, delete votes for that proposal //and delete proposal from all storages - deleteProposalAndRefundDeposit(ctx, am, proposal, true) + deleteForbiddenProposal(ctx, am, proposal, true) return false }) return am.AppModule.EndBlock(ctx, request) } -func deleteProposalAndRefundDeposit(ctx sdk.Context, am AppModule, proposal govtypes.Proposal, isActive bool) { - if !am.isProposalWhitelisted(proposal.GetContent()) { - //if the proposal is active, delete the votes related to it - if isActive { - //Tally's return result won't be used in decision if the tokens will be burned or refunded (they are always refunded), but - //this function needs to be called to delete the votes related to the given proposal, since the deleteVote function is - // private and cannot be called directly from the overridden app module - - am.keeper.Tally(ctx, proposal) - } - am.keeper.DeleteProposal(ctx, proposal.ProposalId) - am.keeper.RefundDeposits(ctx, proposal.ProposalId) +func deleteForbiddenProposal(ctx sdk.Context, am AppModule, proposal govtypes.Proposal, isActive bool) { + if am.isProposalWhitelisted(proposal.GetContent()) { + return } + + eventType := govtypes.EventTypeInactiveProposal + //if the proposal is active, delete the votes related to it + if isActive { + //Tally's return result won't be used in decision if the tokens will be burned or refunded (they are always refunded), but + //this function needs to be called to delete the votes related to the given proposal, since the deleteVote function is + // private and cannot be called directly from the overridden app module + eventType = govtypes.EventTypeActiveProposal + am.keeper.Tally(ctx, proposal) + } + am.keeper.DeleteProposal(ctx, proposal.ProposalId) + am.keeper.RefundDeposits(ctx, proposal.ProposalId) + + ctx.EventManager().EmitEvent( + sdk.NewEvent( + eventType, + sdk.NewAttribute(govtypes.AttributeKeyProposalID, fmt.Sprintf("%d", proposal.ProposalId)), + sdk.NewAttribute(govtypes.AttributeKeyProposalResult, AttributeValueProposalForbidden), + ), + ) + + logger := am.keeper.Logger(ctx) + logger.Info( + "proposal is not whitelisted; deleted", + "proposal", proposal.ProposalId, + "title", proposal.GetTitle(), + "total_deposit", proposal.TotalDeposit.String()) } From 9bd25043db486c286141e7e8ec3c48b7a72b675f Mon Sep 17 00:00:00 2001 From: stana-ethernal Date: Thu, 29 Sep 2022 18:31:44 +0200 Subject: [PATCH 3/4] Gov whitelisting - delete only active forbidden prop. --- x/ccv/democracy/governance/module.go | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/x/ccv/democracy/governance/module.go b/x/ccv/democracy/governance/module.go index 2f1a098574..4e7fe7b4a5 100644 --- a/x/ccv/democracy/governance/module.go +++ b/x/ccv/democracy/governance/module.go @@ -43,42 +43,33 @@ func NewAppModule(cdc codec.Codec, keeper keeper.Keeper, ak govtypes.AccountKeep func (am AppModule) EndBlock(ctx sdk.Context, request abci.RequestEndBlock) []abci.ValidatorUpdate { - am.keeper.IterateInactiveProposalsQueue(ctx, ctx.BlockHeader().Time, func(proposal govtypes.Proposal) bool { - //if there are forbidden proposals in inactive proposals queue, refund deposit and delete proposal from all storages - deleteForbiddenProposal(ctx, am, proposal, false) - return false - }) - am.keeper.IterateActiveProposalsQueue(ctx, ctx.BlockHeader().Time, func(proposal govtypes.Proposal) bool { //if there are forbidden proposals in active proposals queue, refund deposit, delete votes for that proposal //and delete proposal from all storages - deleteForbiddenProposal(ctx, am, proposal, true) + deleteForbiddenProposal(ctx, am, proposal) return false }) return am.AppModule.EndBlock(ctx, request) } -func deleteForbiddenProposal(ctx sdk.Context, am AppModule, proposal govtypes.Proposal, isActive bool) { +func deleteForbiddenProposal(ctx sdk.Context, am AppModule, proposal govtypes.Proposal) { if am.isProposalWhitelisted(proposal.GetContent()) { return } - eventType := govtypes.EventTypeInactiveProposal - //if the proposal is active, delete the votes related to it - if isActive { - //Tally's return result won't be used in decision if the tokens will be burned or refunded (they are always refunded), but - //this function needs to be called to delete the votes related to the given proposal, since the deleteVote function is - // private and cannot be called directly from the overridden app module - eventType = govtypes.EventTypeActiveProposal - am.keeper.Tally(ctx, proposal) - } + //delete the votes related to the proposal calling Tally + //Tally's return result won't be used in decision if the tokens will be burned or refunded (they are always refunded), but + //this function needs to be called to delete the votes related to the given proposal, since the deleteVote function is + // private and cannot be called directly from the overridden app module + am.keeper.Tally(ctx, proposal) + am.keeper.DeleteProposal(ctx, proposal.ProposalId) am.keeper.RefundDeposits(ctx, proposal.ProposalId) ctx.EventManager().EmitEvent( sdk.NewEvent( - eventType, + govtypes.EventTypeActiveProposal, sdk.NewAttribute(govtypes.AttributeKeyProposalID, fmt.Sprintf("%d", proposal.ProposalId)), sdk.NewAttribute(govtypes.AttributeKeyProposalResult, AttributeValueProposalForbidden), ), From 6b086cd6cdff7653cbd8a3c724fca8063aa0143a Mon Sep 17 00:00:00 2001 From: stana-ethernal Date: Wed, 5 Oct 2022 10:36:27 +0200 Subject: [PATCH 4/4] Democracy gov whitelisting - test moved --- .../proposals_whitelisting.go | 6 +++--- .../proposals_whitelisting_test.go | 21 +++++++++++++++++++ tests/e2e/democracy_test.go | 10 --------- 3 files changed, 24 insertions(+), 13 deletions(-) create mode 100644 app/consumer-democracy/proposals_whitelisting_test.go diff --git a/app/consumer-democracy/proposals_whitelisting.go b/app/consumer-democracy/proposals_whitelisting.go index 718631515b..3519d8d7ac 100644 --- a/app/consumer-democracy/proposals_whitelisting.go +++ b/app/consumer-democracy/proposals_whitelisting.go @@ -14,7 +14,7 @@ func IsProposalWhitelisted(content govtypes.Content) bool { switch c := content.(type) { case *proposal.ParameterChangeProposal: - return isParamProposalWhitelisted(c.Changes) + return isParamChangeWhitelisted(c.Changes) default: return false @@ -22,7 +22,7 @@ func IsProposalWhitelisted(content govtypes.Content) bool { } -func isParamProposalWhitelisted(paramChanges []proposal.ParamChange) bool { +func isParamChangeWhitelisted(paramChanges []proposal.ParamChange) bool { for _, paramChange := range paramChanges { _, found := WhitelistedParams[paramChangeKey{Subspace: paramChange.Subspace, Key: paramChange.Key}] if !found { @@ -64,5 +64,5 @@ var WhitelistedParams = map[paramChangeKey]struct{}{ //ibc transfer {Subspace: ibctransfertypes.ModuleName, Key: "SendEnabled"}: {}, {Subspace: ibctransfertypes.ModuleName, Key: "ReceiveEnabled"}: {}, - //TODO: interchain accounts are listed in spec but app does not include the module - check if something should be done + //add interchain account params(HostEnabled, AllowedMessages) once the module is added to the consumer app } diff --git a/app/consumer-democracy/proposals_whitelisting_test.go b/app/consumer-democracy/proposals_whitelisting_test.go new file mode 100644 index 0000000000..a83fb686b5 --- /dev/null +++ b/app/consumer-democracy/proposals_whitelisting_test.go @@ -0,0 +1,21 @@ +package app_test + +import ( + "testing" + + ibctesting "github.com/cosmos/ibc-go/v3/testing" + appConsumer "github.com/cosmos/interchain-security/app/consumer-democracy" + simapp "github.com/cosmos/interchain-security/testutil/simapp" + "github.com/stretchr/testify/require" +) + +func TestDemocracyGovernanceWhitelistingKeys(t *testing.T) { + chain := ibctesting.NewTestChain(t, simapp.NewBasicCoordinator(t), simapp.SetupTestingAppConsumerDemocracy, "test") + paramKeeper := chain.App.(*appConsumer.App).ParamsKeeper + for paramKey := range appConsumer.WhitelistedParams { + ss, ok := paramKeeper.GetSubspace(paramKey.Subspace) + require.True(t, ok, "Unknown subspace %s", paramKey.Subspace) + hasKey := ss.Has(chain.GetContext(), []byte(paramKey.Key)) + require.True(t, hasKey, "Invalid key %s for subspace %s", paramKey.Key, paramKey.Subspace) + } +} diff --git a/tests/e2e/democracy_test.go b/tests/e2e/democracy_test.go index da48cf13ff..68445c618e 100644 --- a/tests/e2e/democracy_test.go +++ b/tests/e2e/democracy_test.go @@ -336,16 +336,6 @@ func (s *ConsumerDemocracyTestSuite) TestDemocracyGovernanceWhitelisting() { s.Assert().Equal(votersOldBalances, getAccountsBalances(s.consumerCtx(), bankKeeper, bondDenom, votingAccounts)) } -func (s *ConsumerDemocracyTestSuite) TestDemocracyGovernanceWhitelistingKeys() { - paramKeeper := s.consumerChain.App.(*appConsumer.App).ParamsKeeper - for paramKey := range appConsumer.WhitelistedParams { - ss, ok := paramKeeper.GetSubspace(paramKey.Subspace) - s.Assert().True(ok) - hasKey := ss.Has(s.consumerCtx(), []byte(paramKey.Key)) - s.Assert().True(hasKey, "Invalid key %s for subspace %s", paramKey.Key, paramKey.Subspace) - } -} - func submitProposalWithDepositAndVote(govKeeper govkeeper.Keeper, ctx sdk.Context, paramChange proposaltypes.ParameterChangeProposal, accounts []ibctesting.SenderAccount, depositAmount sdk.Coins) error { proposal, err := govKeeper.SubmitProposal(ctx, ¶mChange)