refactor(simapp): remove public module basic manager by julienrbrt · Pull Request #15958 · cosmos/cosmos-sdk

julienrbrt · 2023-04-26T15:39:05Z

Description

PR to investigate how to get group genesis working nicely with app v1 and app v2. Currently, InitGenesis utilizes the basic module manager, but we need an app module instantiated for creating an app module basic for appmodule.HasGenesis.

This removes the need to define a list AppModuleBasic in app.go and app_v2.go. Now it will infer it from the module manager.

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)

itsdevbear · 2023-04-26T17:07:12Z

@julienrbrt lmk if you need help here, I have a deep hated passion for having to define this variable LOL

julienrbrt · 2023-04-26T17:35:23Z

@julienrbrt lmk if you need help here, I have a deep hated passion for having to define this variable LOL

Haha thanks, the biggest wonder is how to make it nice when you need to manually instantiate AppModuleBasic (for gov and genutil f.e), but I have an idea.

julienrbrt · 2023-04-28T20:58:56Z

Feels a bit like a hack but what do you think?

itsdevbear · 2023-04-28T21:03:09Z

+	_ "cosmossdk.io/x/nft/module"                     // import for side-effects
+	_ "cosmossdk.io/x/upgrade"                        // import for side-effects
+	_ "github.com/cosmos/cosmos-sdk/x/auth/tx/config" // import for side-effects
+	_ "github.com/cosmos/cosmos-sdk/x/auth/vesting"   // import for side-effects


this is so hood.

Yeah, because we do not import the module directl now that there is no app basic imports, we need these for app wiring.
Fortunately depinject has a descriptive error message and points which import is missing.

itsdevbear · 2023-04-28T21:03:32Z

 	}

-	initRootCmd(rootCmd, encodingConfig)
+	initRootCmd(rootCmd, encodingConfig, tempApp.BasicModuleManager)


this feels big bad

Why do we need this tempApp and where is this coming from?

The origin is not to use depinject for app v1 (as this means we do not show how to get those values for app v1 apps).
For app v2 app, we can just use depinject. I can remove it to clean it up, but then it does mean that when building with the app legacy flag, it will still use the app wiring config.

One thing I can simply do is try to create a root.go and root_v2.go for both implementations. So we can simplify the example for those using depinject, where it is way cleaner.

Yeah that'd be fire actually!

Done, for app v1 it does need to stay like this. app v2 is way cleaner :)

itsdevbear · 2023-04-28T21:04:48Z

yeah this is kinda gross. I feel like it adds more complexity than it helps solve.

What if the Appliation interface forces the app to expose what modules it supports?

I think the thing that bugs me the most is the whole tempApp thing

julienrbrt · 2023-04-28T21:14:26Z

Yeah, the tempApp thing is definitely ugly. Unfortunately there are things about the app that we get only after app instantiation. We were already using it for that; hence I thought maybe it was ok to use it again for getting the app module basic.

+	for name, module := range manager.Modules {
+		if customBasicMod, ok := customModuleBasics[name]; ok {
+			moduleMap[name] = customBasicMod
+			continue
+		}
+
+		if basicMod, ok := module.(AppModuleBasic); ok {
+			moduleMap[name] = basicMod
+		}
+	}


aaronc · 2023-05-01T18:19:59Z

-	appConfig         *appv1alpha1.Config
-	logger            log.Logger
+	ModuleManager      *module.Manager
+	BasicModuleManager module.BasicManager


Why does this need to be exported?

I did that for parity with app v1, I won't do that anymore given I will create a second root.go for app v2 only 👍🏾

aaronc · 2023-05-01T18:21:39Z

 	}

-	initRootCmd(rootCmd, encodingConfig)
+	initRootCmd(rootCmd, encodingConfig, tempApp.BasicModuleManager)


Why do we need this tempApp and where is this coming from?

aaronc · 2023-05-01T18:22:17Z

+
+		if basicMod, ok := module.(AppModuleBasic); ok {
+			moduleMap[name] = basicMod
+		}


Can we use an app module basic adapter for core app modules?

