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

kocubinski · 2022-06-27T21:36:51Z

Description

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

x/gov/module.go

aaronc

That’s the right idea. ManyPerContainer is probably more appropriate. We should think of a better name than RouteHandlerWrapper. Can we define the interface method directly on the Handler type?

api/cosmos/gov/module/v1/module.pulsar.go

+	protoiface "google.golang.org/protobuf/runtime/protoiface"
+	protoimpl "google.golang.org/protobuf/runtime/protoimpl"
+	io "io"
+	reflect "reflect"


aaronc · 2022-06-28T17:10:16Z

x/gov/types/v1beta1/content.go

@@ -18,3 +18,11 @@ type Content interface {
 // Handler defines a function that handles a proposal after it has passed the
 // governance process.
 type Handler func(ctx sdk.Context, content Content) error
+
+type RoutedHandler struct {


Suggested change

type RoutedHandler struct {

type HandlerRoute struct {

- post construction mutation is required to overcome circular dependencies

…ov-app-wiring

kocubinski · 2022-06-30T15:23:08Z

depinject/container.go

@@ -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)


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

kocubinski · 2022-06-30T15:24:56Z

depinject/container.go

@@ -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())


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

kocubinski · 2022-06-30T15:26:33Z

x/gov/keeper/keeper.go

-		legacyRouter: legacyRouter,
-		router:       router,
-		config:       config,
+	return &Keeper{


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

x/params/module.go

…ov-app-wiring

julienrbrt · 2022-06-30T21:17:12Z

simapp/app.go

@@ -422,5 +388,4 @@ func GetMaccPerms() map[string][]string {

 // initParamsKeeper init params keeper and its subspaces


Can this be removed?

julienrbrt · 2022-06-30T21:21:55Z

x/gov/module.go

+	return runtime.WrapAppModuleBasic(AppModuleBasic{})
+}
+
+func provideModule(


For consistency I'd think we should use a struct named govInputs and govOutputs.

julienrbrt · 2022-06-30T21:24:30Z

x/gov/keeper/keeper.go

@@ -55,34 +55,27 @@ type Keeper struct {
 func NewKeeper(


I think this requires a changelog

100% this is api breaking

julienrbrt · 2022-06-30T21:26:29Z

simapp/app.go

@@ -236,41 +232,11 @@ func NewSimApp(

 	initParamsKeeper(app.ParamsKeeper)


I think this can be removed

tac0turtle · 2022-06-30T22:00:33Z

x/gov/types/expected_keepers.go

+type GovHooksWrapper struct{ GovHooks }
+
+// IsOnePerModuleType implements the depinject.OnePerModuleType interface.
+func (GovHooksWrapper) IsOnePerModuleType() {}


out of scope but what is IsOnePerModule I don't find this intuitive, could be missing something

Implementing depinject.OnePerModuleType is an instruction for depinject, the IoC container will create one instance of GovHooksWrapper per module and collect them into a map[string]GovHooksWrapper as input to a provider further down the graph.

I kind of agree how this is not intuitive and could seem like spooky action at a distance. An alternative is to configure this behavior in the composition root.

depinject.ProvideOncePerModule(new(govkeeper.GovHooksWrapper))

…ov-app-wiring

julienrbrt

lgtm!

codecov · 2022-07-04T12:19:34Z

Codecov Report

Merging #12369 (e4bd03c) into main (74f265c) will increase coverage by 0.32%.
The diff coverage is 88.00%.

@@            Coverage Diff             @@
##             main   #12369      +/-   ##
==========================================
+ Coverage   65.71%   66.04%   +0.32%     
==========================================
  Files         680      692      +12     
  Lines       71733    72603     +870     
==========================================
+ Hits        47142    47948     +806     
- Misses      21943    21986      +43     
- Partials     2648     2669      +21

Impacted Files	Coverage Δ
x/gov/types/expected_keepers.go	`0.00% <0.00%> (ø)`
x/gov/types/v1beta1/content.go	`0.00% <0.00%> (ø)`
x/params/types/table.go	`97.72% <0.00%> (-2.28%)`	⬇️
x/params/module.go	`57.14% <72.72%> (ø)`
depinject/container.go	`90.49% <100.00%> (ø)`
simapp/app.go	`69.01% <100.00%> (-5.99%)`	⬇️
x/distribution/module.go	`56.66% <100.00%> (+0.48%)`	⬆️
x/gov/abci.go	`90.38% <100.00%> (ø)`
x/gov/genesis.go	`87.50% <100.00%> (ø)`
x/gov/keeper/grpc_query.go	`76.56% <100.00%> (ø)`
... and 29 more

github-actions bot added C:x/distribution distribution module related C:x/gov labels Jun 27, 2022

github-advanced-security bot found potential problems Jun 27, 2022

View reviewed changes

x/gov/module.go Fixed Show fixed Hide fixed

aaronc reviewed Jun 27, 2022

View reviewed changes

feat(gov): app-wiring in gov WIP

5255833

kocubinski force-pushed the kocubinski/gov-app-wiring branch from 166bcda to 5255833 Compare June 28, 2022 14:39

Return gov handler from params module as output

b41e56a

github-actions bot added the C:x/params label Jun 28, 2022

fix x/distribution types

7338ccc

github-advanced-security bot found potential problems Jun 28, 2022

View reviewed changes

aaronc reviewed Jun 28, 2022

View reviewed changes

wip x/gov

1ca6333

github-actions bot added the C:x/upgrade label Jun 28, 2022

julienrbrt changed the base branch from main to epic/app-wiring June 29, 2022 13:39

julienrbrt changed the base branch from epic/app-wiring to main June 29, 2022 15:52

refactor usages of gov keeper to pointer

9648308

- post construction mutation is required to overcome circular dependencies

github-actions bot added the C:Simulations label Jun 30, 2022

kocubinski added 2 commits June 30, 2022 09:56

Merge branch 'main' of github.com:cosmos/cosmos-sdk into kocubinski/g…

86e89f0

…ov-app-wiring

Add gov module to golang app config

638a31b

kocubinski changed the title ~~feat(gov): app-wiring in gov WIP~~ feat(gov): app-wiring for x/gov Jun 30, 2022

kocubinski commented Jun 30, 2022

View reviewed changes

x/params/module.go Outdated Show resolved Hide resolved

Merge branch 'main' of github.com:cosmos/cosmos-sdk into kocubinski/g…

190d012

…ov-app-wiring

kocubinski marked this pull request as ready for review June 30, 2022 15:32

kocubinski requested a review from a team as a code owner June 30, 2022 15:32

kocubinski added 2 commits June 30, 2022 10:51

Remove unneeded dependencies

16bf8a0

Merge branch 'main' of github.com:cosmos/cosmos-sdk into kocubinski/g…

ee2b8fb

…ov-app-wiring

julienrbrt reviewed Jun 30, 2022

View reviewed changes

simapp/app.go Outdated

@@ -236,41 +232,11 @@ func NewSimApp(

initParamsKeeper(app.ParamsKeeper)

Copy link

Member

julienrbrt Jun 30, 2022

Choose a reason for hiding this comment

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

I think this can be removed

tac0turtle reviewed Jun 30, 2022

View reviewed changes

kocubinski added 3 commits June 30, 2022 18:09

remove initParamsKeeper from simapp/app.go

4c79c5e

Changelog entry & refactor module input/output

8ade797

Merge branch 'main' of github.com:cosmos/cosmos-sdk into kocubinski/g…

f5f0db8

…ov-app-wiring

julienrbrt approved these changes Jun 30, 2022

View reviewed changes

kocubinski and others added 6 commits June 30, 2022 18:38

Move provideKeyTable back to x/gov

924d49b

better impl of keyTable provider

830f48f

clean up subspace provider

2453e17

Merge branch 'main' into kocubinski/gov-app-wiring

bfbb0d9

Merge branch 'main' into kocubinski/gov-app-wiring

995ec82

Merge branch 'main' into kocubinski/gov-app-wiring

e4bd03c

julienrbrt merged commit 61dc023 into main Jul 4, 2022

julienrbrt deleted the kocubinski/gov-app-wiring branch July 4, 2022 12:58

julienrbrt mentioned this pull request Jul 4, 2022

migrate x/gov to use app wiring #12274

Closed

1 task

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023

feat: migrate x/gov use app wiring (cosmos#12369)

27dc686

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kocubinski commented Jun 27, 2022 •

edited

Loading

aaronc left a comment •

edited

Loading

aaronc Jun 28, 2022

kocubinski Jun 30, 2022 •

edited

Loading

kocubinski Jun 30, 2022

julienrbrt Jun 30, 2022 •

edited

Loading

kocubinski Jun 30, 2022 •

edited

Loading

julienrbrt Jun 30, 2022

julienrbrt Jun 30, 2022

julienrbrt Jun 30, 2022

tac0turtle Jun 30, 2022

julienrbrt Jun 30, 2022

tac0turtle Jun 30, 2022

kocubinski Jun 30, 2022 •

edited

Loading

julienrbrt left a comment

codecov bot commented Jul 4, 2022 •

edited

Loading

		@@ -422,5 +388,4 @@ func GetMaccPerms() map[string][]string {

		// initParamsKeeper init params keeper and its subspaces

		@@ -236,41 +232,11 @@ func NewSimApp(

		initParamsKeeper(app.ParamsKeeper)

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

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

Conversation

kocubinski commented Jun 27, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

aaronc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kocubinski Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julienrbrt Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

kocubinski Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kocubinski Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

julienrbrt left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 4, 2022 • edited Loading

Codecov Report

kocubinski commented Jun 27, 2022 •

edited

Loading

aaronc left a comment •

edited

Loading

kocubinski Jun 30, 2022 •

edited

Loading

julienrbrt Jun 30, 2022 •

edited

Loading

kocubinski Jun 30, 2022 •

edited

Loading

kocubinski Jun 30, 2022 •

edited

Loading

codecov bot commented Jul 4, 2022 •

edited

Loading