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

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

Merged
merged 15 commits into from
May 2, 2023

Conversation

julienrbrt
Copy link
Member

@julienrbrt julienrbrt commented Apr 26, 2023

Description

ref: #15304

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...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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)

@julienrbrt julienrbrt changed the title refactor(simapp): remove public app module basic refactor(simapp): remove public module basic manager Apr 26, 2023
types/module/module.go Fixed Show fixed Hide fixed
@itsdevbear
Copy link
Contributor

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

@julienrbrt
Copy link
Member Author

@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
Copy link
Member Author

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

@julienrbrt julienrbrt marked this pull request as ready for review April 28, 2023 20:58
@julienrbrt julienrbrt requested a review from a team as a code owner April 28, 2023 20:58
@github-prbot github-prbot requested review from a team, facundomedica and likhita-809 and removed request for a team April 28, 2023 20:59
_ "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
Copy link
Contributor

Choose a reason for hiding this comment

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

this is so hood.

Copy link
Member Author

@julienrbrt julienrbrt Apr 28, 2023

Choose a reason for hiding this comment

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

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.

@@ -101,7 +102,7 @@ func NewRootCmd() *cobra.Command {
},
}

initRootCmd(rootCmd, encodingConfig)
initRootCmd(rootCmd, encodingConfig, tempApp.BasicModuleManager)
Copy link
Contributor

Choose a reason for hiding this comment

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

this feels big bad

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that'd be fire actually!

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@itsdevbear
Copy link
Contributor

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
Copy link
Member Author

julienrbrt commented Apr 28, 2023

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.

Comment on lines +96 to +105
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
}
}

Check warning

Code scanning / CodeQL

Iteration over map

Iteration over map may be a possible source of non-determinism
simapp/app.go Outdated Show resolved Hide resolved
@julienrbrt julienrbrt requested a review from aaronc April 29, 2023 05:47
runtime/app.go Outdated
appConfig *appv1alpha1.Config
logger log.Logger
ModuleManager *module.Manager
BasicModuleManager module.BasicManager
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to be exported?

Copy link
Member Author

Choose a reason for hiding this comment

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

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 👍🏾

@@ -101,7 +102,7 @@ func NewRootCmd() *cobra.Command {
},
}

initRootCmd(rootCmd, encodingConfig)
initRootCmd(rootCmd, encodingConfig, tempApp.BasicModuleManager)
Copy link
Member

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@julienrbrt julienrbrt May 1, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@julienrbrt julienrbrt May 1, 2023

Choose a reason for hiding this comment

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

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.

@julienrbrt julienrbrt requested a review from aaronc May 1, 2023 20:25
@julienrbrt julienrbrt added this pull request to the merge queue May 2, 2023
@julienrbrt julienrbrt removed this pull request from the merge queue due to a manual request May 2, 2023
@julienrbrt julienrbrt enabled auto-merge May 2, 2023 08:03
@julienrbrt 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 julienrbrt deleted the julien/app-module-basic branch May 2, 2023 08:38
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants