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

feat(gov): app-wiring for x/gov #12369

Merged
merged 19 commits into from
Jul 4, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
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
560 changes: 560 additions & 0 deletions api/cosmos/gov/module/v1/module.pulsar.go

Large diffs are not rendered by default.

4 changes: 1 addition & 3 deletions depinject/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,6 @@ func (c *container) call(provider *ProviderDescriptor, moduleKey *moduleKey) ([]
}

func (c *container) getResolver(typ reflect.Type, key *moduleKey) (resolver, error) {
c.logf("Resolving %v", typ)
Copy link
Member Author

@kocubinski kocubinski Jun 30, 2022

Choose a reason for hiding this comment

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

I accidentally merged this debug code at some point, it makes error messages needlessly verbose and long. Sorry about that.


pr, err := c.getExplicitResolver(typ, key)
if err != nil {
return nil, err
Expand Down Expand Up @@ -417,7 +415,7 @@ func (c *container) resolve(in ProviderInput, moduleKey *moduleKey, caller Locat

markGraphNodeAsFailed(typeGraphNode)
return reflect.Value{}, errors.Errorf("can't resolve type %v for %s:\n%s",
in.Type, caller, c.formatResolveStack())
fullyQualifiedTypeName(in.Type), caller, c.formatResolveStack())
Copy link
Member Author

Choose a reason for hiding this comment

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

A much improved error message will result, e.g.

Failed to locate provider for github.com/cosmos/cosmos-sdk/x/gov/keeper/*keeper.Keeper

vs

Failed to locate provider *keeper.Keeper

Copy link
Member

@julienrbrt julienrbrt Jun 30, 2022

Choose a reason for hiding this comment

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

YES!

}

res, err := vr.resolve(c, moduleKey, caller)
Expand Down
14 changes: 14 additions & 0 deletions proto/cosmos/gov/module/v1/module.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
syntax = "proto3";

package cosmos.gov.module.v1;

import "cosmos/app/v1alpha1/module.proto";

// Module is the config object of the gov module.
message Module {
option (cosmos.app.v1alpha1.module) = {
go_import: "github.com/cosmos/cosmos-sdk/x/gov"
};

uint64 max_metadata_len = 1;
}
32 changes: 2 additions & 30 deletions simapp/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,6 @@ import (
govclient "github.com/cosmos/cosmos-sdk/x/gov/client"
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"
"github.com/cosmos/cosmos-sdk/x/group"
groupkeeper "github.com/cosmos/cosmos-sdk/x/group/keeper"
groupmodule "github.com/cosmos/cosmos-sdk/x/group/module"
Expand All @@ -80,7 +78,6 @@ import (
paramsclient "github.com/cosmos/cosmos-sdk/x/params/client"
paramskeeper "github.com/cosmos/cosmos-sdk/x/params/keeper"
paramstypes "github.com/cosmos/cosmos-sdk/x/params/types"
paramproposal "github.com/cosmos/cosmos-sdk/x/params/types/proposal"
"github.com/cosmos/cosmos-sdk/x/slashing"
slashingkeeper "github.com/cosmos/cosmos-sdk/x/slashing/keeper"
slashingtypes "github.com/cosmos/cosmos-sdk/x/slashing/types"
Expand Down Expand Up @@ -165,7 +162,7 @@ type SimApp struct {
SlashingKeeper slashingkeeper.Keeper
MintKeeper mintkeeper.Keeper
DistrKeeper distrkeeper.Keeper
GovKeeper govkeeper.Keeper
GovKeeper *govkeeper.Keeper
CrisisKeeper crisiskeeper.Keeper
UpgradeKeeper upgradekeeper.Keeper
ParamsKeeper paramskeeper.Keeper
Expand Down Expand Up @@ -219,14 +216,13 @@ func NewSimApp(
&app.EvidenceKeeper,
&app.DistrKeeper,
&app.UpgradeKeeper,
&app.GovKeeper,
); err != nil {
panic(err)
}

app.App = appBuilder.Build(logger, db, traceStore, baseAppOptions...)

app.keys = sdk.NewKVStoreKeys(govtypes.StoreKey)

// configure state listening capabilities using AppOptions
// we are doing nothing with the returned streamingServices and waitGroup in this case
if _, _, err := streaming.LoadStreamingServices(app.App.BaseApp, appOpts, app.appCodec, app.keys); err != nil {
Expand All @@ -239,28 +235,6 @@ func NewSimApp(
app.GetSubspace(crisistypes.ModuleName), invCheckPeriod, app.BankKeeper, authtypes.FeeCollectorName,
)

// register the proposal types
govRouter := govv1beta1.NewRouter()
govRouter.AddRoute(govtypes.RouterKey, govv1beta1.ProposalHandler).
AddRoute(paramproposal.RouterKey, params.NewParamChangeProposalHandler(app.ParamsKeeper)).
AddRoute(distrtypes.RouterKey, distr.NewCommunityPoolSpendProposalHandler(app.DistrKeeper)).
AddRoute(upgradetypes.RouterKey, upgrade.NewSoftwareUpgradeProposalHandler(app.UpgradeKeeper))
govConfig := govtypes.DefaultConfig()
/*
Example of setting gov params:
govConfig.MaxMetadataLen = 10000
*/
govKeeper := govkeeper.NewKeeper(
app.appCodec, app.keys[govtypes.StoreKey], app.GetSubspace(govtypes.ModuleName), app.AccountKeeper, app.BankKeeper,
app.StakingKeeper, govRouter, app.MsgServiceRouter(), govConfig,
)

app.GovKeeper = *govKeeper.SetHooks(
govtypes.NewMultiGovHooks(
// register the governance hooks
),
)

/**** Module Options ****/

// Sets the version setter for the upgrade module
Expand All @@ -274,7 +248,6 @@ func NewSimApp(
// must be passed by reference here.
if err := app.RegisterModules(
crisis.NewAppModule(&app.CrisisKeeper, skipGenesisInvariants),
gov.NewAppModule(app.appCodec, app.GovKeeper, app.AccountKeeper, app.BankKeeper),
); err != nil {
panic(err)
}
Expand Down Expand Up @@ -430,6 +403,5 @@ func GetMaccPerms() map[string][]string {

// initParamsKeeper init params keeper and its subspaces
Copy link
Member

Choose a reason for hiding this comment

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

Can this be removed?

func initParamsKeeper(paramsKeeper paramskeeper.Keeper) {
paramsKeeper.Subspace(govtypes.ModuleName).WithKeyTable(govv1.ParamKeyTable())
paramsKeeper.Subspace(crisistypes.ModuleName)
}
5 changes: 5 additions & 0 deletions simapp/app_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
evidencemodulev1 "cosmossdk.io/api/cosmos/evidence/module/v1"
feegrantmodulev1 "cosmossdk.io/api/cosmos/feegrant/module/v1"
genutilmodulev1 "cosmossdk.io/api/cosmos/genutil/module/v1"
govmodulev1 "cosmossdk.io/api/cosmos/gov/module/v1"
groupmodulev1 "cosmossdk.io/api/cosmos/group/module/v1"
mintmodulev1 "cosmossdk.io/api/cosmos/mint/module/v1"
nftmodulev1 "cosmossdk.io/api/cosmos/nft/module/v1"
Expand Down Expand Up @@ -190,6 +191,10 @@ var AppConfig = appconfig.Compose(&appv1alpha1.Config{
Name: feegrant.ModuleName,
Config: appconfig.WrapAny(&feegrantmodulev1.Module{}),
},
{
Name: govtypes.ModuleName,
Config: appconfig.WrapAny(&govmodulev1.Module{}),
},
},
})

Expand Down
3 changes: 3 additions & 0 deletions x/distribution/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"encoding/json"
"fmt"
govv1beta1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1"
"math/rand"

gwruntime "github.com/grpc-ecosystem/grpc-gateway/runtime"
Expand Down Expand Up @@ -240,6 +241,7 @@ type distrOutputs struct {
DistrKeeper keeper.Keeper
Module runtime.AppModuleWrapper
Hooks staking.StakingHooksWrapper
GovHandler govv1beta1.HandlerRoute
}

func provideModule(in distrInputs) distrOutputs {
Expand All @@ -250,5 +252,6 @@ func provideModule(in distrInputs) distrOutputs {
DistrKeeper: k,
Module: runtime.WrapAppModule(m),
Hooks: staking.StakingHooksWrapper{StakingHooks: k.Hooks()},
GovHandler: govv1beta1.HandlerRoute{Handler: NewCommunityPoolSpendProposalHandler(k), RouteKey: types.RouterKey},
}
}
2 changes: 1 addition & 1 deletion x/gov/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
)

// EndBlocker called every block, process inflation, update validator set.
func EndBlocker(ctx sdk.Context, keeper keeper.Keeper) {
func EndBlocker(ctx sdk.Context, keeper *keeper.Keeper) {
defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyEndBlocker)

logger := keeper.Logger(ctx)
Expand Down
4 changes: 2 additions & 2 deletions x/gov/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
)

// InitGenesis - store genesis parameters
func InitGenesis(ctx sdk.Context, ak types.AccountKeeper, bk types.BankKeeper, k keeper.Keeper, data *v1.GenesisState) {
func InitGenesis(ctx sdk.Context, ak types.AccountKeeper, bk types.BankKeeper, k *keeper.Keeper, data *v1.GenesisState) {
k.SetProposalID(ctx, data.StartingProposalId)
k.SetDepositParams(ctx, *data.DepositParams)
k.SetVotingParams(ctx, *data.VotingParams)
Expand Down Expand Up @@ -55,7 +55,7 @@ func InitGenesis(ctx sdk.Context, ak types.AccountKeeper, bk types.BankKeeper, k
}

// ExportGenesis - output genesis parameters
func ExportGenesis(ctx sdk.Context, k keeper.Keeper) *v1.GenesisState {
func ExportGenesis(ctx sdk.Context, k *keeper.Keeper) *v1.GenesisState {
startingProposalID, _ := k.GetProposalID(ctx)
depositParams := k.GetDepositParams(ctx)
votingParams := k.GetVotingParams(ctx)
Expand Down
4 changes: 2 additions & 2 deletions x/gov/keeper/grpc_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -276,10 +276,10 @@ func (q Keeper) TallyResult(c context.Context, req *v1.QueryTallyResultRequest)
var _ v1beta1.QueryServer = legacyQueryServer{}

type legacyQueryServer struct {
keeper Keeper
keeper *Keeper
}

func NewLegacyQueryServer(k Keeper) v1beta1.QueryServer {
func NewLegacyQueryServer(k *Keeper) v1beta1.QueryServer {
return &legacyQueryServer{keeper: k}
}

Expand Down
2 changes: 1 addition & 1 deletion x/gov/keeper/hooks_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func TestHooks(t *testing.T) {
govHooksReceiver := MockGovHooksReceiver{}

keeper.UnsafeSetHooks(
&app.GovKeeper, types.NewMultiGovHooks(&govHooksReceiver),
app.GovKeeper, types.NewMultiGovHooks(&govHooksReceiver),
)

require.False(t, govHooksReceiver.AfterProposalSubmissionValid)
Expand Down
6 changes: 3 additions & 3 deletions x/gov/keeper/invariants.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,20 @@ import (
)

// RegisterInvariants registers all governance invariants
func RegisterInvariants(ir sdk.InvariantRegistry, keeper Keeper, bk types.BankKeeper) {
func RegisterInvariants(ir sdk.InvariantRegistry, keeper *Keeper, bk types.BankKeeper) {
ir.RegisterRoute(types.ModuleName, "module-account", ModuleAccountInvariant(keeper, bk))
}

// AllInvariants runs all invariants of the governance module
func AllInvariants(keeper Keeper, bk types.BankKeeper) sdk.Invariant {
func AllInvariants(keeper *Keeper, bk types.BankKeeper) sdk.Invariant {
return func(ctx sdk.Context) (string, bool) {
return ModuleAccountInvariant(keeper, bk)(ctx)
}
}

// ModuleAccountInvariant checks that the module account coins reflects the sum of
// deposit amounts held on store
func ModuleAccountInvariant(keeper Keeper, bk types.BankKeeper) sdk.Invariant {
func ModuleAccountInvariant(keeper *Keeper, bk types.BankKeeper) sdk.Invariant {
return func(ctx sdk.Context) (string, bool) {
var expectedDeposits sdk.Coins

Expand Down
37 changes: 19 additions & 18 deletions x/gov/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,34 +55,27 @@ type Keeper struct {
func NewKeeper(
Copy link
Member

Choose a reason for hiding this comment

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

I think this requires a changelog

Copy link
Member

Choose a reason for hiding this comment

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

100% this is api breaking

cdc codec.BinaryCodec, key storetypes.StoreKey, paramSpace types.ParamSubspace,
authKeeper types.AccountKeeper, bankKeeper types.BankKeeper, sk types.StakingKeeper,
legacyRouter v1beta1.Router, router *baseapp.MsgServiceRouter,
config types.Config,
) Keeper {
router *baseapp.MsgServiceRouter, config types.Config,
) *Keeper {
// ensure governance module account is set
if addr := authKeeper.GetModuleAddress(types.ModuleName); addr == nil {
panic(fmt.Sprintf("%s module account has not been set", types.ModuleName))
}

// It is vital to seal the governance proposal router here as to not allow
// further handlers to be registered after the keeper is created since this
// could create invalid or non-deterministic behavior.
legacyRouter.Seal()

// If MaxMetadataLen not set by app developer, set to default value.
if config.MaxMetadataLen == 0 {
config.MaxMetadataLen = types.DefaultConfig().MaxMetadataLen
}

return Keeper{
storeKey: key,
paramSpace: paramSpace,
authKeeper: authKeeper,
bankKeeper: bankKeeper,
sk: sk,
cdc: cdc,
legacyRouter: legacyRouter,
router: router,
config: config,
return &Keeper{
Copy link
Member Author

@kocubinski kocubinski Jun 30, 2022

Choose a reason for hiding this comment

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

The gov keeper constructor must return a pointer to Keeper since we're setting hooks and routes in a post-construction step.

storeKey: key,
paramSpace: paramSpace,
authKeeper: authKeeper,
bankKeeper: bankKeeper,
sk: sk,
cdc: cdc,
router: router,
config: config,
}
}

Expand All @@ -97,6 +90,14 @@ func (keeper *Keeper) SetHooks(gh types.GovHooks) *Keeper {
return keeper
}

func (keeper *Keeper) SetLegacyRouter(router v1beta1.Router) {
// It is vital to seal the governance proposal router here as to not allow
// further handlers to be registered after the keeper is created since this
// could create invalid or non-deterministic behavior.
router.Seal()
keeper.legacyRouter = router
}

// Logger returns a module-specific logger.
func (keeper Keeper) Logger(ctx sdk.Context) log.Logger {
return ctx.Logger().With("module", "x/"+types.ModuleName)
Expand Down
4 changes: 2 additions & 2 deletions x/gov/keeper/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ import (

// Migrator is a struct for handling in-place store migrations.
type Migrator struct {
keeper Keeper
keeper *Keeper
}

// NewMigrator returns a new Migrator.
func NewMigrator(keeper Keeper) Migrator {
func NewMigrator(keeper *Keeper) Migrator {
return Migrator{keeper: keeper}
}

Expand Down
4 changes: 2 additions & 2 deletions x/gov/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ import (
)

type msgServer struct {
Keeper
*Keeper
}

// NewMsgServerImpl returns an implementation of the gov MsgServer interface
// for the provided Keeper.
func NewMsgServerImpl(keeper Keeper) v1.MsgServer {
func NewMsgServerImpl(keeper *Keeper) v1.MsgServer {
return &msgServer{Keeper: keeper}
}

Expand Down
Loading