From 78c4a936c4161157c86956472d61e392a060a26d Mon Sep 17 00:00:00 2001 From: stana-ethernal Date: Mon, 4 Nov 2024 12:26:25 +0100 Subject: [PATCH] remove governance whitelisting --- .../ante/forbidden_proposals_ante.go | 57 ------ .../ante/forbidden_proposals_ante_test.go | 169 ------------------ app/consumer-democracy/ante_handler.go | 2 - app/consumer-democracy/app.go | 4 +- .../proposals_whitelisting.go | 55 ------ x/ccv/democracy/governance/doc.go | 11 -- x/ccv/democracy/governance/module.go | 165 ----------------- 7 files changed, 2 insertions(+), 461 deletions(-) delete mode 100644 app/consumer-democracy/ante/forbidden_proposals_ante.go delete mode 100644 app/consumer-democracy/ante/forbidden_proposals_ante_test.go delete mode 100644 app/consumer-democracy/proposals_whitelisting.go delete mode 100644 x/ccv/democracy/governance/doc.go delete 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 deleted file mode 100644 index 2856aa4e5b..0000000000 --- a/app/consumer-democracy/ante/forbidden_proposals_ante.go +++ /dev/null @@ -1,57 +0,0 @@ -package ante - -import ( - "fmt" - - sdk "github.com/cosmos/cosmos-sdk/types" - govv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" - govv1beta1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1" -) - -type ForbiddenProposalsDecorator struct { - isLegacyProposalWhitelisted func(govv1beta1.Content) bool - isModuleWhiteList func(string) bool -} - -func NewForbiddenProposalsDecorator( - whiteListFn func(govv1beta1.Content) bool, - isModuleWhiteList func(string) bool, -) ForbiddenProposalsDecorator { - return ForbiddenProposalsDecorator{ - isLegacyProposalWhitelisted: whiteListFn, - isModuleWhiteList: isModuleWhiteList, - } -} - -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() { - // if the message is MsgSubmitProposal, check if proposal is whitelisted - submitProposalMgs, ok := msg.(*govv1.MsgSubmitProposal) - if !ok { - continue - } - - messages := submitProposalMgs.GetMessages() - for _, message := range messages { - if sdkMsg, isLegacyProposal := message.GetCachedValue().(*govv1.MsgExecLegacyContent); isLegacyProposal { - // legacy gov proposal content - content, err := govv1.LegacyContentFromMessage(sdkMsg) - if err != nil { - return ctx, fmt.Errorf("tx contains invalid LegacyContent") - } - if !decorator.isLegacyProposalWhitelisted(content) { - return ctx, fmt.Errorf("tx contains unsupported proposal message types at height %d", currHeight) - } - continue - } - // not legacy gov proposal content and not whitelisted - if !decorator.isModuleWhiteList(message.TypeUrl) { - 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 deleted file mode 100644 index 58b8284b78..0000000000 --- a/app/consumer-democracy/ante/forbidden_proposals_ante_test.go +++ /dev/null @@ -1,169 +0,0 @@ -package ante_test - -import ( - "testing" - - ibctransfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types" - "github.com/stretchr/testify/require" - - sdk "github.com/cosmos/cosmos-sdk/types" - authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" - govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" - govv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" - minttypes "github.com/cosmos/cosmos-sdk/x/mint/types" - "github.com/cosmos/cosmos-sdk/x/params/types/proposal" - - app "github.com/cosmos/interchain-security/v6/app/consumer-democracy" - "github.com/cosmos/interchain-security/v6/app/consumer-democracy/ante" -) - -// in SDKv47 parameter updates full params object is required -// either all params can be updated or none can be updated -func TestForbiddenProposalsDecorator(t *testing.T) { - txCfg := app.MakeTestEncodingConfig().TxConfig - - // here we try to set whatever params exist to their default values - // the actual parameter setting is not important, what's being tested is the ante handle filter - // Note: mint params CAN be changed according to WhiteListModule in proposals_whitelisting.go - updateMintParams := &minttypes.MsgUpdateParams{ - Authority: authtypes.NewModuleAddress(govtypes.ModuleName).String(), - Params: minttypes.DefaultParams(), - } - - // Note: auth params CANNOT be changed according to WhiteListModule in proposals_whitelisting.go - updateAuthParams := &authtypes.MsgUpdateParams{ - Authority: authtypes.NewModuleAddress(govtypes.ModuleName).String(), - Params: authtypes.DefaultParams(), - } - - testCases := []struct { - name string - ctx sdk.Context - msgs []sdk.Msg - expectErr bool - }{ - { - name: "Allowed param change - mint module", - ctx: sdk.Context{}, - msgs: []sdk.Msg{ - newParamChangeProposalMsg([]sdk.Msg{updateMintParams}), - }, - expectErr: false, - }, - { - name: "Forbidden param change - auth module", - ctx: sdk.Context{}, - msgs: []sdk.Msg{ - newParamChangeProposalMsg([]sdk.Msg{updateAuthParams}), - }, - expectErr: true, - }, - { - name: "Allowed and forbidden param changes in the same msg", - ctx: sdk.Context{}, - msgs: []sdk.Msg{ - newParamChangeProposalMsg([]sdk.Msg{updateMintParams, updateAuthParams}), - }, - expectErr: true, - }, - { - name: "Allowed and forbidden param changes in different msg", - ctx: sdk.Context{}, - msgs: []sdk.Msg{ - newParamChangeProposalMsg([]sdk.Msg{updateMintParams}), - newParamChangeProposalMsg([]sdk.Msg{updateAuthParams}), - }, - expectErr: true, - }, - } - - for _, tc := range testCases { - tc := tc - - t.Run(tc.name, func(t *testing.T) { - handler := ante.NewForbiddenProposalsDecorator(app.IsProposalWhitelisted, app.IsModuleWhiteList) - - 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) - } - }) - } -} - -// Legacy parameter proposals are not supported in cosmos-sdk v0.50 -// since modules parameters were moved to their respective modules -// this test is to ensure that legacy parameter proposals are not allowed -func TestForbiddenLegacyProposalsDecorator(t *testing.T) { - txCfg := app.MakeTestEncodingConfig().TxConfig - - testCases := []struct { - name string - ctx sdk.Context - msgs []sdk.Msg - expectErr bool - }{ - { - name: "Forbidden param change", - ctx: sdk.Context{}, - msgs: []sdk.Msg{ - newLegacyParamChangeProposalMsg([]proposal.ParamChange{ - {Subspace: authtypes.ModuleName, Key: "MaxMemoCharacters", Value: ""}, - }), - }, - expectErr: true, - }, - { - name: "Multiple forbidden param changes in the same msg", - ctx: sdk.Context{}, - msgs: []sdk.Msg{ - newLegacyParamChangeProposalMsg([]proposal.ParamChange{ - {Subspace: ibctransfertypes.ModuleName, Key: "SendEnabled", Value: "true"}, - {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, app.IsModuleWhiteList) - - 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) - } - }) - } -} - -// Use ParamChangeProposal -func newLegacyParamChangeProposalMsg(changes []proposal.ParamChange) *govv1.MsgSubmitProposal { - paramChange := proposal.ParameterChangeProposal{Changes: changes} - msgContent, err := govv1.NewLegacyContent(¶mChange, authtypes.NewModuleAddress(govtypes.ModuleName).String()) - if err != nil { - return nil - } - msg, _ := govv1.NewMsgSubmitProposal([]sdk.Msg{msgContent}, sdk.NewCoins(), sdk.AccAddress{}.String(), "", "", "", false) - return msg -} - -func newParamChangeProposalMsg(msgs []sdk.Msg) *govv1.MsgSubmitProposal { - msg, _ := govv1.NewMsgSubmitProposal(msgs, sdk.NewCoins(), sdk.AccAddress{}.String(), "", "", "", false) - return msg -} diff --git a/app/consumer-democracy/ante_handler.go b/app/consumer-democracy/ante_handler.go index 42016627b7..4d7b788603 100644 --- a/app/consumer-democracy/ante_handler.go +++ b/app/consumer-democracy/ante_handler.go @@ -10,7 +10,6 @@ import ( sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/x/auth/ante" - democracyante "github.com/cosmos/interchain-security/v6/app/consumer-democracy/ante" consumerante "github.com/cosmos/interchain-security/v6/app/consumer/ante" ibcconsumerkeeper "github.com/cosmos/interchain-security/v6/x/ccv/consumer/keeper" ) @@ -45,7 +44,6 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) { ante.NewExtensionOptionsDecorator(nil), consumerante.NewMsgFilterDecorator(options.ConsumerKeeper), consumerante.NewDisabledModulesDecorator("/cosmos.evidence", "/cosmos.slashing"), - democracyante.NewForbiddenProposalsDecorator(IsProposalWhitelisted, IsModuleWhiteList), ante.NewValidateBasicDecorator(), ante.NewTxTimeoutHeightDecorator(), ante.NewValidateMemoDecorator(options.AccountKeeper), diff --git a/app/consumer-democracy/app.go b/app/consumer-democracy/app.go index cef5aa8f9c..646babd205 100644 --- a/app/consumer-democracy/app.go +++ b/app/consumer-democracy/app.go @@ -38,6 +38,7 @@ import ( "cosmossdk.io/x/feegrant" feegrantkeeper "cosmossdk.io/x/feegrant/keeper" feegrantmodule "cosmossdk.io/x/feegrant/module" + // add mint "cosmossdk.io/x/upgrade" upgradekeeper "cosmossdk.io/x/upgrade/keeper" @@ -114,7 +115,6 @@ import ( consumerkeeper "github.com/cosmos/interchain-security/v6/x/ccv/consumer/keeper" consumertypes "github.com/cosmos/interchain-security/v6/x/ccv/consumer/types" ccvdistr "github.com/cosmos/interchain-security/v6/x/ccv/democracy/distribution" - ccvgov "github.com/cosmos/interchain-security/v6/x/ccv/democracy/governance" ccvstaking "github.com/cosmos/interchain-security/v6/x/ccv/democracy/staking" ) @@ -549,7 +549,7 @@ func New( capability.NewAppModule(appCodec, *app.CapabilityKeeper, false), crisis.NewAppModule(&app.CrisisKeeper, skipGenesisInvariants, app.GetSubspace(crisistypes.ModuleName)), feegrantmodule.NewAppModule(appCodec, app.AccountKeeper, app.BankKeeper, app.FeeGrantKeeper, app.interfaceRegistry), - ccvgov.NewAppModule(appCodec, app.GovKeeper, app.AccountKeeper, app.BankKeeper, IsProposalWhitelisted, app.GetSubspace(govtypes.ModuleName), IsModuleWhiteList), + gov.NewAppModule(appCodec, &app.GovKeeper, app.AccountKeeper, app.BankKeeper, app.GetSubspace(govtypes.ModuleName)), mint.NewAppModule(appCodec, app.MintKeeper, app.AccountKeeper, nil, app.GetSubspace(minttypes.ModuleName)), slashing.NewAppModule(appCodec, app.SlashingKeeper, app.AccountKeeper, app.BankKeeper, app.ConsumerKeeper, app.GetSubspace(slashingtypes.ModuleName), app.interfaceRegistry), ccvdistr.NewAppModule(appCodec, app.DistrKeeper, app.AccountKeeper, app.BankKeeper, *app.StakingKeeper, authtypes.FeeCollectorName, app.GetSubspace(distrtypes.ModuleName)), diff --git a/app/consumer-democracy/proposals_whitelisting.go b/app/consumer-democracy/proposals_whitelisting.go deleted file mode 100644 index 975c9dbbad..0000000000 --- a/app/consumer-democracy/proposals_whitelisting.go +++ /dev/null @@ -1,55 +0,0 @@ -package app - -import ( - banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" - "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1" - "github.com/cosmos/cosmos-sdk/x/params/types/proposal" -) - -func IsProposalWhitelisted(content v1beta1.Content) bool { - switch c := content.(type) { - case *proposal.ParameterChangeProposal: - return isLegacyParamChangeWhitelisted(c.Changes) - default: - return false - } -} - -func isLegacyParamChangeWhitelisted(paramChanges []proposal.ParamChange) bool { - for _, paramChange := range paramChanges { - _, found := LegacyWhitelistedParams[legacyParamChangeKey{Subspace: paramChange.Subspace, Key: paramChange.Key}] - if !found { - return false - } - } - return true -} - -type legacyParamChangeKey struct { - Subspace, Key string -} - -// these parameters don't exist in the consumer app -- keeping them as an example -var LegacyWhitelistedParams = map[legacyParamChangeKey]struct{}{ - // add whitelisted legacy parameters here [cosmos-sdk <= 0.47] - // commented parameters are just an example - most params have been moved to their respective modules - // and they cannot be changed through legacy governance proposals - {Subspace: banktypes.ModuleName, Key: "SendEnabled"}: {}, -} - -// add whitelisted module param update messages [cosmos-sdk >= 0.47] -var WhiteListModule = map[string]struct{}{ - "/cosmos.gov.v1.MsgUpdateParams": {}, - "/cosmos.bank.v1beta1.MsgUpdateParams": {}, - "/cosmos.staking.v1beta1.MsgUpdateParams": {}, - "/cosmos.distribution.v1beta1.MsgUpdateParams": {}, - "/cosmos.mint.v1beta1.MsgUpdateParams": {}, - "/cosmos.gov.v1beta1.TextProposal": {}, - "/ibc.applications.transfer.v1.MsgUpdateParams": {}, - "/interchain_security.ccv.consumer.v1.MsgUpdateParams": {}, -} - -func IsModuleWhiteList(typeUrl string) bool { - _, found := WhiteListModule[typeUrl] - return found -} diff --git a/x/ccv/democracy/governance/doc.go b/x/ccv/democracy/governance/doc.go deleted file mode 100644 index 3b920e95e6..0000000000 --- a/x/ccv/democracy/governance/doc.go +++ /dev/null @@ -1,11 +0,0 @@ -/* -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 deleted file mode 100644 index 1ac1da886a..0000000000 --- a/x/ccv/democracy/governance/module.go +++ /dev/null @@ -1,165 +0,0 @@ -package governance - -import ( - "context" - "fmt" - "time" - - "cosmossdk.io/collections" - "cosmossdk.io/core/appmodule" - - "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" - govkeeper "github.com/cosmos/cosmos-sdk/x/gov/keeper" - govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" - govv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" - govv1beta1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1" -) - -const ( - AttributeValueProposalForbidden = "proposal_forbidden" -) - -var ( - _ module.AppModule = AppModule{} - _ module.AppModuleSimulation = AppModule{} - - _ appmodule.HasEndBlocker = AppModule{} -) - -// AppModule embeds the Cosmos SDK's x/governance AppModule -type AppModule struct { - // embed the Cosmos SDK's x/governance AppModule - gov.AppModule - - keeper govkeeper.Keeper - isLegacyProposalWhitelisted func(govv1beta1.Content) bool - isModuleWhiteList func(string) bool -} - -type ParamChangeKey struct { - MsgType, Key string -} - -// NewAppModule creates a new AppModule object using the native x/governance module AppModule constructor. -func NewAppModule(cdc codec.Codec, - keeper govkeeper.Keeper, - ak govtypes.AccountKeeper, - bk govtypes.BankKeeper, - isProposalWhitelisted func(govv1beta1.Content) bool, - ss govtypes.ParamSubspace, - isModuleWhiteList func(string) bool, -) AppModule { - govAppModule := gov.NewAppModule(cdc, &keeper, ak, bk, ss) - return AppModule{ - AppModule: govAppModule, - keeper: keeper, - isLegacyProposalWhitelisted: isProposalWhitelisted, - isModuleWhiteList: isModuleWhiteList, - } -} - -func (am AppModule) EndBlock(c context.Context) error { - ctx := sdk.UnwrapSDKContext(c) - rng := collections.NewPrefixUntilPairRange[time.Time, uint64](ctx.BlockTime()) - keeper := am.keeper - // if there are forbidden proposals in active proposals queue, refund deposit, delete votes for that proposal - // and delete proposal from all storages - err := am.keeper.ActiveProposalsQueue.Walk(ctx, rng, func(key collections.Pair[time.Time, uint64], _ uint64) (bool, error) { - proposal, err := keeper.Proposals.Get(ctx, key.K2()) - if err != nil { - return false, err - } - deleteForbiddenProposal(ctx, am, proposal) - return false, nil - }) - if err != nil { - return err - } - return am.AppModule.EndBlock(ctx) -} - -// isProposalWhitelisted checks whether a proposal is whitelisted -func isProposalWhitelisted(ctx sdk.Context, am AppModule, proposal govv1.Proposal) bool { - messages := proposal.GetMessages() - - // iterate over all the proposal messages - for _, message := range messages { - sdkMsg, isLegacyProposal := message.GetCachedValue().(*govv1.MsgExecLegacyContent) - if isLegacyProposal { - // legacy gov proposal content - content, err := govv1.LegacyContentFromMessage(sdkMsg) - if err != nil { - continue - } - if !am.isLegacyProposalWhitelisted(content) { - // not whitelisted - return false - } - // not legacy gov proposal content - } else if !am.isModuleWhiteList(message.TypeUrl) { - // not whitelisted - return false - } - } - return true -} - -// deleteForbiddenProposal removes proposals that are not whitelisted -func deleteForbiddenProposal(ctx sdk.Context, am AppModule, proposal govv1.Proposal) { - if isProposalWhitelisted(ctx, am, proposal) { - return - } - - logger := am.keeper.Logger(ctx) - - // 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 - _, _, _, err := am.keeper.Tally(ctx, proposal) - if err != nil { - logger.Warn( - "failed to tally disallowed proposal", - "proposal", proposal.Id, - "title", proposal.GetTitle(), - "total_deposit", proposal.TotalDeposit) - return - } - - err = am.keeper.DeleteProposal(ctx, proposal.Id) - if err != nil { - logger.Warn( - "failed to delete disallowed proposal", - "proposal", proposal.Id, - "title", proposal.GetTitle(), - "total_deposit", proposal.TotalDeposit) - return - } - - err = am.keeper.RefundAndDeleteDeposits(ctx, proposal.Id) - if err != nil { - logger.Warn( - "failed to refund deposits for disallowed proposal", - "proposal", proposal.Id, - "title", proposal.GetTitle(), - "total_deposit", proposal.TotalDeposit) - return - } - - ctx.EventManager().EmitEvent( - sdk.NewEvent( - govtypes.EventTypeActiveProposal, - sdk.NewAttribute(govtypes.AttributeKeyProposalID, fmt.Sprintf("%d", proposal.Id)), - sdk.NewAttribute(govtypes.AttributeKeyProposalResult, AttributeValueProposalForbidden), - ), - ) - - logger.Info( - "proposal is not allowed; deleted", - "proposal", proposal.Id, - "title", proposal.GetTitle(), - "total_deposit", proposal.TotalDeposit) -}