We can add a condition for that but we will still need this one.
In runtime, we can indeed directly use the adapter.

Okay, after a few trials, the core app module adapter does not work. I will leave it out of this PR.

Done the following in SetupAppBuilder

app.basicManager[name] = basicMod basicMod.RegisterInterfaces(inputs.InterfaceRegistry) basicMod.RegisterLegacyAminoCodec(inputs.LegacyAmino) basicMod := module.CoreAppModuleBasicAdaptor(name, mod)

And the following in NewBasicManagerFromManager

if appModule, ok := module.(appmodule.AppModule); ok { moduleMap[name] = CoreAppModuleBasicAdaptor(name, appModule) continue }

Something to investigate in a follow-up.

refactor(simapp): remove public app module basic

9f555aa

julienrbrt changed the title ~~refactor(simapp): remove public app module basic~~ refactor(simapp): remove public module basic manager Apr 26, 2023

github-advanced-security AI found potential problems Apr 26, 2023

View reviewed changes

Comment thread types/module/module.go Fixed

julienrbrt added 4 commits April 27, 2023 22:57

Merge branch 'main' into julien/app-module-basic

24ab464

updates

78835a8

updates

10f2217

Merge branch 'main' into julien/app-module-basic

e033c7f

julienrbrt marked this pull request as ready for review April 28, 2023 20:58

julienrbrt requested a review from a team April 28, 2023 20:58

ghost requested review from a team, facundomedica and likhita-809 and removed request for a team April 28, 2023 20:59

itsdevbear reviewed Apr 28, 2023

View reviewed changes

Comment thread simapp/app_v2.go

itsdevbear reviewed Apr 28, 2023

View reviewed changes

github-advanced-security AI found potential problems Apr 28, 2023

View reviewed changes

alexanderbez approved these changes Apr 28, 2023

View reviewed changes

Comment thread simapp/app.go Outdated

julienrbrt requested a review from aaronc April 29, 2023 05:47

julienrbrt added 3 commits April 29, 2023 10:37

docs

86b0855

linting

b045e6d

Merge branch 'main' into julien/app-module-basic

bfe737c

aaronc reviewed May 1, 2023

View reviewed changes

updates

5b113cb

julienrbrt requested a review from aaronc May 1, 2023 20:25

Merge branch 'main' into julien/app-module-basic

ccdec7f

julienrbrt added this pull request to the merge queue May 2, 2023

julienrbrt removed this pull request from the merge queue due to a manual request May 2, 2023

julienrbrt added 3 commits May 2, 2023 09:43

Merge branch 'main' into julien/app-module-basic

65d6691

add upgrading.md

d8f465d

typo

dad695d

julienrbrt enabled auto-merge May 2, 2023 08:03

julienrbrt added 2 commits May 2, 2023 10:07

word

3432b32

rename

a18b226

julienrbrt added this pull request to the merge queue May 2, 2023

Merged via the queue into main with commit 946f3d6 May 2, 2023

julienrbrt deleted the julien/app-module-basic branch May 2, 2023 08:38

julienrbrt mentioned this pull request May 2, 2023

fix(types): add fallbacks for CoreAppModuleBasicAdaptor #16010

Merged

19 tasks

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

refactor(simapp): remove public module basic manager (cosmos#15958)

e250c89

Conversation

julienrbrt commented Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Author Checklist

Reviewers Checklist

Uh oh!

Uh oh!

itsdevbear commented Apr 26, 2023

Uh oh!

julienrbrt commented Apr 26, 2023

Uh oh!

julienrbrt commented Apr 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julienrbrt Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itsdevbear commented Apr 28, 2023

Uh oh!

julienrbrt commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Check warning

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julienrbrt May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julienrbrt May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

julienrbrt commented Apr 26, 2023 •

edited

Loading

julienrbrt Apr 28, 2023 •

edited

Loading

julienrbrt commented Apr 28, 2023 •

edited

Loading

julienrbrt May 1, 2023 •

edited

Loading

julienrbrt May 1, 2023 •

edited

Loading